jspecify / jspecify

An artifact of fully-specified annotations to power static-analysis checks, beginning with nullness analysis.
http://jspecify.org
Apache License 2.0
584 stars 30 forks source link

@PolyNull #79

Open kevinb9n opened 5 years ago

kevinb9n commented 5 years ago

Informally, and approximately: a method or constructor with any occurrences of @PolyNull in its signature is treated as if it is two distinct overloads: one having all occurrences of @PolyNull replaced with @Nullable, the other having all replaced with @NotNull (under the fantasy that nullness information can actually be used for overload resolution.)

All the cases I'm aware of this being useful involve applying @PolyNull to at least one in-type (e.g parameter type) and at least one out-type (e.g. return type).

The alternative (if this annotation is not available) is that these parameters must be marked @Nullable; it will be like the previous situation but with the @NotNull overload missing. Consumers passing in a not-null expression (which should generally be common) will get a non-nullable result that the compiler will not know is non-nullable.

A bit of early discussion was in #3.


cpovirk edit: various cases to consider if we end up including @PolyNull:

amaembo commented 5 years ago

I already said it somewhere, but cannot find it now. @Polynull is a very limited thing. E.g. you cannot express "if and only if any of the two arguments is null the result is null" or "if and only if both arguments are null the result is null". Also, you cannot express other useful contracts like "if an argument is null the result is false". So I would prefer a contract language which allows expressing all of these. Otherwise, we will need to create dozens of annotations to support real-world cases.

mernst commented 5 years ago

I don't agree that @PolyNull is "very limited". Hundreds of uses are needed in the JDK, Daikon, and utilities, as just a few examples.

I agree that there exist properties that @PolyNull does not express, but that is not an argument against @PolyNull. In my experience, the examples you gave are needed much less often than @PolyNull, and @PolyNull carries its weight: it provides valuable documentation of programmer intent, and it eliminates large numbers of false positive warnings.

kevinb9n commented 5 years ago

@amaembo Your earlier comments are at the top of the issue I linked to, #3.

If we were to become convinced that @PolyNull is necessary, and that others were necessary, and could find no reasonable basis for where to draw the line, then I understand why we would be compelled to consider a more general contract language. And to me, that would be a pretty drastic outcome and I would end up feeling sad that we ever took the first steps on that journey at all.

I'm not sure whether @PolyNull is the thing that inexorably slips us onto that slope.

cpovirk commented 5 years ago

Quick data dump of very quickly grepping through:

  1. the annotations.xml files under intellij-community-master/java/jdkAnnotations for @Contract annotations
  2. the class files under checker-framework/checker/jdk/nullness/src for @PolyNull annotations (excluding toArray because it dominates the results)

Here are the results.

The first thing that jumps out to me from the IntelliJ annotations is that there's only a single occurrence of its @PolyNull equivalent, !null -> !null -- for Optional.orElse, one of the cases I'd expect. I don't see Class.cast. (Maybe it's treated as @NullnessUnspecified, which is more forgiving than straight @Nullable? I discussed using @NullnessUnspecified as a naive substitute for @PolyNull in #71.)

The first thing that jumps out to me from the Checker Framework annotations is that there aren't as many @PolyNull annotations as I expected. The full(?) list:

I do know of other cases for @PolyNull, especially for "conversion" methods:

I still don't have a strong opinion here, but this has nudged me slightly from "@PolyNull feels somewhat common" to "@PolyNull feels somewhat less common than I thought." If we're really talking primarily about a handful of methods, it's easier to imagine that users can write wrappers (castNotNull(Class, Object), etc.) when needed.

cpovirk commented 5 years ago

Also, more anecdotally, among the usages of @PolyNull in Google's codebase, I see sizable minorities of both:

Both are often signs that the API could have used a different feature instead.

Still, the majority of use cases (not a ton, but it comes up) look legitimate at first glance.

kevinb9n commented 5 years ago

Re: the previous-previous comment, I'm a little surprised, not that I expected it to be much more popular than this.

I know that other utility libraries that don't have Guava's null-hostile attitude are more fond of null-passthrough, and might have much more use for this.

