Skip to content

Commit

Permalink
UnnecessaryBoxedVariable: scan the entire compilation unit at once fo…
Browse files Browse the repository at this point in the history 
…r 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
  • Loading branch information
@graememorgan
graememorgan authored and Error Prone Team committed Apr 6, 2022
1 parent 80fdaa8 commit 112b5d1
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 71 deletions.
142 changes: 71 additions & 71 deletions core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariable.java
View file
Expand Up @@ -23,17 +23,20 @@
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;
import com.google.errorprone.util.ASTHelpers;
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;
Expand All @@ -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;

/**
Expand All @@ -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<ExpressionTree> 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<Void, Void>() {
@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<Description> handleVariable(
Type unboxed, VariableTree tree, VisitorState state) {
Type unboxed, FindBoxedUsagesScanner usages, VariableTree tree, VisitorState state) {
VarSymbol varSymbol = getSymbol(tree);
switch (varSymbol.getKind()) {
case PARAMETER:
Expand All @@ -99,19 +115,16 @@ private Optional<Description> handleVariable(
return Optional.empty();
}

return getEnclosingMethod(state.getPath())
.flatMap(enclosingMethod -> fixVariable(unboxed, tree, enclosingMethod, state));
return fixVariable(unboxed, usages, tree, state);
}

private Optional<Description> 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();
Expand All @@ -120,9 +133,9 @@ private Optional<Description> 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 =
Expand Down Expand Up @@ -198,15 +211,12 @@ private static void fixMethodInvocations(
}

private static void fixCastingInvocations(
List<MethodInvocationTree> castMethodInvocations,
TreePath enclosingMethod,
SuggestedFix.Builder fixBuilder,
VisitorState state) {
for (MethodInvocationTree castInvocation : castMethodInvocations) {
List<TreePath> 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
Expand Down Expand Up @@ -267,18 +277,6 @@ private static boolean variableMatches(VariableTree tree, VisitorState state) {
return VALUE_OF_MATCHER.matches(expression, state);
}

private static Optional<TreePath> 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;
Expand Down Expand Up @@ -314,25 +312,25 @@ private static class FindBoxedUsagesScanner extends TreePathScanner<Void, Void>
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<MethodInvocationTree> fixableSimpleMethodInvocations = new ArrayList<>();
private final List<TreePath> fixableNullCheckInvocations = new ArrayList<>();
private final List<MethodInvocationTree> fixableCastMethodInvocations = new ArrayList<>();
private final ListMultimap<VarSymbol, MethodInvocationTree> fixableSimpleMethodInvocations =
ArrayListMultimap.create();
private final ListMultimap<VarSymbol, TreePath> fixableNullCheckInvocations =
ArrayListMultimap.create();
private final ListMultimap<VarSymbol, TreePath> fixableCastMethodInvocations =
ArrayListMultimap.create();

private boolean boxedUsageFound;
private boolean used;
private final Set<VarSymbol> boxedUsageFound = new HashSet<>();
private final Set<VarSymbol> 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);
Expand All @@ -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;
}

Expand All @@ -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;
}
}
Expand All @@ -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;
}

Expand All @@ -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;
Expand All @@ -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);
}
}
15 changes: 15 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariableTest.java
View file
Expand Up @@ -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();
}
}

0 comments on commit 112b5d1

Please sign in to comment.