VoidMissingNullable: handle implicit parameterized types #4123

Stephan202 · 2023-09-28T19:51:21Z

While there, simplify a few other places where implicit AST nodes are
identified.

While there, simplify a few other places where implicit AST nodes are identified.

Stephan202

I'm open to alternative implementations. Also, let me know if I should extract the cleanup logic to a separate PR.

CC @cpovirk as the author of VoidMissingNullable.

Stephan202 · 2023-09-28T19:51:51Z

check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java

@@ -1117,7 +1118,7 @@ private static Tree suppressibleNode(TreePath path) {
                        && ((ClassTree) tree).getSimpleName().length() != 0)
                    // Lambda parameters can't be suppressed unless they have Type decls
                    || (tree instanceof VariableTree
-                        && getStartPosition(((VariableTree) tree).getType()) != -1))
+                        && !hasNoExplicitType((VariableTree) tree, state)))


This shows that hasNoExplicitType should perhaps be renamed to hasExplicitType, with negated semantics. WDYT?

I agree the double-negation is unfortunate, either hasExplicitType with negated semantics, or hasImplicitType with the same semantics seem like an improvement. If you want to make one of those changes here I'm happy to approve it :)

Cool! Perhaps hasImplicitType better describes the "edge case" that developers looking for this method are interested in; will use that.

Stephan202 · 2023-09-28T19:52:15Z

core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java

+    if (!hasExplicitSource(parameterizedTypeTree, state)) {
+      /* Implicit types cannot be annotated. */
+      return NO_MATCH;
+    }


We could expand the comment like here, but I'm not sure that we want to make a similarly strong statement about inference of local parameterized types.

Stephan202 · 2023-09-28T19:52:35Z

core/src/test/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullableTest.java

+  @Test
+  public void negativeGenericLambdaParameterNoType() {
+    aggressiveCompilationHelper
+        .addSourceLines(
+            "Test.java",
+                "import org.jspecify.annotations.Nullable;",
+            "interface Test {",
+            "  void consume(Iterable<@Nullable Void> it);",
+            "",
+            "  Test TEST = it -> {};",
+            "}")
+        .doTest();
+  }


Without these changes the test fails with:

java.lang.AssertionError: An unhandled exception was thrown by the Error Prone static analysis plugin. Please report this at https://github.com/google/error-prone/issues/new and include the following: error-prone version: unknown version BugPattern: VoidMissingNullable Stack Trace: java.util.NoSuchElementException: No value present at java.base/java.util.Optional.get(Optional.java:143) at com.google.errorprone.bugpatterns.nullness.NullnessUtils.fixByAddingKnownTypeUseNullableAnnotation(NullnessUtils.java:235) at com.google.errorprone.bugpatterns.nullness.NullnessUtils.fixByAnnotatingTypeUseOnlyLocationWithNullableAnnotation(NullnessUtils.java:200) at com.google.errorprone.bugpatterns.nullness.VoidMissingNullable.checkTree(VoidMissingNullable.java:191) at com.google.errorprone.bugpatterns.nullness.VoidMissingNullable.matchParameterizedType(VoidMissingNullable.java:107)

cushon

Looks good overall, thanks!

cushon · 2023-09-28T23:30:33Z

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

     * Note that the .isImplicitlyTyped() method on JCVariableDecl returns the wrong answer after
     * type attribution has occurred.
     */
-    return getStartPosition(tree.getType()) == -1;
+    return !hasExplicitSource(tree.getType(), state);


We used to check getEndPosition in hasNoExplicitType to work around a javac8 bug, and the workaround was removed in 1b8bf17. I can't remember if there was any motivation for that beyond removing references to javac8 bugs after we dropped JDK 8 support.

I also remembered that recent versions of JCVariableDecl (including in JDK 11u) have a declaredUsingVar getter that I think we could be using here. We might also need to check isImplicitlyTyped.

Thoughts on using that approach instead of delegating? I think it reads as well and avoids having to check end positions.

