From 66537dbfaac6b10cbd9051e4f8399cdc778f7e71 Mon Sep 17 00:00:00 2001 From: ghm Date: Wed, 6 Sep 2023 10:00:14 -0700 Subject: [PATCH] Attempt some fixes for UnnecessaryAsync. PiperOrigin-RevId: 563136533 --- .../bugpatterns/UnnecessaryAsync.java | 133 +++++++++++++++++- .../bugpatterns/UnnecessaryAsyncTest.java | 97 +++++++++++++ 2 files changed, 228 insertions(+), 2 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 d50f037e660..8a978fd1f06 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java @@ -22,21 +22,28 @@ import static com.google.errorprone.matchers.Matchers.constructor; import static com.google.errorprone.util.ASTHelpers.getSymbol; import static com.google.errorprone.util.ASTHelpers.isConsideredFinal; +import static java.lang.String.format; 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.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.LambdaExpressionTree; import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; +import com.sun.source.tree.NewClassTree; import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; 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.concurrent.atomic.AtomicBoolean; import java.util.stream.Stream; import javax.lang.model.element.ElementKind; @@ -114,7 +121,129 @@ private boolean isVariableDeclarationItself(Tree parentTree) { return parentTree instanceof VariableTree && getSymbol(parentTree).equals(symbol); } }.scan(state.getPath().getParentPath(), null); - // TODO(ghm): Include an attempted fix, if possible. - return escapes.get() ? NO_MATCH : describeMatch(tree); + return escapes.get() ? NO_MATCH : describeMatch(tree, attemptFix(tree, state)); + } + + private SuggestedFix attemptFix(VariableTree tree, VisitorState state) { + var symbol = getSymbol(tree); + if (!symbol.type.toString().startsWith("java.util.concurrent.atomic")) { + return SuggestedFix.emptyFix(); + } + + AtomicBoolean fixable = new AtomicBoolean(true); + SuggestedFix.Builder fix = SuggestedFix.builder(); + + var constructor = (NewClassTree) tree.getInitializer(); + + fix.replace( + tree, + format( + "%s %s = %s;", + getPrimitiveType(symbol.type, state.getTypes()), + symbol.getSimpleName(), + constructor.getArguments().isEmpty() + ? getDefaultInitializer(symbol) + : state.getSourceForNode(constructor.getArguments().get(0)))); + + new TreePathScanner() { + @Override + public Void visitIdentifier(IdentifierTree tree, Void unused) { + if (!getSymbol(tree).equals(symbol)) { + return super.visitIdentifier(tree, null); + } + var parentTree = getCurrentPath().getParentPath().getLeaf(); + if (isVariableDeclarationItself(parentTree)) { + return super.visitIdentifier(tree, null); + } + if (parentTree instanceof MemberSelectTree) { + var grandparent = + (MethodInvocationTree) getCurrentPath().getParentPath().getParentPath().getLeaf(); + if (((MemberSelectTree) parentTree).getIdentifier().contentEquals("set")) { + fix.replace( + grandparent, + format( + "%s = %s", + state.getSourceForNode(tree), + state.getSourceForNode(grandparent.getArguments().get(0)))); + } else if (((MemberSelectTree) parentTree).getIdentifier().contentEquals("get")) { + fix.replace(grandparent, state.getSourceForNode(tree)); + } else if (((MemberSelectTree) parentTree) + .getIdentifier() + .contentEquals("getAndIncrement")) { + fix.replace(grandparent, format("%s++", state.getSourceForNode(tree))); + } else if (((MemberSelectTree) parentTree) + .getIdentifier() + .contentEquals("getAndDecrement")) { + fix.replace(grandparent, format("%s--", state.getSourceForNode(tree))); + } else if (((MemberSelectTree) parentTree) + .getIdentifier() + .contentEquals("incrementAndGet")) { + fix.replace(grandparent, format("++%s", state.getSourceForNode(tree))); + } else if (((MemberSelectTree) parentTree) + .getIdentifier() + .contentEquals("decrementAndGet")) { + fix.replace(grandparent, format("--%s", state.getSourceForNode(tree))); + } else if (((MemberSelectTree) parentTree) + .getIdentifier() + .contentEquals("compareAndSet")) { + fix.replace( + grandparent, + format( + "%s = %s", + state.getSourceForNode(tree), + state.getSourceForNode(grandparent.getArguments().get(1)))); + } else if (((MemberSelectTree) parentTree).getIdentifier().contentEquals("addAndGet")) { + fix.replace( + grandparent, + format( + "%s += %s", + state.getSourceForNode(tree), + state.getSourceForNode(grandparent.getArguments().get(0)))); + } else { + fixable.set(false); + } + + } else { + fixable.set(false); + } + return super.visitIdentifier(tree, null); + } + + private boolean isVariableDeclarationItself(Tree parentTree) { + return parentTree instanceof VariableTree && getSymbol(parentTree).equals(symbol); + } + }.scan(state.getPath().getParentPath(), null); + return fixable.get() ? fix.build() : SuggestedFix.emptyFix(); + } + + private static String getPrimitiveType(Type type, Types types) { + switch (types.erasure(type).toString()) { + case "java.util.concurrent.atomic.AtomicBoolean": + return "boolean"; + case "java.util.concurrent.atomic.AtomicReference": + return type.allparams().isEmpty() + ? "Object" + : type.allparams().get(0).tsym.getSimpleName().toString(); + case "java.util.concurrent.atomic.AtomicInteger": + return "int"; + case "java.util.concurrent.atomic.AtomicLong": + return "long"; + default: // fall out + } + throw new AssertionError(); + } + + private static String getDefaultInitializer(VarSymbol symbol) { + switch (symbol.type.toString()) { + case "java.util.concurrent.atomic.AtomicBoolean": + return "false"; + case "java.util.concurrent.atomic.AtomicReference": + return "null"; + case "java.util.concurrent.atomic.AtomicInteger": + case "java.util.concurrent.atomic.AtomicLong": + return "0"; + default: // fall out + } + throw new AssertionError(); } } 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 cf6ed680cfa..8d46355b9a7 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java @@ -16,6 +16,7 @@ package com.google.errorprone.bugpatterns; +import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.CompilationTestHelper; import org.junit.Test; import org.junit.runner.RunWith; @@ -25,6 +26,8 @@ public final class UnnecessaryAsyncTest { private final CompilationTestHelper helper = CompilationTestHelper.newInstance(UnnecessaryAsync.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoring = + BugCheckerRefactoringTestHelper.newInstance(UnnecessaryAsync.class, getClass()); @Test public void positive() { @@ -43,6 +46,100 @@ public void positive() { .doTest(); } + @Test + public void refactoringInteger() { + refactoring + .addInputLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicInteger;", + "class Test {", + " int test() {", + " var ai = new AtomicInteger();", + " ai.set(1);", + " return ai.get();", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicInteger;", + "class Test {", + " int test() {", + " int ai = 0;", + " ai = 1;", + " return ai;", + " }", + "}") + .doTest(); + } + + @Test + public void refactoringReference() { + refactoring + .addInputLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicReference;", + "class Test {", + " String test() {", + " var ar = new AtomicReference(null);", + " ar.compareAndSet(null, \"foo\");", + " return ar.get();", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicReference;", + "class Test {", + " String test() {", + " String ar = null;", + " ar = \"foo\";", + " return ar;", + " }", + "}") + .doTest(); + } + + @Test + public void refactoring_unfixable_noAttempt() { + refactoring + .addInputLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicReference;", + "class Test {", + " String test() {", + " var ar = new AtomicReference(null);", + " return ar.toString();", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void refactoring_rawType() { + refactoring + .addInputLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicReference;", + "class Test {", + " Object test() {", + " var ar = new AtomicReference();", + " ar.set(\"foo\");", + " return ar.get();", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicReference;", + "class Test {", + " Object test() {", + " Object ar = null;", + " ar = \"foo\";", + " return ar;", + " }", + "}") + .doTest(); + } + @Test public void negative_escapesScope() { helper