Consider annotating InvocationTargetException, ExecutionException, and CompletionException causes as non-nullable

[cpovirk]

First, InvocationTargetException.

Points in favor of non-nullable:

• The no-Throwable constructor overload is protected, not public—and AFAICT is unused both in the JDK and Google's codebase.
• Every InvocationTargetException that I have ever seen has had a non-nullable cause.

Points against:

• getCause() explicitly documents that the return value "may be null" :( This may have been copied without a ton of thought from Throwable.getCause(), but I notice that the phrasing does not match that in Throwable.getCause() (at least currently), so it wasn't literal copy-and-paste.
• Neither the constructors nor the getters perform a null check on the value.

No signal either way:

• getTargetException doesn't mention null.

[cpovirk]

(A somewhat similar case is ExecutionException. However, annoyingly, the JDK will throw a no-cause ExecutionException, IIRC if you pass an empty list to ExecutorService.invokeAny or some similar method.)

[edit: As noted in the next post, the JDK actually gets this right.]

[cpovirk]

Hmm, I may be wrong about ExecutionException.

The code I'd remembered from before is here:

https://github.com/jspecify/jdk/blob/cb922063ca52298a36146d73c247cff8195035cd/src/java.base/share/classes/java/util/concurrent/AbstractExecutorService.java#L215-L216

My read was that that would resulting in throwing if the list of tasks were empty.

But the Javadoc (even as far back as JDK 11, maybe earlier) says "throws IllegalArgumentException - if tasks is empty."

And I see multiple guards against emptiness:

• https://github.com/jspecify/jdk/blob/cb922063ca52298a36146d73c247cff8195035cd/src/java.base/share/classes/java/util/concurrent/AbstractExecutorService.java#L160-L161
• https://github.com/jspecify/jdk/blob/cb922063ca52298a36146d73c247cff8195035cd/src/java.base/share/classes/java/util/concurrent/AbstractExecutorService.java#L180

I'd have to read the code more to see if the code I was worried about can ever actually execute. I could also check whether the behavior changed over time, and I could check whether I wrote about this somewhere in the reference checker.

[cpovirk]

Here's what I had written in the reference checker: https://github.com/jspecify/jspecify-reference-checker/blob/75aa1de2a8f88f8c23347d86465844df9887874c/src/main/java/com/google/jspecify/nullness/NullSpecAnnotatedTypeFactory.java#L1126-L1148

The link there has broken, but yes, I was referring the code I thought I was: https://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/jsr166/src/main/java/util/concurrent/AbstractExecutorService.java?revision=1.54&view=markup#l185

Even at that version, I see the emptiness checks. So I think I was just wrong about the possibility of a cause-less ExecutionException from that code.

None of that takes away from the fact that ExecutionException (like InvocationTargetException) declares constructors that don't require a cause (and, also like InvocationTargetException, would accept null in its constructors that do require a cause).

In Google's codebase, I don't see any usages of the no-arg ExecutionException constructor except for some in tests (which I didn't look at) and one from a subclass that isn't ever used "as an ExecutionException" AFAICT. (Various classes call that constructor through a super call from one of their own constructors, but then none of those constructors are used except for the one I mentioned.) I didn't go through the larger number of String-only calls (again, calls from subclass constructors that may or may not ever be used). Maybe we'd be so lucky that all those users call initCause, but I doubt it. (In theory, checkers could be made to produce errors for calls to the no-cause constructors, at least when not followed by initCause.)

Anyway, I'm generalizing this issue to be about both types, plus CompletionException, which is similar to the other two. (The only call to its no-cause constructors that I see in Google's codebase is one in Caffeine.)

There are likely even more exception classes whose cause is never null, as noted recently in jspecify/jdk@cb92206. In those cases, we already don't use @Nullable on the getCause overloads. I forget whether I checked the corresponding constructor parameters (if any) or checked the classes for @NullMarked.

[cpovirk]

On the Guava front:

• We followed the ExecutionException pattern for UncheckedExecutionException and ExecutionError :( Perhaps we should deprecate their no-cause constructors, given that I see a very small number of calls for the group as a whole (at least one of which is "for compatibility reasons"). It's not even clear to me that the callers have a need to extend UncheckedExecutionException in the first place (though I wouldn't be at all surprised if some code had special handling for that that they wanted to trigger). Anyone who really wants to have a null cause can call super((Throwable) null) or super(message, null), assuming that we don't additionally crack down on that. In an ideal world, we would have required a non-nullable cause and then overridden getCause() to express that.
• Guava's invokeAny implementation (in the form of the package-private MoreExecutors.invokeAnyImpl, which I don't think has any callers in the package) has the preferred behavior of throwing IllegalArgumentException for an empty input. (Other executor implementations, like MoreExecutors.listeningDecorator, inherit the JDK's from AbstractExecutorService.)
• I thought I had some other Guava point, but if I did, I've forgotten....

[agentgt]

FWIW the times I have used null in exception constructors has always been calling the super from an inherited constructors and often it is a protected constructor.

So I think you have the right idea on nonnull at least from my experience.

Also the internal constructors being passed null I think was largely because of annoyance with constructor call ordering. With JEP 447 it maybe easier to avoid the edge cases of having to call a super with null (just a gut... don't have an example).

[cpovirk]

Thanks. That's an interesting point about JEP 447.

(If anyone has experience with really wanting to pass null to these constructors, please also speak up before I do anything regrettable to Guava :))

The other point I'd meant to add (previously expressed in this code comment) was that it could be inconvenient for code like throw new UncheckedExecutionException(executionException.getCause()) to become a nullness error. (To be clear, I'm not planning to go that far for Guava's types just yet (if ever), only to probably deprecate the constructors that don't have a cause parameter at all.) So, if we do make both the constructor parameters and getCause non-nullable for some classes, then it would be nice to keep Guava's classes and the JDK's classes relatively in sync, at least for the small number of users who already use both :) Granted, probably not all that many users are writing code like throw new UncheckedExecutionException(executionException.getCause()) in the first place.

