[moved to #230] Decide whether to include a @NonNull annotation

[dzharkov]

[edit: closed for being too long and disorganized. See #230.]

[TODO: direct link to doc about this issue]

The question was initially asked by @cpovirk

To be honest, I didn't get the question

[eaftan]

We use the Checker Framework inside Google. The CF defaults to non-null, i.e. if an element is not annotated, it is assumed to be @NonNull. I think cpovirk is asking whether we can do that in this project as well.

[donnerpeter]

IntelliJ IDEA has an option to add a runtime assertion whenever there's something @NotNull-annotated. This works fine with explicit annotations, but not too well with defaults (scoped or not). I wonder if people will expect runtime instrumentation to be added if the default is non-null. This would be problematic because it'd depend on whether the project is using these annotations or not, and it might be not easy to tell.

[kevin1e100]

I think @NotNull will be rarely used in Java source code, but I do think it's valuable to have it for a few cases:

• as an override, e.g., to specify a method parameter or result as @NotNull T in the context of a type parameter <T extends @nullable Object>
• in cases where code authors don't want to use @DefaultNotNull
• so compilers can insert the annotation automatically (kotlinc does this, for instance).

[donnerpeter]

Currently IntelliJ IDEA can only generate runtime assertions (which are quite useful) when @NotNull is specified locally. Honoring default package-level not-nulls is a challenge, because if one appears or disappears, the IDE has to detect that and recompile the whole package.

[eaftan]

I think it's important to have @NotNull so checkers can distinguish unannotated code from annotated-but-defaulted-to-@NotNull code.

[wmdietlGC]

In our last discussion, the decision whether something is annotated or not is purely based on whether there is a NotNullByDefault (TBD) annotation on the enclosing top-level class or directly enclosing package.

Having @NotNull as an explicit annotation is still useful. In addition to the examples above, it can be useful to suppress warnings and pick an explicit annotation.

[kevinb9n]

I concur that we are going to need @NotNull. I think we can probably close this?

[kevinb9n]

From @cpovirk

It's largely about flipping the default, but it sort of goes deeper because it means more cases for users to understand (and us to make sure that we're getting right).

The main concrete example I have is the "overrides"/"projects" example. Granted, a large part of that idea still needs to exist no matter what: It's always possible to have @Nullable T (like Map.get()). But to me, it feels easier to understand "It's possible to add null to the permissible values" than "It's possible to add or remove null." I mean, it sounds simple, but then I start trying to think about how @NullnessUnknown plays into all this, and I really don't want to have to think about 3x3 cases instead of 2x2.

(I should also acknowledge that there's some case, which is escaping me at the moment, in which @NotNull is "necessary." But IIRC it's only because the bounds aren't following PECS -- though I think they can't in that case for some reason, but the point is that the signature is already "broken" before nullness gets involved. Plus, handling this one usual case feels like the tail wagging the dog. Sorry for being so vague; I can pull up details if we discuss later.

[cpovirk]

Following up:

The case where @NotNull is necessary is actually (to the best of my knowledge) hypothetical:

class Foo<T extends @Nullable Object> { 
  @NotNull T getOrDefault(@NotNull T default) { ... } 
}

Because of the name, I'd been thinking that the use case was Map.getOrDefault, but I was wrong.

I can believe that someone does want this somewhere, but I'd expect it to be rare.

I searched our code base for usage [^<]@NonNull\b\s*[A-Z]\b -- basically, example of @NotNull T. I looked at the first few ~30 results, and I was not impressed: As expected, it's almost exclusively code that could be trivially converted to declare the type parameter itself as @NotNull. Declaring the type parameter itself as @NotNull is both better (since it will allow the code to use, say, an Optional<T> [edit: rather than needing to project to Optional<@NotNull T>]) and, with our proposed defaulting rules, easier (since, unlike the Checker Framework, <T> will mean <T extends [@NotNull] Object rather than <T extends @Nullable Object>) [edit: disputed in #12]

I did see one instance of <E extends Enum<E>>. I'm not sure if we'll allow <E extends [@NotNull] Enum<E>>, so maybe there's a use case there?

Another speculative(?) use case that @kevinb9n had raised is in specifying a nullness for type parameters with unknown nullness:

[@DefaultNullnessUnknown]
class SomeForeignList<E> {
  E get(int i);
}

@DefaultNotNull
class MyClass<E> {
  // Checkers might *require* a nullness annotation here, where it would mean
  // to treat E as if it had been declared as <E extends @NotNull Object>.
  SomeForeignList<@NotNull E> delegate;
}

There might be a need for this, but:

1. I have been feeling that operating with type parameters of unknown nullness is just going to be too messy, and people should define stubs or leave the nullness unknown.

2. This feels like a very, very different meaning than @NotNull normally has. If we need to support it (see (1) :)), then it may deserve its own annotations. [edit: Or possibly @NotNull doesn't have much effect here, since SomeForeignList.get(i) is still treated as returning @NullnessUnknown E?]

One other point:

Providing @Nullable but not @NotNull is (IIUC) a better match for the types declarable in Kotlin source. That should help programmers move from one language to the other -- and presumably help code be converted, too. (I wonder if @NotNull could even complicate Kotlin's ability to interpret these nullness annotations, but I haven't tried to reason through that.)

---

(I also said above the the signature is already "broken." I think I was wrong about that, too, sorry: I believe my point was that the method should really return <U super T>, so you could call fooOfDouble.getOrDefault(1) and get back a Number. But that doesn't help with adding @NotNull, which would a <U extends T> type (which doesn't make sense here).)

[cpovirk]

so compilers can insert the annotation automatically (kotlinc does this, for instance).

Could they automatically insert @DefaultNotNull and then insert @Nullable where needed? As noted above, this seems like a closer match to the Kotlin model, so if anything, I would hope that it would be easier.

I think it's important to have @NotNull so checkers can distinguish unannotated code from annotated-but-defaulted-to-@NotNull code.

I think this is solved now by having annotations like @DefaultNotNull and @DefaultNullnessUnknown (with the latter as the "default default"), right?

In addition to the examples above, it can be useful to suppress warnings and pick an explicit annotation.

I'm not sure I understand. Can you elaborate? Is this about explicitly specifying nullness on a local variable, whose nullness would otherwise be inferred? (If so, would we prefer an explicit test (e.g., requireNonNull) to a suppression? Might we require a suppression in any case?)

Currently IntelliJ IDEA can only generate runtime assertions (which are quite useful) when @NotNull is specified locally. Honoring default package-level not-nulls is a challenge, because if one appears or disappears, the IDE has to detect that and recompile the whole package.

This should also be solved now that we are planning not to support per-package defaults.

[cpovirk]

Providing @Nullable but not @NotNull is (IIUC) a better match for the types declarable in Kotlin source. That should help programmers move from one language to the other -- and presumably help code be converted, too. (I wonder if @NotNull could even complicate Kotlin's ability to interpret these nullness annotations, but I haven't tried to reason through that.)

This probably applies to other languages, too.

For example, we discussed this morning how Scala 3 is talking about supporting null using union types (Foo | Null). This makes it (possibly?) easy to model @Nullable T. But it's less clear that they'd have a way to model @NotNull T (barring support for set difference, which I suppose is possible -- oh, or maybe they can just intersect T with Any or AnyRef or something? Most languages won't have as many features as Scala to fall back on, though).

And if Java itself were to gain built-in nullability support, we don't know if they'd want to build in support for the equivalent of @NotNull T, especially given that they'd likely need to support it in their user-facing APIs [edit: meaning reflection and annotation-processing APIs] (though maybe that doesn't end up as hard as I'd think) and they'd know that other Java tools might not handle it correctly.

Now of course we've accepted soundiness, so it's probably OK if not every user of our annotations handles them completely. But I think the even better soundiness argument is that, if @NotNull T would be needed only rarely, then we don't need it at all :) It would be especially sad if some tools didn't implement @NotNull T correctly and therefore couldn't check code when that code shouldn't have used @NotNull T in the first place :)

[amaembo]

Currently, IntelliJ IDEA codebase doesn't use the default annotations, and default nullability of all methods is really unknown. For some non-annotated interface method without checking all the implementation, one cannot tell whether the null return or parameter is possible. We have dozens of thousands of method parameters which are annotated as NotNull, but also dozens of thousands of non-annotated method parameters. Changing to DefaultNotNull would be a big pain for our project because we will need to explicitly annotate every non-annotated method parameter and return value as @NullnessUnknown (at least until we figure out the actual nullability of every parameter and return value). So for our project migrating to the annotations package which doesn't provide a @NotNull annotation is not an option. I could agree that our codebase is a mess, but don't expect that most real-world Java code is in the better shape. After all, the high adoption of our project is one of the primary goals. Absence of @NotNull clearly goes against.

We may create some official guidelines or a tutorial on how to start a new project with our annotations where we may discourage the @NotNull usage, suggesting that it should be used in exceptional cases only. We may even mention it in the @NotNull JavaDoc. In general, I agree that in the new project the @NotNull annotation is not that necessary. However, if you migrate an existing project, this may vary.

[stephan-herrmann]

With Eclipse, even for code that has a top level @NonNullByDefault, still the following reasons exist for explicit @NonNull (aside from migration issues, where users may want to start with individual annotations before jumping at the big default):

• local variables initialized from (a) unspecified nullness, or (b) a call to a generic method (want to give a hint into type inference)
• individual type references in a signature that is excluded from the default

[cpovirk]

Thanks, that's interesting.

For local variables, I would think requireNonNull would help, and/or users would suppress warnings (warnings that they might have to suppress even if they annotate the variable as @NotNull? I'm not 100% sure: For local variables overall, I think we've mostly just said waved our hands and said "type inference," rather than talking about how we treat annotations -- or maybe I've just missed those discussions).

For calls to a generic method, I think that any explicit type arguments would default like other type arguments do. If so, there would be no need for @NotNull there in a @NotNullByDefault context.

For a signature that's excluded from the default, I'm OK with saying that, if someone wants to declare parts of the signature as not-null, then we'll require that the whole thing be @DefaultNotNull, with @UnknownNullness sprinkled in where necessary. While I'm in favor of making incremental migration easy, I think that it's enough for us to say "You can change the default of the class but keep some methods' defaults unknown, and then you can change a method's default but keep parts of it unknown, but you can't change the default of the class, keep the default of the method unknown, and change parts of it."

[stephan-herrmann]

For local variables, I would think requireNonNull would help

This reminded me to file #56 (mildly related), at lest that method would need to be annotated.

[...] waved our hands and said "type inference," [...]

This reminded me to file #59

[stephan-herrmann]

For me enabling combined inference with target typing (as discussed in #59) is a sufficient reason for needing an explicit nonnull annotation.

[cpovirk]

In an example like this...

@SuppressWarnings("nullness")
@NotNull Foo foo = getTheFoo();

...I would recommend writing...

Foo foo = requireNonNull(getTheFoo());

But from #59, I get the impression that you're talking more about a case in which the suppression shouldn't be necessary? Do you have examples in mind? As I implied somewhere or other recently, in-method type inference is mostly magic to me :)

[edit: Sorry, I see that I said much the same thing previously. Sorry if you've already tried to answer this and I just misunderstood.]

[cpovirk]

Prompted by #58, a question for @abreslav or others responsible for Kotlin and other languages:

Do you see demand in the Kotlin (or <your language here>) world for projecting to @NotNull (which I think is not possible in its syntax, but correct me if I'm wrong)? I understand that it conceptually makes sense (as in the example above), but I think we've been right to generally err on the side of "when in doubt, leave it out" for less commonly used annotations. But maybe we have reason to believe that this would be more commonly used than I think from my brief survey of Google usages?

[update: See KT-26245.]

[stephan-herrmann]

I get the impression that you're talking more about a case in which the suppression shouldn't be necessary? Do you have examples in mind?

The key feature is that Java 8 type inference uses the target type (LHS of the assignment) to guide type inference, and my concern is about ability to express intention, not about suppressing a warning.

Let's start with an overly simplistic example:

<T> T id(T in) { return in; }
...
@NonNull String local = id(null);

This could cause inference to instantiate T to @NonNull String (so that the return type matches the target type) to the effect that passing null is the precise location of error.

For a bit more sophistication let's think of:

<T> T first(List<T> in) { return in.get(0); } // T could be hidden deeper in the type structure ...
...
List<@Nullable String> dubious = new ArrayList<>();
dubious.add(null);
@NonNull String = first(dubious);

Firstly, list dubious can obtain null information for its type argument only from the declaration, not from its initialization (there is no type argument to look at)! Type inference lets information flow left-to-right!

Then again type inference could instantiate T to @NonNull String and then we'd report that a List<@Nullable String> is not a legal argument to the instantiated method <@NonNull String>first(). If list dubious would have type List<@NonNull String then type inference would see that all types match nicely. By expressing my intention warnings (if any) are shown at the location of the root cause.

OTOH, if the target type were unannotated, than simply extending the rules of type inference would result in the following: since the target type is seen as a legacy type, type inference will only infer the legacy type @LegacyNullness String, and at best we can produce a null-unchecked warning by wrapping the expression in a properly annotated requireNonNull(). We'd loose two bits of precision: we introduce an unwanted legacy type into type inference which weakens null warnings to null-unchecked warnings, and secondly, we cannot point to the argument of the method invocation that causes the conflict in the first place. Imagine:

<T> T oneOf(T t1, T t2, T t3, T t4) { return t2; }
...
Object result = oneOf(m1(), m2(), m3(), m4());
print(result.toString());

If this prints an unchecked warning against the last line, how will you find that m3() is the only guy that could potentially introduce a null value? If you declare @NonNull Object result, inference will do the hard work for you.

TL:DR; for local variable null inference information flows in the direction of data flows (right-to-left in an assignment), whereas type inference can use the target type to let information flow in the opposite direction. If the target type has no nullness information, type inference is crippled in terms of nullness.

[cpovirk]

Thanks for all the details! Some scattered replies:

• I'm of course happy to hear that this would apply to implementations only and not to APIs, since our priority is to provide annotations that help on APIs :)

• For what it's worth, it looks like, of all usages of the Checker Framework's @NonNull annotation in the Google depot, about 1% are on local variables. (Caveat: I did this with a fairly hacky regex search, so it's possible that I did something wrong.)

• It sounds like users could sometimes get equivalent checking by specifying explicit type arguments. For example, using your third example:

<T> T oneOf(T t1, T t2, T t3, T t4) { return t2; }
...
Object result = MyClass.<[@NonNull] Object>oneOf(m1(), m2(), m3(), m4());
print(result.toString());

That said:

• That requires restating the Object type.
• All else equal, some users might prefer to see @NotNull on the variable, rather than on the method call. (After all, maybe the method returns @Nullable T.)

But at least you can turn it on for debugging.

Sometimes it might not work so well, like if you have:

List<@Nullable String> strings = ...;
// ... much later ...
String s = strings.get(0);
s.getBytes(...);

The get call doesn't have a type parameter for you to specify. You might have to do something ugly like:

List<@Nullable String> strings = ...;
// ... much later ...
String s = MyClass.<[@NonNull] String>id(strings.get(0));
s.getBytes(...);

There are probably parallels to var: Sometimes you want inference to do its thing; other times, you want to be explicit so that you get more precise errors and you reveal your intention to readers.

[cpovirk]

More data on usage: The Checker Framework uses @NotNull on some methods on Class:

• @NonNull T newInstance()
• @NonNull T @Nullable [] getEnumConstants()

This is necessary because it lets Class be instantiated with a @Nullable type. I wouldn't have guessed that it would do that, since there's (I think?) no difference between Class<String> and Class<@Nullable String> (and those types end up inconvertible, or at best @Covariant). But apparently it has advantages when calling methods that use Class objects to work around the lack of reified generics. (Then, for consistency, they do the same for Optional.) I would want to think about this more.

The Class decision ends up at least a little viral. It and Optional appear to account for nearly all usages of @NonNull in the JDK stubs (other than as a type-parameter bound, since we are likely to default those to "not null"). Here's a search (in which I filtered out a few usages that looked unnecessary in addition to a few that wouldn't apply under our rules):

$ cd checker-framework/checker/jdk/nullness/src
$ find -type f | sort | xargs grep -r '@NonNull' | grep -v -e 'extends @NonNull' -e ': *//' -e 'implements Enumeration<@NonNull Object>' -e @PolyAll -e ./java/lang/reflect/ParameterizedType.java
./java/lang/Class.java:    public @NonNull T newInstance()
./java/lang/Class.java:    public @NonNull T @Nullable [] getEnumConstants() {
./java/lang/Class.java:    Map<String, @NonNull T> enumConstantDirectory() {
./java/lang/reflect/Constructor.java:    public @NonNull T newInstance(Object ... initargs) throws InstantiationException,IllegalAccessException,IllegalArgumentException,InvocationTargetException { throw new RuntimeException("skeleton method"); }
./java/lang/reflect/Field.java:  @SideEffectFree public <T extends java.lang.annotation. @Nullable Annotation> @Nullable T getAnnotation(Class<@NonNull T> obj) { throw new RuntimeException("skeleton method"); }
./java/util/Comparator.java:    public static <T extends Comparable<@NonNull ? super @NonNull T>> Comparator<T> naturalOrder() {
./java/util/Optional.java:public final @NonNull class Optional<T> {
./java/util/Optional.java:    private Optional(@NonNull T value) {
./java/util/Optional.java:    public static <T> Optional<T> of(@NonNull T value) {
./java/util/Optional.java:    public @NonNull T get() {

The remaining non-Class, non-Optional case is Comparator.naturalOrder(). It has @NonNull both on the bound and on the wildcard itself (the latter of which we would likely not permit). I don't trust myself to think through that one fully right now, so I'll leave it for another day :)

[cpovirk]

The larger discussion about avoiding default annotations has been happening more on #89, so just a quick note here:

The filterNonNull example is an interesting one. I did a quick search for APIs fitting that pattern in the Google depot (both by searching for similar names and by searching for methods that return Something<@NonNull T>) without any luck.

But of course I see tons of filter(Objects::nonNull) and filter(x -> x != null), so it's easy to imagine that Java developers can benefit from a Kotlin-like filterNonNull once their types comes with nullness information. (They could use a static method or flatMap, but that's a little ugly.) Then again, until common APIs like Stream are retrofitted with filterNonNull (unlikely unless nullness becomes a language feature?), this remains hypothetical.

[cpovirk]

Oh, I just thought to grep over the Kotlin sources for Sequence<T?> (which maybe you've already done?). The only hits other than filterNotNull are the similar requireNoNulls and the very similar filterNotNullTo.

[kevin1e100]

Re filter see also: typetools/checker-framework#1345

I'm quite willing to believe this is not a case that comes up in a lot of APIs, but it's still an important one for users, since these particular APIs, as you were able to confirm, are unsurprisingly used a lot.

Here's a related example this brings to mind (Javadoc):

list.removeIf(Objects::isNull)

Really capturing this would require "changing" list's type argument. I'm not sure I've seen a tool that can express this, though it's seemingly doable in principle.

[cpovirk]

Thanks for the link.

Another thing that libraries could conceivably do is to add methods like filter(Class), similar to what FluentIterable has. This would naturally return a Sequence<T> for some non-null type T.

But there are significant downsides:

• It works only if you have a Class object. That's easy if you're operating on, say, a Stream<String>, but it's inconvenient or impossible if you have to handle a generic Stream<T>.
• It's possible to pass the wrong Class object, and you get a mysteriously empty stream. (Probably Error Prone could help if such an API were added. I guess it could help with FluentIterable today if we wanted to write a check.)
• It's more code.
• It's much less clear about what it's doing. ("Why is it filtering a Stream<Foo> for instances of Foo?") It's even possible that people would expect filter(String.class) to keep null values (though that might be a stretch, and of course nullness annotations would help clear that up).

Also, the JDK was not interested. It's conceivable that nullness could make them feel differently, but if they were making a change specifically for nullness, then filterNonNull might make sense -- if it were expressible, which I still sort of doubt would be the case for a nullness language feature, even if it is with our annotations.

[cpovirk]

(A digression, but: I see that NullAway has special handling for the filter case.)

[kevinb9n]

Here is what we are working on:

1. Explaining, convincingly, that there is actually a reasonably sane world in which @NonNull does not exist in our project. (This step is NOT convincing anyone to like that world, just that it does fundamentally make sense.)

2. Explaining the benefits and the drawbacks of having a @NonNull annotation relative to that picture. Warning: we have so far come up with about TEN arguments on each side. We are working to distill that morass down into something consumable for you. If you are keen to have it shared to you before then, let me know.

For a long time, I believed there was really no controversy to be had here; of course we would need @NonNull. I was dead wrong: while we may want it, there really is a case to be made either way, and our sacred duty is to fully explore both sides.

We've been talking about an initial 0.1 pre-release that supports only the use cases of (a) fully, properly annotated source files and (b) all-legacy source files, but nothing in between. (Of course, that fails a key project requirement, which is why it's not called 1.0.) Also, support for non-nullable type projections would come later as well (if at all; #86). If our working decisions on issues like #12 go the way the currently seem to be, then this means that we would not actually have much need for @NonNull for this 0.1 release yet no matter which way we end up deciding here.

Hopefully that means the time pressure is a little reduced on making this decision.

[sdeleuze]

FYI we have 16 concrete use cases of @NonNull in Spring Framework codebase which is (a) fully, properly annotated source files. The use case is to avoid unnecessary checks on a subclass that returns for sure a non-nullable value whereas the interface or the superclass define @Nullable. For example https://github.com/spring-projects/spring-framework/blob/a11a592734b4518332239425973cf11cb4f80fcf/spring-jdbc/src/main/java/org/springframework/jdbc/core/namedparam/MapSqlParameterSource.java#L169.

We can't really avoid @Nullable on the interface or superclass because we want to be defensive and force null-safety checks when they are used, and only skip those when the specific subclass annotated with @NonNull is used.

I guess we can live with 16 warnings in 0.1 release, but we would like a proper solution that allows 0 warnings after.

[cpovirk]

Thanks: I am embarrassed to discover that, as far as I can tell, I had not searched through Spring for usages of @NonNull.

For the 16 that you have, all would be unnecessary under jspecify semantics: Our annotations aren't automatically inherited by overrides, so overriding @Nullable Foo getParameterNames() with Foo getParameterNames() already makes the return of the latter non-null.

Our recent discussions about sample inputs remind me that I should add some samples for this case. I'll try to come back and link them from this post once I've done so.

Edit: Samples are in 149f3df...d068c45

(Of course, in all this, I'm assuming that you apply @DefaultNonNull (or make your @NonNullApi an alias for it), since your files are fully annotated. The big use case for @NonNull is for code that does not use @DefaultNonNull. We have been preparing some giant docs for the eventual discussion :))

[cpovirk]

kevinb9n suggested that we close this thread in favor of a new one -- not with the intention of having this discussion tomorrow but at least to give interested parties a rough idea of the areas of past conversation.

When we're ready to make a decision, we'll clean up and old, unpublished doc we have about this -- or throw it out and write a new one, too :) But for a quick summary, see #230.