What will be the consequence of not having this? I am not sure how good the castNotNull() outcome is. Will users complain to their checker vendor about it, and will vendors start to hardcode lists of APIs that have this property? Or, we could share those with each other as data files. In fact, those data files might as well be the same as externally-applied-annotation data files, meaning that a definition for "PolyNull" would have to exist somewhere but just not be part of the artifact you can actually depend on from code.

Maybe that seems like a weird outcome, but I can imagine there being quite a few things that all fall into a bucket like this, and we really wouldn't want to have 29 real annotations.

Random other point: applying @PolyNull to a type variable usage (as Class.cast() would) highlights a weird issue: that @PolyNull is important when the type argument is nullable, but I'd expect it not to have any effect when the type argument is non-nullable.

cpovirk commented 5 years ago

Hmm, I think I'm seeing it the opposite way: @PolyNull on a non-nullable T would have an effect, but on a nullable T, it's likely not to change much, since the method is likely to already permit, say, both @NotNull String and @Nullable String?

kevinb9n commented 5 years ago

For Class<@Nullable Foo>, cast already accepts both nullable and not, yes, you just need PolyNull to carry that nullness information through to the return type.

Why would it have an effect on a non-nullable T? There's no other "imaginary overload" to add; null isn't allowed. Oh: maybe you just mean you would expect to interpret @PolyNull as also implying coercion to @Nullable. That doesn't seem justified to me. How would you get it not do that, if that's what it did by default?

cpovirk commented 5 years ago

We're probably looking at multiple things differently. Here's what I'm picturing:

@DefaultNotNull
public final class Class<T [extends @NotNull Object]> {
  ...
  public @Nullable T cast(@Nullable Object obj) { ... }
}

And what @PolyNull buys us is...

  public @PolyNull T cast(@PolyNull Object obj) { ... }

...which effectively provides...

  public [@NotNull] T cast([@NotNull] Object obj) { ... }
  public @Nullable T cast(@Nullable Object obj) { ... }
kevinb9n commented 5 years ago

Right, I was instead thinking of the case where the type parameter is nullable, and thus intentionally admits both nullable and non-nullable instantiations. And that the cast method doesn't want to override that. It wants to not accept null and not return null when the type argument is non-nullable. And when the type argument nullable, that's the only place we need a refinement: it should accept nullable (or non-nullable), but it should return the same kind of nullness that was passed in.

