Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@PolyNull #79

Open
kevinb9n opened this issue Oct 31, 2019 · 61 comments
Open

@PolyNull #79

kevinb9n opened this issue Oct 31, 2019 · 61 comments
Labels
design An issue that is resolved by making a decision, about whether and how something should work. nullness For issues specific to nullness analysis.
Milestone

Comments

@kevinb9n
Copy link
Collaborator

kevinb9n commented Oct 31, 2019

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
Copy link
Collaborator

amaembo commented Nov 1, 2019

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
Copy link
Contributor

mernst commented Nov 1, 2019

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
Copy link
Collaborator Author

kevinb9n commented Nov 1, 2019

@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
Copy link
Collaborator

cpovirk commented Nov 1, 2019

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:

  • Optional.orElse
  • Class.cast
  • various array things (which I don't want to fully discount, but arrays are weird)
  • a few methods for "get system property or default" (a pattern that generalizes beyond system properties, of course)
  • some ConcurrentHashMap methods

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
Copy link
Collaborator

cpovirk commented Nov 1, 2019

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

  • methods that use @PolyNull only once in the signature
  • APIs that use @PolyNull T (that is, that use it on a type variable)

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
Copy link
Collaborator Author

kevinb9n commented Nov 1, 2019

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
Copy link
Collaborator

cpovirk commented Nov 1, 2019

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
Copy link
Collaborator Author

kevinb9n commented Nov 1, 2019

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
Copy link
Collaborator

cpovirk commented Nov 1, 2019

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
Copy link
Collaborator Author

kevinb9n commented Nov 1, 2019

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
Copy link
Collaborator Author

kevinb9n commented Nov 1, 2019

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
Copy link
Collaborator

cpovirk commented Nov 1, 2019

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
Copy link
Collaborator

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
Copy link
Collaborator

cpovirk commented Nov 4, 2019

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:

  • "return a value from this container or a user-specified default":
    • @PolyNull V getOrDefault(Object key, @PolyNull V default)
      (This turns out not to be very compelling for Map.getOrDefault, since users who want to pass null can call plain Map.get. But we could see benefits for other, similar APIs. [edit: Android came to a similar conclusion.] [edit 2: I did come across a large number of calls to getOrDefault(key, null) in Google's depot.])
    • (One near-miss example is Guava's ValueGraph.edgeValueOrDefault. @PolyNull is a good fit there regardless of which of the two definitions we use, as ValueGraph forbids null values.)
    • CompletableFuture.exceptionally + FluentFuture.catching
      If @PolyNull means "@Nullable or no qualifier," then we can apply it to these methods so that users can say both:
      • "If the input Future fails, return a Future whose value is null" (producing a Future<@Nullable V>)
      • "If the input Future fails, return a Future whose value is <some V>" (producing a Future<V>, where V retains its original nullness)
  • "return a container that contains the values from this container plus some others":
    • FluentIterable.append
      This more easily permits combining an Iterable of not-null elements with an Iterableof nullable elements. I doubt that this comes up a ton, though.

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:

  • we know that the container type can never hold null, or
  • we tweak the definition of @PolyNullas described above

(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
Copy link
Collaborator

cpovirk commented Nov 4, 2019

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:

  • U extends @NotNull T
  • U extends @Nullable T
  • U extends T

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
Copy link
Collaborator

amaembo commented Nov 5, 2019

@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
Copy link
Collaborator

amaembo commented Nov 5, 2019

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
Copy link
Collaborator

cpovirk commented Nov 5, 2019

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 #29 (comment)).)

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
Copy link
Collaborator

cpovirk commented Nov 8, 2019

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...

  • all possible nullnesses (in the case of parametric nullness, or in the case of @NullnessUnspecified in strict mode)
  • some possible nullness (in the case of @NullnessUnspecified in lenient mode, or in the case of @PolyNull)

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 kevinb9n added requirements nullness For issues specific to nullness analysis. labels Dec 5, 2019
@kevinb9n
Copy link
Collaborator Author

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
Copy link
Collaborator

cpovirk commented Jan 10, 2020

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
Copy link
Collaborator

cpovirk commented Jan 13, 2020

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
Copy link
Collaborator

cpovirk commented Jan 13, 2020

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:

  • not support "@Nullable or no qualifier"
  • work only on root types

@kevinb9n
Copy link
Collaborator Author

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
Copy link
Collaborator

cpovirk commented Aug 12, 2021

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

More compelling:

  • conversions: @PolyNull Foo convert(@PolyNull Bar bar)
  • default values: @PolyNull Foo getOrDefault(K key, @PolyNull Foo defaultValue)

Less compelling:

  • List<@PolyNull Foo> can typically be List<? extends @Nullable Foo> (which also offers more flexibility to people who don't even use nullness checking).
  • <T> @PolyNull T passthrough(@PolyNull T value) can be <T extends @Nullable Object> T passthrough(T value).
  • A method that uses @PolyNull only once typically doesn't need it (though there are occasional exceptions).
  • [edit: 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 it permits any combination of those, since, e.g., non-null string and null listener fulfills @Nullable String and @Nullable Listener.]

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
Copy link
Collaborator

cpovirk commented Aug 12, 2021

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
Copy link
Collaborator Author

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.

@kevinb9n kevinb9n added this to the Post-1.0 milestone Aug 18, 2021
@cpovirk
Copy link
Collaborator

cpovirk commented Sep 22, 2021

#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
Copy link
Collaborator Author

#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
Copy link
Collaborator

cpovirk commented Oct 11, 2021

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
Copy link
Collaborator

cpovirk commented Jan 4, 2022

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
Copy link

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
Copy link
Collaborator

cpovirk commented May 13, 2022

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
Copy link
Collaborator Author

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

@kevinb9n kevinb9n added design An issue that is resolved by making a decision, about whether and how something should work. and removed requirements labels Nov 30, 2022
@cpovirk
Copy link
Collaborator

cpovirk commented May 22, 2023

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 #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:

  • If I require that subtyping to hold in only some world, then everything is a subtype of Object UNSPECIFIED, even types like T UNION_NULL that we don't want to cover here.
  • If I require that subtyping to hold in all worlds, then the check will fail for T NO_CHANGE.

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.)

@cpovirk
Copy link
Collaborator

cpovirk commented Jun 12, 2023

One thing mentioned earlier is the question of whether @PolyNull-like functionality would be useful if it meant "@Nullable or @NonNull" or if it meant "@Nullable or @NullMarked with no annotation". For most usages (e.g., Class.cast), the two are equivalent. But the link in the first sentence shows cases in which the latter could be nicer, and another post shows a case in which the former could be nicer (specifically, Map.merge—and in fact other Map methods, too, though in practice either version is probably fine, given that most maps don't contain null values and that we might get away with just using parametric nullness).

(That's all mostly just to tie together a few existing posts. The main new part is that there are other Map methods like Map.merge (but still also, confusingly, Map.getOrDefault, which is not like merge and thus would benefit from the other version...).)

@cpovirk
Copy link
Collaborator

cpovirk commented Jun 20, 2023

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).

Actually, an internal discussion with kevin1e100 just touched on the idea that we might need that for casts, too: If you have an expression of type T ___ and you cast it to Foo (or vice versa?), we might also need to know that? I'm not sure, and I don't have time to think about it today :)

[edit: This might be especially true if you assign the result of the cast to a var variable or do something else type-inference-y with it.]

@cpovirk
Copy link
Collaborator

cpovirk commented Aug 1, 2023

One thing that we've discussed but seemingly not written about yet here is how Kotlin reduces the need for @PolyNull, at least if you design APIs with Kotlin in mind.

?.let

In Kotlin, developers are less likely to define @PolyNull-style APIs and more likely to define APIs that accept and return non-nullable types. Then, callers who know that they have a non-nullable type can call the method directly (foo.bar()), and callers who know that they have a nullable type can call the method with ?.let (foo?.let { it.bar() }). The compiler ensures that you don't call the method directly on a nullable type (but it does let you call it on a platform type, which can be unfortunate!), yet it ensures that the return type of the ?.let approach is a nullable type.

extension functions

In at least the specific case of Optional.orElse, Kotlin provides (thanks to some people :)) an extension function (getOrDefault) that provides @PolyNull-like behavior. I think the reason that this works for Optional.orElse but doesn't work for Class.cast or Converter.convert is that the input type and output type are the same type T. (It might also be important that Optional has its type parameter declared as non-nullable under our approach. That's the case for Class and Converter, too, so it's not part of the problem for those classes' methods; it just might be part of the problem for others.)

other stuff?

I don't know, probably? You could reasonably claim that the various nullness-handling operators (like ?:) would make it somewhat practical for a Kotlin-focused Optional type to choose not to provide orElse/getOrDefault functionality at all: If it instead provided orNull, users could combine that with ?: to get the other methods' behavior.

@cpovirk
Copy link
Collaborator

cpovirk commented Aug 1, 2023

(The point here isn't that JSpecify shouldn't continue to be open to adding @PolyNull, just that we are seeing Optional.getOrDefault save us from some Kotlin ugliness that we would otherwise been stuck with until Kotlin supported something like @PolyNull.)

@agentgt
Copy link

agentgt commented Aug 1, 2023

I despise PolyNull. Some probably have seen me on reddit openly rant about it so apologies if you are seeing me again.

One of the reasons why we (my company at least) see way less NPE is because PolyNull APIs are nowhere near as wide spread as they used to be. I understand folks are going to complain about PolyNull and with the exception of Optional.orElse I see much bigger problems like monotonic and someStream.filter(p -> p != null)... eg.. why is the stream still nullable for example.

The problem with PolyNull in the context of the JDK is that it seems very easy to assume something is PolyNull when it very well might not be in the future. After all the JDK is not annotated.

For example most assume Map.getOrDefault is poly null but until the JDK authors actually go off and annotate precisely what Map.getOrDefault is we are still assuming.

In fact I think making @PolyNull always @Nullable might actually encourage JDK authors to address or add expression based conditionals (I believe that another reason why it is less of a problem with Kotlin):

Map<String, SomeNonNull> m =...
// yes this is painful but it is safer
SomeNonNull n = requireNonNullElse(m.get("a"), other);

I admit that Caffeine has a strong argument for PolyNull but JSpecify really needs to be like done now and it needs to be simple for adoption and most importantly easier to annotate existing APIs. Having PolyNull begs the question should this ancient method here be PolyNull and naturally people can easily just look and see if (x == null) return null;... ah I will just annotate this PolyNull. You see the danger? @Nullable is safer. Yes it is inconvenient but is safer and that brings me to my next point:

Without the JDK correctly safely annotated by JSpecify OOB I fear JSpecify will have the same adoption as Eclipse null analysis. Its pretty sad how few know about JDT null analysis. It's equally sad how freaking painful it is to setup because of lack of OOB annotations on the bare minimum: the JDK. (checker is also not exactly any easier to be honest).

I don't know what the plan is for external annotation but I feel adding more annotations is going to make this harder, longer and possibly error prone...

@agentgt
Copy link

agentgt commented Aug 1, 2023

BTW speaking of Caffeine

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.

It is funny you mention that because this confused me when going to see if we were using Caffeines getOrDefault.

	private static final LoadingCache<CacheKey, ParsedSql> cache = Caffeine.newBuilder()
		.maximumSize(100)
		.<CacheKey, ParsedSql>build(new CacheLoader<CacheKey, ParsedSql>() {
			@Override
			public @NonNull ParsedSql load(
					CacheKey key)
					throws Exception {
				return _parseSql(key.sql, key.config);
			}

		});

	private static ParsedSql _parseSqlCache(
			String sql,
			SqlPlaceholderParserConfig config) {
		return requireNonNull(cache.get(new CacheKey(config, sql)));
	}

If I don't have that requireNonNull Eclipse will complain with:

Unsafe interpretation of method return type as '@NonNull' based on the receiver type '@NonNull LoadingCache<@NonNull CacheKey,@NonNull ParsedSql>'. Type 'LoadingCache<K,V>' doesn't seem to be designed with null type annotations in mind

And it does not because Eclipse still cannot read Checkers default annotation placement:

@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.FIELD)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.PARAMETER)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.RETURN)
package com.github.benmanes.caffeine.cache;

