diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java b/core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java index 18c28bfa53f..3b39350b9f8 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.ImportTreeMatcher; import com.google.errorprone.bugpatterns.StaticImports.StaticImportInfo; @@ -47,6 +48,7 @@ import java.util.Arrays; import java.util.List; import java.util.Set; +import javax.inject.Inject; import javax.lang.model.element.Name; /** @@ -99,6 +101,26 @@ public class BadImport extends BugChecker implements ImportTreeMatcher { private static final String MESSAGE_LITE = "com.google.protobuf.MessageLite"; + /** + * Enclosing types that their nested type imports are vague. + * + *

Some types are meant to provide a namespace; therefore, imports for their nested types can + * be confusing. + * + *

For instance, unlike its name suggests, {@code org.immutables.value.Value.Immutable} is used + * to generate immutable value types, and its import can be misleading. So, importing {@code + * org.immutables.value.Value} and using {@code @Value.Immutable} is more favorable than importing + * {@code org.immutables.value.Value.Immutable} and using {@code @Immutable}. + * + *

Note that this does not disallow import an enclosing type but its nested types instead. + */ + private final ImmutableSet badEnclosingTypes; + + @Inject + BadImport(ErrorProneFlags errorProneFlags) { + this.badEnclosingTypes = errorProneFlags.getSetOrEmpty("BadImport:BadEnclosingTypes"); + } + @Override public Description matchImport(ImportTree tree, VisitorState state) { Symbol symbol; @@ -168,9 +190,11 @@ private static VisitorState getCheckState(VisitorState state) { TreePath.getPath(compilationUnit, ((ClassTree) tree).getMembers().get(0))); } - private static boolean isAcceptableImport(Symbol symbol, Set badNames) { + private boolean isAcceptableImport(Symbol symbol, Set badNames) { + Name ownerName = symbol.owner.getQualifiedName(); Name simpleName = symbol.getSimpleName(); - return badNames.stream().noneMatch(simpleName::contentEquals); + return badEnclosingTypes.stream().noneMatch(ownerName::contentEquals) + && badNames.stream().noneMatch(simpleName::contentEquals); } private Description buildDescription( 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 2a79e7dff3f..e2373a5d557 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java @@ -33,6 +33,7 @@ import com.google.common.collect.Table; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; @@ -40,6 +41,7 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.ImportTree; import com.sun.source.tree.MemberSelectTree; @@ -60,6 +62,7 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import javax.inject.Inject; import javax.lang.model.element.Name; /** Flags uses of fully qualified names which are not ambiguous if imported. */ @@ -71,6 +74,25 @@ public final class UnnecessarilyFullyQualified extends BugChecker private static final ImmutableSet EXEMPTED_NAMES = ImmutableSet.of("Annotation"); + /** + * Exempted types that fully qualified name usages are acceptable for their nested types when + * importing the enclosing type is ambiguous. + * + *

Some types are meant to provide a namespace; therefore, imports for their nested types can + * be confusing. + * + *

For instance, unlike its name suggests, {@code org.immutables.value.Value.Immutable} is used + * to generate immutable value types, and its import can be misleading. So, importing {@code + * org.immutables.value.Value} and using {@code @Value.Immutable} is more favorable than importing + * {@code org.immutables.value.Value.Immutable} and using {@code @Immutable}. + */ + private final ImmutableSet exemptedEnclosingTypes; + + @Inject + UnnecessarilyFullyQualified(ErrorProneFlags errorProneFlags) { + this.exemptedEnclosingTypes = errorProneFlags.getSetOrEmpty("BadImport:BadEnclosingTypes"); + } + @Override public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { if (tree.getTypeDecls().stream() @@ -138,11 +160,8 @@ private void handle(TreePath path) { if (!isFullyQualified(tree)) { return; } - if (BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString())) { - if (tree.getExpression() instanceof MemberSelectTree - && getSymbol(tree.getExpression()) instanceof ClassSymbol) { - handle(new TreePath(path, tree.getExpression())); - } + if (isBadNestedClass(tree) || isExemptedEnclosingType(tree)) { + handle(new TreePath(path, tree.getExpression())); return; } Symbol symbol = getSymbol(tree); @@ -160,6 +179,23 @@ && getSymbol(tree.getExpression()) instanceof ClassSymbol) { treePaths.add(path); } + private boolean isBadNestedClass(MemberSelectTree tree) { + return BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString()); + } + + private boolean isExemptedEnclosingType(MemberSelectTree tree) { + ExpressionTree expression = tree.getExpression(); + if (!(expression instanceof MemberSelectTree)) { + return false; + } + Symbol symbol = getSymbol(expression); + if (!(symbol instanceof ClassSymbol)) { + return false; + } + + return exemptedEnclosingTypes.contains(symbol.getQualifiedName().toString()); + } + private boolean isFullyQualified(MemberSelectTree tree) { AtomicBoolean isFullyQualified = new AtomicBoolean(); new SimpleTreeVisitor() { diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/BadImportTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/BadImportTest.java index cc048e48141..26bdf94f124 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/BadImportTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/BadImportTest.java @@ -388,4 +388,63 @@ public void doesNotMatchProtos() { "}") .doTest(); } + + @Test + public void badEnclosingTypes() { + refactoringTestHelper + .setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value") + .addInputLines( + "org/immutables/value/Value.java", + "package org.immutables.value;", + "", + "public @interface Value {", + " @interface Immutable {}", + "}") + .expectUnchanged() + .addInputLines( + "Test.java", + "import org.immutables.value.Value.Immutable;", + "", + "@Immutable", + "interface Test {}") + .addOutputLines( + "Test.java", + "import org.immutables.value.Value;", + "", + "@Value.Immutable", + "interface Test {}") + .doTest(); + } + + @Test + public void badEnclosingTypes_doesNotMatchFullyQualifiedName() { + compilationTestHelper + .setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value") + .addSourceLines( + "org/immutables/value/Value.java", + "package org.immutables.value;", + "", + "public @interface Value {", + " @interface Immutable {}", + "}") + .addSourceLines("Test.java", "@org.immutables.value.Value.Immutable", "interface Test {}") + .doTest(); + } + + @Test + public void badEnclosingTypes_staticMethod() { + compilationTestHelper + .setArgs("-XepOpt:BadImport:BadEnclosingTypes=com.google.common.collect.ImmutableList") + .addSourceLines( + "Test.java", + "import static com.google.common.collect.ImmutableList.toImmutableList;", + "import com.google.common.collect.ImmutableList;", + "import java.util.stream.Collector;", + "", + "class Test {", + " // BUG: Diagnostic contains: ImmutableList.toImmutableList()", + " Collector> immutableList = toImmutableList();", + "}") + .doTest(); + } } 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 99cb1835611..a6d239590fe 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java @@ -224,4 +224,95 @@ public void packageInfo() { "package b;") .doTest(); } + + @Test + public void staticNestedClass() { + helper + .addInputLines( + "test/EnclosingType.java", + "package test;", + "", + "public final class EnclosingType {", + " public static final class StaticNestedClass {}", + "}") + .expectUnchanged() + .addInputLines( + "Test.java", + "interface Test {", + " test.EnclosingType.StaticNestedClass method();", + "}") + .addOutputLines( + "Test.java", + "import test.EnclosingType.StaticNestedClass;", + "interface Test {", + " StaticNestedClass method();", + "}") + .doTest(); + } + + @Test + public void exemptedEnclosingTypes() { + helper + .setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value") + .addInputLines( + "org/immutables/value/Value.java", + "package org.immutables.value;", + "", + "public @interface Value {", + " @interface Immutable {}", + "}") + .expectUnchanged() + .addInputLines( + "Test.java", + "import org.immutables.value.Value.Immutable;", + "", + "class Test {", + " @org.immutables.value.Value.Immutable", + " abstract class AbstractType {}", + "}") + .addOutputLines( + "Test.java", + "import org.immutables.value.Value;", + "import org.immutables.value.Value.Immutable;", + "", + "class Test {", + " @Value.Immutable", + " abstract class AbstractType {}", + "}") + .doTest(); + } + + @Test + public void exemptedEnclosingTypes_importWouldBeAmbiguous() { + helper + .setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value") + .addInputLines( + "org/immutables/value/Value.java", + "package org.immutables.value;", + "", + "public @interface Value {", + " @interface Immutable {}", + "}") + .expectUnchanged() + .addInputLines( + "annotation/Value.java", + "package annotation;", + "", + "public @interface Value {", + " String value();", + "}") + .expectUnchanged() + .addInputLines( + "Test.java", + "import annotation.Value;", + "", + "final class Test {", + " Test(@Value(\"test\") String value) {}", + "", + " @org.immutables.value.Value.Immutable", + " abstract class AbstractType {}", + "}") + .expectUnchanged() + .doTest(); + } }