(This might be a weird thing to do for Class itself, but if we don't then see issue #78...)

kevinb9n commented 5 years ago

So I'm saying I would not expect @PolyNull to imply coercion to @Nullable when used on a type variable. However, in the case of @PolyNull String it would weird if it did not imply @Nullable. Sigh.

cpovirk commented 5 years ago

I think I am still missing something on both this and #78. I'd suggest a code example, but I haven't gotten my head around the significance of the one on #78, so maybe that's not what I need. Hmm.

As noted above, I suspect that @PolyNull T in practice could usually be written better in another way. (I based that on looking at nearly every example in the Google codebase.) Still, it's spot-on for Class.cast and Optional.orElse. Unfortunately, I picture both of those with <T extends @NotNull Object>, so that doesn't help illuminate your nullable case.

Maybe we're mostly just agreeing on the weirdness of @PolyNull T when T permits nullable instantiations?

wmdietlGC commented 5 years ago

Instead of thinking of @PolyNull as two overloads of a method, I like thinking of it as a "qualifier parameter", that is, a type parameter that only varies over the nullness information. This makes thinking about PolyNull similar to thinking about @NullnessUnspecified.

Let's take

@PolyNull Object id(@PolyNull Object in)

that is conceptually like:

<<@T>> @T Object id(@T Object in)

where I use the <<@T>> syntax to introduce the qualifier parameter. (Which is something we don't actually support, it's just something to write these examples.) All uses of @PolyNull in a signature correspond to the same qualifier parameter and in the end bind to either @NonNull or @Nullable.

In contrast, each use of @NullnessUnspecified conceptually introduces a separate qualifier parameter:

@NullnessUnspecified Object foo(@NullnessUnspecified Object in)

corresponds to:

<<@S, @T>> @S Object foo(@T Object in)

This is similar to how each wildcard is independent of each other and captures some underlying type in a call.

Thinking about @PolyNull like this also helps in thinking about it's use on a type parameter use - it caries the nullness information separately from the underlying type and allows linking the nullness information from separate type parameters.

class Box<S extends @Nullable Object, T extends @Nullable Object> {
  S convert(T in) ...
  @PolyNull S bar(@PolyNull T in) ...
}

Method convert is nullness-parametric dependent on the instantiations for S, T. In contrast, method bar changes the nullness from S, T and instead links them together.

Maybe the correspondence between @PolyNull and @NullnessUnspecified helps us think through these examples and maybe even unify them.

cpovirk commented 5 years ago

Thanks. Among other things, that got me thinking more about @PolyNull on type parameters. Your example is like Converter.convert, and I'm sure various people have written generic functions that they want to convert null to null (like "safe calls").

One change that might make @PolyNull more widely usable is to define it to mean "@Nullable or no qualifier" rather than "@Nullable or @NotNull." (Yes, I'm back to talking about @NotNull :))

This can come in handy in two related cases:

One way to look at this definition of @PolyNull is that it provides a very limited ability to write <T super E>: I have a Container<E>, and I'd like to combine it with some values of type T, resulting in something with type lub(E, T). This is easy with a static method because the PECS principle handles it "automatically":

public static <T extends @Nullable Object> T getFirst(
    Iterable<? extends T> values, T defaultValue);

But on the occasion that we want it for an instance method, we're out of luck -- except in the specific case of adding nullness, and then only if:

(Call it @JointlyNullable? Kind of hard to ever imagine it as a full-on language feature, but I wouldn't let that stop us from adding it if we think it's useful.)

[edit: I keep getting myself confused here :( I do see that I claimed that @JointlyNullable would be useful even for a static method in the case of Iterables.find—though I'm wondering if that is one of the cases that would be doable without @JointlyNullable if only we could add a second type parameter: ResultT extends @Nullable Object, ElementT extends R or something??? Either way, is it actually the case that classic @PolyNull would be wrong there? Finally, I may want to think about the private helper method MutableClassToInstanceMap.cast, though note that I'm working to change its declaration slightly in a pending CL. edit edit: I changed its usages much later so that @PolyNull isn't necessary.]

cpovirk commented 5 years ago

Another thing that the @PolyNull-@NullnessUnspecified correspondence brings to mind: For both annotations, we've often talked about the "2 possibilities" that each permits. That's meant @Nullable and @NotNull -- or now maybe @Nullable and "no qualifier." Another way to look at it is that there are 3 possibilities: @Nullable, @NotNull, and "no qualifier."

This of course isn't useful for concrete types like String, only for type variables. And it seems more likely to matter with @NullnessUnspecified than with @PolyNull. For example:

[@DefaultNullnessUnspecified]
interface Foo<T, U extends T> {}

When this class is annotated for nullness, it could have:

So maybe we treat @NullnessUnspecified as meaning "one of those 3?"

(The idea of having 3 possibilities might(?) fall more naturally out of the <<@T>> syntax, since we're used to looking at type parameters as being selected from a variety of possible values, rather than just a boolean decision?)

amaembo commented 5 years ago

@cpovirk thank you for the research!

The first thing that jumps out to me from the IntelliJ annotations is that there's only a single occurrence of its @PolyNull equivalent, !null -> !null -- for Optional.orElse, one of the cases I'd expect. I don't see Class.cast.

For Class.cast we rely on our bytecode inference algorithm, and it infers another contract: _ -> param1 which means: we always return our first argument unless we fail. In our contract language, we assume that an exception could always be a possible outcome (after all any method call could cause StackOverflowError). This also implies PolyNull semantics and actually provides more information. E.g. consider the following code:

<T> void test(Object obj, Class<T> clazz) {
    T val = clazz.cast(obj);
    if (val != obj) { // IDEA warns as "condition is always false" thanks to cast contract
        System.out.println("Impossible");
    }
}

a few methods for "get system property or default"

Probably you are speaking about methods like Integer.getInteger(String, Integer). If yes, then they were indeed unannotated in IDEA. I fixed this. By the way, the best contract in our contract language for such a method is _, !null -> !null; null, _ -> param2 (we definitely return the second parameter if the first is null, and we definitely return a non-null if the second is not-null).

amaembo commented 5 years ago

We may emulate complex nullability rules with something more type-system like instead of contract language or PolyNull annotation. E.g.:

@NullityParameter("T") @Nullity("? extends T") orElse(@Nullity("T") other);

The @NullityParameter annotation has METHOD target and declares a nullity type parameter which is either nullable, unspecified, or notnull; nullable > unspecified > notnull. The @Nullity is a type-use annotation that allows specifying a nullity for a given type. So here we say that the result type nullity is not less specific than the argument nullity. Previously suggested annotations like @NotNull are just shortcuts for @Nullity("notnull").

Something more complex is also possible:

@NullityParameter({"VAL", "DEF"}) @Nullity("? extends VAL & DEF") String coalesce(@Nullity("VAL") String value, @Nullity("DEF") String defaultValue);
cpovirk commented 5 years ago

Thanks! I had seen some inferred annotations before but didn't realize it was happening even for classes that aren't being built as "part of the project." (For my reference: announcement of bytecode inference, documentation of @Contract, Kotlin's contracts (via https://github.com/google/codeanalysis-annotations/issues/29#issuecomment-502483411).)

It might someday be interesting to know which methods have an inferred contract like @PolyNull (or @EnsuresNotNull, etc., like in #29) That could help us judge the utility of each individual annotation vs. the utility of more generic approaches vs. doing nothing (at least in the core annotations artifact). I don't think there's any urgency for that, though.

cpovirk commented 5 years ago

To state the obvious (which nevertheless escaped me until now): We also see "qualifier parameter"-like behavior from straight-up parametric nullness:

class Foo<T extends @Nullable Object> {
  T foo(T in) { ... }
}

It's reassuring to think that this, @PolyNull, and @NullnessUnspecified are all fundamentally similar.

The main difference may be: Does an operation need to be valid for...

And then there's a question about whether @NullnessUnspecified creates a new type every time it appears, unlike @PolyNull and parametric nullness.

[edit: I think I later saw that this came up somewhere already. Sorry for forgetting about that.]

kevinb9n commented 4 years ago

I will just state that I view ideas like the @Nullity examples shown a few comments back as being pretty far outside the scope of what we should do in this project. The power is high, but the complexity is high and the applicability is low. Even just @PolyNull itself I would only want to accept if we can find a way to make it simple and easily understood.

cpovirk commented 4 years ago

~Another Guava API that would benefit from this: closer.register(closeable). Thankfully, there is no reason to use that API if you can use try-with-resources. And if you do need to call it, you can do so by calling it as a separate statement, rather than as foo = closer.register(open()).~

edit: No, it doesn't need it.

cpovirk commented 4 years ago

Belatedly following up on mernst's comment:

Hits from Daikon:

The first is the classic "null passthrough." Its callers are already null-checking the result, so they could null-check the parameter instead. (The parameter comes from a field, though, so, if they're using -AconcurrentSemantics, they'd have to read it to a local variable.)

The other two are more interesting. In those files, I see:

First, concatenation of arrays. When I saw that, I was going to say:

An alternative approach to concatenating arrays is to use a type parameter:

<T> T[] concat(T[] a, T[] b);

That is probably a simpler signature, anyway (compared to using @PolyNull 3 times). The downside is that it's an API change, but it would be fairly unlikely to break callers. (The main case I can think of is if the callers are assuming that they get an Object[] back. After the change, they might get a Foo[] back. Occasionally this kind of change can interfere with type inference.)

But it's actually not that simple. The method needs a Class<T> so that it can generate an array of the proper type: If you pass a String[] and an Integer[], it needs to know whether to create a Comparable[], Serializable[], or Object[].

Guava's ObjectArrays.concat went the way of requiring a Class. (Aside: This investigation just made me realize that the other overloads of that method have a bug!) But if callers don't actually need more than an Object[], then the Daikon signature is superior.

Second, there are some more complex null-passthrough-like cases. For example, slice(nullArray, start, end) returns null. I didn't look into callers for these. (Mainly, I was lazy: The files get run through the C preprocessor, and I didn't want to bite that off right now.)

Hits from plume-util:

The first file is the classic "null passthrough." (As the name suggests, it's for interning. In Guava's Interner, in contrast, we reject null.)

The other 2 files are arrays. Arrays are weird in general. My guess is that all those files would actually work fine if we replaced @PolyNull Foo[] with @Nullable Foo[] -- unless maybe plume-util (or its users) is using -AinvariantArrays?

cpovirk commented 4 years ago

Also: I think Map.merge may be another case that would benefit ~not from CF-style @PolyNull but from the "@Nullable or no qualifier" style that I discussed above~. [edit: Hmm, I think I have that backward: CF-style @PolyNull may be needed there after all, sorry. I may have misinterpreted "returning the current value or null if absent."]

Finally: I forget if I've mentioned this explicitly before or not, but I suspect that a @Contract-like feature would probably:

kevinb9n commented 4 years ago

It seems very likely to me now that we will ship a version of our product without @PolyNull (but make sure CF is able to layer it on sanely).

cpovirk commented 4 years ago

a few methods for "get system property or default" (a pattern that generalizes beyond system properties, of course)

I just dug up a handful more of these in Google's internal codebase (though not very many) by searching for methods that accept a Class<T> and a T and also return a T. (A discovery that I found interesting is that I had a hard time finding such methods by actually searching for @PolyNull. Of course part of the reason for that is that only a fraction of our codebase uses the Checker Framework and its annotations.) Most (all?) of the methods I found weren't actually in CF-checked code, but I think they could have been made to pass CF checking even without @PolyNull, thanks to CF's willingness for Class<T> to be instantiated with either a nullable type or a non-null type. That is, the code looked like this...

<T [extends @Nullable Object]> T getOrDefault(String key, Class<T> clazz, T default)

...rather than this...

<T extends [@NonNull] Object> @PolyNull T getOrDefault(String key, Class<T> clazz, @PolyNull T default)

(Googlers can see the list I put together in a doc titled "google3 users of Class<T> who would benefit from the CF behavior of permitting both non-null and nullable T.")

Again, though, this came up rarely.

(I will post about this on #86, too.)

cpovirk commented 4 years ago

I see that NullAway has partial support for IntelliJ's @Contract annotation.

cpovirk commented 4 years ago

I was just talking on #32 about using an annotation element on @Nullable like @Nullable(unspecified = true). We could consider doing the same for @PolyNull.

To be concrete, let's look at Class.cast. Here's a version with @PolyNull:

class Class<T> {
  @PolyNull T cast(@PolyNull Object o);
}

Maybe we'd want to do that with an element on @Nullable instead:

class Class<T> {
  @Nullable(onlyJointly = true) T cast(@Nullable(onlyJointly = true) Object o);
}

Aside: If a tool doesn't understand onlyJointly (perhaps because it wasn't present in jspecify 1.0), then this approach degrades somewhat gracefully. That's kind of nice.

Another nice feature is that this approach somewhat "hides" @PolyNull, just as I've talked about "hiding" @NullnessUnspecified.

I'm about to post more about the idea of annotation elements in general on #32 [edit: done].

kevinb9n commented 4 years ago

Solving this might be a little more important than I was first thinking, because it should mean that Map.getOrDefault can become an actually useful remedy for the problems with Map.get.

kevin1e100 commented 4 years ago

(I will admit rarely using getOrDefault to solve such problems. In my own experience the primary "solution" is to iterate a map's entries() instead of keys(), thereby avoiding map.get() in the loop.)

cpovirk commented 4 years ago

I have a similar reaction to @kevin1e100 . Even in the non-iteration case, I'm not sure how often getOrDefault is helpful.

Yes, sometimes I can call map.get and treat an absent key just like it was present with a default value. For example, if my Map<String, List<Object>> is missing a key, that's probably much like mapping it to an empty list.

But often what I want is to do something different entirely: Maybe it's to throw an exception, to look up the value elsewhere, etc.

In that case, I can switch to getOrDefault with a dummy value. But I still need an if statement, so I'm not much better off. Arguably I'm actually worse off, since my null-checking tool won't force me to handle the "not present" case anymore, so I might accidentally pass the dummy value through many layers of my program before it's detected.

(Not to mention that I can't even employ this approach in the first place unless there's some distinguished value I can pass as the default, knowing that it will never appear as a true value.)

Now, all this is just my memory of anecdotal experience. If we want better data, we could do some searches over our codebase.

cpovirk commented 4 years ago

Additionally: To support the cases in which getOrDefault is helpful, we can consider the other sound way of annotating the method:

V getOrDefault(@Nullable Object key, V defaultValue)

(This is roughly what Checker Framework does.)

Of course, the signature forbids some valid calls. So this leads us back to larger questions about the "right" way to annotate APIs. My current view is that, just as there's not always one definitively "right" way to write a plain Java API today, there's not always one definitively "right" way to add nullness annotations, either. It can be a trade-off, and I think both possible getOrDefault signatures deserve some consideration.

cpovirk commented 4 years ago

there's not always one definitively "right" way to write a plain Java API today

We can actually take methods like Collection.contains as an example of this: The API is contains(Object), and there are reasonable arguments for that, but other languages get by with contains(E), which prevents a class of bugs.

That said, when we make such decisions for nullness in particular, we always want to keep in mind that Kotlin might outright prevent passing a nullable value for a non-nullable parameter. But that is probably tolerable for getOrDefault(..., nullableDefaultValue), since users can still call get as needed. (And creative users can do something like Collections.<Foo, @Nullable Bar>unmodifiableMap(map).getOrDefault(foo, nullableBar) :))

[edit: oops, now on a tangent of a tangent, so I encourage people to either open a new issue about the tangents or do a better job than I did at staying focused here]

kevin1e100 commented 4 years ago

Interesting point that being able to do getOrDefault(key, null) isn't super-valuable, since that's the same as get(key) only more expensive.

kevinb9n commented 3 years ago

Note that as we struggle to make Google's massive internal codebase more refactorable and evolvable, we are tackling more and more "type migrations" where one type (say, Date) must be replaced with another (say, Instant). In doing this we have to spend a lot of time in an intermediate state where conversion methods (forward or backward) are being called all over the place.

It is just terrifically convenient to keep these conversion methods orthogonal to nullness; i.e., always convert null to null and non-null to non-null. So they're @PolyNull methods. If we don't have @PolyNull I guess we would have no choice but to create two differently-named conversion methods in each direction for any type migration.

At the moment I don't see how long we can be happy without @PolyNull.

kevinb9n commented 3 years ago

This is just one opinion (from the internets), but users calling something their "biggest pain point" (after converting 400 KLOC!) is significant even in small numbers.

"The biggest pain point with Eclipse NP analysis is its lack of PolyNull."

https://www.reddit.com/r/java/comments/obscjo/handling_null_and_upgrading_past_java_8_inside/h3pso50/?utm_source=reddit&utm_medium=web2x&context=3

kevinb9n commented 3 years ago

Collecting up thoughts on all the above.

cpovirk commented 3 years ago

I agree that a Java nullness checker needs to contain @PolyNull or similar functionality (at least for a small number of hardcoded methods) to be practically usable at large scale. While I feel OK having checkers hardcode methods in the short term, I expect that that will mostly just give us time to:

As for naming: My only fear with the name "PolyNull" is that people may reasonably assume that it has the behavior of the existing Checker Framework @PolyNull. (The naming precedent runs even deeper than that, extending beyond nullness and into the Checker Framework's other type systems, as in @PolyPresent.) It would be particularly sad for Checker Framework users to see subtly different behavior depending on which @PolyNull they had imported.

(As for @Nullable(jointly = true), I am hoping that I brought that up largely for completeness after a comment about boolean parameters in #32 :) I'm at least not a fan now -- and possibly not a fan of annotation parameters in general.)

And for the no-@PolyNull world, my thinking has matched your thoughts above:

("Treat it like @Nullable" may lead to some unfortunate behavior in edge cases: If you implement a method with @PolyNull in its signature, then a "treat it like @Nullable" tool will let you return null even if the input wasn't null. But it seems clear that that's preferable to assuming that the method never accepts or returns null. And those are our only real options, barring abuse of unspecified nullness.)

kevinb9n commented 3 years ago

(to the last bit, a reminder we have explained in our requirements doc that the needs of API consumers have precedence over those of implementors in case of conflict, so 👍🏼 )

cpovirk commented 3 years ago

True. There are probably ways for consumers to suffer, too, like having to suppress when calling this contrived method:

void rebalance(List<@PolyNull String> left, List<@PolyNull String> right);

But again, that's contrived. And it's a false positive rather than a false negative. (Maybe there's a way to produce a false negative, but I haven't thought about it much.)

Anyway, this may be another case in which it's hard to see a true conflict between consumers and implementors. I think our principle of "If you want nulls to pass through, then it needs to be @Nullable" continues to serve both use cases well.

cpovirk commented 3 years ago

Trying to categorize use cases into "more compelling" and "less compelling"

More compelling:

Less compelling:

My very rough sense from a quick skim of usages in Google's codebase is that ~2/3 fall into the "more compelling" bucket (mostly "conversions") and ~1/3 fall into the "less compelling" bucket (mostly List<@PolyNull Foo>). I would need to look more to say with confidence, but that should give a general sense.

cpovirk commented 3 years ago

I have a few other things I've been considering saying here, but I'll limit myself to 2 :)

  1. KT-8889 is the Kotlin feature request for "if a given function parameter is not null, the result is not null." It links to issues about @Contract, which I have suggested is not as flexible as @PolyNull could be.

  2. One interesting feature of JSpecify is "unspecified nullness." Above, we've been talking about treating @PolyNull as "@Nullable or not." We would eventually want to think more about whether the "right" treatment is "@Nullable, unspecified nullness, or neither." If we had only "@Nullable or not," then lenient tools would probably consider a call to silently convert an unspecified-nullness input to a non-null output, and strict tools would probably consider a call to silently convert an unspecified-nullness input to a nullable output. Maybe that's tolerable?

kevinb9n commented 3 years ago

Your answers to my last couple July 2 questions seem encouraging to me: this seems to suggest that there can be a reasonably well-behaved JSpecify without a @PolyNull annotation, and either a future version of JSpecify or just a checker-specific artifact could provide a @PolyNull that has @Implies(Nullable.class). Then APIs that want to adopt that can replace some usages of @Nullable with it, and tools that don't recognize it will simply follow the "implies" link.

If this is correct, it's a win, I think.

cpovirk commented 3 years ago

214 raises the idea of @Nullable("s"). I have three thoughts:

First: That could be used to support "unrelated" pairs of @PolyNull types in the same signature. For example, here's a real-world edge case from Google's codebase:

class BiOptional<A, B> {
  Pair<@PolyNull A, @PolyNull B> orElse(@PolyNull A a, @PolyNull B b);
}

In that example, it would be nice to be able to replace the 4 usages of @PolyNull with 2 usages of @Nullable("a") and 2 usages of @Nullable("b").

Second: We should think about how this interacts with aliasing/implies. As we've noted in other discussions, we're not sure if there will be a good way to say that one annotation implies another when annotation elements (like "a" here) are involved.

Third: I worry that @Nullable("a") alone won't make clear to users what the "a" means. Maybe this is solvable by picking a name for the attribute other than the default name "value," thereby forcing users to write @Nullable(condition = "a") or similar -- though of course that is longer.

kevinb9n commented 3 years ago

214 raises the idea of @Nullable("s").

With two seconds' thought, it sounds like we have a pretty smooth ramp toward that, i.e. if we released a @PolyNull without that we could still add it later. If that sounds right, then to the extent we want to discuss that parameter idea, let's make it a separate issue.

cpovirk commented 3 years ago

Here's an example where we'd put @PolyNull on zillions of APIs -- far more than I'd seen during my first look:

For map fields, the protobuf compiler generates a method like this:

Foo getFooOrDefault(Key key, Foo defaultValue);

And default values are one of the canonical use cases, so I'd want it to generate:

@PolyNull Foo getFooOrDefault(Key key, @PolyNull Foo defaultValue);

Users do in fact pass null as a default fairly commonly, so I wouldn't want to leave the type as non-null. But they more frequently pass non-null values, so a @Nullable return type will get annoying after a while.

cpovirk commented 2 years ago

protobuf followup: I am a little surprised to see very few errors in our codebase if I just use @Nullable instead of @PolyNull. That said, I had found that we would see a lot more if we adopted nullness checking more widely.

So I would still expect the average user to get by tolerably well with a checker that just implements special cases for APIs like Class.cast. But I also still expect for enough users to need this annotation for us to need to provide it eventually.

ben-manes commented 2 years ago

A usage in Caffeine is,

@PolyNull V get(K key, Function<? super K, ? extends @PolyNull V> mappingFunction);

This corresponds to Map.computeIfAbsent, which internally it delegates to. If the value is not computable, such as a login email being absent in the database (e.g. a typo) then the result is null.

This differs from Guava's cache which instead throws an exception. This was a difficult decision on being null hostile vs real data being absent. While I prefer the null approach, that was settled by the addition of compute methods. Given the Guava's philosophy of being a natural extension to the Collections Framework, Caffeine shifted to that newer style.

A quirk is LoadingCache.get(key) which can return null since the CacheLoader.load(key) may. There is no way to encode that in an understandable fashion, so initially it was marked as @Nullable for correctness and documentation. As tooling has improved that annoyed Kotlin users, so we dropped it to be effectively @NonNull per the majority of usages.

AsyncCache doesn't have these problems because it computes a CompletableFuture<V>. That future being null is a programming bug, but it could resolve to a null value. If so, then the future is considered to have failed (though not exceptionally) and failed future are removed from the cache.

It wouldn't be awful if Cache.get(key, mappingFunction) similarly feigned a non-null return value, even if not correct. The vast majority of usages don't take advantage of this property and would instead surface an application exception, e.g. a JAX-RS BadRequestException in the failed login example. That wouldn't be the case for Map computations like compute and computeIfPresent where a null return is often the logics intent (e.g. to synchronously notify a listener on removal).

cpovirk commented 2 years ago

I just came across another pattern of @PolyNull usages that turns out not to be necessary. I edited it into my list above, but to repeat it here:

I saw some methods along the lines of foo(@PolyNull String string, @PolyNull Listener listener). The intent was presumably to require that both values be present or both values be absent. But in fact the methods permit any combination of those, since, e.g., a non-null string and null listener fulfills @Nullable String and @Nullable Listener. These declarations might as well just be foo(@Nullable String string, @Nullable Listener listener).

kevinb9n commented 2 years ago

I believe @PolyNull has to appear at least once on an in-type and once on an out-type to make sense.

cpovirk commented 1 year ago

2. Above, we've been talking about treating @PolyNull as "@Nullable or not." We would eventually want to think more about whether the "right" treatment is "@Nullable, unspecified nullness, or neither."

For some reason, I was thinking of this again recently. I'll dump my dense notes here for possible future cleanup:

I was tying this question to https://github.com/jspecify/jspecify/issues/69: If I have a type-variable usage T NO_CHANGE with the type parameter T declared with a bound of Foo UNSPECIFIED, then how do I know that class.cast(t) should return UNSPECIFIED? I can't just ask "Is T NO_CHANGE a subtype of Object UNSPECIFIED?" because:

How do we "let in" T NO_CHANGE but not T UNION_NULL? We may need an additional concept of "T NO_CHANGE as Object ___," which is a little like substitution (but doesn't involve type arguments) and a little like subtyping (but passes through UNSPECIFIED).

(This may be important for getting the types right at all. That may make it more important than the rest of #69, which is about avoiding some "But why?" warnings or errors.)