Skip to content

Commit

Permalink
Add more suggested fixes to JdkObsolete
Browse files Browse the repository at this point in the history 
Including construction of obsolete classes in lambdas, return statements,
method arguments, and method references.

MOE_MIGRATED_REVID=176500430
  • Loading branch information
@cushon
cushon committed Nov 27, 2017
1 parent 4d1244e commit 65e2048
Show file tree
Hide file tree
Showing 2 changed files with 145 additions and 23 deletions.
102 changes: 79 additions & 23 deletions core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java
View file
Expand Up @@ -27,21 +27,26 @@
import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState; import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.MemberReferenceTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.ClassTree; import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.LambdaExpressionTree;
import com.sun.source.tree.MemberReferenceTree;
import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree; import com.sun.source.tree.MethodTree;
import com.sun.source.tree.NewClassTree; import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.ParameterizedTypeTree; import com.sun.source.tree.ParameterizedTypeTree;
import com.sun.source.tree.ReturnTree;
import com.sun.source.tree.Tree; import com.sun.source.tree.Tree;
import com.sun.source.tree.VariableTree; import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath; import com.sun.source.util.TreePath;
Expand All @@ -54,14 +59,16 @@
import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types; import com.sun.tools.javac.code.Types;
import java.util.Optional; import java.util.Optional;
import javax.annotation.Nullable;


