Skip to content

Commit

Permalink
Improve WildcardImport fix
Browse files Browse the repository at this point in the history 
If a large number of members are imported from a single wildcard import,
qualify accesses to the members by the owner's simple name instead of
adding an import for each member.

MOE_MIGRATED_REVID=124804191
  • Loading branch information
@cushon
cushon committed Jun 16, 2016
1 parent 08e96fe commit 8a8c0dd
Show file tree
Hide file tree
Showing 2 changed files with 130 additions and 17 deletions.
88 changes: 71 additions & 17 deletions core/src/main/java/com/google/errorprone/bugpatterns/WildcardImport.java
View file
Expand Up @@ -23,6 +23,7 @@


import com.google.auto.value.AutoValue; import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.Multimap; import com.google.common.collect.Multimap;
import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern;
Expand All @@ -33,21 +34,28 @@
import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.ASTHelpers;


import com.sun.source.tree.CaseTree;
import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.CompilationUnitTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.ImportTree; import com.sun.source.tree.ImportTree;
import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.Tree; import com.sun.source.tree.Tree;
import com.sun.source.util.TreePathScanner;
import com.sun.tools.javac.code.Scope.StarImportScope; import com.sun.tools.javac.code.Scope.StarImportScope;
import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree;
import com.sun.tools.javac.tree.JCTree.JCCompilationUnit;
import com.sun.tools.javac.tree.JCTree.JCIdent;
import com.sun.tools.javac.tree.TreeScanner; import com.sun.tools.javac.tree.TreeScanner;
import com.sun.tools.javac.util.Name;


import java.util.Collection;
import java.util.LinkedHashSet; import java.util.LinkedHashSet;
import java.util.List; import java.util.List;
import java.util.Map;
import java.util.Set; import java.util.Set;


import javax.annotation.Nullable; import javax.annotation.Nullable;
import javax.lang.model.element.ElementKind;


/** /**
* Enforce style guide §3.3.1. * Enforce style guide §3.3.1.
Expand All @@ -65,25 +73,28 @@
) )
public class WildcardImport extends BugChecker implements CompilationUnitTreeMatcher { public class WildcardImport extends BugChecker implements CompilationUnitTreeMatcher {


/** Maximum number of members to import before switching to qualified names. */
public static final int MAX_MEMBER_IMPORTS = 20;

