Skip to content

Commit a29b13a

Browse files
graememorgancpovirk
graememorgan
authored and
cpovirk
committed
Add a check to suggest importing unnecessarily qualified names.
------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=308780548
1 parent 2c0ad30 commit a29b13a

4 files changed

Lines changed: 284 additions & 2 deletions

File tree

‎core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@
5757
severity = WARNING)
5858
public class BadImport extends BugChecker implements ImportTreeMatcher {
5959

60-
private static final ImmutableSet<String> BAD_NESTED_CLASSES =
60+
static final ImmutableSet<String> BAD_NESTED_CLASSES =
6161
ImmutableSet.of(
6262
"Builder",
6363
"Callback",

‎core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
/*
2+
* Copyright 2020 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.google.errorprone.bugpatterns;
17+
18+
import static com.google.common.collect.Iterables.getOnlyElement;
19+
import static com.google.errorprone.matchers.Description.NO_MATCH;
20+
import static com.google.errorprone.util.ASTHelpers.getGeneratedBy;
21+
import static com.google.errorprone.util.ASTHelpers.getSymbol;
22+
import static com.google.errorprone.util.FindIdentifiers.findIdent;
23+
import static com.sun.tools.javac.code.Kinds.KindSelector.VAL_TYP;
24+
25+
import com.google.common.base.Ascii;
26+
import com.google.common.collect.HashBasedTable;
27+
import com.google.common.collect.Table;
28+
import com.google.errorprone.BugPattern;
29+
import com.google.errorprone.BugPattern.SeverityLevel;
30+
import com.google.errorprone.VisitorState;
31+
import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
32+
import com.google.errorprone.fixes.SuggestedFix;
33+
import com.google.errorprone.matchers.Description;
34+
import com.sun.source.tree.CompilationUnitTree;
35+
import com.sun.source.tree.IdentifierTree;
36+
import com.sun.source.tree.ImportTree;
37+
import com.sun.source.tree.MemberSelectTree;
38+
import com.sun.source.tree.Tree;
39+
import com.sun.source.util.SimpleTreeVisitor;
40+
import com.sun.source.util.TreePath;
41+
import com.sun.tools.javac.code.Symbol;
42+
import com.sun.tools.javac.code.Symbol.ClassSymbol;
43+
import com.sun.tools.javac.code.Symbol.PackageSymbol;
44+
import com.sun.tools.javac.code.Symbol.TypeSymbol;
45+
import com.sun.tools.javac.util.Position;
46+
import java.util.ArrayList;
47+
import java.util.HashSet;
48+
import java.util.List;
49+
import java.util.Map;
50+
import java.util.Set;
51+
import java.util.concurrent.atomic.AtomicBoolean;
52+
import javax.lang.model.element.Name;
53+
54+
/** Flags uses of fully qualified names which are not ambiguous if imported. */
55+
@BugPattern(
56+
name = "UnnecessarilyFullyQualified",
57+
severity = SeverityLevel.WARNING,
58+
summary = "This fully qualified name is unambiguous if imported.")
59+
public final class UnnecessarilyFullyQualified extends BugChecker
60+
implements CompilationUnitTreeMatcher {
61+
62+
@Override
63+
public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
64+
if (tree.getTypeDecls().stream()
65+
.anyMatch(
66+
t -> getSymbol(tree) != null && !getGeneratedBy(getSymbol(tree), state).isEmpty())) {
67+
return NO_MATCH;
68+
}
69+
Table<Name, TypeSymbol, List<TreePath>> table = HashBasedTable.create();
70+
Set<Name> identifiersSeen = new HashSet<>();
71+
new SuppressibleTreePathScanner<Void, Void>() {
72+
@Override
73+
public Void visitImport(ImportTree importTree, Void aVoid) {
74+
return null;
75+
}
76+
77+
@Override
78+
public Void visitMemberSelect(MemberSelectTree memberSelectTree, Void unused) {
79+
if (!shouldIgnore()) {
80+
handle(getCurrentPath());
81+
}
82+
return super.visitMemberSelect(memberSelectTree, null);
83+
}
84+
85+
@Override
86+
public Void visitIdentifier(IdentifierTree identifierTree, Void unused) {
87+
identifiersSeen.add(identifierTree.getName());
88+
return null;
89+
}
90+
91+
private boolean shouldIgnore() {
92+
// Don't report duplicate hits if we're not at the tail of a series of member selects on
93+
// classes.
94+
Tree parentTree = getCurrentPath().getParentPath().getLeaf();
95+
return parentTree instanceof MemberSelectTree
96+
&& getSymbol(parentTree) instanceof ClassSymbol;
97+
}
98+
99+
private void handle(TreePath path) {
100+
MemberSelectTree tree = (MemberSelectTree) path.getLeaf();
101+
if (!isFullyQualified(tree)) {
102+
return;
103+
}
104+
if (BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString())) {
105+
if (tree.getExpression() instanceof MemberSelectTree
106+
&& getSymbol(tree.getExpression()) instanceof ClassSymbol) {
107+
handle(new TreePath(path, tree.getExpression()));
108+
}
109+
return;
110+
}
111+
Symbol symbol = getSymbol(tree);
112+
if (symbol instanceof ClassSymbol) {
113+
List<TreePath> treePaths = table.get(tree.getIdentifier(), symbol.type.tsym);
114+
if (treePaths == null) {
115+
treePaths = new ArrayList<>();
116+
table.put(tree.getIdentifier(), symbol.type.tsym, treePaths);
117+
}
118+
if (state.getEndPosition(tree) != Position.NOPOS) {
119+
treePaths.add(path);
120+
}
121+
}
122+
}
123+
124+
private boolean isFullyQualified(MemberSelectTree tree) {
125+
AtomicBoolean isFullyQualified = new AtomicBoolean();
126+
new SimpleTreeVisitor<Void, Void>() {
127+
@Override
128+
public Void visitMemberSelect(MemberSelectTree memberSelectTree, Void unused) {
129+
return visit(memberSelectTree.getExpression(), null);
130+
}
131+
132+
@Override
133+
public Void visitIdentifier(IdentifierTree identifierTree, Void aVoid) {
134+
if (getSymbol(identifierTree) instanceof PackageSymbol) {
135+
isFullyQualified.set(true);
136+
}
137+
return null;
138+
}
139+
}.visit(tree, null);
140+
return isFullyQualified.get();
141+
}
142+
}.scan(state.getPath(), null);
143+
144+
for (Map.Entry<Name, Map<TypeSymbol, List<TreePath>>> rows : table.rowMap().entrySet()) {
145+
Name name = rows.getKey();
146+
Map<TypeSymbol, List<TreePath>> types = rows.getValue();
147+
// Skip places where the same simple name refers to multiple types.
148+
if (types.size() > 1) {
149+
continue;
150+
}
151+
// Skip weird Android classes which don't look like classes.
152+
if (Ascii.isLowerCase(name.charAt(0))) {
153+
continue;
154+
}
155+
if (identifiersSeen.contains(name)) {
156+
continue;
157+
}
158+
List<TreePath> pathsToFix = getOnlyElement(types.values());
159+
if (pathsToFix.stream()
160+
.anyMatch(path -> findIdent(name.toString(), state, VAL_TYP) != null)) {
161+
continue;
162+
}
163+
SuggestedFix.Builder fix = SuggestedFix.builder();
164+
fix.addImport(getOnlyElement(types.keySet()).getQualifiedName().toString());
165+
for (TreePath path : pathsToFix) {
166+
fix.replace(path.getLeaf(), name.toString());
167+
}
168+
state.reportMatch(describeMatch(pathsToFix.get(0).getLeaf(), fix.build()));
169+
}
170+
return NO_MATCH;
171+
}
172+
}