/** @author cushon@google.com (Liam Miller-Cushon) */ /** @author cushon@google.com (Liam Miller-Cushon) */
@BugPattern( @BugPattern(
name = "JdkObsolete", name = "JdkObsolete",
summary = "Suggests alternatives to obsolete JDK classes.", summary = "Suggests alternatives to obsolete JDK classes.",
severity = WARNING severity = WARNING
) )
public class JdkObsolete extends BugChecker implements NewClassTreeMatcher, ClassTreeMatcher { public class JdkObsolete extends BugChecker
implements NewClassTreeMatcher, ClassTreeMatcher, MemberReferenceTreeMatcher {


static class Obsolete { static class Obsolete {
final String qualifiedName; final String qualifiedName;
Expand All @@ -80,7 +87,7 @@ String message() {
return message; return message;
} }


Optional<Fix> fix(VisitorState state) { Optional<Fix> fix(Tree tree, VisitorState state) {
return Optional.empty(); return Optional.empty();
} }
} }
Expand All @@ -92,8 +99,8 @@ Optional<Fix> fix(VisitorState state) {
"It is very rare for LinkedList to out-perform ArrayList or ArrayDeque. Avoid it" "It is very rare for LinkedList to out-perform ArrayList or ArrayDeque. Avoid it"
+ " unless you're willing to invest a lot of time into benchmarking.") { + " unless you're willing to invest a lot of time into benchmarking.") {
@Override @Override
Optional<Fix> fix(VisitorState state) { Optional<Fix> fix(Tree tree, VisitorState state) {
return linkedListFix(state); return linkedListFix(tree, state);
} }
}, },
new Obsolete( new Obsolete(
Expand All @@ -112,7 +119,7 @@ Optional<Fix> fix(VisitorState state) {
"StringBuffer performs synchronization that is usually unnecessary;" "StringBuffer performs synchronization that is usually unnecessary;"
+ " prefer StringBuilder.") { + " prefer StringBuilder.") {
@Override @Override
Optional<Fix> fix(VisitorState state) { Optional<Fix> fix(Tree tree, VisitorState state) {
return stringBufferFix(state); return stringBufferFix(state);
} }
}, },
Expand Down Expand Up @@ -148,7 +155,8 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) {
Symbol owner = constructor.owner; Symbol owner = constructor.owner;
Description description = Description description =
describeIfObsolete( describeIfObsolete(
tree, // don't refactor anonymous implementations of LinkedList
tree.getClassBody() == null ? tree.getIdentifier() : null,
owner.name.isEmpty() owner.name.isEmpty()
? state.getTypes().directSupertypes(owner.asType()) ? state.getTypes().directSupertypes(owner.asType())
: ImmutableList.of(owner.asType()), : ImmutableList.of(owner.asType()),
Expand Down Expand Up @@ -185,10 +193,18 @@ public Description matchClass(ClassTree tree, VisitorState state) {
if (symbol == null) { if (symbol == null) {
return NO_MATCH; return NO_MATCH;
} }
return describeIfObsolete(tree, state.getTypes().directSupertypes(symbol.asType()), state); return describeIfObsolete(null, state.getTypes().directSupertypes(symbol.asType()), state);
} }


private Description describeIfObsolete(Tree tree, Iterable<Type> types, VisitorState state) { @Override
public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) {
MethodSymbol symbol = ASTHelpers.getSymbol(tree);
return describeIfObsolete(
tree.getQualifierExpression(), ImmutableList.of(symbol.owner.asType()), state);
}

private Description describeIfObsolete(
@Nullable Tree tree, Iterable<Type> types, VisitorState state) {
for (Type type : types) { for (Type type : types) {
Obsolete obsolete = OBSOLETE.get(type.asElement().getQualifiedName().toString()); Obsolete obsolete = OBSOLETE.get(type.asElement().getQualifiedName().toString());
if (obsolete == null) { if (obsolete == null) {
Expand All @@ -197,35 +213,74 @@ private Description describeIfObsolete(Tree tree, Iterable<Type> types, VisitorS
if (implementingObsoleteMethod(state, type)) { if (implementingObsoleteMethod(state, type)) {
continue; continue;
} }
Description.Builder description = buildDescription(tree).setMessage(obsolete.message()); Description.Builder description =
obsolete.fix(state).ifPresent(description::addFix); buildDescription(state.getPath().getLeaf()).setMessage(obsolete.message());
if (tree != null) {
obsolete.fix(tree, state).ifPresent(description::addFix);
}
return description.build(); return description.build();
} }
return NO_MATCH; return NO_MATCH;
} }


// rewrite e.g. `List<Object> xs = new LinkedList<>()` -> `... = new ArrayList<>()` private static Type getMethodOrLambdaReturnType(VisitorState state) {
private static Optional<Fix> linkedListFix(VisitorState state) { for (Tree tree : state.getPath()) {
switch (tree.getKind()) {
case LAMBDA_EXPRESSION:
return state.getTypes().findDescriptorType(ASTHelpers.getType(tree)).getReturnType();
case METHOD:
return ASTHelpers.getType(tree).getReturnType();
case CLASS:
return null;
default: // fall out
}
}
return null;
}

static Type getTargetType(VisitorState state) {
Tree parent = state.getPath().getParentPath().getLeaf(); Tree parent = state.getPath().getParentPath().getLeaf();
if (!(parent instanceof VariableTree)) { Type type;
return Optional.empty(); if (parent instanceof VariableTree || parent instanceof AssignmentTree) {
type = ASTHelpers.getType(parent);
} else if (parent instanceof ReturnTree || parent instanceof LambdaExpressionTree) {
type = getMethodOrLambdaReturnType(state);
} else if (parent instanceof MethodInvocationTree) {
MethodInvocationTree tree = (MethodInvocationTree) parent;
int idx = tree.getArguments().indexOf(state.getPath().getLeaf());
if (idx == -1) {
return null;
}
Type methodType = ASTHelpers.getType(tree.getMethodSelect());
if (idx >= methodType.getParameterTypes().size()) {
return null;
}
return methodType.getParameterTypes().get(idx);
} else {
return null;
} }
VariableTree varTree = (VariableTree) parent; Tree tree = state.getPath().getLeaf();
if (!(varTree.getInitializer() instanceof NewClassTree)) { if (tree instanceof MemberReferenceTree) {
type = state.getTypes().findDescriptorType(ASTHelpers.getType(tree)).getReturnType();
}
return type;
}

// rewrite e.g. `List<Object> xs = new LinkedList<>()` -> `... = new ArrayList<>()`
private static Optional<Fix> linkedListFix(Tree tree, VisitorState state) {
Type type = getTargetType(state);
if (type == null) {
return Optional.empty(); return Optional.empty();
} }
NewClassTree init = (NewClassTree) varTree.getInitializer();
Type varType = ASTHelpers.getType(parent);
Types types = state.getTypes(); Types types = state.getTypes();
for (String replacement : ImmutableList.of("java.util.ArrayList", "java.util.ArrayDeque")) { for (String replacement : ImmutableList.of("java.util.ArrayList", "java.util.ArrayDeque")) {
Symbol sym = state.getSymbolFromString(replacement); Symbol sym = state.getSymbolFromString(replacement);
if (types.isAssignable(types.erasure(sym.asType()), types.erasure(varType))) { if (types.isAssignable(types.erasure(sym.asType()), types.erasure(type))) {
SuggestedFix.Builder fix = SuggestedFix.builder(); SuggestedFix.Builder fix = SuggestedFix.builder();
Tree typeTree = init.getIdentifier(); while (tree instanceof ParameterizedTypeTree) {
if (typeTree instanceof ParameterizedTypeTree) { tree = ((ParameterizedTypeTree) tree).getType();
typeTree = ((ParameterizedTypeTree) typeTree).getType();
} }
fix.replace(typeTree, SuggestedFixes.qualifyType(state, fix, sym)); fix.replace(tree, SuggestedFixes.qualifyType(state, fix, sym));
return Optional.of(fix.build()); return Optional.of(fix.build());
} }
} }
Expand Down Expand Up @@ -320,3 +375,4 @@ private static boolean implementingObsoleteMethod(VisitorState state, Type type)
return true; return true;
} }
} }

66 changes: 66 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/JdkObsoleteTest.java
View file
Expand Up @@ -123,15 +123,27 @@ public void refactoring() throws IOException {
"class Test {", "class Test {",
" Deque<Object> d = new LinkedList<>();", " Deque<Object> d = new LinkedList<>();",
" List<Object> l = new LinkedList<>();", " List<Object> l = new LinkedList<>();",
" {",
" l = new LinkedList<>();",
" }",
" LinkedList<Object> ll = new LinkedList<>();", " LinkedList<Object> ll = new LinkedList<>();",
" List<Object> lll = new LinkedList<Object>() {{",
" add(null); // yikes",
" }};",
"}") "}")
.addOutputLines( .addOutputLines(
"out/Test.java", // "out/Test.java", //
"import java.util.*;", "import java.util.*;",
"class Test {", "class Test {",
" Deque<Object> d = new ArrayDeque<>();", " Deque<Object> d = new ArrayDeque<>();",
" List<Object> l = new ArrayList<>();", " List<Object> l = new ArrayList<>();",
" {",
" l = new ArrayList<>();",
" }",
" LinkedList<Object> ll = new LinkedList<>();", " LinkedList<Object> ll = new LinkedList<>();",
" List<Object> lll = new LinkedList<Object>() {{",
" add(null); // yikes",
" }};",
"}") "}")
.doTest(TEXT_MATCH); .doTest(TEXT_MATCH);
} }
Expand Down Expand Up @@ -195,4 +207,58 @@ public void obsoleteOverride() throws IOException {
"}") "}")
.doTest(); .doTest();
} }

