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.

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

Treat Void formal arguments as @Nullable #613

Merged
merged 2 commits into from
Jun 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -1445,6 +1445,16 @@ private Description handleInvocation(
} else {
continue;
}
} else if (ASTHelpers.isSameType(
param.type, Suppliers.JAVA_LANG_VOID_TYPE.get(state), state)) {
// Temporarily treat a Void argument type as if it were @Nullable Void. Handling of Void
// without
// special-casing, as recommeded by JSpecify might: a) require generics support and, b)
// require
// checking that third-party libraries considered annotated adopt JSpecify semantics.
// See the suppression in https://github.com/uber/NullAway/pull/608 for an example of why
// this is needed.
continue;
}
// we need to call paramHasNullableAnnotation here since the invoked method may be defined
// in a class file
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,12 @@ public NullAwayCFGTranslationPhaseOne(
this.handler = handler;
}

@SuppressWarnings("NullAway") // (Void)null issue
private void scanWithVoid(Tree tree) {
this.scan(tree, (Void) null);
}

/**
* Obtain the type mirror for a given class, used for exception throwing.
*
* <p>We use this method to expose the otherwise protected method {@link #getTypeMirror(Class)}
* to handlers.
*
* @param klass a Java class
* @return the corresponding type mirror
*/
Expand All @@ -144,7 +142,7 @@ public void insertThrowOnFalse(Node booleanExpressionNode, TypeMirror errorType)
Preconditions.checkNotNull(booleanExpressionTree);
Label falsePreconditionEntry = new Label();
Label endPrecondition = new Label();
this.scanWithVoid(booleanExpressionTree);
this.scan(booleanExpressionTree, (Void) null);
ConditionalJump cjump = new ConditionalJump(endPrecondition, falsePreconditionEntry);
this.extendWithExtendedNode(cjump);
this.addLabelForNextNode(falsePreconditionEntry);
Expand Down
30 changes: 30 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,36 @@ public void nullableOnJavaLangVoid() {
.doTest();
}

@Test
public void nullableOnJavaLangVoidWithCast() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"class Test {",
" // Currently unhandled. In fact, it *should* produce an error. This entire test case",
" // needs to be rethought once we properly support generics, such that it works on T v",
" // when T == @Nullable Void, but not when T == Void. Without generics, though, this is the",
" // best we can do.",
" @SuppressWarnings(\"NullAway\")",
" private Void v = (Void)null;",
" Void foo1() {",
" // temporarily, we treat a Void return type as if it was @Nullable Void",
" return (Void)null;",
" }",
" // Temporarily, we treat any Void formal as if it were @Nullable Void",
" void consumeVoid(Void v) {",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

" }",
" @Nullable Void foo2() {",
" consumeVoid(null); // See comment on consumeVoid for why this is allowed",
" consumeVoid((Void)null);",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@lazaroclapp lazaroclapp Jun 20, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

" return (Void)null;",
" }",
"}")
.doTest();
}

@Test
public void staticCallZeroArgsNullCheck() {
defaultCompilationHelper
Expand Down