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 index f448941037be..2f4b04d24352 100644 --- 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 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 index 71e069fceeab..e97d6f2da029 100644 --- 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 index 62aaef2d2764..73b26cb7c14d 100644 --- 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 index e3640904b5e2..43bd5d0860dd 100644 --- 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 index 5806a744ebd3..450c91f32a29 100644 --- 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 index 5ef34d412eaa..2ec38843e2db 100644 --- 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 index 0991a851a59e..a27fb236b601 100644 --- 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 index 11bba91f1070..5caf54ec10f9 100644 --- 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").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 index fa1742671a29..b93080bb1c3d 100644 --- 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 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 index 9155a9647e67..03a61264e23b 100644 --- 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 index 8c0ce2dace2e..4759a7e39908 100644 --- 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 index fc81664b782c..116295bbae15 100644 --- 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 index 8cf45b61b605..b984cb6da7a2 100644 --- 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