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.

Sign up for GitHub

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

Jump to bottom

VoidMissingNullable: handle implicit parameterized types #4123

Closed
wants to merge 3 commits into from
Closed

VoidMissingNullable: handle implicit parameterized types #4123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce3c251
VoidMissingNullable: handle implicit parameterized types
Stephan202 Sep 28, 2023
74f9ac5
Organize imports
Stephan202 Sep 28, 2023
3741524
Apply suggestion, format
Stephan202 Sep 29, 2023
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
9 changes: 5 additions & 4 deletions check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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 ->
Expand All @@ -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);
}
Expand Down
18 changes: 11 additions & 7 deletions check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for trying that out, and for the comment.

}

/** Returns {@code true} if 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. */
Expand Down
5 changes: 2 additions & 3 deletions core/src/main/java/com/google/errorprone/bugpatterns/ConstantOverflow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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");
}
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/DefaultCharset.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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");
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/MixedMutabilityReturnType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
5 changes: 2 additions & 3 deletions core/src/main/java/com/google/errorprone/bugpatterns/NonCanonicalType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/StringSplitter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/Varifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/nullness/ParameterMissingNullable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down
10 changes: 8 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
Comment on lines +90 to +93
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if (beingConservative && state.errorProneOptions().isTestOnlyTarget()) {
return NO_MATCH;
}
Expand Down Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/nullness/VoidMissingNullableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading