Skip to content

Commit

Permalink
UnnecessarilyFullyQualified: handle java.lang and inconsistent impo…
Browse files Browse the repository at this point in the history
…rt usages

This introduces two improvements:
- Where possible `java.lang.SomeType` references are simplified to `SomeType`
  without introducing an associated `import` statement.
- References to `some.package.SomeType` are now also replaced with `SomeType`
  if said type is already imported and referenced by its simple name elsewhere
  in the compilation unit.

Fixes #2776

PiperOrigin-RevId: 419556583
  • Loading branch information
rickie authored and Error Prone Team committed Jan 4, 2022
1 parent 65eb009 commit c07a7a9
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 6 deletions.
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -74,7 +80,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
return NO_MATCH;
}
Table<Name, TypeSymbol, List<TreePath>> table = HashBasedTable.create();
Set<Name> identifiersSeen = new HashSet<>();
SetMultimap<Name, TypeSymbol> identifiersSeen = HashMultimap.create();
new SuppressibleTreePathScanner<Void, Void>() {
@Override
public Void visitImport(ImportTree importTree, Void unused) {
Expand All @@ -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;
}

Expand Down Expand Up @@ -157,24 +166,33 @@ 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();
if (EXEMPTED_NAMES.contains(nameString)) {
continue;
}
List<TreePath> pathsToFix = getOnlyElement(types.values());
if (pathsToFix.stream()
.anyMatch(path -> findIdent(nameString, state.withPath(path), VAL_TYP) != null)) {
Set<Symbol> 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);
}
Expand Down
Expand Up @@ -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
Expand Down

0 comments on commit c07a7a9

Please sign in to comment.