Void should be annotated @Nullable

[jensdietrich]

There are multiple methods using java.lang.Void as return or parameter type, not annotated as @Nullable. Examples:

com.google.errorprone.bugpatterns.threadsafety.ConstantExpressions::visitIdentifier
com.google.errorprone.bugpatterns.threadsafety.ConstantExpressions::visitParenthesized
com.google.errorprone.bugpatterns.threadsafety.ConstantExpressions::visitBinary
com.google.errorprone.bugpatterns.threadsafety.DoubleCheckedLocking::vistBlock
com.google.errorprone.bugpatterns.threadsafety.ImmutableChecker::visitMemberSelect
com.google.errorprone.refaster.RefasterScanner::scan

etc

As Void cannot be instantiated, the respective values must be null and should therefore be annotated to aid static checkers, and error-prone actually contains a rule saying this:

https://errorprone.info/bugpattern/VoidMissingNullable

[cpovirk]

Thanks. I am personally more of an adjunct to the Error Prone team than a proper "member," but I wrote the check, so I can speak to it at least a bit :)

I expect us to start writing @Nullable Void across our codebase more often as we increase our use of Kotlin: Kotlin is more aggressive about requiring nullness annotations than most other tools are. Fortunately, from the Kotlin perspective, our types aren't "non-null Void"; they're more like "unspecified-nullness Void"—at least until we add further annotations.

@Nullable Void is of course quite verbose, especially as a way to say "nothing to see here." If we commit to thoroughly annotating Error Prone for nullness, we may well want to make some deeper changes to support that, like providing our own visitor classes that declare plain-void return types or omit unused parameters entirely.

I'm interested in whether VoidMissingNullable has been useful from your perspective and, if so, how. Maybe we should be prioritizing it higher than we currently are.

(And ideally I would write all of this in the bugpattern doc that you linked....)

[jensdietrich]

Thanks for the quick response.

Whether annotating uses of java.lang.Void as nullable is useful depends IMO on two things: (1) whether tools, in particular static checkers, will infer it as Nullable anyway. (2) How common it is.

I dont have full answers for those two questions but at least some data points. Re (1) NullAway for instance seems to always treat Void as Nullable – see for instance NullAway::checkReturnExpression https://github.com/uber/NullAway/blob/master/nullaway/src/main/java/com/uber/nullaway/NullAway.java . I would expect that a lot of other tools do the same, so this might make the check less useful.

Re (2) I am writing and studying Java code for a long time, and in my experience Void is not common. Error-prone is the one project I am aware of that uses it a lot. But I did not study this systematically (perhaps I will when I find some time).

BTW, there is also a related discussion in jspecify (jspecify/jspecify#51 ).

As you mentioned that you might commit to “thoroughly annotating Error Prone for nullness” – the reason I came across this is that we are developing a tool to infer nullness annotations from instrumented tests – basically looking for the use of null when tests execute (with some additional static analyses to increase accuracy). We evaluate this on projects with existing nullness annotations to act as ground truth (mainly spring modules, but also guava and error-prone), and this is how we came across this issue. The tool generally archives high precision, but lower recall (around 90% / 50% respectively for the programs we evaluated on). This is an academic tool, we are just about to submit a paper about this. Details here: https://github.com/jensdietrich/null-annotation-inference

[cpovirk]

Thanks for all the information. Sorry that I'm not so quick to respond this time :)

Runtime instrumentation is something that I am expecting us to explore someday, just probably not anytime soon, sadly. One of my concerns is that we know we have tests that pass null arguments to or return null values from APIs that would never do that in prod code. Sometimes that happens because test authors write return null; because it's easy (maybe even filled in by an IDE?); even more often, it happens because Mockito returns null by default from its mocks/stubs (and also from matcher methods like any()). We may be able to sidestep some of those issues by ignoring tests that use Mockito or even restricting ourselves to tests that run a fairly "real" server and interact with it only as a black box. Or we could try running the instrumentation in prod, but of course that is much scarier than the alternative :)

I'll be interested to hear what else comes out of your work.

Anyway, I think it makes sense to mostly treat Void as implicitly @Nullable for now, given the reasons you mention. Someday we'll probably still try to annotate it explicitly inside Google... or maybe KT-20379 will go in an unexpected direction and save us some effort.

[jensdietrich]

Thanks for the response, this all makes sense. The tool we have developed works somehow like what you describe – tests are filtered to remove false positives. We dont filter for mockito atm (but I will look into this), but for a few other things, like classes from shaded dependencies, and “negative” tests (eg where null is passed to test a defensive API, those tests are recognised by the oracle, they usually expect some runtime exception to be thrown, and those checks can be done statically with a simple ASM bytecode analysis). To assess how this works we measure precision and recall against existing annotations. We evaluate on seven spring modules (core, beans, context, web, webmvc, orm, oxm) as they are heavily annotated and generally get a recall of around 0.5 and a precision on >0.9 with our approach. I.e. we can infer only half, but what we infer tends to be correct. Precision here is more like a “lower precision bound” – it could be higher as annotations are missing (a spring PR we made supports this). For error-prone, if we treat Void as nullable, the recall is higher (0.73) but the precision a bit lower (0.79). So there might be some missing annotations in here, I will have another look at the FPs whether I can spot any patterns our sanitisation does not yet cover. I can share the paper if you could provide an upload link or email - as this is under review I cannot yet make this completely open.

[cpovirk]

Thanks. I'm not a very experienced paper-reader, but if you have no particular expectations that I'll be useful, I'd be happy to have the opportunity to have a look.