VoidMissingNullable: handle implicit parameterized types

While there, simplify a few other places where implicit AST nodes are identified. Fixes #4123 COPYBARA_INTEGRATE_REVIEW=#4123 from PicnicSupermarket:sschroevers/fix-VoidMissingNullable 3741524 PiperOrigin-RevId: 569514313
google · Sep 29, 2023 · 3b0371e · 3b0371e
1 parent 43a6c45
commit 3b0371e
Show file tree

Hide file tree

Showing 13 changed files with 61 additions and 30 deletions.
diff --git a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java
@@ -27,6 +27,7 @@
 import static com.google.errorprone.util.ASTHelpers.getModifiers;
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.hasImplicitType;
 import static com.sun.source.tree.Tree.Kind.ASSIGNMENT;
 import static com.sun.source.tree.Tree.Kind.CONDITIONAL_EXPRESSION;
 import static com.sun.source.tree.Tree.Kind.NEW_ARRAY;
@@ -1018,7 +1019,7 @@ public static void addSuppressWarnings(
       @Nullable String lineComment,
       boolean commentOnNewLine) {
     // Find the nearest tree to add @SuppressWarnings to.
-    Tree suppressibleNode = suppressibleNode(state.getPath());
+    Tree suppressibleNode = suppressibleNode(state.getPath(), state);
     if (suppressibleNode == null) {
       return;
     }
@@ -1069,7 +1070,7 @@ public static void addSuppressWarnings(
   public static void removeSuppressWarnings(
       SuggestedFix.Builder fixBuilder, VisitorState state, String warningToRemove) {
     // Find the nearest tree to remove @SuppressWarnings from.
-    Tree suppressibleNode = suppressibleNode(state.getPath());
+    Tree suppressibleNode = suppressibleNode(state.getPath(), state);
     if (suppressibleNode == null) {
       return;
     }
@@ -1107,7 +1108,7 @@ private static List<? extends AnnotationTree> findAnnotationsTree(Tree tree) {
   }
 
   @Nullable
-  private static Tree suppressibleNode(TreePath path) {
+  private static Tree suppressibleNode(TreePath path, VisitorState state) {
     return StreamSupport.stream(path.spliterator(), false)
         .filter(
             tree ->
@@ -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))
+                        && !hasImplicitType((VariableTree) tree, state)))
         .findFirst()
         .orElse(null);
   }

diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
@@ -148,6 +148,7 @@
 import com.sun.tools.javac.util.Log;
 import com.sun.tools.javac.util.Log.DeferredDiagnosticHandler;
 import com.sun.tools.javac.util.Name;
+import com.sun.tools.javac.util.Position;
 import java.io.IOException;
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
@@ -2644,16 +2645,27 @@ private void scan(Type from, Type to) {
     return result.build();
   }
 
-  /** Returns {@code true} if this is a `var` or a lambda parameter that has no explicit type. */
+  /**
+   * @deprecated use {@link #hasImplicitType(VariableTree, VisitorState)} instead
+   */
+  @Deprecated
   public static boolean hasNoExplicitType(VariableTree tree, VisitorState state) {
+    return hasImplicitType(tree, state);
+  }
+
+  /** Returns whether this is a {@code var} or a lambda parameter that has no explicit type. */
+  public static boolean hasImplicitType(VariableTree tree, VisitorState state) {
     /*
-     * We detect the absence of an explicit type by looking for an absent start position for the
-     * type tree.
-     *
-     * Note that the .isImplicitlyTyped() method on JCVariableDecl returns the wrong answer after
-     * type attribution has occurred.
+     * For lambda expression parameters without an explicit type, both
+     * `JCVariableDecl#declaredUsingVar()` and `#isImplicitlyTyped()` may be false. So instead we
+     * check whether the variable's type is explicitly represented in the source code.
      */
-    return getStartPosition(tree.getType()) == -1;
+    return !hasExplicitSource(tree.getType(), state);
+  }
+
+  /** Returns whether the given tree has an explicit source code representation. */
+  public static boolean hasExplicitSource(Tree tree, VisitorState state) {
+    return getStartPosition(tree) != Position.NOPOS && state.getEndPosition(tree) != Position.NOPOS;
   }
 
   /** Returns {@code true} if this symbol was declared in Kotlin source. */

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ConstantOverflow.java b/core/src/main/java/com/google/errorprone/bugpatterns/ConstantOverflow.java
@@ -18,8 +18,8 @@
 
 import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
-import static com.google.errorprone.util.ASTHelpers.getStartPosition;
 import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.hasExplicitSource;
 import static com.google.errorprone.util.ASTHelpers.isSameType;
 
 import com.google.common.math.IntMath;
@@ -47,7 +47,6 @@
 import com.sun.source.util.SimpleTreeVisitor;
 import com.sun.source.util.TreePath;
 import com.sun.tools.javac.code.Type;
-import com.sun.tools.javac.util.Position;
 import javax.annotation.Nullable;
 import javax.lang.model.type.TypeKind;
 
@@ -99,7 +98,7 @@ private static Fix longFix(ExpressionTree expr, VisitorState state) {
     Tree parent = state.getPath().getParentPath().getLeaf();
     if (parent instanceof VariableTree && isSameType(getType(parent), intType, state)) {
       Tree type = ((VariableTree) parent).getType();
-      if (getStartPosition(type) != Position.NOPOS) {
+      if (hasExplicitSource(type, state)) {
         fix.replace(type, "long");
       }
     }

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DefaultCharset.java b/core/src/main/java/com/google/errorprone/bugpatterns/DefaultCharset.java
@@ -390,7 +390,7 @@ public Void visitVariable(VariableTree node, Void unused) {
         if (!sym.equals(ASTHelpers.getSymbol(node))) {
           return null;
         }
-        if (ASTHelpers.getStartPosition(node.getType()) == -1) {
+        if (ASTHelpers.hasImplicitType(node, state)) {
           // ignore synthetic tree nodes for `var`
           return null;
         }

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java b/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java
@@ -60,7 +60,6 @@
 import com.sun.tools.javac.code.Symbol.VarSymbol;
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.code.Types;
-import com.sun.tools.javac.util.Position;
 import java.util.Optional;
 import javax.annotation.Nullable;
 
@@ -347,7 +346,7 @@ public Void visitIdentifier(IdentifierTree tree, Void unused) {
     }
     SuggestedFix.Builder fix =
         SuggestedFix.builder().replace(newClassTree.getIdentifier(), "StringBuilder");
-    if (ASTHelpers.getStartPosition(varTree.getType()) != Position.NOPOS) {
+    if (!ASTHelpers.hasImplicitType(varTree, state)) {
       // If the variable is declared with `var`, there's no declaration type to change
       fix = fix.replace(varTree.getType(), "StringBuilder");
     }

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MixedMutabilityReturnType.java b/core/src/main/java/com/google/errorprone/bugpatterns/MixedMutabilityReturnType.java
@@ -388,7 +388,7 @@ public Void visitVariable(VariableTree variableTree, Void unused) {
 
       Tree erasedType = ASTHelpers.getErasedTypeTree(variableTree.getType());
       // don't try to replace synthetic nodes for `var`
-      if (ASTHelpers.getStartPosition(erasedType) != -1) {
+      if (ASTHelpers.hasExplicitSource(erasedType, state)) {
         fix.replace(erasedType, qualifyType(state, fix, details.builderType()));
       }
       if (variableTree.getInitializer() != null) {

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NonCanonicalType.java b/core/src/main/java/com/google/errorprone/bugpatterns/NonCanonicalType.java
@@ -20,8 +20,8 @@
 import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.enclosingClass;
-import static com.google.errorprone.util.ASTHelpers.getStartPosition;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.hasExplicitSource;
 
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
@@ -34,7 +34,6 @@
 import com.sun.source.tree.Tree;
 import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Symbol.TypeSymbol;
-import com.sun.tools.javac.util.Position;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import javax.annotation.Nullable;
@@ -67,7 +66,7 @@ public Description matchMemberSelect(MemberSelectTree tree, VisitorState state)
         return NO_MATCH;
       }
     }
-    if (getStartPosition(tree) == Position.NOPOS) {
+    if (!hasExplicitSource(tree, state)) {
       // Can't suggest changing a synthetic type tree
       return NO_MATCH;
     }

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StringSplitter.java b/core/src/main/java/com/google/errorprone/bugpatterns/StringSplitter.java
@@ -24,7 +24,7 @@
 import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
 import static com.google.errorprone.util.ASTHelpers.getType;
-import static com.google.errorprone.util.ASTHelpers.hasNoExplicitType;
+import static com.google.errorprone.util.ASTHelpers.hasImplicitType;
 import static com.google.errorprone.util.ASTHelpers.isSubtype;
 import static com.google.errorprone.util.Regexes.convertRegexToLiteral;
 import static java.lang.String.format;
@@ -211,7 +211,7 @@ public Boolean visitMemberSelect(MemberSelectTree tree, Void unused) {
     }
 
     Tree varType = varTree.getType();
-    boolean isImplicitlyTyped = hasNoExplicitType(varTree, state); // Is it a use of `var`?
+    boolean isImplicitlyTyped = hasImplicitType(varTree, state); // Is it a use of `var`?
     if (needsList[0]) {
       if (!isImplicitlyTyped) {
         fix.replace(varType, "List<String>").addImport("java.util.List");

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
@@ -29,6 +29,7 @@
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.getType;
 import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.hasExplicitSource;
 import static com.google.errorprone.util.ASTHelpers.isStatic;
 import static com.google.errorprone.util.ASTHelpers.isSubtype;
 import static com.google.errorprone.util.ASTHelpers.shouldKeep;
@@ -522,7 +523,7 @@ private void removeByIndex(List<? extends Tree> trees) {
         }
         if (trees.size() == 1) {
           Tree tree = getOnlyElement(trees);
-          if (getStartPosition(tree) == -1 || state.getEndPosition(tree) == -1) {
+          if (!hasExplicitSource(tree, state)) {
             // TODO(b/118437729): handle bogus source positions in enum declarations
             return;
           }

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/Varifier.java b/core/src/main/java/com/google/errorprone/bugpatterns/Varifier.java
@@ -24,7 +24,7 @@
 import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.getType;
-import static com.google.errorprone.util.ASTHelpers.hasNoExplicitType;
+import static com.google.errorprone.util.ASTHelpers.hasImplicitType;
 import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
 import static com.google.errorprone.util.ASTHelpers.isSameType;
 import static com.google.errorprone.util.ASTHelpers.streamReceivers;
@@ -74,7 +74,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
     if (!symbol.getKind().equals(LOCAL_VARIABLE)
         || !isConsideredFinal(symbol)
         || initializer == null
-        || hasNoExplicitType(tree, state)) {
+        || hasImplicitType(tree, state)) {
       return NO_MATCH;
     }
     // Foo foo = (Foo) bar;

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ParameterMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ParameterMissingNullable.java
@@ -28,7 +28,7 @@
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.getType;
-import static com.google.errorprone.util.ASTHelpers.hasNoExplicitType;
+import static com.google.errorprone.util.ASTHelpers.hasImplicitType;
 import static javax.lang.model.element.ElementKind.PARAMETER;
 import static javax.lang.model.type.TypeKind.TYPEVAR;
 
@@ -143,7 +143,7 @@ public Description matchBinary(BinaryTree tree, VisitorState state) {
     if (param == null) {
       return NO_MATCH; // hopefully impossible: A parameter must come from the same compilation unit
     }
-    if (hasNoExplicitType(param, state)) {
+    if (hasImplicitType(param, state)) {
       return NO_MATCH;
     }
     SuggestedFix fix = fixByAddingNullableAnnotationToType(state, param);

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java
@@ -26,7 +26,8 @@
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.getType;
-import static com.google.errorprone.util.ASTHelpers.hasNoExplicitType;
+import static com.google.errorprone.util.ASTHelpers.hasExplicitSource;
+import static com.google.errorprone.util.ASTHelpers.hasImplicitType;
 import static com.sun.source.tree.Tree.Kind.METHOD;
 import static javax.lang.model.element.ElementKind.LOCAL_VARIABLE;
 
@@ -86,6 +87,11 @@ public class VoidMissingNullable extends BugChecker
   @Override
   public Description matchParameterizedType(
       ParameterizedTypeTree parameterizedTypeTree, VisitorState state) {
+    if (!hasExplicitSource(parameterizedTypeTree, state)) {
+      /* Implicit types cannot be annotated. */
+      return NO_MATCH;
+    }
+
     if (beingConservative && state.errorProneOptions().isTestOnlyTarget()) {
       return NO_MATCH;
     }
@@ -141,7 +147,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
       return NO_MATCH;
     }
 
-    if (hasNoExplicitType(tree, state)) {
+    if (hasImplicitType(tree, state)) {
       /*
        * In the case of `var`, a declaration-annotation @Nullable would be valid. But a type-use
        * @Nullable would not be. But more importantly, we expect that tools will infer the

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullableTest.java
@@ -294,6 +294,20 @@ public void negativeLambdaParameterNoType() {
         .doTest();
   }
 
+  @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();
+  }
+
   // TODO(cpovirk): Test under Java 11+ with `(var x) -> {}` lambda syntax.
 
   @Test