[kevinb9n]

So these are exception types which "should" never have a null cause, as they are specified to be thrown in reaction to some previous exception.

I think the getCause return type should pretty clearly be non-null, because even in the event that null gets returned, it means something has gone wrong somewhere already. There's a bug, but this is not the place where that bug will be found. This is just another initialization-gap issue like #145, right?

I think the same goes for setCause. There could be code that is calling it repeatedly with nullable expressions and only making sure it's non-null by the end. But I don't think it's too bad if such code has to either restructure or suppress.

As for the constructors, JEP 447 changes nothing for us (for a long time). I feel like they should also be non-null; again, user can restructure or suppress.

(Recall that I view suppressions as just part of life, not evil. I just want the user to have a feeling while doing it like "okay, I can understand why it doesn't know this is safe".)

Do we align?

[cpovirk]

• initCause can be called only once, with initCause(null) meaning "I don't want to set a cause, and I don't want to let anyone else set a cause, either." I think we can probably set that one aside? It's kind of weird, but I'm not going to argue with anyone who wants to make an Exception as close to immutable as possible.
• I agree that there are possible connections to other initialization discussions. I don't know that I've actually seen initCause used to fill in the exception for an ExecutionException, etc. in practice. But if that were to come up, then I would agree that that's an initialization question.
• My fear has been more that someone has decided that it's Very Important to create an ExecutionException with no cause, presumably in order to get it through some method whose throws clause contains only ExecutionException or to trigger some special handling for ExecutionException (which hopefully doesn't actually look at the cause :)). I am getting fairly comfortable with throwing that use case under the bus. Maybe I should test what would happen inside Google if I were to do that. (But see a couple bullets down for a note about why that could be hard.) I suspect that the biggest fallout would be from cases like throw new UncheckedExecutionException(executionException.getCause()), discussed above.
• OK, and also it's sad that InvocationTargetException explicitly calls out null as a possible cause—especially when it's the class whose cause I might guess would least often be null in practice. But I think I can claim that that's copypasta until proven otherwise.
• It's sad that, out of the 3 classes listed in the title, only InvocationTargetException declares an override of getCause. That means that it's the only one that we can easily annotate as non-nullable. Tool owners could of course do better, but the surest course forward would be for stub-file tooling to rewrite the compile-time views of the other classes to include an override of getCause. We've discussed that before, and I'll leave a note in a couple different docs that have comment threads about adding overrides. (That includes the old but externally available "Specification language for reannotating JVM class files.")
• Finally, I'll take you up on your offer of discussing initialization/suppressions/whatever offline :)