From 2547a3c06d9e923c2a0488ad4a2d1a1afc3ed3e0 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 14 Oct 2023 16:22:31 +0200 Subject: [PATCH] Suggestions --- error-prone-contrib/pom.xml | 5 + .../bugpatterns/NonStaticImport.java | 159 ++++++++++-------- .../errorprone/bugpatterns/StaticImport.java | 25 +-- .../bugpatterns/NonStaticImportTest.java | 58 +++---- .../bugpatterns/StaticImportTest.java | 17 +- 5 files changed, 156 insertions(+), 108 deletions(-) diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index 93c0c7a8d88..6e102c380b6 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -72,6 +72,11 @@ auto-service-annotations provided + + com.google.auto.value + auto-value-annotations + provided + com.google.googlejavaformat google-java-format 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 0d32a167fbb..49335432408 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,15 +3,19 @@ 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 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; +import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; +import com.google.common.collect.ImmutableTable; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.IdentifierTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; @@ -19,26 +23,29 @@ import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.ImportTree; +import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.Tree; +import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.code.Symbol.ClassSymbol; -import com.sun.tools.javac.code.Symbol.TypeSymbol; +import org.jspecify.annotations.Nullable; 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 static imports of type members 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. +// XXX: Add suppression support. If qualification of one more more identifiers is suppressed, then +// the associated static import should *not* be removed. +// XXX: Also introduce logic that disallows statically importing `ZoneOffset.ofHours` and other +// `ofXXX`-style methods. @AutoService(BugChecker.class) @BugPattern( - summary = "Identifier should not be statically imported", + summary = "Member should not be statically imported", link = BUG_PATTERNS_BASE_URL + "NonStaticImport", linkType = CUSTOM, severity = SUGGESTION, tags = STYLE) -public final class NonStaticImport extends BugChecker implements IdentifierTreeMatcher { +public final class NonStaticImport extends BugChecker implements CompilationUnitTreeMatcher { private static final long serialVersionUID = 1L; /** @@ -48,16 +55,21 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM *

