Annotate InvocationHandler for nullness.

[cpovirk]

No description provided.

[cpovirk]

Maybe this deserves a @CFComment similar to the ones in Method.invoke and Field?

Also, if this is merged, InvocationHandler will become another class that could be covered by a standard astub file for unsound treatment of reflection (typetools/checker-framework#3258).

[mernst]

This change looks wrong to me. @Nullable on a formal parameter means "null is always a legal argument". (This is something that we should change in the future, such as by adding a @NullOk that means that and changing @Nullable to mean "there exists at least one invocation for which null is a legal argument.)

That's why Method.invoke isn't annotated with @Nullable. I think InvocationHandler.invoke should be consistent with Method.invoke.

Adding a @CfComment here, as you suggest, would definitely be useful. (And if I am confused, it would clarify my thinking.)

[cpovirk]

Ah, thanks, I was looking at this differently. I'll lay out why I was looking at it that way, and based on what you think, I can change the signature and add a @CfComment.

I was thinking less about the meaning of the annotations and more about the effect on the Checker Framework user. (This is probably not a promising start to my explanation ;)) And I was doing that in the light of the fact that InvocationHandler.invoke is a method that users implement, rather than a method that they call. This is the opposite of Method.invoke. Admittedly, "implemented, not called" is not a distinction that is enforced in any way. And I even see a surprising number of calls to the method in the Google codebase (though typically just to delegate from one InvocationHandler to another; otherwise, to test an InvocationHandler (which may not be the best way to do that, but setting that aside)).

Based on that, my thinking is: Users who implement invoke will receive a null args array when callers invoke no-arg methods, like toString(). To protect those users from NullPointerException, the Checker Framework could issue an error if they try to dereference args without a null check. The way for us to make that happen is to annotate the parameter with @Nullable.

For what it's worth, I managed to dig up one report of a prod bug in an Android app as a result of a missing null check. (Bonus: The bug was initially hidden because Android didn't originally pass null as the contract requires :)) I assume that most people are lucky enough to first hit it during development, before deploying to prod.

Let me know what you think.

[cpovirk]

Building on those thoughts:

I wondered if there were other APIs in the JDK like this -- that is, "callback" APIs in which the user is expected to implement a method, and the user may need to check a parameter for null in some cases. I couldn't think of much offhand. The one that came to mind was CompletionStage.whenComplete (inherited by CompletableFuture) and the similar handle method:

CompletionStage<T> whenComplete​(BiConsumer<? super T,​ ? super Throwable> action)

When this stage is complete, the given action is invoked with the result (or null if none) and the exception (or null if none) of this stage as arguments.

Using my "effect on the Checker Framework user" model, I would want to encourage the user to null-check the values before dereferencing them, so I would write:

CompletionStage<T> whenComplete​(BiConsumer<? super @Nullable T,​ ? super @Nullable Throwable> action)

But as with InvocationHandler, my @Nullable here wouldn't really mean "null is always a legal argument." For example, it wouldn't be legal for CompletionStage<@NonNull String>.whenComplete to invoke its callback with the arguments (null, null): At least one of the arguments must be non-null. The "null is always a legal argument" model says that neither type should be marked @Nullable.

[cpovirk]

The other part of this that I found interesting: Does @Nullable on a return type mean "null is always a legal return value?" That doesn't seem to be how it's used currently. (I know you've referred in the past to "two definitions with different meanings" for @Nullable. Perhaps the difference in usage is a result of the difference in definition?) For example:

• null is a valid return value from Queue.poll only if the queue is empty.
• null is a valid return value from SortedSet.comparator and Spliterator.getComparator only if the collection uses natural order.

More generally, methods rarely declare null to always be a legal return value, excluding the occasional case like ExecutorService.submit:

jdk/src/java.base/share/classes/java/util/concurrent/ExecutorService.java

Line 269 in aadd76c

Future<@Nullable ?> submit(Runnable task);

Instead, it's almost always legal under certain conditions. This feels similar to the case we're discussing with InvocationHandler, only with parameters to a callback.

(When it comes to parameters to methods that users call, the story is sometimes different: Methods do more frequently declare null to always be a valid parameter. For example, there's String.valueOf.)

[cpovirk]

(I think this all remains compatible with what you're saying: We don't have the distinction between @Nullable and @NullOk currently. So, for "null sometimes" types, we prefer @Nullable for return types and @NonNull for parameter types, without trying to make more subjective distinctions between "APIs that users call" and "APIs that users implement.")