From 112b5d18f8b7386898d92180a5a806fd9b684f70 Mon Sep 17 00:00:00 2001 From: ghm Date: Wed, 6 Apr 2022 03:27:45 -0700 Subject: [PATCH] UnnecessaryBoxedVariable: scan the entire compilation unit at once for boxed variable usages rather than doing so for each method on each boxed variable. I'd assume this is more efficient, or at least has more bounded inefficiency. It's in preparation for handling private fields, too, given that requires scanning the entire compilation unit, and doing that for each field would be... oops. PiperOrigin-RevId: 439791137 --- .../bugpatterns/UnnecessaryBoxedVariable.java | 142 +++++++++--------- .../UnnecessaryBoxedVariableTest.java | 15 ++ 2 files changed, 86 insertions(+), 71 deletions(-) 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(); + } }