Though the unconditional downcast casts always make me a little uncomfortable, there's precedent even in this class to unconditionally cast to JCVariableDecl. So I tried this:

public static boolean hasImplicitType(VariableTree tree, VisitorState state) { JCVariableDecl varTree = (JCVariableDecl) tree; return varTree.declaredUsingVar() || varTree.isImplicitlyTyped(); }

Unfortunately this causes the existing negativeLambdaParameterNoType test to fail:

java.lang.AssertionError: Saw unexpected error on line 5. All errors: /Test.java:5: Note: [VoidMissingNullable] The type Void is not annotated @Nullable Test TEST = v -> {}; ^ (see https://errorprone.info/bugpattern/VoidMissingNullable) Did you mean 'Test TEST = @Nullable v -> {};'? at org.junit.Assert.fail(Assert.java:89) at com.google.errorprone.DiagnosticTestHelper.assertHasDiagnosticOnAllMatchingLines(DiagnosticTestHelper.java:348) at com.google.errorprone.CompilationTestHelper.doTest(CompilationTestHelper.java:342) at com.google.errorprone.bugpatterns.nullness.VoidMissingNullableTest.negativeLambdaParameterNoType(VoidMissingNullableTest.java:294) ...

When I attach a debugger then I see that indeed declaredUsingVar is false, and vartype is a JCIdent representing Void. I'm guessing (but did not investigate deeper) that this has something to do with the lambda expression being assigned the type of the targeted functional interface.

I'll propose a new comment to document this observation.

(Tested with Temurin-11.0.20.1+1 and Temurin-17.0.8.1+1.)

Thanks for trying that out, and for the comment.

cushon · 2023-09-28T23:32:34Z

check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java

@@ -1117,7 +1118,7 @@ private static Tree suppressibleNode(TreePath path) {
                        && ((ClassTree) tree).getSimpleName().length() != 0)
                    // Lambda parameters can't be suppressed unless they have Type decls
                    || (tree instanceof VariableTree
-                        && getStartPosition(((VariableTree) tree).getType()) != -1))
+                        && !hasNoExplicitType((VariableTree) tree, state)))


I agree the double-negation is unfortunate, either hasExplicitType with negated semantics, or hasImplicitType with the same semantics seem like an improvement. If you want to make one of those changes here I'm happy to approve it :)

Stephan202

Added a commit; also fixed some formatting.

Stephan202 · 2023-09-29T04:57:47Z

check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java

@@ -1117,7 +1118,7 @@ private static Tree suppressibleNode(TreePath path) {
                        && ((ClassTree) tree).getSimpleName().length() != 0)
                    // Lambda parameters can't be suppressed unless they have Type decls
                    || (tree instanceof VariableTree
-                        && getStartPosition(((VariableTree) tree).getType()) != -1))
+                        && !hasNoExplicitType((VariableTree) tree, state)))


Cool! Perhaps hasImplicitType better describes the "edge case" that developers looking for this method are interested in; will use that.

Stephan202 · 2023-09-29T05:20:55Z

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

     * Note that the .isImplicitlyTyped() method on JCVariableDecl returns the wrong answer after
     * type attribution has occurred.
     */
-    return getStartPosition(tree.getType()) == -1;
+    return !hasExplicitSource(tree.getType(), state);


Though the unconditional downcast casts always make me a little uncomfortable, there's precedent even in this class to unconditionally cast to JCVariableDecl. So I tried this:

public static boolean hasImplicitType(VariableTree tree, VisitorState state) { JCVariableDecl varTree = (JCVariableDecl) tree; return varTree.declaredUsingVar() || varTree.isImplicitlyTyped(); }

Unfortunately this causes the existing negativeLambdaParameterNoType test to fail:

java.lang.AssertionError: Saw unexpected error on line 5. All errors: /Test.java:5: Note: [VoidMissingNullable] The type Void is not annotated @Nullable Test TEST = v -> {}; ^ (see https://errorprone.info/bugpattern/VoidMissingNullable) Did you mean 'Test TEST = @Nullable v -> {};'? at org.junit.Assert.fail(Assert.java:89) at com.google.errorprone.DiagnosticTestHelper.assertHasDiagnosticOnAllMatchingLines(DiagnosticTestHelper.java:348) at com.google.errorprone.CompilationTestHelper.doTest(CompilationTestHelper.java:342) at com.google.errorprone.bugpatterns.nullness.VoidMissingNullableTest.negativeLambdaParameterNoType(VoidMissingNullableTest.java:294) ...

When I attach a debugger then I see that indeed declaredUsingVar is false, and vartype is a JCIdent representing Void. I'm guessing (but did not investigate deeper) that this has something to do with the lambda expression being assigned the type of the targeted functional interface.

I'll propose a new comment to document this observation.

(Tested with Temurin-11.0.20.1+1 and Temurin-17.0.8.1+1.)

cushon · 2023-09-29T15:34:03Z

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

     * Note that the .isImplicitlyTyped() method on JCVariableDecl returns the wrong answer after
     * type attribution has occurred.
     */
-    return getStartPosition(tree.getType()) == -1;
+    return !hasExplicitSource(tree.getType(), state);


Thanks for trying that out, and for the comment.

cushon · 2023-09-29T15:57:22Z

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

@@ -2645,15 +2646,18 @@ private void scan(Type from, Type to) {
  }

  /** Returns {@code true} if this is a `var` or a lambda parameter that has no explicit type. */
-  public static boolean hasNoExplicitType(VariableTree tree, VisitorState state) {


I'm going to temporarily add hasNoExplicitType back, but make it deprecated and have it delegate to hasImplicitType. We have some internal checks calling the old method and it's easier than fixing everything atomically, and it might also be helpful to other users to leave the old method around for a release or two.

(I have started importing the change and can make that fix on my side, you don't need to update the PR.)

While there, simplify a few other places where implicit AST nodes are identified. Fixes #4123 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4123 from PicnicSupermarket:sschroevers/fix-VoidMissingNullable 3741524 PiperOrigin-RevId: 569503189

Stephan202 commented Sep 28, 2023

View reviewed changes

Organize imports

74f9ac5

cushon requested changes Sep 28, 2023

View reviewed changes

Apply suggestion, format

3741524

Stephan202 commented Sep 29, 2023

View reviewed changes

cushon approved these changes Sep 29, 2023

View reviewed changes

cushon reviewed Sep 29, 2023

View reviewed changes

copybara-service bot mentioned this pull request Sep 29, 2023

VoidMissingNullable: handle implicit parameterized types #4124

Closed

copybara-service bot closed this in 3b0371e Sep 29, 2023

Stephan202 deleted the sschroevers/fix-VoidMissingNullable branch September 29, 2023 17:46

carterkozak mentioned this pull request Oct 23, 2023

Upgrade error-prone to the latest release palantir/gradle-baseline#2651

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VoidMissingNullable: handle implicit parameterized types #4123

VoidMissingNullable: handle implicit parameterized types #4123

Stephan202 commented Sep 28, 2023

Stephan202 left a comment

Stephan202 Sep 28, 2023

cushon Sep 28, 2023

Stephan202 Sep 29, 2023

Stephan202 Sep 28, 2023

Stephan202 Sep 28, 2023

cushon left a comment

cushon Sep 28, 2023

Stephan202 Sep 29, 2023

cushon Sep 29, 2023

cushon Sep 28, 2023

Stephan202 left a comment

Stephan202 Sep 29, 2023

Stephan202 Sep 29, 2023

cushon Sep 29, 2023

cushon Sep 29, 2023

VoidMissingNullable: handle implicit parameterized types #4123

VoidMissingNullable: handle implicit parameterized types #4123

Conversation

Stephan202 commented Sep 28, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cushon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment