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.

Sign up for GitHub

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

Annotate InvocationHandler for nullness. #74

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Annotate InvocationHandler for nullness. #74

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6cb85b
Annotate InvocationHandler for nullness.
cpovirk Aug 5, 2020
b3e6a4d
Merge ../jdk-branch-master into invocationhandler
mernst Aug 12, 2020
c925975
Merge ../jdk-branch-master into invocationhandler
mernst Sep 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/java.base/share/classes/java/lang/reflect/InvocationHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@

package java.lang.reflect;

import org.checkerframework.framework.qual.AnnotatedFor;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* {@code InvocationHandler} is the interface implemented by
* the <i>invocation handler</i> of a proxy instance.
Expand All @@ -38,6 +41,7 @@
* @see Proxy
* @since 1.3
*/
@AnnotatedFor({"nullness"})
public interface InvocationHandler {

/**
Expand Down Expand Up @@ -90,6 +94,6 @@ public interface InvocationHandler {
*
* @see UndeclaredThrowableException
*/
public Object invoke(Object proxy, Method method, Object[] args)
public Object invoke(Object proxy, Method method, @Nullable Object @Nullable [] args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throws Throwable;
}