@Test
public void additionalRefactorings() throws IOException {
BugCheckerRefactoringTestHelper.newInstance(new JdkObsolete(), getClass())
.addInputLines(
"in/Test.java", //
"import java.util.*;",
"import java.util.function.*;",
"class Test {",
" Supplier<Deque<Object>> a = () -> new LinkedList<>();",
" Supplier<Deque<Object>> b = () -> {",
" return new LinkedList<>();",
" };",
" Supplier<Deque<Object>> c = LinkedList::new;",
" Deque<Object> f() {",
" return new LinkedList<>();",
" }",
" void g(Deque<Object> x) {}",
" {",
" g(new LinkedList<>());",
" }",
" {",
" List<LinkedList<String>> xs = new ArrayList<>();",
" List<List<String>> ys = new ArrayList<>();",
" xs.add(new LinkedList<>());",
" ys.add(new LinkedList<>());",
" }",
"}")
.addOutputLines(
"out/Test.java", //
"import java.util.*;",
"import java.util.function.*;",
"class Test {",
" Supplier<Deque<Object>> a = () -> new ArrayDeque<>();",
" Supplier<Deque<Object>> b = () -> {",
" return new ArrayDeque<>();",
" };",
" Supplier<Deque<Object>> c = ArrayDeque::new;",
" Deque<Object> f() {",
" return new ArrayDeque<>();",
" }",
" void g(Deque<Object> x) {}",
" {",
" g(new ArrayDeque<>());",
" }",
" {",
" List<LinkedList<String>> xs = new ArrayList<>();",
" List<List<String>> ys = new ArrayList<>();",
" xs.add(new LinkedList<>());",
" ys.add(new ArrayList<>());",
" }",
"}")
.doTest(TEXT_MATCH);
}
} }

0 comments on commit 65e2048

Please sign in to comment.