Skip to content

Commit

Permalink
Attempt some fixes for UnnecessaryAsync.
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 562739388
  • Loading branch information
@graememorgan
graememorgan authored and Error Prone Team committed Sep 6, 2023
1 parent 83ce08c commit 47369d7
Show file tree
Hide file tree
Showing 2 changed files with 228 additions and 2 deletions.
133 changes: 131 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Void, Void>() {
@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();
}
}
97 changes: 97 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand All @@ -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<String>(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<String>(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
Expand Down

0 comments on commit 47369d7

Please sign in to comment.