Types listed here should be mutually exclusive with {@link * StaticImport#STATIC_IMPORT_CANDIDATE_TYPES}. */ + @VisibleForTesting 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 #NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should be omitted - * from this collection. + *

Please note that: * - *

This should be mutually exclusive with {@link StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS}. + *

*/ // 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 @@ -87,11 +99,14 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM /** * Identifiers that should never be statically imported. * - *

Identifiers listed by {@link StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS} should be - * mutually exclusive with identifiers listed here. + *

Please note that: * - *

This should be a superset of the identifiers flagged by {@link - * com.google.errorprone.bugpatterns.BadImport}. + *

*/ static final ImmutableSet NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS = ImmutableSet.of( @@ -117,68 +132,80 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM public NonStaticImport() {} @Override - public Description matchIdentifier(IdentifierTree tree, VisitorState state) { - Symbol symbol = ASTHelpers.getSymbol(tree); - if (symbol == null || !shouldAddQualifier(symbol, state)) { - return Description.NO_MATCH; - } - - SuggestedFix.Builder builder = SuggestedFix.builder(); - String replacement = SuggestedFixes.qualifyType(state, builder, symbol.owner) + "."; + public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { + ImmutableTable undesiredStaticImports = + getUndesiredStaticImports(tree, state); - return describeMatch(tree, builder.prefixWith(tree, replacement).build()); - } + if (!undesiredStaticImports.isEmpty()) { + replaceUndesiredStaticImportUsages(tree, state, undesiredStaticImports); - 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; + for (UndesiredStaticImport staticImport : undesiredStaticImports.values()) { + state.reportMatch( + describeMatch(staticImport.importTree(), staticImport.fixBuilder().build())); + } } - String qualifiedTypeName = symbol.owner.getQualifiedName().toString(); - return !isStaticImportCandidateMember(qualifiedTypeName, identifierName) - && isNonStaticImportCandidate(qualifiedTypeName, identifierName); + /* Any violations have been flagged against the offending static import statement. */ + return Description.NO_MATCH; } - private static boolean isIdentifierDefinedInFile(Symbol symbol, CompilationUnitTree tree) { - return tree.getTypeDecls().stream() - .anyMatch( - t -> { - Symbol topLevelClass = ASTHelpers.getSymbol(t); - return topLevelClass instanceof ClassSymbol - && symbol.isEnclosedBy((ClassSymbol) topLevelClass); - }); - } + private static ImmutableTable getUndesiredStaticImports( + CompilationUnitTree tree, VisitorState state) { + ImmutableTable.Builder imports = + ImmutableTable.builder(); + for (ImportTree importTree : tree.getImports()) { + Tree qualifiedIdentifier = importTree.getQualifiedIdentifier(); + if (importTree.isStatic() && qualifiedIdentifier instanceof MemberSelectTree) { + MemberSelectTree memberSelectTree = (MemberSelectTree) qualifiedIdentifier; + String type = SourceCode.treeToString(memberSelectTree.getExpression(), state); + String member = memberSelectTree.getIdentifier().toString(); + if (shouldNotBeStaticallyImported(type, member)) { + imports.put( + type, + member, + new AutoValue_NonStaticImport_UndesiredStaticImport( + importTree, SuggestedFix.builder().removeStaticImport(type + '.' + member))); + } + } + } - 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); + return imports.build(); } - private static boolean isStaticImportCandidateMember( - String qualifiedTypeName, String identifierName) { - return StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry( - qualifiedTypeName, identifierName); + private static boolean shouldNotBeStaticallyImported(String type, String member) { + return (NON_STATIC_IMPORT_CANDIDATE_TYPES.contains(type) + && !STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(type, member)) + || NON_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(type, member) + || NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(member); } - private static boolean isIdentifierStaticallyImported(String identifierName, VisitorState state) { - return state.getPath().getCompilationUnit().getImports().stream() - .filter(ImportTree::isStatic) - .map(ImportTree::getQualifiedIdentifier) - .map(tree -> getStaticImportIdentifier(tree, state)) - .anyMatch(identifierName::contentEquals); + private static void replaceUndesiredStaticImportUsages( + CompilationUnitTree tree, + VisitorState state, + ImmutableTable undesiredStaticImports) { + new TreeScanner<@Nullable Void, @Nullable Void>() { + @Override + public @Nullable Void visitIdentifier(IdentifierTree node, @Nullable Void unused) { + Symbol symbol = ASTHelpers.getSymbol(node); + if (symbol != null) { + UndesiredStaticImport staticImport = + undesiredStaticImports.get( + symbol.owner.getQualifiedName().toString(), symbol.name.toString()); + if (staticImport != null) { + SuggestedFix.Builder fix = staticImport.fixBuilder(); + fix.prefixWith(node, SuggestedFixes.qualifyType(state, fix, symbol.owner) + '.'); + } + } + + return super.visitIdentifier(node, unused); + } + }.scan(tree, null); } - private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) { - String source = SourceCode.treeToString(tree, state); - return source.subSequence(source.lastIndexOf('.') + 1, source.length()); + @AutoValue + abstract static class UndesiredStaticImport { + abstract ImportTree importTree(); + + abstract SuggestedFix.Builder fixBuilder(); } } 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 9cee4c243a7..020e9fd9226 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 @@ -2,13 +2,14 @@ import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; -import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; +import static com.google.errorprone.BugPattern.StandardTags.STYLE; import static java.util.Objects.requireNonNull; 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; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; import com.google.errorprone.BugPattern; @@ -28,21 +29,19 @@ import com.sun.tools.javac.code.Type; import java.util.Optional; -/** - * A {@link BugChecker} that flags methods and constants that can and should be statically imported. - */ +/** A {@link BugChecker} that flags type members that can and should be statically imported. */ // XXX: Tricky cases: // - `org.springframework.http.HttpStatus` (not always an improvement, and `valueOf` must // certainly be excluded) // - `com.google.common.collect.Tables` -// - `ch.qos.logback.classic.Level.{DEBUG, ERROR, INFO, TRACE, WARN"}` +// - `ch.qos.logback.classic.Level.{DEBUG, ERROR, INFO, TRACE, WARN}` @AutoService(BugChecker.class) @BugPattern( summary = "Identifier should be statically imported", link = BUG_PATTERNS_BASE_URL + "StaticImport", linkType = CUSTOM, severity = SUGGESTION, - tags = SIMPLIFICATION) + tags = STYLE) public final class StaticImport extends BugChecker implements MemberSelectTreeMatcher { private static final long serialVersionUID = 1L; @@ -54,6 +53,7 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa *

Types listed here should be mutually exclusive with {@link * NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_TYPES}. */ + @VisibleForTesting static final ImmutableSet STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of( "com.google.common.base.Preconditions", @@ -103,11 +103,16 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa /** * Type members that should be statically imported. * - *

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

Please note that: * - *

Identifiers listed by {@link NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should - * be mutually exclusive with identifiers listed here. + *

    + *
  • Types listed by {@link #STATIC_IMPORT_CANDIDATE_TYPES} should be omitted from this + * collection. + *
  • This collection should be mutually exclusive with {@link + * NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_MEMBERS}. + *
  • This collection should not list members contained in {@link + * NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS}. + *
*/ static final ImmutableSetMultimap STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() 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 ef4d46ae5f7..982a87f942d 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 @@ -8,6 +8,12 @@ import org.junit.jupiter.api.Test; final class NonStaticImportTest { + @Test + void candidateTypesDoNotClash() { + assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_TYPES) + .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); + } + @Test void candidateMembersAreNotRedundant() { assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) @@ -18,19 +24,13 @@ void candidateMembersAreNotRedundant() { } @Test - void badTypesDontClashWithStaticImportCandidates() { - assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_TYPES) - .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); - } - - @Test - void badMembersDontClashWithStaticImportCandidates() { + void candidateMembersDoNotClash() { assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS.entries()) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.entries()); } @Test - void badIdentifiersDontClashWithStaticImportCandidates() { + void candidateIdentifiersDoNotClash() { assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.values()); } @@ -52,15 +52,23 @@ void identification() { "pkg/A.java", "package pkg;", "", + "// BUG: Diagnostic contains:", "import static com.google.common.collect.ImmutableList.copyOf;", + "// BUG: Diagnostic contains:", "import static java.lang.Integer.MAX_VALUE;", + "// BUG: Diagnostic contains:", "import static java.lang.Integer.MIN_VALUE;", + "// BUG: Diagnostic contains:", "import static java.time.Clock.systemUTC;", + "// BUG: Diagnostic contains:", "import static java.time.Instant.MIN;", "import static java.time.ZoneOffset.UTC;", + "// BUG: Diagnostic contains:", "import static java.util.Collections.min;", "import static java.util.Locale.ENGLISH;", + "// BUG: Diagnostic contains:", "import static java.util.Locale.ROOT;", + "// BUG: Diagnostic contains:", "import static java.util.Optional.empty;", "", "import com.google.common.collect.ImmutableList;", @@ -76,29 +84,23 @@ void identification() { " private Integer MIN_VALUE = 12;", "", " void m() {", - " // BUG: Diagnostic contains:", " systemUTC();", " Clock.systemUTC();", " ZoneOffset utcIsExempted = UTC;", "", - " // BUG: Diagnostic contains:", " Optional optional1 = empty();", " Optional optional2 = Optional.empty();", "", - " // BUG: Diagnostic contains:", " ImmutableList list1 = copyOf(ImmutableList.of());", " ImmutableList list2 = ImmutableList.copyOf(ImmutableList.of());", "", - " // 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:", " ImmutableSet.of(min(ImmutableSet.of()));", "", " Object builder = null;", @@ -111,19 +113,12 @@ void identification() { "", " Integer from = 12;", " Integer i1 = from;", - "", - " create();", - " }", - "", - " void create() {", - " Integer MIN_VALUE = 12;", - " // Not flagged because identifier is not statically imported.", - " Integer i1 = MIN_VALUE;", " }", "}") .doTest(); } + // XXX: Test multiple replacements of the same import. @Test void replacement() { BugCheckerRefactoringTestHelper.newInstance(NonStaticImport.class, getClass()) @@ -164,18 +159,14 @@ void replacement() { "", " ImmutableSet.of(min(of()));", " }", + "", + " private static final class WithCustomConstant {", + " private static final int MIN = 42;", + " private static final int MAX = MIN;", + " }", "}") .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;", @@ -203,6 +194,11 @@ void replacement() { "", " ImmutableSet.of(Collections.min(ImmutableSet.of()));", " }", + "", + " private static final class WithCustomConstant {", + " private static final int MIN = 42;", + " private static final int MAX = MIN;", + " }", "}") .doTest(TestMode.TEXT_MATCH); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportTest.java index 6aca45a9010..bcfb097d399 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportTest.java @@ -9,11 +9,26 @@ final class StaticImportTest { @Test - void candidateMethodsAreNotRedundant() { + void candidateTypesDoNotClash() { + assertThat(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES) + .doesNotContainAnyElementsOf(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_TYPES); + } + + @Test + void candidateMembersAreNotRedundant() { assertThat(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); } + @Test + void candidateMembersDoNotClash() { + assertThat(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.entries()) + .doesNotContainAnyElementsOf(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS.entries()); + + assertThat(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.values()) + .doesNotContainAnyElementsOf(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS); + } + @Test void identification() { CompilationTestHelper.newInstance(StaticImport.class, getClass())