But originally I did not read that error or it indeed changed on version upgrade as Ben alluded to from @Nullable to Eclipses unspecified. I just assumed cache.get was always nullable.

The reality is I just haven't gone and externally annotated (EEA) Caffeines type parameters yet.

Anyway I thought that might be just another example of how much the pain of going around annotating existing libraries is and how potentially error prone it can be.

@ben-manes
Copy link

@agentgt fyi ben-manes/caffeine#594 in v3.0.5. It seemed reasonable to follow Guava's lead which adopted CheckerFramework's annotations as a replacement to jsr305's, where I had no strong opinion and prefered having those who want to discuss it to come to the like minded Guava/JSpecify folks than to me. 🙂

@agentgt
Copy link

agentgt commented Aug 1, 2023

@ben-manes Yes I agree. I didn't mean to make it out as bad choice because I think the first time around when I did it I was annoyed probably similar to the kotlin devs about LoadingCache.get being nullable.

It was a safe choice and did not break anything on our end. My jaded tone was more for the libraries that either do not provide any sort of annotations or incorrectly annotated... (well that and Eclipse but Eclipse kind did ok here).

@cpovirk
Copy link
Collaborator

cpovirk commented Aug 22, 2023

Sorry, I'm ignoring the posts above for the moment. I haven't caught up yet, but I wanted to dump an unrelated thought.

