diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java index 72042e9dd59..0d32a167fbb 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java @@ -3,8 +3,6 @@ import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.StandardTags.STYLE; -import static com.sun.tools.javac.code.Kinds.Kind.TYP; -import static tech.picnic.errorprone.bugpatterns.StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; @@ -24,9 +22,12 @@ import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.TypeSymbol; import tech.picnic.errorprone.bugpatterns.util.SourceCode; -/** A {@link BugChecker} that flags methods and constants that should not be statically imported. */ +/** + * A {@link BugChecker} that flags methods and constants that should *not* be statically imported. + */ // XXX: Also introduce checks that disallows the following candidates: // - `ZoneOffset.ofHours` and other `ofXXX`-style methods. // - Several other `java.time` classes. @@ -47,13 +48,13 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM *

Types listed here should be mutually exclusive with {@link * StaticImport#STATIC_IMPORT_CANDIDATE_TYPES}. */ - static final ImmutableSet BAD_STATIC_IMPORT_CANDIDATE_TYPES = + static final ImmutableSet NON_STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of("com.google.common.base.Strings", "java.time.Clock", "java.time.ZoneOffset"); /** * Type members that should never be statically imported. * - *

Identifiers listed by {@link #BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should be omitted + *

Identifiers listed by {@link #NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should be omitted * from this collection. * *

This should be mutually exclusive with {@link StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS}. @@ -61,7 +62,7 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM // XXX: Perhaps the set of exempted `java.util.Collections` methods is too strict. For now any // method name that could be considered "too vague" or could conceivably mean something else in a // specific context is left out. - static final ImmutableSetMultimap BAD_STATIC_IMPORT_CANDIDATE_MEMBERS = + static final ImmutableSetMultimap NON_STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() .put("com.google.common.base.Predicates", "contains") .put("com.mongodb.client.model.Filters", "empty") @@ -92,7 +93,7 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM *

This should be a superset of the identifiers flagged by {@link * com.google.errorprone.bugpatterns.BadImport}. */ - static final ImmutableSet BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS = + static final ImmutableSet NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS = ImmutableSet.of( "builder", "copyOf", @@ -118,45 +119,34 @@ public NonStaticImport() {} @Override public Description matchIdentifier(IdentifierTree tree, VisitorState state) { Symbol symbol = ASTHelpers.getSymbol(tree); - if (symbol == null || !isMatch(symbol, state)) { + if (symbol == null || !shouldAddQualifier(symbol, state)) { return Description.NO_MATCH; } SuggestedFix.Builder builder = SuggestedFix.builder(); String replacement = SuggestedFixes.qualifyType(state, builder, symbol.owner) + "."; - return describeMatch( - tree, - builder - .removeStaticImport(getImportToRemove(symbol)) - .prefixWith(tree, replacement) - .build()); + return describeMatch(tree, builder.prefixWith(tree, replacement).build()); } - private static String getImportToRemove(Symbol symbol) { - return String.join(".", symbol.owner.getQualifiedName(), symbol.getSimpleName()); - } - - private static boolean isMatch(Symbol symbol, VisitorState state) { - Symbol owner = symbol.owner; - if (owner == null || owner.kind != TYP) { - return false; - } - - if (isDefinedInThisFile(symbol, state.getPath().getCompilationUnit())) { + private static boolean shouldAddQualifier(Symbol symbol, VisitorState state) { + if (symbol instanceof TypeSymbol) { return false; } String identifierName = symbol.getSimpleName().toString(); if (!isIdentifierStaticallyImported(identifierName, state)) { return false; } + if (isIdentifierDefinedInFile(symbol, state.getPath().getCompilationUnit())) { + return false; + } - String qualifiedTypeName = owner.getQualifiedName().toString(); - return !isExempted(qualifiedTypeName, identifierName) - && isCandidate(qualifiedTypeName, identifierName); + String qualifiedTypeName = symbol.owner.getQualifiedName().toString(); + return !isStaticImportCandidateMember(qualifiedTypeName, identifierName) + && isNonStaticImportCandidate(qualifiedTypeName, identifierName); } - private static boolean isDefinedInThisFile(Symbol symbol, CompilationUnitTree tree) { + private static boolean isIdentifierDefinedInFile(Symbol symbol, CompilationUnitTree tree) { return tree.getTypeDecls().stream() .anyMatch( t -> { @@ -166,14 +156,17 @@ private static boolean isDefinedInThisFile(Symbol symbol, CompilationUnitTree tr }); } - private static boolean isCandidate(String qualifiedTypeName, String identifierName) { - return BAD_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedTypeName) - || BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName) - || BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifierName); + private static boolean isNonStaticImportCandidate( + String qualifiedTypeName, String identifierName) { + return NON_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedTypeName) + || NON_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName) + || NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifierName); } - private static boolean isExempted(String qualifiedTypeName, String identifierName) { - return STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName); + private static boolean isStaticImportCandidateMember( + String qualifiedTypeName, String identifierName) { + return StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry( + qualifiedTypeName, identifierName); } private static boolean isIdentifierStaticallyImported(String identifierName, VisitorState state) { diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java index 1713eef60aa..9cee4c243a7 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java @@ -4,8 +4,8 @@ import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; import static java.util.Objects.requireNonNull; -import static tech.picnic.errorprone.bugpatterns.NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS; -import static tech.picnic.errorprone.bugpatterns.NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS; +import static tech.picnic.errorprone.bugpatterns.NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS; +import static tech.picnic.errorprone.bugpatterns.NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; @@ -48,11 +48,11 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa /** * Types whose members should be statically imported, unless exempted by {@link - * NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS} or {@link - * NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS}. + * NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_MEMBERS} or {@link + * NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS}. * *

Types listed here should be mutually exclusive with {@link - * NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_TYPES}. + * NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_TYPES}. */ static final ImmutableSet STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of( @@ -104,9 +104,9 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa * Type members that should be statically imported. * *

This should be mutually exclusive with {@link - * NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS}. + * NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_MEMBERS}. * - *

Identifiers listed by {@link NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should + *

Identifiers listed by {@link NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should * be mutually exclusive with identifiers listed here. */ static final ImmutableSetMultimap STATIC_IMPORT_CANDIDATE_MEMBERS = @@ -183,13 +183,13 @@ private static boolean isCandidateContext(VisitorState state) { private static boolean isCandidate(MemberSelectTree tree) { String identifier = tree.getIdentifier().toString(); - if (BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifier)) { + if (NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifier)) { return false; } Type type = ASTHelpers.getType(tree.getExpression()); return type != null - && !BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(type.toString(), identifier); + && !NON_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(type.toString(), identifier); } private static Optional getCandidateSimpleName(StaticImportInfo importInfo) { diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NonStaticImportTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NonStaticImportTest.java index 1e4ad625203..ef4d46ae5f7 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NonStaticImportTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NonStaticImportTest.java @@ -10,28 +10,28 @@ final class NonStaticImportTest { @Test void candidateMembersAreNotRedundant() { - assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) - .doesNotContainAnyElementsOf(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES); + assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) + .doesNotContainAnyElementsOf(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_TYPES); - assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.values()) - .doesNotContainAnyElementsOf(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS); + assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS.values()) + .doesNotContainAnyElementsOf(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS); } @Test void badTypesDontClashWithStaticImportCandidates() { - assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES) + assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_TYPES) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); } @Test void badMembersDontClashWithStaticImportCandidates() { - assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.entries()) + assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS.entries()) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.entries()); } @Test void badIdentifiersDontClashWithStaticImportCandidates() { - assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS) + assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.values()); } @@ -56,7 +56,6 @@ void identification() { "import static java.lang.Integer.MAX_VALUE;", "import static java.lang.Integer.MIN_VALUE;", "import static java.time.Clock.systemUTC;", - "import static java.time.Instant.MAX;", "import static java.time.Instant.MIN;", "import static java.time.ZoneOffset.UTC;", "import static java.util.Collections.min;", @@ -80,6 +79,7 @@ void identification() { " // BUG: Diagnostic contains:", " systemUTC();", " Clock.systemUTC();", + " ZoneOffset utcIsExempted = UTC;", "", " // BUG: Diagnostic contains:", " Optional optional1 = empty();", @@ -92,46 +92,27 @@ void identification() { " // BUG: Diagnostic contains:", " Locale locale1 = ROOT;", " Locale locale2 = Locale.ROOT;", + " Locale isNotACandidate = ENGLISH;", "", " // BUG: Diagnostic contains:", " Instant instant1 = MIN;", " Instant instant2 = Instant.MIN;", - " // BUG: Diagnostic contains:", - " Instant instant3 = MAX;", - " Instant instant4 = Instant.MAX;", "", " // BUG: Diagnostic contains:", " ImmutableSet.of(min(ImmutableSet.of()));", "", " Object builder = null;", - " // Not flagged because identifier is variable", - " Object lBuilder = ImmutableList.of(builder);", - "", - " // Not flagged because member of type is not a candidate.", - " Locale lo3 = ENGLISH;", - "", - " // Not flagged because member of type is exempted.", - " ZoneOffset utc = UTC;", + " ImmutableList list = ImmutableList.of(builder);", "", - " // Not flagged because method is not statically imported.", - " create();", - "", - " // Not flagged because identifier is not statically imported.", - " // A member variable did overwrite the statically imported identifier.", - " Integer i1 = MIN_VALUE;", - "", - " // Not flagged because identifier is not statically imported.", - " Integer i2 = new B().MIN;", - "", - " // Not flagged because identifier is not statically imported.", - " Object inst = new INSTANCE();", + " Integer refersToMemberVariable = MIN_VALUE;", + " Integer minIsNotStatic = new B().MIN;", + " Object regularImport = new INSTANCE();", + " MAX_VALUE maxValue = new MAX_VALUE();", "", " Integer from = 12;", - " // Not flagged because identifier is not statically imported.", - " Integer i3 = from;", + " Integer i1 = from;", "", - " // Not flagged because identifier does not belong to a type.", - " MAX_VALUE maxValue = new MAX_VALUE();", + " create();", " }", "", " void create() {", @@ -149,6 +130,7 @@ void replacement() { .addInputLines( "A.java", "import static com.google.common.collect.ImmutableList.copyOf;", + "import static com.google.common.collect.ImmutableSet.of;", "import static java.time.Clock.systemUTC;", "import static java.time.Instant.MAX;", "import static java.time.Instant.MIN;", @@ -180,11 +162,20 @@ void replacement() { " Instant i1 = MIN;", " Instant i2 = MAX;", "", - " ImmutableSet.of(min(ImmutableSet.of()));", + " ImmutableSet.of(min(of()));", " }", "}") .addOutputLines( "A.java", + "import static com.google.common.collect.ImmutableList.copyOf;", + "import static com.google.common.collect.ImmutableSet.of;", + "import static java.time.Clock.systemUTC;", + "import static java.time.Instant.MAX;", + "import static java.time.Instant.MIN;", + "import static java.util.Collections.min;", + "import static java.util.Locale.ROOT;", + "import static java.util.Optional.empty;", + "", "import com.google.common.collect.ImmutableList;", "import com.google.common.collect.ImmutableSet;", "import java.time.Clock;",