‎core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,7 @@
303303
import com.google.errorprone.bugpatterns.URLEqualsHashCode;
304304
import com.google.errorprone.bugpatterns.UndefinedEquals;
305305
import com.google.errorprone.bugpatterns.UngroupedOverloads;
306+
import com.google.errorprone.bugpatterns.UnnecessarilyFullyQualified;
306307
import com.google.errorprone.bugpatterns.UnnecessaryAnonymousClass;
307308
import com.google.errorprone.bugpatterns.UnnecessaryBoxedAssignment;
308309
import com.google.errorprone.bugpatterns.UnnecessaryBoxedVariable;
@@ -840,8 +841,8 @@ public static ScannerSupplier errorChecks() {
840841

841842
/** A list of all checks that are off by default. */
842843
public static final ImmutableSet<BugCheckerInfo> DISABLED_CHECKS =
843-
// start
844844
getSuppliers(
845+
// start
845846
AndroidJdkLibsChecker.class,
846847
AnnotationPosition.class,
847848
AssertFalse.class,
@@ -925,6 +926,7 @@ public static ScannerSupplier errorChecks() {
925926
TypeParameterNaming.class,
926927
UngroupedOverloads.class,
927928
UnlockMethodChecker.class,
929+
UnnecessarilyFullyQualified.class,
928930
UnnecessaryBoxedAssignment.class,
929931
UnnecessaryBoxedVariable.class,
930932
UnnecessaryDefaultInEnumSwitch.class,

‎core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/*
2+
* Copyright 2020 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.google.errorprone.bugpatterns;
17+
18+
import com.google.errorprone.BugCheckerRefactoringTestHelper;
19+
import org.junit.Test;
20+
import org.junit.runner.RunWith;
21+
import org.junit.runners.JUnit4;
22+
23+
/** Tests for {@link UnnecessarilyFullyQualified}. */
24+
@RunWith(JUnit4.class)
25+
public final class UnnecessarilyFullyQualifiedTest {
26+
private final BugCheckerRefactoringTestHelper helper =
27+
BugCheckerRefactoringTestHelper.newInstance(new UnnecessarilyFullyQualified(), getClass());
28+
29+
@Test
30+
public void singleUse() {
31+
helper
32+
.addInputLines(
33+
"Test.java", //
34+
"interface Test {",
35+
" java.util.List foo();",
36+
" java.util.List bar();",
37+
"}")
38+
.addOutputLines(
39+
"Test.java", //
40+
"import java.util.List;",
41+
"interface Test {",
42+
" List foo();",
43+
" List bar();",
44+
"}")
45+
.doTest();
46+
}
47+
48+
@Test
49+
public void wouldBeAmbiguous() {
50+
helper
51+
.addInputLines("List.java", "class List {}")
52+
.expectUnchanged()
53+
.addInputLines(
54+
"Test.java", //
55+
"interface Test {",
56+
" java.util.List foo();",
57+
"}")
58+
.expectUnchanged()
59+
.doTest();
60+
}
61+
62+
@Test
63+
public void refersToMultipleTypes() {
64+
helper
65+
.addInputLines(
66+
"List.java", //
67+
"package a;",
68+
"public class List {}")
69+
.expectUnchanged()
70+
.addInputLines(
71+
"Test.java",
72+
"package b;",
73+
"interface Test {",
74+
" java.util.List foo();",
75+
" a.List bar();",
76+
"}")
77+
.expectUnchanged()
78+
.doTest();
79+
}
80+
81+
@Test
82+
public void builder() {
83+
helper
84+
.addInputLines(
85+
"Foo.java", //
86+
"package a;",
87+
"public class Foo {",
88+
" public static final class Builder {}",
89+
"}")
90+
.expectUnchanged()
91+
.addInputLines(
92+
"Test.java",
93+
"package b;",
94+
"interface Test {",
95+
" a.Foo foo();",
96+
" a.Foo.Builder fooBuilder();",
97+
"}")
98+
.addOutputLines(
99+
"Test.java",
100+
"package b;",
101+
"import a.Foo;",
102+
"interface Test {",
103+
" Foo foo();",
104+
" Foo.Builder fooBuilder();",
105+
"}")
106+
.doTest();
107+
}
108+
}

0 commit comments

Comments
 (0)