Non-null projection of a type variable usage

[kevinb9n]

We've agreed to support the "nullable projection of a type variable"; that is, for a type variable T, the ability to denote the type representing "T union null"? This would presumably be written @Nullable T. (Of course, we get that for free with the way we've conceptualized what "Nullable" means.)

Should we support the "non-null projection of a type variable"; denoting the type "T minus null"? On this we seem to have less agreement, so let's look at use cases.

[kevinb9n]

I am currently thinking this concept may not apply to wildcards. The way to think about wildcards is perhaps instead #88.

[cpovirk]

Trying to summarize where I think we stand, part 1: Projecting to non-null

We don't think there is much need for this in practice:

• Existing usages in the Checker Framework JDK appear unlikely to be necessary under our system.
• We were able to annotate Guava without using it. (OK, I made one compromise on Maps.difference because I didn't want to add a type parameter. It doesn't appear that the compromise affects any existing callers. [edit: And now see some other small examples.])
• The only existing method we're aware of that it would be necessary for is filterNonNull -- but that's a Kotlin method, not a Java method. And it seems somewhat unlikely for such methods to become common in Java unless nullness becomes a language feature and that feature supports projecting to non-null (which seems somewhat unlikely, even if our project supports it). [edit: I later learned of a JDK method that does use non-null projection: Map.merge, discussed below.]

But this is all distinct from other questions:

