UnnecessaryCheckNotNull doesn't work with @NullMarked

[jbellis]

I'm not sure how UnnecessaryCheckNotNull is supposed to work, it doesn't do anything out of the box with @NullMarked packages.

Edit: it doesn't do anything with explicitly annotated @NotNull parameters, either.

Edit 2: unless the docs are incorrect and it's not actually on by default.

[cpovirk]

I think UnnecessaryCheckNotNull is doing something different than what you have in mind—probably because it's detecting something weird enough that you wouldn't even think to expect it :) I believe that what it's looking for is checkNotNull(new Foo()), checkNotNull("blah"), and checkNotNull(somePrimitiveValue), only the last of which is something that I would probably ever have thought of :)

We should have better docs.

I do also think there could be some value in a check like the one you're expecting.

That said, I do like to perform such "unnecessary" checks in common libraries, just to make sure that callers don't happen to get away with passing null in certain situations (e.g., someCollection.removeAll(null) if someCollection turns out to be empty), only to fail when the situation changes slightly in production or when we change the implementation—including perhaps someday porting it to Kotlin, which performs null check automatically, or some potential future version of Java with null-restricted types, which would likewise perform automatic checks. (In fact, I tend to reserve Preconditions.checkNotNull almost exclusively for cases in which static analysis "knows" that the value isn't null :))

Outside of common libraries, though, I'm all in favor of having a way to detect and remove such checks automatically, with support for both @NonNull and @NullMarked. I don't know whether it will become a priority, but I can add it to my giant list of nullness projects :)

[jbellis]

Thank you!

Is this something that a newcomer to the project could potentially contribute? I.e. is it tractable without changing core data structures or touching potentially fragile code paths?

[cpovirk]

My initial reaction is that it's probably doable, especially with existing utilities like NullnessUtils.isInNullMarkedScope and NullnessAnnotations.fromAnnotations*). I hope I am not leading you astray :)

[bdragan]

@cpovirk Thank you for the details, I submitted PR #5121 to add RedundantNullCheck bug pattern.

[jbellis]

Hi @cpovirk, we've been using this implementation successfully for a couple months now, could you review the PR?

[cpovirk]

I'm sorry, I made some very poor estimates of how long some other things would take, and that's left you and a couple other people hanging. Thanks for the nudge. I had a couple thoughts, and I've now taken a little closer look and done some testing. That's led to https://github.com/google/guava/pull/8010, but it also means that I will go leave comments on #5121 now.

[bdragan]

@cpovirk Many thanks for the detailed review, I'll take a look at the suggestions.