ReturnMissingNullable should ignore @DoNotCall and always throwing methods

[PhilippWendler]

Sometimes we have methods that we need to implement due to an interface requiring them, but we do not want to support them and make them always throw, for example when implementing immutable collections. ReturnMissingNullable wants to add @Nullable in such cases if the interface method is known to be nullable, but this is useless. It would be nice if ReturnMissingNullable would ignore such cases.

Concrete example:

public class Test {

  abstract interface MyNavigableSet<E> extends NavigableSet<E> {

    /**
     * @throws UnsupportedOperationException Always.
     * @deprecated Unsupported operation.
     */
    @Deprecated
    @DoNotCall
    @Override
    /** Always throws. */
    E pollFirst();
  }

  abstract class MySet<E> implements MyNavigableSet<E> {

    @Override
    public E pollFirst() {
      throw new UnsupportedOperationException();
    }

    @Override
    public E pollLast() {
      throw new UnsupportedOperationException();
    }
  }
}

ReturnMissingNullable from Error Prone 2.11.0 warns about all 3 methods:

    [javac] Test.java:16: warning: [ReturnMissingNullable] Method returns a definitely null value but is not annotated @Nullable
    [javac]     E pollFirst();
    [javac]       ^
    [javac]     (see https://errorprone.info/bugpattern/ReturnMissingNullable)
    [javac]   Did you mean '@Nullable @Deprecated'?
    [javac] Test.java:22: warning: [ReturnMissingNullable] Method returns a definitely null value but is not annotated @Nullable
    [javac]     public E pollFirst() {
    [javac]              ^
    [javac]     (see https://errorprone.info/bugpattern/ReturnMissingNullable)
    [javac]   Did you mean '@Nullable @Override'?
    [javac] Test.java:27: warning: [ReturnMissingNullable] Method returns a definitely null value but is not annotated @Nullable
    [javac]     public E pollLast() {
    [javac]              ^
    [javac]     (see https://errorprone.info/bugpattern/ReturnMissingNullable)
    [javac]   Did you mean '@Nullable @Override'?

Concrete suggestions:

1. ReturnMissingNullable should ignore all methods marked @DoNotCall or if the method implements a @DoNotCall method from an interface.
2. ReturnMissingNullable should ignore all methods that can be detected to always throw. At least cases where the method body consists of a single throw should be possible, I guess.

[cpovirk]

Thanks. I'd wondered if this would be an issue, but I should have looked into it more at the time. Probably I didn't take it seriously enough because, inside Google, we're rolling out these changes automatically, so there's relatively little cost to adding @Nullable where it's unnecessary. But your report is a reminder that the rest of the world is different and that we're inflicting an unnecessary cost on future code even inside Google.

As it happens, I looked at all the findings for this kind of method about a month ago. I ended up finding a lot of "interesting" Map implementations, so I've had a CL pending to stop issuing errors for this kind of method entirely case entirely, except for people who set the Conservative flag to false -- which I assume you don't do?

However, I've delayed in submitting that CL because we're still planning to roll out even those @Nullable annotations across our depot (or insert @SuppressWarnings("ReturnMissingNullable") where desired). Arguably I should just submit it....

Once I do submit it, this problem will go away for normal users. However, I should still look at implementing your suggestions. Those would help users who do set Conservative flag to false.

[PhilippWendler]

Thanks for your response!

I in principle would have no problem with letting Error Prone automatically insert the missing @Nullable declarations, but your point about future code where users see unexpected warnings is a good one. And adding the annotations could actually distract and confuse readers of these methods, for example when looking at Javadoc APIs or so.

I would also like to point out that this check did find a handful of actually missing @Nullable annotations in the same map implementations where it reported the redundant warnings, so I consider it a useful check and would not suggest to just disable it for collections.

Regarding Conservative, I did not know about this flag before, but I might actually be interested in using it on one of our projects that is smaller and has higher-quality code, where I also use -XepAllDisabledChecksAsWarnings. But it is fine for me if I am on my own when setting this.

[cpovirk]

Thanks for sharing more from your experiences.

Maybe I can try keeping the handling of Map, etc. on by default if I make your suggested fixes: A lot of the existing "weird" Map implementations are from the days before we had things like convenient cache libraries, Map.computeIfAbsent, etc. So maybe we won't see so many such implementations in the future. If so, then Map handling is likely to do the right thing almost all the time. (I think it was already something like 92% correct in my testing, so the future might bring a very small percentage of false positives, and that comes out of a small number of implementations of interfaces like Map in the first place.)

While we're on the topic, one other point about Conservative: I also plan to evaluate the other heuristics that it applies. For example, it currently ignores any method whose body is the single statement return null;! It also ignores methods with type-variable return types (which means that it would ignore methods on Map, etc. if not for the special handling we've been discussing). I had some theories about why those were the conservative things to do, but I have a suspicion that both may be over-conservative.

[cpovirk]

For example, it currently ignores any method whose body is the single statement return null;!

Some quick testing on Google's codebase suggests that this particular exception is highly justified :(

return null is very commonly used for methods that "must be overridden" or otherwise "should never be called." In other words:

error-prone/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java

Lines 320 to 323 in 9941ae5

	* When the entire method body is `return null`, I worry that this may be a stub
	* implementation that all "real" implementations are meant to override. Ideally such
	* stubs would use implementation like `throw new UnsupportedOperationException()`, but
	* let's assume the worst.

Hopefully you'll have a better experience if you set -XepOpt:Nullness:Conservative=false on select projects.

(I hope to report back about the type-variable case later today.)

[PhilippWendler]

Interesting. I don't think we have any stubs like this. (And if there were, I would like to find them.)

For the above mentioned smaller project of us, using Conservative=false found two SortedMap.comparator() implementations were return null; was used as the correct implementation but which missed the annotation. Apart from this no other additional findings. So I would tend to use this flag for this project once the current issue is resolved. (I want this project to be fully nullness-annotated, and I think we are close already.)

[cpovirk]

The type-variable case turns out to be more interesting. Nearly all the findings from it are correct, but there are some patterns that lead to false positives. For example:

static <T> T clone(T t) {
  if (t == null) {
    return null;
  }
  ...
}

Here, the output is null only if the input is. The best way to annotate such a method would be to say that it both accepts and returns "plain T," not "@Nullable T." That way, tools can know that, if you pass a non-null input, you'll get a non-null output.

I want to look more into the type-variable case someday. For now, I'm going to leave it disabled in the conservative mode, and people can turn it on as desired.