• We are still likely to include a @NonNull annotation, as it helps users incrementally annotate codebases ([moved to #230] Decide whether to include a @NonNull annotation #4). [edit: The common case will be applying it to type usages that aren't type variables: For example, we'll see @NonNull String.]
• @NonNull T will still be valid Java. (And even if our checkers forbid it, users may write it in code that is not checked by our checker -- possibly including stub files.) So we will need to settle the behavior of that -- which could still be "project to non-null" if we think that would be least confusing, even if that's not a behavior we need to support (or necessarily even want to support).

[cpovirk]

Trying to summarize where I think we stand, part 2: Projecting to unspecified nullness

This is a bit harder to summarize.

We suspect that users will want something kind of like this on occasion:

• Android is likely to use @NullnessUnspecified for APIs that can return null but never do so in normal usage (i.e., if certain common preconditions are met). Some such Android APIs are in this list -- including at least one generic API, findViewById[*]. We are not necessarily "in favor" of this, but we are likely to want it to work sanely.
• Users (perhaps with the help of automation from IDEs, etc.) may spray @NullnessUnspecified on all API types in a class or method as part of "flipping the default" to @DefaultNonNull for new code and incrementally annotating old code.

However, in the second case (and perhaps the first, too, if @NullnessUnspecified is applied to methods like Map.get) users probably don't want full projection. That is, they don't want @NullnessUnspecified T to unconditionally override the nullness of T. Instead, if they instantiate with T as @Nullable String, they'll want @NullnessUnspecified T to turn into @Nullable String.

However, if they instantiate T as @NonNull String, they may want the type to become @NullnessUnspecified String. That case would be a projection.

My model for this is that @NullnessUnspecified means "@Nullable or no annotation, but I don't know which." Under that model, it makes sense that @NullnessUnspecified T could "add" nullness (because it might be meant to be @Nullable T) but never "remove" it (because it might be meant to be plain T).

As in the previous post, though, we might or might not feel that we can give @NullnessUnspecified T the behavior that we ideally might want.

[*] To produce an effect much like @NullnessUnspecified T (possibly exactly like it? or is it "inconsistent?"), Android could just leave that whole method as @DefaultNullnessUnspecified. That might not be a good solution in general, since the blast radius can be large, but if such cases are rare enough, it might be sufficient.

[kevin1e100]

(FWIW, findViewById's type parameter seemingly could be bounded by non-null. Similar to Map.get, another way of looking at it could be that a nullable type argument seemingly justifiably "could" result in a proper nullable, since the caller code went through the trouble of using a nullable type argument, and maybe even "should", since that's probably what Kotlin would do today with findViewById<MyView?>(...).)

[cpovirk]

I don't have a particularly strong opinion, but FWIW:

I would see that as covered by the second paragraph of Type Arguments of Parameterized Types, which says to ignore the ? (well, @Nullable) in that case. I suspect that that's how you interpret the current text, too, but you're saying that we could change it? I would guess that the current rule is more useful on net. Do you think it's worth a new issue for this? (If so, I will try to remember to link it from the design proposal.)

[kevin1e100]

Agreed on the ignoring if the type parameter was bounded by non-null. I meant to say that that's not the only possible way of specifying findViewById, though: it could alternatively seemingly be declared like this:

<T extends @Nullable View> @NullnessUnspecified T findViewById(int id) {...}

[cpovirk]

Ah, thanks, now I get it, and I'm not sure what else I could have thought the first sentence of your previous post meant :)

While I've spoken against "unnecessarily" nullable bounds in the past (like on Class and Optional), I see that this is different:

• The nullable bound actually affects the result you get from the method (at least assuming we make a particular decision about substitution rules).
• This is a method, not an object, so there's no danger that some method will require a Foo<@NonNull Object> when a user has a Foo<@Nullable Object>.

I am maybe somewhat still vaguely against a nullable bound here, simply because I like the idea of encouraging users to always pick the "obvious" signature when annotating their own code (and especially writing stub files, since multiple users might write stubs for the same API, and it would be nice for them to agree). But I'm not that opposed. And it's not like we'd have to add a new feature to allow it.

(This ends up not being that relevant, but out of curiosity, I looked at what percentage of callers of findViewById in our depot perform a null check on the result. It looks like maybe a little under 2%.)

[cpovirk]

I tried to search Google's codebase for cases in which CF users needed Class<T> to be instantiable with a nullable type. (That's relevant to this issue because, if it's possible to instantiate Class<@Nullable Foo>, then we need (or at least might want) to project the return value of methods like newInstance to @NonNull T.)

I didn't find much, though I was searching for a very specific pattern, as discussed in this post, in which I also report the results. (Most of what my search turned up was various proxying/intercepting use cases, like retrying, rate limiting, and memoizing, in which the input object had to be non-null.)

If anyone has ideas for patterns of "project to non-null" that I could look for, including other patterns of using Class<T> with a potentially nullable type argument, let me know, and I may see what I can find.

[cpovirk]

[@wmdietlGC , since we recently spoke about nullable instantiations of Class<T>]

[cpovirk]

RE: needing Class<T> to be instantiable with a nullable type:

I could also imagine searching for methods that accept a Class<T> and a Foo<T>[*]. I suspect that most of those are either only reading from the Foo or only writing to it, though, in which case they could accept both nullable and non-nullable instantiations by using wildcards like Foo<? extends @Nullable T>. (And wildcards are likely a good idea for non-nullness-related PECS reasons, too.)

[*] maybe starting with a search like Class<T>.*\w[^s]+<T> lang:java

[cpovirk]

(Possibly exception: arrays, for which you can't use wildcards. But arrays are trouble all around.)

[kevin1e100]

I can believe that examples like class Optional<T> can be annotated to not allow nullable type arguments, and then their methods can refer to plain T, not needing non-null projections. I even feel that ruling out other possible annotation schemes for the same classes that do rely on projections is maybe a good thing: give users one way of doing things but not more than one. But some existing systems seem to allow things like Optional<@Nullable String>, raising questions I think:

• Are there adoption concerns here? If Optional (as an example) in our system cannot be annotated such that Optional<@Nullable String> is allowable, will that create issues for users trying to adopt our annotations?
• Are there cases where using something like Optional<@Nullable String> (or Optional<U>, where U is some nullable type parameter in scope) is useful / necessary? If so then that would suggest there is a need to non-null project somewhere, though it could be when specifying Optional's type argument instead of annotating Optional itself.

@wmdietlGC seems to feel that things like Optional<@Nullable String> and Class<@Nullable String> aren't needed, IIRC, though I'm not sure right now where he said that, so that's encouraging to me.

[cpovirk]

You might be looking for this @wmdietlGC post.

I should note that Guava implementation code did contain one usage of Optional<U> with a nullable U parameter. But that usage turned out to be not just avoidable but obfuscating. You can see that it forces wrapping and unwrapping so that it can distinguish between null (i.e., no Optional) and Optional.absent(), but then it doesn't use that distinction at all. I have it ripped out in my annotated Guava, and I'll be ripping it out in the mainline.

I will see how I can search for users of Optional who really do want to permit nullable instantiations. I might just change the stub file to forbid nullable instantiations, identify all the breakages, and look at a sample.

[cpovirk]

Oops, I did already look into that, but I put the information into a comment thread and then resolved it....

I moved it into the doc body, but it's short enough to quote here:

cpovirk to see if any of these really want a nullable bound on their type parameter. Results:

• I looked at about a quarter of them. Based on that, I would say: no.
• All those came from declaring class Foo<T> (or a similar method type parameter) but only ever instantiating it with non-null arguments -- and pretty clearly intending only to do so.

[cpovirk]

I should note that Guava implementation code did contain one usage of Optional<U> with a nullable U parameter. But that usage turned out to be not just avoidable but obfuscating.

Sorry, I take that back. While Optional is not helpful for the methods that currently exist externally, we do need a wrapper object (or, if we want to reduce allocations, a sentinel value):

• for a method that exists only internal to Google, and
• for a method we're likely to add (both internally and externally) someday.

Still, the workaround is straightforward.

[cpovirk]

One use case for projecting to non-null that is still hypothetical but looks more realistic (if far away): a default method on Map:

Optional<@NonNull V> getOptional(@Nullable Object key);

See JDK-8204662 and JDK-8232647. The former notes that the JDK owners may consider this once value types exist.

The alternative to supporting such a projection is:

• declare that anyone who needs to implement this method (which should be rare) should just suppress
• ideally, have our spec prescribe some behavior for Optional<V> when V can take on nullable types. (Currently, our design proposal's section on Members of Parameterized Types proposes to make optional.get() return a non-null value in this case. Specifically, see the first bullet.)

(Or maybe someday we'll have a way to say that the method can be called only on a Map whose value type is non-null, similar to Checker Framework issue 3203.)

(My personal position is still to avoid supporting projection to non-null, but this is at least a small argument to the contrary.)

[kevin1e100]

Hmm we might be approaching critical mass here. Another somewhat similar example that would benefit from projections was a filterNonNull method, and given how getOptional is discussed in JDK-8204662, a method like firstOptional could conceivably be defined in a similar way.

[cpovirk]

I do think these are all important possibilities to have an eye on. At the same time, I think it's worth remembering that they are all currently hypothetical. (Additionally, Valhalla in particular is still far off, and the subsequent library improvements could be further still, and both could happen in non-LTS releases and thus take even longer to get to users.)

It's still nice to support what we can. I claim only that the use cases are small enough that they're not decisive on their own. We get to weigh them against the costs (which I grant are not enormous but which I think are non-obvious), similar to how we weigh the (larger) costs and benefits of including @PolyNull for Optional.orElse and Class.cast.

(All that said, it does seem fairly likely to me that, if we include @NonNull, it's going to project to non-null. I say that based less on use cases and more on the obviousness theorem :) Unless I can pull a different name for @NonNull out of the air, I am currently more focused on arguing against it entirely, as you may have noticed :) But for purposes of the current discussion about requirements, I'm making the claim that our current examples are more suggestive than concrete.)

[kevin1e100]

To me, the fact that these are examples from APIs that Java developers use ~ every day does suggest significant importance. They may be hypothetical, but on the other hand, if we don't have ideas on how to design them to avoid projections, that still suggests to me that we may be missing functionality that could be important more generally. `@PolyNull` to me is a bit different only because it seems that it can be added "later" with relative ease, and the question there is more how best to do it (`@PolyNull` vs `@Contract`). There are also relatively clear ways of avoiding the need for `@PolyNull` in new APIs (e.g., `orNullI()` :) ). Just to repeat something I mentioned before, *if* we define `Optional<Anything>` to implicitly project the type argument to `@NonNull` because that's `Optional`'s type parameter bound, then we still don't need explicit projections to handle `Optional`-related examples.

[kevinb9n]

I believe @cpovirk has been collecting up use cases for this.

(If we decide to have this then it will need an annotation; and if that annotation wants to be @NonNull then we will have to face the question of whether that annotation should also be useful in other ways, like in null-unmarked code. But one step at at time.)

[cpovirk]

3 known APIs from Guava, all around "A type parameter that wants nothing to do with nullness" (#78), either in the form of Class or the form of Equivalence:

[edit: And maybe someday we'll also want this for a hypothetical Map.getOptional (JDK-8204662, JDK-8232647) in the JDK.]

Iterables.toArray

(and its FluentIterable equivalent, though note that FluentIterable has many more problems around nullness)

Today:

<T> @Nullable T[] toArray​(Iterable<? extends @Nullable T> iterable, Class<T> type)

With non-null projection:

<T extends @Nullable Object> T[] toArray​(Iterable<? extends T> iterable, Class<@NonNull T> type)

Alternatively, with @PolyNull (but note that I see this use case like the first "less compelling" case for @PolyNull; the only reason that the usual solution doesn't apply here is because of Class):

<T> @PolyNull T[] toArray​(Iterable<? extends @PolyNull T> iterable, Class<T> type)

This matters to some existing Google users of the Checker Framework, so we have a stub file set up for them so that they don't need to suppress a warning. I forget how many people are affected.

Maps.difference (3-arg)

Today:

<K extends @Nullable Object,​ V> MapDifference<K, ​V> difference​(
    Map<? extends K, ​? extends V> left,
    Map<? extends K, ​? extends V> right,
    Equivalence<? super V> valueEquivalence)

With non-null projection:

<K extends @Nullable Object,​ V extends @Nullable Object> MapDifference<K, ​V> difference​(
    Map<? extends K, ​? extends V> left,
    Map<? extends K, ​? extends V> right,
    Equivalence<? super @NonNull V> valueEquivalence)

Edit: I forgot that there's a way for this API to provide maximum flexibility even without non-null projection. The reason that we didn't do so is that it requires an extra type parameter, so it would be a source-incompatible change (which we're allowed to make but we avoid without a good enough reason):

<K extends @Nullable Object, Q,​ V extends @Nullable Q> MapDifference<K, ​V> difference​(
    Map<? extends K, ​? extends V> left,
    Map<? extends K, ​? extends V> right,
    Equivalence<Q> valueEquivalence)

(I guess @PolyNull might work here, too? I hadn't thought about it before, and I'm not going to think deeply about it now.)

This matters to no Google users of the Checker Framework, and I even verified that it would matter to no Google users even if everyone used the Checker Framework.

It does require us to suppress a warning in the 2-arg overload of Maps.difference itself (or at least it will when I finish the current batch of changes to our hacky checker :)) -- or else duplicate some code [edit: if only by mostly duplicating the signature when we declare a private helper method with the third signature above].

ClassToInstanceMap

Today:

interface ClassToInstanceMap<B> extends Map<Class<? extends B>, B>

Or maybe what we should do today (even though it would be inconvenient to users), given that just a couple users inside Google insert null values, is:

interface ClassToInstanceMap<B> extends Map<Class<? extends B>, @Nullable B>

With non-null projection:

interface ClassToInstanceMap<B extends @Nullable Object> extends Map<Class<? extends @NonNull B>, B>

[kevinb9n]

Is FluentIterable.getFirst() a case for it too?

Today:

class Optional<T extends Object>

class FluentIterable<E extends @Nullable Object> {
  @SuppressWarnings("nullness") // it's broken, but we seem to have no choice
  Optional<E> first() { ... }
}

Alternative?

  Optional<@NonNull E> first() {
    return Optional.of(...); // if it wouldn't satisfy the projected type it will throw anyway
  }

[cpovirk edit: Similarly, SetView has an immutableCopy() method that returns an ImmutableSet, and Ordering has an immutableSortedCopy() method that returns an ImmutableList.]

[cpovirk]

Yes, thanks. (And to be clear, there are multiple similar methods on that class.) I had gotten too discouraged and distracted about FluentIterable in #157 (comment). But no matter how bad the user's experience is from false negatives, we'd rather get the return type right than wrong :)

And as noted in that same comment, tools could conceivably be made smart enough to identify bad usages of the methods. (Hmm, maybe they could be made to identify the need to project to non-null automatically even without the annotation?)

A nice bonus of being able to declare the return type correctly is that we would no longer have to suppress warnings at the method level, only at the actual unsafe call. (Granted, if we're able to design sophisticated warning suppression and get it adopted widely, then the method-level suppression could perhaps be written in a way less likely to mask other errors.)

[kevinb9n]

(Hmm, maybe they could be made to identify the need to project to non-null automatically even without the annotation?)

This sounds dodgy. If yes on this issue then they should project the normal way and remove the warning suppression. If no on it, the warning and need to suppress are justified. And then even at the call site (still referencing your linked comment, here) I would have wanted to get a warning any time any expression at any granularity has a bogus static type.

[kevinb9n]

minor note: I've been referring to @NonNull T as being a "difference type", and maybe it technically is, but a general difference type could be much more complex (e.g. Number - Integer). The most this will ever do is to "undo" a union-null. Maybe "projection" really isn't so bad.

[kevinb9n]

Using the FluentIterable example:

1. I was raising (in Is the declaration "Foo<@NullnessUnspecified Object>" a possible mismatch when Foo forbids nullable type arguments? #158 (comment)) the question of whether a type like Optional<@Nullable String> even exists at all. It seems right to say it does not ...
2. Which then raises the question of whether Optional<E> even exists when E is nullable-bounded. It would seem also not to exist, and...
3. IF we are hard-no on explicit non-null projection, then I guess (re: cpovirk last comment) it's worth considering having Optional<E> in the first() signature just do it implicitly. It just doesn't seem like a great place to end up.

Separately: what is the very best we can do for these use cases if we don't have non-null projection? It would of course be very strange for people to actually get an Optional that "thinks" it might return null.

[kevinb9n]

(question for @cpovirk there)

[cpovirk]

I've subsequently said a couple things on #158, and I'm not sure if they were the right direction to take the conversation or the right thing to say them on. But there's a fairly direct question here that I think I can belatedly answer.

What we have now is probably the best option:

@NullMarked
public abstract class FluentIterable<E extends @Nullable Object> implements Iterable<E> {
...
  @SuppressWarnings("nullness") // Unsafe, but we can't do much about it now.
  public final Optional<E> first() {

The result of that is that it's possible to end up with what our rules would consider to be an Optional<@Nullable String> or an Optional<T> for some <T extends @Nullable Object>.

But "the ImmutableList.Builder rule" says that calling get() on those nevertheless returns String and @NonNull T, respectively.

So I'd anticipate issues only if someone is trying to implement a method whose return type is Optional<String>, since that's still considered to be distinct from Optional<@Nullable String>. Could we "fix" that by adding another rule? I haven't really thought about it. It would be great if we knew how often it would matter to users. For that matter, it would be great if we knew how often the rule that we do have would matter to users :)

Until we can convince ourselves that a new rule would be important to users, I'd lean against having one: While the handling of get() fits naturally into our rules for substitution (which we need anyway), this rule for "fixing" type arguments may be the first of its kind: It feels like a new "step" in the process: After we figure out what the annotations say, we then go back and reevaluate whether that's reasonable in this context, and then we use the output of that for the rest of our work. I worry about the cost of that, from implementation complexity to runtime, with the worst case being some kind of infinite recursion. (Hopefully it wouldn't come to that. But we've already had challenges avoiding infinite recursion in some cases where the rules aren't fundamentally infinitely recursive but our implementation ends up going down such a path.)

(But assuming that we do #230, we'll very likely do the topic of this issue, too, in which case we don't have to worry much about FluentIterable.first() specifically.)

[kevinb9n]

Working decision (as discussed in last meeting): yes, we will have @NonNull in 0.3 and it will work in the 2-3 ways expected.

[cpovirk]

I should admit here that I have since learned of a JDK method that does use non-null projection in the way that people normally think of it (i.e., not just for Class<@NonNull T> or Optional<@NonNull T>: Map.merge, whose value parameter (and whose remappingFunction's parameter types) should be @NonNull V: jspecify/jdk#14, eisop/jdk#41.

[kevinb9n]

Hmm. They've continued to prove rare?

Was there an issue where we were considering whether Optional</*parametric*/T> should automatically conform to bounds? It still feels highly problematic to me, but do you feel like it's worth reconsidering?

[cpovirk]

Up until the Map.merge thing, it was used only for the checked* collections (Class<@NonNull E>) and the stream single-element methods (Optional<@NonNull E>):

$ git grep @NonNull src/java.base/share/classes/java | grep -v ElementType.java
src/java.base/share/classes/java/util/Collections.java:                                                      Class<@NonNull E> type) {
src/java.base/share/classes/java/util/Collections.java:    public static <E extends @Nullable Object> Queue<E> checkedQueue(Queue<E> queue, Class<@NonNull E> type) {
src/java.base/share/classes/java/util/Collections.java:    public static <E extends @Nullable Object> Set<E> checkedSet(Set<E> s, Class<@NonNull E> type) {
src/java.base/share/classes/java/util/Collections.java:                                                    Class<@NonNull E> type) {
src/java.base/share/classes/java/util/Collections.java:                                                    Class<@NonNull E> type) {
src/java.base/share/classes/java/util/Collections.java:    public static <E extends @Nullable Object> List<E> checkedList(List<E> list, Class<@NonNull E> type) {
src/java.base/share/classes/java/util/Collections.java:                                              Class<@NonNull K> keyType,
src/java.base/share/classes/java/util/Collections.java:                                              Class<@NonNull V> valueType) {
src/java.base/share/classes/java/util/Collections.java:                                                        Class<@NonNull K> keyType,
src/java.base/share/classes/java/util/Collections.java:                                                        Class<@NonNull V> valueType) {
src/java.base/share/classes/java/util/Collections.java:                                                        Class<@NonNull K> keyType,
src/java.base/share/classes/java/util/Collections.java:                                                        Class<@NonNull V> valueType) {
src/java.base/share/classes/java/util/stream/Stream.java:    Optional<@NonNull T> reduce(BinaryOperator<T> accumulator);
src/java.base/share/classes/java/util/stream/Stream.java:    Optional<@NonNull T> min(Comparator<? super T> comparator);
src/java.base/share/classes/java/util/stream/Stream.java:    Optional<@NonNull T> max(Comparator<? super T> comparator);
src/java.base/share/classes/java/util/stream/Stream.java:    Optional<@NonNull T> findFirst();
src/java.base/share/classes/java/util/stream/Stream.java:    Optional<@NonNull T> findAny();

We may of course still be missing others, but I think we'd be hearing from our Checker Framework users if any of them were of great importance.

[cpovirk]

Was there an issue where we were considering whether Optional</*parametric*/T> should automatically conform to bounds? It still feels highly problematic to me, but do you feel like it's worth reconsidering?

If anything, I'm inclined to view the rarity as more reason not to reconsider: Apparently users won't have to reach for @NonNull T often, so I'm less inclined to save them the trouble. (And I agree that "saving them the trouble" is likely to complicate things.)

In fairness, I do expect users to hit this more than I'm suggesting here: If you're writing Kotlin, or if you're writing Java using the Checker Framework's "default defaults," then you'll get in trouble if you write a "class Foo<T>" that uses a "Class<T>." Or actually, you won't get in trouble with Class and Optional with the Checker Framework as long as you use its stubs. But you could have similar problems with ImmutableList and similar. And you can definitely see problems with Kotlin, though probably less, given the lesser usage of Class and Optional (and perhaps ImmutableList) there.

So I do expect users to be confused sometimes. Hopefully we can find ways to manage that, but "automatically conform to bounds" is not at the top of my list.

[netdpb]

Current decision: Yes, @NonNull T is a valid augmented type usage.

Argument for changing: It would be simpler not to support nullability projections. However, no strong case was made for changing the decision.

Timing: This must be decided before version 1.0 of the jar.

Proposal for 1.0: Finalize the current decision. If you agree, please add a thumbs-up emoji (👍) to this comment. If you disagree, please add a thumbs-down emoji (👎) to this comment and briefly explain your disagreement. Please only add a thumbs-down if you feel you can make a strong case why this decision will be materially worse for users or tool providers than an alternative.

Results: This proposal received six 👍s and no other votes. It is finalized. I'll edit the title to reflect that.

[netdpb]

Decision: @NonNull T is a valid non-null projection.