diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariable.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariable.java index e3d9f7d3508..6ea59e68ca4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariable.java @@ -23,10 +23,12 @@ import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; import static com.google.errorprone.util.ASTHelpers.getSymbol; +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ListMultimap; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; @@ -34,6 +36,7 @@ import com.google.errorprone.util.ASTHelpers.TargetType; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.CompoundAssignmentTree; import com.sun.source.tree.EnhancedForLoopTree; import com.sun.source.tree.ExpressionTree; @@ -54,10 +57,10 @@ import com.sun.tools.javac.code.Symbol.VarSymbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.TypeTag; -import java.util.ArrayList; +import java.util.HashSet; import java.util.List; -import java.util.Objects; import java.util.Optional; +import java.util.Set; import javax.lang.model.element.ElementKind; /** @@ -72,17 +75,30 @@ + " corresponding primitive type, which avoids the cost of constructing an unnecessary" + " object.", severity = SeverityLevel.SUGGESTION) -public class UnnecessaryBoxedVariable extends BugChecker implements VariableTreeMatcher { +public class UnnecessaryBoxedVariable extends BugChecker implements CompilationUnitTreeMatcher { private static final Matcher VALUE_OF_MATCHER = staticMethod().onClass(UnnecessaryBoxedVariable::isBoxableType).named("valueOf"); @Override - public Description matchVariable(VariableTree tree, VisitorState state) { - return unboxed(tree, state).flatMap(u -> handleVariable(u, tree, state)).orElse(NO_MATCH); + public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { + FindBoxedUsagesScanner usages = new FindBoxedUsagesScanner(state); + usages.scan(tree, null); + + new SuppressibleTreePathScanner() { + @Override + public Void visitVariable(VariableTree tree, Void unused) { + VisitorState innerState = state.withPath(getCurrentPath()); + unboxed(tree, innerState) + .flatMap(u -> handleVariable(u, usages, tree, innerState)) + .ifPresent(state::reportMatch); + return super.visitVariable(tree, null); + } + }.scan(tree, null); + return NO_MATCH; } private Optional handleVariable( - Type unboxed, VariableTree tree, VisitorState state) { + Type unboxed, FindBoxedUsagesScanner usages, VariableTree tree, VisitorState state) { VarSymbol varSymbol = getSymbol(tree); switch (varSymbol.getKind()) { case PARAMETER: @@ -99,19 +115,16 @@ private Optional handleVariable( return Optional.empty(); } - return getEnclosingMethod(state.getPath()) - .flatMap(enclosingMethod -> fixVariable(unboxed, tree, enclosingMethod, state)); + return fixVariable(unboxed, usages, tree, state); } private Optional fixVariable( - Type unboxed, VariableTree tree, TreePath enclosingMethod, VisitorState state) { + Type unboxed, FindBoxedUsagesScanner usages, VariableTree tree, VisitorState state) { VarSymbol varSymbol = getSymbol(tree); - FindBoxedUsagesScanner scanner = new FindBoxedUsagesScanner(varSymbol, enclosingMethod, state); - scanner.scan(enclosingMethod, null); - if (scanner.boxedUsageFound) { + if (usages.boxedUsageFound.contains(varSymbol)) { return Optional.empty(); } - if (!scanner.used && varSymbol.getKind() == ElementKind.PARAMETER) { + if (!usages.dereferenced.contains(varSymbol) && varSymbol.getKind() == ElementKind.PARAMETER) { // If it isn't used and it is a parameter, don't fix it, because this could introduce a new // NPE. return Optional.empty(); @@ -120,9 +133,9 @@ private Optional fixVariable( SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); fixBuilder.replace(tree.getType(), unboxed.tsym.getSimpleName().toString()); - fixMethodInvocations(scanner.fixableSimpleMethodInvocations, fixBuilder, state); - fixNullCheckInvocations(scanner.fixableNullCheckInvocations, fixBuilder, state); - fixCastingInvocations(scanner.fixableCastMethodInvocations, enclosingMethod, fixBuilder, state); + fixMethodInvocations(usages.fixableSimpleMethodInvocations.get(varSymbol), fixBuilder, state); + fixNullCheckInvocations(usages.fixableNullCheckInvocations.get(varSymbol), fixBuilder, state); + fixCastingInvocations(usages.fixableCastMethodInvocations.get(varSymbol), fixBuilder, state); // Remove @Nullable annotation, if present. AnnotationTree nullableAnnotation = @@ -198,15 +211,12 @@ private static void fixMethodInvocations( } private static void fixCastingInvocations( - List castMethodInvocations, - TreePath enclosingMethod, - SuggestedFix.Builder fixBuilder, - VisitorState state) { - for (MethodInvocationTree castInvocation : castMethodInvocations) { + List castMethodInvocations, SuggestedFix.Builder fixBuilder, VisitorState state) { + for (TreePath castPath : castMethodInvocations) { + MethodInvocationTree castInvocation = (MethodInvocationTree) castPath.getLeaf(); ExpressionTree receiver = ASTHelpers.getReceiver(castInvocation); Type expressionType = ASTHelpers.getType(castInvocation); - TreePath castPath = TreePath.getPath(enclosingMethod, castInvocation); if (castPath.getParentPath() != null && castPath.getParentPath().getLeaf().getKind() == Kind.EXPRESSION_STATEMENT) { // If we were to replace X.intValue(); with (int) x;, the code wouldn't compile because @@ -267,18 +277,6 @@ private static boolean variableMatches(VariableTree tree, VisitorState state) { return VALUE_OF_MATCHER.matches(expression, state); } - private static Optional getEnclosingMethod(TreePath path) { - while (path != null - && path.getLeaf().getKind() != Kind.CLASS - && path.getLeaf().getKind() != Kind.LAMBDA_EXPRESSION) { - if (path.getLeaf().getKind() == Kind.METHOD) { - return Optional.of(path); - } - path = path.getParentPath(); - } - return Optional.empty(); - } - private static boolean isBoxableType(Type type, VisitorState state) { Type unboxedType = state.getTypes().unboxedType(type); return unboxedType != null && unboxedType.getTag() != TypeTag.NONE; @@ -314,25 +312,25 @@ private static class FindBoxedUsagesScanner extends TreePathScanner staticMethod().onClass("com.google.common.base.Verify").named("verifyNonNull"), staticMethod().onClass("java.util.Objects").named("requireNonNull")); - private final VarSymbol varSymbol; - private final TreePath path; private final VisitorState state; - private final List fixableSimpleMethodInvocations = new ArrayList<>(); - private final List fixableNullCheckInvocations = new ArrayList<>(); - private final List fixableCastMethodInvocations = new ArrayList<>(); + private final ListMultimap fixableSimpleMethodInvocations = + ArrayListMultimap.create(); + private final ListMultimap fixableNullCheckInvocations = + ArrayListMultimap.create(); + private final ListMultimap fixableCastMethodInvocations = + ArrayListMultimap.create(); - private boolean boxedUsageFound; - private boolean used; + private final Set boxedUsageFound = new HashSet<>(); + private final Set dereferenced = new HashSet<>(); - FindBoxedUsagesScanner(VarSymbol varSymbol, TreePath path, VisitorState state) { - this.varSymbol = varSymbol; - this.path = path; + FindBoxedUsagesScanner(VisitorState state) { this.state = state; } @Override public Void scan(Tree tree, Void unused) { - if (boxedUsageFound) { + var symbol = getSymbol(tree); + if (boxedUsageFound.contains(symbol)) { return null; } return super.scan(tree, unused); @@ -341,17 +339,16 @@ public Void scan(Tree tree, Void unused) { @Override public Void visitAssignment(AssignmentTree node, Void unused) { Symbol nodeSymbol = getSymbol(node.getVariable()); - if (!Objects.equals(nodeSymbol, varSymbol)) { + if (!isBoxed(nodeSymbol, state)) { return super.visitAssignment(node, unused); } - used = true; // The variable of interest is being assigned. Check if the expression is non-primitive, // and go on to scan the expression. if (!checkAssignmentExpression(node.getExpression())) { return scan(node.getExpression(), unused); } - boxedUsageFound = true; + boxedUsageFound.add((VarSymbol) nodeSymbol); return null; } @@ -363,21 +360,19 @@ private boolean checkAssignmentExpression(ExpressionTree expression) { // If the value is assigned a non-primitive value, we need to keep it non-primitive. // Unless it's an invocation of Boxed.valueOf or new Boxed, in which case it doesn't need to // be kept boxed since we know the result of valueOf is non-null. - return !VALUE_OF_MATCHER.matches( - expression, state.withPath(TreePath.getPath(path, expression))) + return !VALUE_OF_MATCHER.matches(expression, state.withPath(getCurrentPath())) && expression.getKind() != Kind.NEW_CLASS; } @Override public Void visitIdentifier(IdentifierTree node, Void unused) { Symbol nodeSymbol = getSymbol(node); - if (Objects.equals(nodeSymbol, varSymbol)) { - used = true; - TreePath identifierPath = TreePath.getPath(path, node); - VisitorState identifierState = state.withPath(identifierPath); + if (isBoxed(nodeSymbol, state)) { + dereferenced.add((VarSymbol) nodeSymbol); + VisitorState identifierState = state.withPath(getCurrentPath()); TargetType targetType = ASTHelpers.targetType(identifierState); if (targetType != null && !targetType.type().isPrimitive()) { - boxedUsageFound = true; + boxedUsageFound.add((VarSymbol) nodeSymbol); return null; } } @@ -395,26 +390,26 @@ public Void visitCompoundAssignment(CompoundAssignmentTree node, Void unused) { public Void visitMethodInvocation(MethodInvocationTree node, Void unused) { if (NULL_CHECK_MATCH.matches(node, state)) { Symbol firstArgSymbol = getSymbol(ASTHelpers.stripParentheses(node.getArguments().get(0))); - if (Objects.equals(firstArgSymbol, varSymbol)) { - used = true; - fixableNullCheckInvocations.add(getCurrentPath()); + if (isBoxed(firstArgSymbol, state)) { + dereferenced.add((VarSymbol) firstArgSymbol); + fixableNullCheckInvocations.put((VarSymbol) firstArgSymbol, getCurrentPath()); return null; } } Tree receiver = ASTHelpers.getReceiver(node); - if (receiver != null && Objects.equals(getSymbol(receiver), varSymbol)) { - used = true; + Symbol receiverSymbol = getSymbol(receiver); + if (receiver != null && isBoxed(receiverSymbol, state)) { if (SIMPLE_METHOD_MATCH.matches(node, state)) { - fixableSimpleMethodInvocations.add(node); + fixableSimpleMethodInvocations.put((VarSymbol) receiverSymbol, node); return null; } if (CAST_METHOD_MATCH.matches(node, state)) { - fixableCastMethodInvocations.add(node); + fixableCastMethodInvocations.put((VarSymbol) receiverSymbol, getCurrentPath()); return null; } - boxedUsageFound = true; + boxedUsageFound.add((VarSymbol) receiverSymbol); return null; } @@ -424,19 +419,19 @@ public Void visitMethodInvocation(MethodInvocationTree node, Void unused) { @Override public Void visitReturn(ReturnTree node, Void unused) { Symbol nodeSymbol = getSymbol(ASTHelpers.stripParentheses(node.getExpression())); - if (!Objects.equals(nodeSymbol, varSymbol)) { + if (!isBoxed(nodeSymbol, state)) { return super.visitReturn(node, unused); } - used = true; + dereferenced.add((VarSymbol) nodeSymbol); // Don't count a return value as a boxed usage, except if we are returning a parameter, and // the method's return type is boxed. - if (varSymbol.getKind() == ElementKind.PARAMETER) { + if (nodeSymbol.getKind() == ElementKind.PARAMETER) { MethodTree enclosingMethod = ASTHelpers.findEnclosingNode(getCurrentPath(), MethodTree.class); Type returnType = ASTHelpers.getType(enclosingMethod.getReturnType()); if (!returnType.isPrimitive()) { - boxedUsageFound = true; + boxedUsageFound.add((VarSymbol) nodeSymbol); } } return null; @@ -447,13 +442,18 @@ public Void visitMemberReference(MemberReferenceTree node, Void unused) { ExpressionTree qualifierExpression = node.getQualifierExpression(); if (qualifierExpression.getKind() == Kind.IDENTIFIER) { Symbol symbol = getSymbol(qualifierExpression); - if (Objects.equals(symbol, varSymbol)) { - boxedUsageFound = true; - used = true; + if (isBoxed(symbol, state)) { + boxedUsageFound.add((VarSymbol) symbol); + dereferenced.add((VarSymbol) symbol); return null; } } return super.visitMemberReference(node, unused); } } + + private static boolean isBoxed(Symbol symbol, VisitorState state) { + return symbol instanceof VarSymbol + && !state.getTypes().isSameType(state.getTypes().unboxedType(symbol.type), Type.noType); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariableTest.java index e7dddff7a49..5ea8f143a2c 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariableTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariableTest.java @@ -36,4 +36,19 @@ public void testCases() { .addOutput("testdata/UnnecessaryBoxedVariableCases_expected.java") .doTest(); } + + @Test + public void suppression() { + helper + .addInputLines( + "Test.java", + "class Test {", + " @SuppressWarnings(\"UnnecessaryBoxedVariable\")", + " private int a(Integer o) {", + " return o;", + " }", + "}") + .expectUnchanged() + .doTest(); + } }