diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java index ffdd6ad70f4..ca52ab57321 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java @@ -19,12 +19,16 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.util.ASTHelpers.getGeneratedBy; import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; import static com.google.errorprone.util.FindIdentifiers.findIdent; import static com.sun.tools.javac.code.Kinds.KindSelector.VAL_TYP; +import static java.util.stream.Collectors.toCollection; import com.google.common.base.Ascii; import com.google.common.collect.HashBasedTable; +import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.SetMultimap; import com.google.common.collect.Table; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; @@ -44,11 +48,13 @@ import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.PackageSymbol; import com.sun.tools.javac.code.Symbol.TypeSymbol; +import com.sun.tools.javac.code.Type; import com.sun.tools.javac.util.Position; import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import javax.lang.model.element.Name; @@ -74,7 +80,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s return NO_MATCH; } Table> table = HashBasedTable.create(); - Set identifiersSeen = new HashSet<>(); + SetMultimap identifiersSeen = HashMultimap.create(); new SuppressibleTreePathScanner() { @Override public Void visitImport(ImportTree importTree, Void unused) { @@ -91,7 +97,10 @@ public Void visitMemberSelect(MemberSelectTree memberSelectTree, Void unused) { @Override public Void visitIdentifier(IdentifierTree identifierTree, Void unused) { - identifiersSeen.add(identifierTree.getName()); + Type type = getType(identifierTree); + if (type != null) { + identifiersSeen.put(identifierTree.getName(), type.tsym); + } return null; } @@ -157,11 +166,13 @@ public Void visitIdentifier(IdentifierTree identifierTree, Void aVoid) { if (types.size() > 1) { continue; } + TypeSymbol type = getOnlyElement(types.keySet()); // Skip weird Android classes which don't look like classes. if (Ascii.isLowerCase(name.charAt(0))) { continue; } - if (identifiersSeen.contains(name)) { + // Don't replace if this name is already used in the file to refer to a _different_ type. + if (identifiersSeen.get(name).stream().anyMatch(s -> !s.equals(type))) { continue; } String nameString = name.toString(); @@ -169,12 +180,19 @@ public Void visitIdentifier(IdentifierTree identifierTree, Void aVoid) { continue; } List pathsToFix = getOnlyElement(types.values()); - if (pathsToFix.stream() - .anyMatch(path -> findIdent(nameString, state.withPath(path), VAL_TYP) != null)) { + Set meaningAtUsageSites = + pathsToFix.stream() + .map(path -> findIdent(nameString, state.withPath(path), VAL_TYP)) + .collect(toCollection(HashSet::new)); + // Don't report a finding if the simple name has a different meaning elsewhere. + if (meaningAtUsageSites.stream().anyMatch(s -> s != null && !s.equals(type))) { continue; } SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - fixBuilder.addImport(getOnlyElement(types.keySet()).getQualifiedName().toString()); + // Only add the import if any of the usage sites don't already resolve to this type. + if (meaningAtUsageSites.stream().anyMatch(Objects::isNull)) { + fixBuilder.addImport(getOnlyElement(types.keySet()).getQualifiedName().toString()); + } for (TreePath path : pathsToFix) { fixBuilder.replace(path.getLeaf(), nameString); } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java index 671dfcd6b1f..159d0ca6348 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java @@ -81,6 +81,52 @@ public void refersToMultipleTypes() { .doTest(); } + @Test + public void refersToMultipleTypes_dependingOnLocation() { + helper + .addInputLines( + "Outer.java", // + "package a;", + "public class Outer {", + " public class List {}", + "}") + .expectUnchanged() + .addInputLines( + "Test.java", + "package b;", + "import a.Outer;", + "interface Test {", + " java.util.List foo();", + " public abstract class Inner extends Outer {", + " abstract List bar();", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void inconsistentImportUsage() { + helper + .addInputLines( + "Test.java", + "import java.util.List;", + "public class Test {", + " public java.util.List foo(List list) {", + " return list;", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.util.List;", + "public class Test {", + " public List foo(List list) {", + " return list;", + " }", + "}") + .doTest(); + } + @Test public void clashesWithTypeInSuperType() { helper