/** A type or member that needs to be imported.*/ /** A type or member that needs to be imported.*/
@AutoValue @AutoValue
abstract static class TypeToImport { abstract static class TypeToImport {


/** Returns the simple name of the imported member. */ /** Returns the simple name of the import. */
abstract Name name(); abstract String name();


/** Returns the fully-qualified name of the owner. */ /** Returns the owner of the imported type or member. */
abstract Name owner(); abstract Symbol owner();


/** Returns true if the import needs to be static (i.e. the import is for a field or method). */ /** Returns true if the import needs to be static (i.e. the import is for a field or method). */
abstract boolean isStatic(); abstract boolean isStatic();


static TypeToImport create(Name name, Name owner, boolean stat) { static TypeToImport create(String name, Symbol owner, boolean stat) {
return new AutoValue_WildcardImport_TypeToImport(name, owner, stat); return new AutoValue_WildcardImport_TypeToImport(name, owner, stat);
} }


private void addFix(SuggestedFix.Builder fix) { private void addFix(SuggestedFix.Builder fix) {
String qualifiedName = owner() + "." + name(); String qualifiedName = owner().getQualifiedName() + "." + name();
if (isStatic()) { if (isStatic()) {
fix.addStaticImport(qualifiedName); fix.addStaticImport(qualifiedName);
} else { } else {
Expand All @@ -100,12 +111,12 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
} }


// Find all of the types that need to be imported. // Find all of the types that need to be imported.
Set<TypeToImport> typesToImport = ImportCollector.collect((JCTree.JCCompilationUnit) tree); Set<TypeToImport> typesToImport = ImportCollector.collect((JCCompilationUnit) tree);


// Group the imported types by the on-demand import they replace. // Group the imported types by the on-demand import they replace.
Multimap<ImportTree, TypeToImport> toFix = groupImports(wildcardImports, typesToImport); Multimap<ImportTree, TypeToImport> toFix = groupImports(wildcardImports, typesToImport);


Fix fix = createFix(wildcardImports, toFix); Fix fix = createFix(wildcardImports, toFix, state);
if (fix.isEmpty()) { if (fix.isEmpty()) {
return NO_MATCH; return NO_MATCH;
} }
Expand Down Expand Up @@ -134,7 +145,7 @@ private ImportTree findMatchingWildcardImport(
// guaranteed to be a MemberSelectTree by getWildcardImports(). // guaranteed to be a MemberSelectTree by getWildcardImports().
String importBase = String importBase =
((MemberSelectTree) importTree.getQualifiedIdentifier()).getExpression().toString(); ((MemberSelectTree) importTree.getQualifiedIdentifier()).getExpression().toString();
if (type.owner().contentEquals(importBase)) { if (type.owner().getQualifiedName().contentEquals(importBase)) {
return importTree; return importTree;
} }
} }
Expand Down Expand Up @@ -168,7 +179,7 @@ static class ImportCollector extends TreeScanner {
this.wildcardScope = wildcardScope; this.wildcardScope = wildcardScope;
} }


public static Set<TypeToImport> collect(JCTree.JCCompilationUnit tree) { public static Set<TypeToImport> collect(JCCompilationUnit tree) {
ImportCollector collector = new ImportCollector(tree.starImportScope); ImportCollector collector = new ImportCollector(tree.starImportScope);
collector.scan(tree); collector.scan(tree);
return collector.seen; return collector.seen;
Expand All @@ -190,7 +201,7 @@ public void visitMethodDef(JCTree.JCMethodDecl method) {
} }


@Override @Override
public void visitIdent(JCTree.JCIdent tree) { public void visitIdent(JCIdent tree) {
Symbol sym = tree.sym; Symbol sym = tree.sym;
if (sym == null) { if (sym == null) {
return; return;
Expand All @@ -202,11 +213,11 @@ public void visitIdent(JCTree.JCIdent tree) {
} }
switch (sym.kind) { switch (sym.kind) {
case TYP: case TYP:
seen.add(TypeToImport.create(sym.getSimpleName(), sym.owner.getQualifiedName(), false)); seen.add(TypeToImport.create(sym.getSimpleName().toString(), sym.owner, false));
break; break;
case VAR: case VAR:
case MTH: case MTH:
seen.add(TypeToImport.create(sym.getSimpleName(), sym.owner.getQualifiedName(), true)); seen.add(TypeToImport.create(sym.getSimpleName().toString(), sym.owner, true));
break; break;
default: default:
return; return;
Expand All @@ -217,8 +228,10 @@ public void visitIdent(JCTree.JCIdent tree) {


/** Creates a {@link Fix} that replaces wildcard imports. */ /** Creates a {@link Fix} that replaces wildcard imports. */
static Fix createFix( static Fix createFix(
ImmutableList<ImportTree> wildcardImports, Multimap<ImportTree, TypeToImport> toFix) { ImmutableList<ImportTree> wildcardImports,
SuggestedFix.Builder fix = SuggestedFix.builder(); Multimap<ImportTree, TypeToImport> toFix,
VisitorState state) {
final SuggestedFix.Builder fix = SuggestedFix.builder();
for (ImportTree importToDelete : wildcardImports) { for (ImportTree importToDelete : wildcardImports) {
String importSpecification = importToDelete.getQualifiedIdentifier().toString(); String importSpecification = importToDelete.getQualifiedIdentifier().toString();
if (importToDelete.isStatic()) { if (importToDelete.isStatic()) {
Expand All @@ -227,9 +240,50 @@ static Fix createFix(
fix.removeImport(importSpecification); fix.removeImport(importSpecification);
} }
} }
Multimap<Symbol, TypeToImport> importsByOwner = LinkedHashMultimap.create();
for (TypeToImport toImport : toFix.values()) { for (TypeToImport toImport : toFix.values()) {
toImport.addFix(fix); importsByOwner.put(toImport.owner(), toImport);
}
for (Map.Entry<Symbol, Collection<TypeToImport>> entries : importsByOwner.asMap().entrySet()) {
final Symbol owner = entries.getKey();
if (entries.getValue().size() > MAX_MEMBER_IMPORTS) {
qualifiedNameFix(fix, owner, state);
} else {
for (TypeToImport toImport : toFix.values()) {
toImport.addFix(fix);
}
}
} }
return fix.build(); return fix.build();
} }

/**
* Add an import for {@code owner}, and qualify all on demand imported references to members of
* owner by owner's simple name.
*/
private static void qualifiedNameFix(
final SuggestedFix.Builder fix, final Symbol owner, VisitorState state) {
fix.addImport(owner.getQualifiedName().toString());
final JCCompilationUnit unit = (JCCompilationUnit) state.getPath().getCompilationUnit();
new TreePathScanner<Void, Void>() {
@Override
public Void visitIdentifier(IdentifierTree tree, Void unused) {
Symbol sym = ASTHelpers.getSymbol(tree);
if (sym == null) {
return null;
}
Tree parent = getCurrentPath().getParentPath().getLeaf();
if (parent.getKind() == Tree.Kind.CASE
&& ((CaseTree) parent).getExpression().equals(tree)
&& sym.owner.getKind() == ElementKind.ENUM) {
// switch cases can refer to enum constants by simple name without importing them
return null;
}
if (sym.owner.equals(owner) && unit.starImportScope.includes(sym)) {
fix.prefixWith(tree, owner.getSimpleName() + ".");
}
return null;
}
}.scan(unit, null);
}
} }
59 changes: 59 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/WildcardImportTest.java
View file
Expand Up @@ -446,4 +446,63 @@ public void memberImport() throws Exception {
"}") "}")
.doTest(); .doTest();
} }

@Test
public void qualifyMembersFix() throws Exception {
testHelper
.addInputLines(
"e/E.java",
"package e;",
"public enum E {",
" A, B, C, D, E, F, G, H, I, J,",
" K, L, M, N, O, P, Q, R, S, T,",
" U, V, W, X, Y, Z",
"}")
.expectUnchanged()
.addInputLines(
"in/Test.java",
"import static e.E.*;",
"public class Test {",
" E ex = {",
" A, B, C, D, E, F, G, H, I, J,",
" K, L, M, N, O, P, Q, R, S, T,",
" U, V, W, X, Y, Z",
" };",
" boolean f(E e) {",
" switch (e) {",
" case A:",
" case E:",
" case I:",
" case O:",
" case U:",
" return true;",
" default:",
" return false;",
" };",
" }",
"}")
.addOutputLines(
"out/Test.java",
"import e.E;",
"public class Test {",
" E ex = {",
" E.A, E.B, E.C, E.D, E.E, E.F, E.G, E.H, E.I, E.J,",
" E.K, E.L, E.M, E.N, E.O, E.P, E.Q, E.R, E.S, E.T,",
" E.U, E.V, E.W, E.X, E.Y, E.Z",
" };",
" boolean f(E e) {",
" switch (e) {",
" case A:",
" case E:",
" case I:",
" case O:",
" case U:",
" return true;",
" default:",
" return false;",
" };",
" }",
"}")
.doTest();
}
} }

0 comments on commit 8a8c0dd

Please sign in to comment.