I'd forgotten another way that Kotlin can get @PolyNull-like behavior: Kotlin lets users declare overloads of a function that differ only in nullness. So you can have fun cast(o: Object): T and fun cast(o: Object?): T?, at least when you're writing Kotlin.

I should look more at what happens if you write a Java API with one set of nullness annotations in a superclass (and/or interface?) and then a Java override with the other set.... I had thought that Kotlin responded by interpreting the API as having platform types, but I thought I might have seen otherwise in some cases. I'd need to look. (Might it matter whether the return type matches?) And even if it does "work," I would be hesitant to rely on what might be a bug, given that the platform-type behavior seemed to be the intended one.

@cpovirk
Copy link
Collaborator

cpovirk commented Dec 8, 2023

I also want to dump a link to a Guava discussion in which @PolyNull could have been part of the answer but in which I concluded that @NonNull would suffice (though tools' type inference might be confused either way): google/guava#6824 (comment)

@cpovirk
Copy link
Collaborator

cpovirk commented Apr 18, 2024

Apparently C# took the approach that you can apply their equivalent of @PolyNull only to a pair of types that are (a) the return type and (b) one parameter type: https://devblogs.microsoft.com/dotnet/try-out-nullable-reference-types/#nullness-dependence-between-inputs-and-outputs-notnullifnotnullstring

That's a nice simplification if you can get away with it. (I do wonder if they could have let you annotate the parameter, instead, so that you wouldn't have to refer to the parameter name with a string?) It definitely does solve the common case of null passthrough, but I was surprised at first glance at how many of the @PolyNull usages I looked at were more involved than that. (Compare my comment about supporting only root types with @Contract.) It remains an option, one that certainly covers lots of calls even if it covers a smaller number of declarations. (For example, covering Class.cast would get us a lot of mileage.)

(Also, more generally: C# had to do a lot of stuff to support its ref and perhaps out parameters, plus more for its representation of nullable primitives with Nullable<int>, etc. It is convenient for us that Java has avoided those particular things.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design An issue that is resolved by making a decision, about whether and how something should work. nullness For issues specific to nullness analysis.
Projects
None yet
Development

No branches or pull requests

8 participants