Treat Void formal arguments as @Nullable

[lazaroclapp]

This is a follow up on #595. See that PR for some of the soundness
caveats related to method overriding and the nullability of Void
in our current implementation.

This does, however, avoid some otherwise awkward suppressions,
as shown in #608.

There are two correct ways of handling Void:

a) Having it default to @Nullable as a type, which requires us
adding a "default nullability" for some types/classes and explicitly
contradicts JSpecify (jspecify/jspecify#51)
b) Supporting generics, and requiring explicitly annotating @Nullable Void.
This will probably also require third-party libraries which we consider
annotated (such as CF), to adopt this convention.

I believe (b) is the way forward long-term, which means that, for now,
this hack might be the best we can do without generics support. Once
NullAway supports generics, both this change and #595 should be reverted.

[coveralls]

Pull Request Test Coverage Report for Build #880

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 19 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.04%) to 92.538%

---

Files with Coverage Reduction	New Missed Lines	%
../nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java	2	95.74%
../nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java	3	90.91%
../nullaway/src/main/java/com/uber/nullaway/NullAway.java	14	95.28%

Totals
Change from base Build #879:	-0.04%
Covered Lines:	4824
Relevant Lines:	5213

---

💛 - Coveralls

[msridhar]

Beyond the comment below, could we add a test for assigning (Void) null to a field? I'm not sure what to expect there (though it is a more contrived case).

[msridhar]

From the test, it seems that this change makes us treat (Void) null as @NonNull Void when it is a parameter, not @Nullable Void (since here consumeVoid's argument is @NonNull). Am I missing something?

[lazaroclapp]

What this is meant to change is the interpretation of a Void formal to automatically mean @Nullable Void. See the change to nonNullPositions here. So, the change does make the argument to consumeVoid implicitly @Nullable.

It is not possible at this time to always annotate the formal as @Nullable, because, usually, in the cases where we want to pass Void around, the formal is of some generic type T and what we really would want is to be able to replace <Void> with <@Nullable Void>, see the example in #608.

[msridhar]

Got it thanks! I was thinking about the actual rather than the formal

[msridhar]

Just doing consumeVoid(null) should also have no error right? Can we test that?

[lazaroclapp]

Beyond the comment below, could we add a test for assigning (Void) null to a field? I'm not sure what to expect there (though it is a more contrived case).

Will try, though I'd expect that to fail within this hack. If we want a more robust fix here, we might need some sort of mechanism to specify the default nullness of a type. Basically, forcing NullAway to expand Void into @Nullable Void everywhere (for now), then revert that once we have support for annotations on generics.

Edit:

As expected:

/Test.java:4: warning: [NullAway] assigning @nullable expression to @nonnull field
private Void v = (Void)null;

Again, if we want to handle all cases, we probably need either generics support (for the 'correct' way according to JSpecify) or types that are @Nullable by default 🤷

[msridhar]

Code change looks good, just a couple comments on the test

[msridhar]

Can we add a comment here indicating the formal is treated as if it were @Nullable? I think having the comment just at the call site confused me a bit

[msridhar]

Just doing consumeVoid(null) should also have no error right? Can we test that?

[msridhar]

As expected:

/Test.java:4: warning: [NullAway] assigning @nullable expression to @nonnull field

private Void v = (Void)null;

Again, if we want to handle all cases, we probably need either generics support (for the 'correct' way according to JSpecify) or types that are @Nullable by default 🤷

Gotcha. It's fine to punt on this for now. We could also add this test with a suppression just as documentation; up to you.