From 4c9728b022436631230768d2c29071ccb4565ebb Mon Sep 17 00:00:00 2001 From: Md Tanvir Alam Date: Wed, 26 Aug 2026 19:05:32 +0000 Subject: [PATCH] Recognize split declaration/assignment in UnnecessaryAsync `AtomicInteger a; a = new AtomicInteger(0);` is equivalent to `AtomicInteger a = new AtomicInteger(0);`, but the check required an initializer on the VariableTree. Find a sole initializing assignment, treat it like a declarator initializer for escape analysis, and delete that assignment statement when rewriting to a plain local. Fixes #5829 --- .../bugpatterns/UnnecessaryAsync.java | 79 ++++++++++++++++--- .../bugpatterns/UnnecessaryAsyncTest.java | 55 +++++++++++++ 2 files changed, 125 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java index bc4c06629c9..0de633a290d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java @@ -33,6 +33,8 @@ import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.ExpressionStatementTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.LambdaExpressionTree; @@ -42,10 +44,13 @@ import com.sun.source.tree.NewClassTree; import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePath; import com.sun.source.util.TreePathScanner; import com.sun.tools.javac.code.Symbol.VarSymbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Stream; import javax.lang.model.element.ElementKind; @@ -75,8 +80,17 @@ public Description matchVariable(VariableTree tree, VisitorState state) { if (!symbol.getKind().equals(ElementKind.LOCAL_VARIABLE) || !isConsideredFinal(symbol)) { return NO_MATCH; } - var initializer = tree.getInitializer(); - if (initializer == null || !NEW_SYNCHRONIZED_THING.matches(initializer, state)) { + ExpressionTree initializer = tree.getInitializer(); + AssignmentTree initializingAssignment = null; + if (initializer == null) { + // Support `T x; x = new Atomic...(...);` the same as `T x = new Atomic...(...);`. + initializingAssignment = findSoleInitializingAssignment(symbol, state); + if (initializingAssignment == null) { + return NO_MATCH; + } + initializer = initializingAssignment.getExpression(); + } + if (!NEW_SYNCHRONIZED_THING.matches(initializer, state)) { return NO_MATCH; } AtomicBoolean escapes = new AtomicBoolean(false); @@ -111,8 +125,10 @@ public Void visitIdentifier(IdentifierTree tree, Void unused) { } var parentTree = getCurrentPath().getParentPath().getLeaf(); // Anything other than a method invocation on our symbol constitutes a reference to it - // escaping. - if (isVariableDeclarationItself(parentTree) || parentTree instanceof MemberSelectTree) { + // escaping. An initializing assignment is equivalent to a declarator initializer. + if (isVariableDeclarationItself(parentTree) + || isAssignmentToThisSymbol(parentTree) + || parentTree instanceof MemberSelectTree) { return super.visitIdentifier(tree, null); } escapes.set(true); @@ -122,11 +138,43 @@ public Void visitIdentifier(IdentifierTree tree, Void unused) { private boolean isVariableDeclarationItself(Tree parentTree) { return parentTree instanceof VariableTree && getSymbol(parentTree).equals(symbol); } + + private boolean isAssignmentToThisSymbol(Tree parentTree) { + return parentTree instanceof AssignmentTree assignmentTree + && symbol.equals(getSymbol(assignmentTree.getVariable())); + } + }.scan(state.getPath().getParentPath(), null); + return escapes.get() + ? NO_MATCH + : describeMatch( + tree, + attemptFix(tree, state, (NewClassTree) initializer, initializingAssignment)); + } + + /** + * Finds the sole assignment to {@code symbol} in the enclosing scope, used when the variable is + * declared without an initializer ({@code T x; x = ...;}). + */ + private static AssignmentTree findSoleInitializingAssignment( + VarSymbol symbol, VisitorState state) { + List assignments = new ArrayList<>(); + new TreePathScanner() { + @Override + public Void visitAssignment(AssignmentTree tree, Void unused) { + if (symbol.equals(getSymbol(tree.getVariable()))) { + assignments.add(tree); + } + return super.visitAssignment(tree, null); + } }.scan(state.getPath().getParentPath(), null); - return escapes.get() ? NO_MATCH : describeMatch(tree, attemptFix(tree, state)); + return assignments.size() == 1 ? assignments.get(0) : null; } - private SuggestedFix attemptFix(VariableTree tree, VisitorState state) { + private SuggestedFix attemptFix( + VariableTree tree, + VisitorState state, + NewClassTree constructor, + AssignmentTree initializingAssignment) { var symbol = getSymbol(tree); if (!symbol.type.toString().startsWith("java.util.concurrent.atomic")) { return SuggestedFix.emptyFix(); @@ -135,8 +183,6 @@ private SuggestedFix attemptFix(VariableTree tree, VisitorState state) { AtomicBoolean fixable = new AtomicBoolean(true); SuggestedFix.Builder fix = SuggestedFix.builder(); - var constructor = (NewClassTree) tree.getInitializer(); - fix.replace( tree, format( @@ -146,6 +192,16 @@ private SuggestedFix attemptFix(VariableTree tree, VisitorState state) { constructor.getArguments().isEmpty() ? getDefaultInitializer(symbol, state.getTypes()) : state.getSourceForNode(constructor.getArguments().get(0)))); + if (initializingAssignment != null) { + TreePath assignmentPath = + TreePath.getPath(state.getPath().getCompilationUnit(), initializingAssignment); + Tree parent = assignmentPath.getParentPath().getLeaf(); + if (parent instanceof ExpressionStatementTree) { + fix.delete(parent); + } else { + fix.delete(initializingAssignment); + } + } new TreePathScanner() { @Override @@ -154,7 +210,7 @@ public Void visitIdentifier(IdentifierTree tree, Void unused) { return super.visitIdentifier(tree, null); } var parentTree = getCurrentPath().getParentPath().getLeaf(); - if (isVariableDeclarationItself(parentTree)) { + if (isVariableDeclarationItself(parentTree) || isAssignmentToThisSymbol(parentTree)) { return super.visitIdentifier(tree, null); } if (parentTree instanceof MemberSelectTree memberSelectTree) { @@ -197,6 +253,11 @@ public Void visitIdentifier(IdentifierTree tree, Void unused) { private boolean isVariableDeclarationItself(Tree parentTree) { return parentTree instanceof VariableTree && getSymbol(parentTree).equals(symbol); } + + private boolean isAssignmentToThisSymbol(Tree parentTree) { + return parentTree instanceof AssignmentTree assignmentTree + && symbol.equals(getSymbol(assignmentTree.getVariable())); + } }.scan(state.getPath().getParentPath(), null); return fixable.get() ? fix.build() : SuggestedFix.emptyFix(); } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java index 1d907c185bc..bfe5239b802 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java @@ -292,4 +292,59 @@ int test() { """) .doTest(); } + + @Test + public void splitDeclarationAndAssignment() { + helper + .addSourceLines( + "Test.java", + """ + import java.util.concurrent.atomic.AtomicInteger; + + class Test { + int f() { + // BUG: Diagnostic contains: + AtomicInteger a; + a = new AtomicInteger(0); + a.incrementAndGet(); + return a.get(); + } + } + """) + .doTest(); + } + + @Test + public void splitDeclarationAndAssignment_refactoring() { + refactoring + .addInputLines( + "Test.java", + """ + import java.util.concurrent.atomic.AtomicInteger; + + class Test { + int f() { + AtomicInteger a; + a = new AtomicInteger(0); + a.incrementAndGet(); + return a.get(); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.concurrent.atomic.AtomicInteger; + + class Test { + int f() { + int a = 0; + + ++a; + return a; + } + } + """) + .doTest(); + } }