From 7a1b6630b69c507ba117268c007976dd760f85c6 Mon Sep 17 00:00:00 2001 From: ccernat Date: Thu, 5 Jan 2023 17:48:44 +0100 Subject: [PATCH 01/20] Introduce bad static import check --- .../bugpatterns/BadStaticImport.java | 95 +++++++++++ .../bugpatterns/BadStaticImportTest.java | 157 ++++++++++++++++++ 2 files changed, 252 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java new file mode 100644 index 0000000000..ba4e3a40ca --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java @@ -0,0 +1,95 @@ +package tech.picnic.errorprone.bugpatterns; + +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 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; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol; + +@AutoService(BugChecker.class) +@BugPattern( + summary = "Identifier should be statically imported", + link = BUG_PATTERNS_BASE_URL + "StaticImport", + linkType = CUSTOM, + severity = SUGGESTION, + tags = SIMPLIFICATION) + public class BadStaticImport extends BugChecker implements BugChecker.MethodInvocationTreeMatcher, BugChecker.IdentifierTreeMatcher { + private static final long serialVersionUID = 1L; + + @VisibleForTesting + static final ImmutableSet BAD_STATIC_IMPORT_CANDIDATE_TYPES = + ImmutableSet.of("com.google.common.base.Strings", "java.time.Clock"); + + @VisibleForTesting + static final ImmutableSetMultimap BAD_STATIC_IMPORT_CANDIDATE_MEMBERS = + ImmutableSetMultimap.builder() + .put("java.util.Locale", "ROOT") + .put("java.util.Optional", "empty") + .build(); + + @VisibleForTesting + static final ImmutableSet BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS = + ImmutableSet.of("builder", "copyOf", "MIN", "MAX"); + + public BadStaticImport() {} + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + ExpressionTree expr = tree.getMethodSelect(); + switch (expr.getKind()) { + case IDENTIFIER: + return matchIdentifier((IdentifierTree) tree.getMethodSelect(), state); + default: + return Description.NO_MATCH; + } + } + + @Override + public Description matchIdentifier(IdentifierTree tree, VisitorState state) { + Symbol symbol = ASTHelpers.getSymbol(tree); + if (isCandidate(symbol)) { + return getDescription(tree, state, symbol); + } + + return Description.NO_MATCH; + } + + private Description getDescription(IdentifierTree tree, VisitorState state, Symbol symbol) { + SuggestedFix.Builder builder = + SuggestedFix.builder().removeStaticImport(getImportToRemove(symbol)); + String replacement = + SuggestedFixes.qualifyType(state, builder, symbol.getEnclosingElement()) + "."; + builder.prefixWith(tree, replacement); + return describeMatch(tree, builder.build()); + } + + private String getImportToRemove(Symbol symbol) { + return String.format( + "%s.%s", + symbol.getEnclosingElement().getQualifiedName().toString(), + symbol.getSimpleName().toString()); + } + + private static boolean isCandidate(Symbol symbol) { + String qualifiedName = symbol.getEnclosingElement().getQualifiedName().toString(); + return BAD_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedName) + || BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry( + qualifiedName, symbol.getSimpleName().toString()) + || BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(symbol.getSimpleName().toString()); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java new file mode 100644 index 0000000000..c735c30a4f --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java @@ -0,0 +1,157 @@ +package tech.picnic.errorprone.bugpatterns; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class BadStaticImportTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(BadStaticImport.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(BadStaticImport.class, getClass()); + + @Test + void candidateMembersAreNotRedundant() { + assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) + .doesNotContainAnyElementsOf(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES); + + assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.values()) + .doesNotContainAnyElementsOf(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS); + } + + @Test + void identifySimpleMethodInvocation() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static java.time.Clock.systemUTC;", + "import static java.util.Optional.empty;", + "import static com.google.common.collect.ImmutableList.copyOf;", + "import static com.google.common.collect.ImmutableMap.of;", + "import java.time.Clock;", + "import com.google.common.collect.ImmutableList;", + + "", + "class A {", + " void m() {", + " // BUG: Diagnostic contains:", + " systemUTC();", + " Clock.systemUTC();", + "", + " // BUG: Diagnostic contains:", + " Object o1 = empty();", + "", + " // BUG: Diagnostic contains:", + " Object l1 = copyOf(ImmutableList.of());", + " Object l2 = ImmutableList.copyOf(ImmutableList.of());", + " }", + "}") + .doTest(); + } + + // XXX: Add more counterexamples + @Test + void replaceSimpleMethodInvocation() { + refactoringTestHelper + .addInputLines( + "A.java", + "import static java.time.Clock.systemUTC;", + "import static java.util.Optional.empty;", + "import static com.google.common.collect.ImmutableList.copyOf;", + "import com.google.common.collect.ImmutableList;", + "import java.time.Clock;", + "", + "class A {", + " void m() {", + " systemUTC();", + " Clock.systemUTC();", + "", + " Object o1 = empty();", + "", + " Object l1 = copyOf(ImmutableList.of());", + " Object l2 = ImmutableList.copyOf(ImmutableList.of());", + " }", + "}") + .addOutputLines( + "A.java", + "import com.google.common.collect.ImmutableList;", + "import java.time.Clock;", + "import java.util.Optional;", + "", + "class A {", + " void m() {", + " Clock.systemUTC();", + " Clock.systemUTC();", + "", + " Object o1 = Optional.empty();", + "", + " Object l1 = ImmutableList.copyOf(ImmutableList.of());", + " Object l2 = ImmutableList.copyOf(ImmutableList.of());", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void identifySimpleFieldAccess() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static java.time.Instant.MAX;", + "import static java.time.Instant.MIN;", + "import static java.util.Locale.ROOT;", + "import java.util.Locale;", + "", + "class A {", + " void m() {", + " // BUG: Diagnostic contains:", + " Locale l1 = ROOT;", + " Locale l2 = Locale.ROOT;", + "", + " // BUG: Diagnostic contains:", + " Object c1 = MIN;", + " // BUG: Diagnostic contains:", + " Object c2 = MAX;", + " }", + "}") + .doTest(); + } + + @Test + void replaceSimpleFieldAccess() { + refactoringTestHelper + .addInputLines( + "A.java", + "import static java.time.Instant.MAX;", + "import static java.time.Instant.MIN;", + "import static java.util.Locale.ROOT;", + "import java.util.Locale;", + "", + "class A {", + " void m() {", + " Locale l1 = ROOT;", + " Locale l2 = Locale.ROOT;", + "", + " Object c1 = MIN;", + " Object c2 = MAX;", + " }", + "}") + .addOutputLines( + "A.java", + "import java.time.Instant;", + "import java.util.Locale;", + "", + "class A {", + " void m() {", + " Locale l1 = Locale.ROOT;", + " Locale l2 = Locale.ROOT;", + "", + " Object c1 = Instant.MIN;", + " Object c2 = Instant.MAX;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } +} \ No newline at end of file From 3ba774fd6730e21e819c274f81746c0c48331380 Mon Sep 17 00:00:00 2001 From: ccernat Date: Fri, 6 Jan 2023 09:47:41 +0100 Subject: [PATCH 02/20] Add tests for clashes with static import candidates --- .../bugpatterns/BadStaticImportTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java index c735c30a4f..5e7a2549f7 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java @@ -21,6 +21,30 @@ void candidateMembersAreNotRedundant() { .doesNotContainAnyElementsOf(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS); } + @Test + void badTypesDontClashWithStaticImportCandidates() { + assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES) + .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); + + assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES) + .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()); + } + + @Test + void badMembersDontClashWithStaticImportCandidates() { + assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) + .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); + + assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.values()) + .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_EXEMPTED_IDENTIFIERS); + } + + @Test + void badIdentifiersDontClashWithStaticImportCandidates() { + assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS) + .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.values()); + } + @Test void identifySimpleMethodInvocation() { compilationTestHelper From 9ea85117dcd72578a7d9e5697e4a6ada630d9c6c Mon Sep 17 00:00:00 2001 From: ccernat Date: Fri, 6 Jan 2023 11:32:17 +0100 Subject: [PATCH 03/20] Move StaticImport exempted members into BadStaticImport --- .../bugpatterns/BadStaticImport.java | 92 +++++++++++++++--- .../errorprone/bugpatterns/StaticImport.java | 82 ++++------------ .../bugpatterns/BadStaticImportTest.java | 97 ++++++------------- .../bugpatterns/StaticImportTest.java | 12 --- 4 files changed, 128 insertions(+), 155 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java index ba4e3a40ca..83c3cec707 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java @@ -6,7 +6,6 @@ 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; @@ -21,31 +20,93 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Symbol; +/** A {@link BugChecker} that flags methods and constants that should not be statically imported. */ +// XXX: Also introduce checks that disallows the following candidates: +// - `com.google.common.base.Strings` +// - `java.util.Optional.empty` +// - `java.util.Locale.ROOT` +// - `ZoneOffset.ofHours` and other `ofXXX`-style methods. +// - `java.time.Clock`. +// - Several other `java.time` classes. +// - Likely any of `*.{ZERO, ONE, MIX, MAX, MIN_VALUE, MAX_VALUE}`. @AutoService(BugChecker.class) @BugPattern( - summary = "Identifier should be statically imported", - link = BUG_PATTERNS_BASE_URL + "StaticImport", + summary = "Identifier should not be statically imported", + link = BUG_PATTERNS_BASE_URL + "BadStaticImport", linkType = CUSTOM, severity = SUGGESTION, tags = SIMPLIFICATION) - public class BadStaticImport extends BugChecker implements BugChecker.MethodInvocationTreeMatcher, BugChecker.IdentifierTreeMatcher { +public final class BadStaticImport extends BugChecker + implements BugChecker.MethodInvocationTreeMatcher, BugChecker.IdentifierTreeMatcher { private static final long serialVersionUID = 1L; - @VisibleForTesting + /** + * Types whose members should not be statically imported, unless exempted by {@link + * StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS}. + * + *

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

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

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 + // specific context is left out. static final ImmutableSetMultimap BAD_STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() + .put("com.google.common.base.Predicates", "contains") + .put("com.mongodb.client.model.Filters", "empty") + .putAll( + "java.util.Collections", + "addAll", + "copy", + "fill", + "list", + "max", + "min", + "nCopies", + "rotate", + "sort", + "swap") .put("java.util.Locale", "ROOT") .put("java.util.Optional", "empty") + .putAll("java.util.regex.Pattern", "compile", "matches", "quote") + .put("org.springframework.http.MediaType", "ALL") .build(); - @VisibleForTesting + /** + * Identifiers that should never be statically imported. + * + *

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

This should be a superset of the identifiers flagged by {@link + * com.google.errorprone.bugpatterns.BadImport}. + */ static final ImmutableSet BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS = - ImmutableSet.of("builder", "copyOf", "MIN", "MAX"); + ImmutableSet.of( + "builder", + "create", + "copyOf", + "from", + "getDefaultInstance", + "INSTANCE", + "MIN", + "MAX", + "newBuilder", + "of", + "valueOf"); + /** Instantiates a new {@link BadStaticImport} instance. */ public BadStaticImport() {} @Override @@ -78,7 +139,8 @@ private Description getDescription(IdentifierTree tree, VisitorState state, Symb return describeMatch(tree, builder.build()); } - private String getImportToRemove(Symbol symbol) { + @SuppressWarnings("NullAway") + private static String getImportToRemove(Symbol symbol) { return String.format( "%s.%s", symbol.getEnclosingElement().getQualifiedName().toString(), @@ -86,10 +148,16 @@ private String getImportToRemove(Symbol symbol) { } private static boolean isCandidate(Symbol symbol) { - String qualifiedName = symbol.getEnclosingElement().getQualifiedName().toString(); - return BAD_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedName) + Symbol enclosingSymbol = symbol.getEnclosingElement(); + + if (enclosingSymbol == null) { + return false; + } + + String qualifiedTypeName = enclosingSymbol.getQualifiedName().toString(); + return BAD_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedTypeName) || BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry( - qualifiedName, symbol.getSimpleName().toString()) + qualifiedTypeName, symbol.getSimpleName().toString()) || BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(symbol.getSimpleName().toString()); } } 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 c5b3e52427..c1df8b9a8f 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,10 +4,11 @@ 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.BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS; +import static tech.picnic.errorprone.bugpatterns.BadStaticImport.BAD_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; @@ -35,14 +36,6 @@ // certainly be excluded) // - `com.google.common.collect.Tables` // - `ch.qos.logback.classic.Level.{DEBUG, ERROR, INFO, TRACE, WARN"}` -// XXX: Also introduce a check that disallows static imports of certain methods. Candidates: -// - `com.google.common.base.Strings` -// - `java.util.Optional.empty` -// - `java.util.Locale.ROOT` -// - `ZoneOffset.ofHours` and other `ofXXX`-style methods. -// - `java.time.Clock`. -// - Several other `java.time` classes. -// - Likely any of `*.{ZERO, ONE, MIX, MAX, MIN_VALUE, MAX_VALUE}`. @AutoService(BugChecker.class) @BugPattern( summary = "Identifier should be statically imported", @@ -55,9 +48,12 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa /** * Types whose members should be statically imported, unless exempted by {@link - * #STATIC_IMPORT_EXEMPTED_MEMBERS} or {@link #STATIC_IMPORT_EXEMPTED_IDENTIFIERS}. + * BadStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS} or {@link + * BadStaticImport#BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS}. + * + *

Types listed here should be mutually exclusive with {@link + * BadStaticImport#BAD_STATIC_IMPORT_CANDIDATE_TYPES} */ - @VisibleForTesting static final ImmutableSet STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of( "com.google.common.base.Preconditions", @@ -104,8 +100,15 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa "reactor.function.TupleUtils", "tech.picnic.errorprone.bugpatterns.util.MoreTypes"); - /** Type members that should be statically imported. */ - @VisibleForTesting + /** + * Type members that should be statically imported. + * + *

This should be mutually exclusive with {@link + * BadStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS} + * + *

Identifiers listed by {@link BadStaticImport#BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should + * be mutually exclusive with identifiers listed here. + */ static final ImmutableSetMultimap STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() .putAll( @@ -143,55 +146,6 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa .putAll("com.google.common.collect.Comparators", "emptiesFirst", "emptiesLast") .build(); - /** - * Type members that should never be statically imported. - * - *

Identifiers listed by {@link #STATIC_IMPORT_EXEMPTED_IDENTIFIERS} should be omitted from - * this collection. - */ - // 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. - @VisibleForTesting - static final ImmutableSetMultimap STATIC_IMPORT_EXEMPTED_MEMBERS = - ImmutableSetMultimap.builder() - .put("com.google.common.base.Predicates", "contains") - .put("com.mongodb.client.model.Filters", "empty") - .putAll( - "java.util.Collections", - "addAll", - "copy", - "fill", - "list", - "max", - "min", - "nCopies", - "rotate", - "sort", - "swap") - .putAll("java.util.regex.Pattern", "compile", "matches", "quote") - .put("org.springframework.http.MediaType", "ALL") - .build(); - - /** - * Identifiers that should never be statically imported. - * - *

This should be a superset of the identifiers flagged by {@link - * com.google.errorprone.bugpatterns.BadImport}. - */ - @VisibleForTesting - static final ImmutableSet STATIC_IMPORT_EXEMPTED_IDENTIFIERS = - ImmutableSet.of( - "builder", - "create", - "copyOf", - "from", - "getDefaultInstance", - "INSTANCE", - "newBuilder", - "of", - "valueOf"); - /** Instantiates a new {@link StaticImport} instance. */ public StaticImport() {} @@ -229,13 +183,13 @@ private static boolean isCandidateContext(VisitorState state) { private static boolean isCandidate(MemberSelectTree tree) { String identifier = tree.getIdentifier().toString(); - if (STATIC_IMPORT_EXEMPTED_IDENTIFIERS.contains(identifier)) { + if (BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifier)) { return false; } Type type = ASTHelpers.getType(tree.getExpression()); return type != null - && !STATIC_IMPORT_EXEMPTED_MEMBERS.containsEntry(type.toString(), identifier); + && !BAD_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/BadStaticImportTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java index 5e7a2549f7..db69502a89 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java @@ -25,18 +25,12 @@ void candidateMembersAreNotRedundant() { void badTypesDontClashWithStaticImportCandidates() { assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); - - assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES) - .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()); } @Test void badMembersDontClashWithStaticImportCandidates() { - assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) - .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); - - assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.values()) - .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_EXEMPTED_IDENTIFIERS); + assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.entries()) + .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.entries()); } @Test @@ -54,9 +48,12 @@ void identifySimpleMethodInvocation() { "import static java.util.Optional.empty;", "import static com.google.common.collect.ImmutableList.copyOf;", "import static com.google.common.collect.ImmutableMap.of;", + "import static java.time.Instant.MAX;", + "import static java.time.Instant.MIN;", + "import static java.util.Locale.ROOT;", "import java.time.Clock;", "import com.google.common.collect.ImmutableList;", - + "import java.util.Locale;", "", "class A {", " void m() {", @@ -70,6 +67,15 @@ void identifySimpleMethodInvocation() { " // BUG: Diagnostic contains:", " Object l1 = copyOf(ImmutableList.of());", " Object l2 = ImmutableList.copyOf(ImmutableList.of());", + "", + " // BUG: Diagnostic contains:", + " Locale lo1 = ROOT;", + " Locale lo2 = Locale.ROOT;", + "", + " // BUG: Diagnostic contains:", + " Object c1 = MIN;", + " // BUG: Diagnostic contains:", + " Object c2 = MAX;", " }", "}") .doTest(); @@ -84,8 +90,12 @@ void replaceSimpleMethodInvocation() { "import static java.time.Clock.systemUTC;", "import static java.util.Optional.empty;", "import static com.google.common.collect.ImmutableList.copyOf;", + "import static java.time.Instant.MAX;", + "import static java.time.Instant.MIN;", + "import static java.util.Locale.ROOT;", "import com.google.common.collect.ImmutableList;", "import java.time.Clock;", + "import java.util.Locale;", "", "class A {", " void m() {", @@ -96,12 +106,20 @@ void replaceSimpleMethodInvocation() { "", " Object l1 = copyOf(ImmutableList.of());", " Object l2 = ImmutableList.copyOf(ImmutableList.of());", + "", + " Locale lo1 = ROOT;", + " Locale lo2 = Locale.ROOT;", + "", + " Object c1 = MIN;", + " Object c2 = MAX;", " }", "}") .addOutputLines( "A.java", "import com.google.common.collect.ImmutableList;", "import java.time.Clock;", + "import java.time.Instant;", + "import java.util.Locale;", "import java.util.Optional;", "", "class A {", @@ -113,64 +131,9 @@ void replaceSimpleMethodInvocation() { "", " Object l1 = ImmutableList.copyOf(ImmutableList.of());", " Object l2 = ImmutableList.copyOf(ImmutableList.of());", - " }", - "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); - } - - @Test - void identifySimpleFieldAccess() { - compilationTestHelper - .addSourceLines( - "A.java", - "import static java.time.Instant.MAX;", - "import static java.time.Instant.MIN;", - "import static java.util.Locale.ROOT;", - "import java.util.Locale;", - "", - "class A {", - " void m() {", - " // BUG: Diagnostic contains:", - " Locale l1 = ROOT;", - " Locale l2 = Locale.ROOT;", - "", - " // BUG: Diagnostic contains:", - " Object c1 = MIN;", - " // BUG: Diagnostic contains:", - " Object c2 = MAX;", - " }", - "}") - .doTest(); - } - - @Test - void replaceSimpleFieldAccess() { - refactoringTestHelper - .addInputLines( - "A.java", - "import static java.time.Instant.MAX;", - "import static java.time.Instant.MIN;", - "import static java.util.Locale.ROOT;", - "import java.util.Locale;", "", - "class A {", - " void m() {", - " Locale l1 = ROOT;", - " Locale l2 = Locale.ROOT;", - "", - " Object c1 = MIN;", - " Object c2 = MAX;", - " }", - "}") - .addOutputLines( - "A.java", - "import java.time.Instant;", - "import java.util.Locale;", - "", - "class A {", - " void m() {", - " Locale l1 = Locale.ROOT;", - " Locale l2 = Locale.ROOT;", + " Locale lo1 = Locale.ROOT;", + " Locale lo2 = Locale.ROOT;", "", " Object c1 = Instant.MIN;", " Object c2 = Instant.MAX;", @@ -178,4 +141,4 @@ void replaceSimpleFieldAccess() { "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } -} \ No newline at end of file +} 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 d876f41082..6aca45a901 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 @@ -14,18 +14,6 @@ void candidateMethodsAreNotRedundant() { .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); } - @Test - void exemptedMembersAreNotVacuous() { - assertThat(StaticImport.STATIC_IMPORT_EXEMPTED_MEMBERS.keySet()) - .isSubsetOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); - } - - @Test - void exemptedMembersAreNotRedundant() { - assertThat(StaticImport.STATIC_IMPORT_EXEMPTED_MEMBERS.values()) - .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_EXEMPTED_IDENTIFIERS); - } - @Test void identification() { CompilationTestHelper.newInstance(StaticImport.class, getClass()) From 27765808c79a1490bdb91346ac1396032410e0a1 Mon Sep 17 00:00:00 2001 From: ccernat Date: Fri, 6 Jan 2023 12:13:43 +0100 Subject: [PATCH 04/20] Add more tests --- .../bugpatterns/BadStaticImport.java | 30 +++++++++++-------- .../bugpatterns/BadStaticImportTest.java | 27 ++++++++++++----- 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java index 83c3cec707..03e655893f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java @@ -3,6 +3,7 @@ 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 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; @@ -22,9 +23,6 @@ /** A {@link BugChecker} that flags methods and constants that should not be statically imported. */ // XXX: Also introduce checks that disallows the following candidates: -// - `com.google.common.base.Strings` -// - `java.util.Optional.empty` -// - `java.util.Locale.ROOT` // - `ZoneOffset.ofHours` and other `ofXXX`-style methods. // - `java.time.Clock`. // - Several other `java.time` classes. @@ -123,7 +121,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState @Override public Description matchIdentifier(IdentifierTree tree, VisitorState state) { Symbol symbol = ASTHelpers.getSymbol(tree); - if (isCandidate(symbol)) { + if (isMatch(symbol)) { return getDescription(tree, state, symbol); } @@ -141,23 +139,29 @@ private Description getDescription(IdentifierTree tree, VisitorState state, Symb @SuppressWarnings("NullAway") private static String getImportToRemove(Symbol symbol) { - return String.format( - "%s.%s", - symbol.getEnclosingElement().getQualifiedName().toString(), - symbol.getSimpleName().toString()); + return String.join( + ".", symbol.getEnclosingElement().getQualifiedName(), symbol.getSimpleName()); } - private static boolean isCandidate(Symbol symbol) { + private static boolean isMatch(Symbol symbol) { Symbol enclosingSymbol = symbol.getEnclosingElement(); - if (enclosingSymbol == null) { return false; } String qualifiedTypeName = enclosingSymbol.getQualifiedName().toString(); + String identifierName = symbol.getSimpleName().toString(); + return !isExempted(qualifiedTypeName, identifierName) + && isCandidate(qualifiedTypeName, identifierName); + } + + private static boolean isCandidate(String qualifiedTypeName, String identifierName) { return BAD_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedTypeName) - || BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry( - qualifiedTypeName, symbol.getSimpleName().toString()) - || BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(symbol.getSimpleName().toString()); + || BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName) + || BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifierName); + } + + private static boolean isExempted(String qualifiedTypeName, String identifierName) { + return STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java index db69502a89..9ebee08275 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java @@ -44,15 +44,17 @@ void identifySimpleMethodInvocation() { compilationTestHelper .addSourceLines( "A.java", - "import static java.time.Clock.systemUTC;", - "import static java.util.Optional.empty;", "import static com.google.common.collect.ImmutableList.copyOf;", - "import static com.google.common.collect.ImmutableMap.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 java.time.Clock;", + "import static java.util.Optional.empty;", + "", "import com.google.common.collect.ImmutableList;", + "import com.google.common.collect.ImmutableSet;", + "import java.time.Clock;", "import java.util.Locale;", "", "class A {", @@ -76,24 +78,29 @@ void identifySimpleMethodInvocation() { " Object c1 = MIN;", " // BUG: Diagnostic contains:", " Object c2 = MAX;", + "", + " // BUG: Diagnostic contains:", + " ImmutableSet.of(min(ImmutableSet.of()));", " }", "}") .doTest(); } - // XXX: Add more counterexamples @Test void replaceSimpleMethodInvocation() { refactoringTestHelper .addInputLines( "A.java", - "import static java.time.Clock.systemUTC;", - "import static java.util.Optional.empty;", "import static com.google.common.collect.ImmutableList.copyOf;", + "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;", "import java.util.Locale;", "", @@ -112,13 +119,17 @@ void replaceSimpleMethodInvocation() { "", " Object c1 = MIN;", " Object c2 = MAX;", + "", + " ImmutableSet.of(min(ImmutableSet.of()));", " }", "}") .addOutputLines( "A.java", "import com.google.common.collect.ImmutableList;", + "import com.google.common.collect.ImmutableSet;", "import java.time.Clock;", "import java.time.Instant;", + "import java.util.Collections;", "import java.util.Locale;", "import java.util.Optional;", "", @@ -137,6 +148,8 @@ void replaceSimpleMethodInvocation() { "", " Object c1 = Instant.MIN;", " Object c2 = Instant.MAX;", + "", + " ImmutableSet.of(Collections.min(ImmutableSet.of()));", " }", "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); From ac84c264f50baea2eba2684a48dce307688c0df7 Mon Sep 17 00:00:00 2001 From: ccernat Date: Fri, 6 Jan 2023 13:40:53 +0100 Subject: [PATCH 05/20] Check ASTHelpers.getSymbol for nulls --- .../errorprone/bugpatterns/BadStaticImport.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java index 03e655893f..c88ba76744 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java @@ -120,15 +120,15 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState @Override public Description matchIdentifier(IdentifierTree tree, VisitorState state) { - Symbol symbol = ASTHelpers.getSymbol(tree); - if (isMatch(symbol)) { - return getDescription(tree, state, symbol); + if (isMatch(tree)) { + return getDescription(tree, state); } return Description.NO_MATCH; } - private Description getDescription(IdentifierTree tree, VisitorState state, Symbol symbol) { + private Description getDescription(IdentifierTree tree, VisitorState state) { + Symbol symbol = ASTHelpers.getSymbol(tree); SuggestedFix.Builder builder = SuggestedFix.builder().removeStaticImport(getImportToRemove(symbol)); String replacement = @@ -143,7 +143,12 @@ private static String getImportToRemove(Symbol symbol) { ".", symbol.getEnclosingElement().getQualifiedName(), symbol.getSimpleName()); } - private static boolean isMatch(Symbol symbol) { + private static boolean isMatch(IdentifierTree tree) { + Symbol symbol = ASTHelpers.getSymbol(tree); + if (symbol == null) { + return false; + } + Symbol enclosingSymbol = symbol.getEnclosingElement(); if (enclosingSymbol == null) { return false; From 2e24be84ac52fdb99f157a65181457f67abaf805 Mon Sep 17 00:00:00 2001 From: ccernat Date: Fri, 6 Jan 2023 14:25:14 +0100 Subject: [PATCH 06/20] Avoid flagging variables and same file classes --- .../bugpatterns/BadStaticImport.java | 29 ++++++++++++++++--- .../bugpatterns/BadStaticImportTest.java | 13 +++++++-- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java index c88ba76744..a347d23153 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java @@ -3,6 +3,7 @@ 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.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; @@ -18,8 +19,11 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.ImportTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** A {@link BugChecker} that flags methods and constants that should not be statically imported. */ // XXX: Also introduce checks that disallows the following candidates: @@ -120,7 +124,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState @Override public Description matchIdentifier(IdentifierTree tree, VisitorState state) { - if (isMatch(tree)) { + if (isMatch(tree, state)) { return getDescription(tree, state); } @@ -143,19 +147,23 @@ private static String getImportToRemove(Symbol symbol) { ".", symbol.getEnclosingElement().getQualifiedName(), symbol.getSimpleName()); } - private static boolean isMatch(IdentifierTree tree) { + private static boolean isMatch(IdentifierTree tree, VisitorState state) { Symbol symbol = ASTHelpers.getSymbol(tree); if (symbol == null) { return false; } Symbol enclosingSymbol = symbol.getEnclosingElement(); - if (enclosingSymbol == null) { + if (enclosingSymbol == null || enclosingSymbol.kind != TYP) { return false; } - String qualifiedTypeName = enclosingSymbol.getQualifiedName().toString(); String identifierName = symbol.getSimpleName().toString(); + if (!isIdentifierStaticallyImported(identifierName, state)) { + return false; + } + + String qualifiedTypeName = enclosingSymbol.getQualifiedName().toString(); return !isExempted(qualifiedTypeName, identifierName) && isCandidate(qualifiedTypeName, identifierName); } @@ -169,4 +177,17 @@ private static boolean isCandidate(String qualifiedTypeName, String identifierNa private static boolean isExempted(String qualifiedTypeName, String identifierName) { return STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName); } + + 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 CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) { + String source = SourceCode.treeToString(tree, state); + return source.subSequence(source.lastIndexOf('.') + 1, source.length()); + } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java index 9ebee08275..45693ece71 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java @@ -40,7 +40,7 @@ void badIdentifiersDontClashWithStaticImportCandidates() { } @Test - void identifySimpleMethodInvocation() { + void identification() { compilationTestHelper .addSourceLines( "A.java", @@ -81,13 +81,22 @@ void identifySimpleMethodInvocation() { "", " // BUG: Diagnostic contains:", " ImmutableSet.of(min(ImmutableSet.of()));", + "", + " Object builder = null;", + " // Not flagged because identifier is variable name", + " Object lBuilder = ImmutableList.of(builder);", + "", + " // Not flagged because method is not statically imported", + " create();", " }", + "", + " void create() {}", "}") .doTest(); } @Test - void replaceSimpleMethodInvocation() { + void replacement() { refactoringTestHelper .addInputLines( "A.java", From b6c10ea6ed66e1739f5619fb0bfbe1de6f1e838c Mon Sep 17 00:00:00 2001 From: ccernat Date: Mon, 9 Jan 2023 22:33:45 +0100 Subject: [PATCH 07/20] Test against more edge cases --- .../bugpatterns/BadStaticImport.java | 34 ++++++++++--------- .../bugpatterns/BadStaticImportTest.java | 34 ++++++++++++++++++- 2 files changed, 51 insertions(+), 17 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java index a347d23153..e1facf994a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java @@ -3,6 +3,7 @@ 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.util.ASTHelpers.getSymbol; 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; @@ -17,10 +18,9 @@ import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.util.ASTHelpers; -import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.ImportTree; -import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; import tech.picnic.errorprone.bugpatterns.util.SourceCode; @@ -38,8 +38,7 @@ linkType = CUSTOM, severity = SUGGESTION, tags = SIMPLIFICATION) -public final class BadStaticImport extends BugChecker - implements BugChecker.MethodInvocationTreeMatcher, BugChecker.IdentifierTreeMatcher { +public final class BadStaticImport extends BugChecker implements BugChecker.IdentifierTreeMatcher { private static final long serialVersionUID = 1L; /** @@ -50,7 +49,7 @@ public final class BadStaticImport extends BugChecker * StaticImport#STATIC_IMPORT_CANDIDATE_TYPES} */ static final ImmutableSet BAD_STATIC_IMPORT_CANDIDATE_TYPES = - ImmutableSet.of("com.google.common.base.Strings", "java.time.Clock"); + ImmutableSet.of("com.google.common.base.Strings", "java.time.Clock", "java.time.ZoneOffset"); /** * Type members that should never be statically imported. @@ -103,6 +102,7 @@ public final class BadStaticImport extends BugChecker "getDefaultInstance", "INSTANCE", "MIN", + "MIN_VALUE", "MAX", "newBuilder", "of", @@ -111,17 +111,6 @@ public final class BadStaticImport extends BugChecker /** Instantiates a new {@link BadStaticImport} instance. */ public BadStaticImport() {} - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - ExpressionTree expr = tree.getMethodSelect(); - switch (expr.getKind()) { - case IDENTIFIER: - return matchIdentifier((IdentifierTree) tree.getMethodSelect(), state); - default: - return Description.NO_MATCH; - } - } - @Override public Description matchIdentifier(IdentifierTree tree, VisitorState state) { if (isMatch(tree, state)) { @@ -159,6 +148,9 @@ private static boolean isMatch(IdentifierTree tree, VisitorState state) { } String identifierName = symbol.getSimpleName().toString(); + if (isDefinedInThisFile(symbol, state.getPath().getCompilationUnit())) { + return false; + } if (!isIdentifierStaticallyImported(identifierName, state)) { return false; } @@ -168,6 +160,16 @@ private static boolean isMatch(IdentifierTree tree, VisitorState state) { && isCandidate(qualifiedTypeName, identifierName); } + private static boolean isDefinedInThisFile(Symbol symbol, CompilationUnitTree tree) { + return tree.getTypeDecls().stream() + .anyMatch( + t -> { + Symbol topLevelClass = getSymbol(t); + return topLevelClass instanceof Symbol.ClassSymbol + && symbol.isEnclosedBy((Symbol.ClassSymbol) topLevelClass); + }); + } + private static boolean isCandidate(String qualifiedTypeName, String identifierName) { return BAD_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedTypeName) || BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java index 45693ece71..b37ed611ef 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java @@ -42,13 +42,25 @@ void badIdentifiersDontClashWithStaticImportCandidates() { @Test void identification() { compilationTestHelper + .addSourceLines( + "pkg/B.java", + "package pkg;", + "", + "public class B {", + " public int MIN = 1;", + "", + " public static class INSTANCE {}", + "}") .addSourceLines( "A.java", "import static com.google.common.collect.ImmutableList.copyOf;", + "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;", + "import static java.util.Locale.ENGLISH;", "import static java.util.Locale.ROOT;", "import static java.util.Optional.empty;", "", @@ -56,8 +68,12 @@ void identification() { "import com.google.common.collect.ImmutableSet;", "import java.time.Clock;", "import java.util.Locale;", + "import pkg.B;", + "import pkg.B.INSTANCE;", "", "class A {", + " private Integer MIN_VALUE = 12;", + "", " void m() {", " // BUG: Diagnostic contains:", " systemUTC();", @@ -83,11 +99,27 @@ void identification() { " ImmutableSet.of(min(ImmutableSet.of()));", "", " Object builder = null;", - " // Not flagged because identifier is variable name", + " // 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", + " Object utc = UTC;", + "", " // 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 x1 = MIN_VALUE;", + "", + " // Not flagged because identifier is not statically imported", + " Integer x2 = new B().MIN;", + "", + " // Not flagged because identifier is not statically imported", + " Object inst = new INSTANCE();", " }", "", " void create() {}", From 22578c2a8349dd7dc72f2108e06467a80ed46eea Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 19 Jan 2023 15:36:05 +0100 Subject: [PATCH 08/20] Suggestions --- ...StaticImport.java => NonStaticImport.java} | 41 ++++----- .../errorprone/bugpatterns/StaticImport.java | 14 +-- ...portTest.java => NonStaticImportTest.java} | 91 ++++++++++--------- 3 files changed, 76 insertions(+), 70 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/{BadStaticImport.java => NonStaticImport.java} (88%) rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{BadStaticImportTest.java => NonStaticImportTest.java} (71%) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java similarity index 88% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java index e1facf994a..faaeea55d5 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java @@ -2,8 +2,7 @@ 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.util.ASTHelpers.getSymbol; +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; @@ -14,6 +13,7 @@ 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.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; @@ -23,6 +23,7 @@ import com.sun.source.tree.ImportTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.ClassSymbol; import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** A {@link BugChecker} that flags methods and constants that should not be statically imported. */ @@ -34,11 +35,11 @@ @AutoService(BugChecker.class) @BugPattern( summary = "Identifier should not be statically imported", - link = BUG_PATTERNS_BASE_URL + "BadStaticImport", + link = BUG_PATTERNS_BASE_URL + "NonStaticImport", linkType = CUSTOM, severity = SUGGESTION, - tags = SIMPLIFICATION) -public final class BadStaticImport extends BugChecker implements BugChecker.IdentifierTreeMatcher { + tags = STYLE) +public final class NonStaticImport extends BugChecker implements IdentifierTreeMatcher { private static final long serialVersionUID = 1L; /** @@ -46,7 +47,7 @@ public final class BadStaticImport extends BugChecker implements BugChecker.Iden * StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS}. * *

Types listed here should be mutually exclusive with {@link - * StaticImport#STATIC_IMPORT_CANDIDATE_TYPES} + * StaticImport#STATIC_IMPORT_CANDIDATE_TYPES}. */ static final ImmutableSet BAD_STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of("com.google.common.base.Strings", "java.time.Clock", "java.time.ZoneOffset"); @@ -57,7 +58,7 @@ public final class BadStaticImport extends BugChecker implements BugChecker.Iden *

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

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

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 @@ -96,31 +97,29 @@ public final class BadStaticImport extends BugChecker implements BugChecker.Iden static final ImmutableSet BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS = ImmutableSet.of( "builder", - "create", "copyOf", + "create", "from", "getDefaultInstance", "INSTANCE", + "MAX", + "MAX_VALUE", "MIN", "MIN_VALUE", - "MAX", "newBuilder", + "newInstance", "of", "valueOf"); - /** Instantiates a new {@link BadStaticImport} instance. */ - public BadStaticImport() {} + /** Instantiates a new {@link NonStaticImport} instance. */ + public NonStaticImport() {} @Override public Description matchIdentifier(IdentifierTree tree, VisitorState state) { - if (isMatch(tree, state)) { - return getDescription(tree, state); + if (!isMatch(tree, state)) { + return Description.NO_MATCH; } - return Description.NO_MATCH; - } - - private Description getDescription(IdentifierTree tree, VisitorState state) { Symbol symbol = ASTHelpers.getSymbol(tree); SuggestedFix.Builder builder = SuggestedFix.builder().removeStaticImport(getImportToRemove(symbol)); @@ -147,10 +146,10 @@ private static boolean isMatch(IdentifierTree tree, VisitorState state) { return false; } - String identifierName = symbol.getSimpleName().toString(); if (isDefinedInThisFile(symbol, state.getPath().getCompilationUnit())) { return false; } + String identifierName = symbol.getSimpleName().toString(); if (!isIdentifierStaticallyImported(identifierName, state)) { return false; } @@ -164,9 +163,9 @@ private static boolean isDefinedInThisFile(Symbol symbol, CompilationUnitTree tr return tree.getTypeDecls().stream() .anyMatch( t -> { - Symbol topLevelClass = getSymbol(t); - return topLevelClass instanceof Symbol.ClassSymbol - && symbol.isEnclosedBy((Symbol.ClassSymbol) topLevelClass); + Symbol topLevelClass = ASTHelpers.getSymbol(t); + return topLevelClass instanceof ClassSymbol + && symbol.isEnclosedBy((ClassSymbol) topLevelClass); }); } 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 c1df8b9a8f..1713eef60a 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.BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS; -import static tech.picnic.errorprone.bugpatterns.BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS; +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.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 - * BadStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS} or {@link - * BadStaticImport#BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS}. + * NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS} or {@link + * NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS}. * *

Types listed here should be mutually exclusive with {@link - * BadStaticImport#BAD_STATIC_IMPORT_CANDIDATE_TYPES} + * NonStaticImport#BAD_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 - * BadStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS} + * NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS}. * - *

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

Identifiers listed by {@link NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should * be mutually exclusive with identifiers listed here. */ static final ImmutableSetMultimap STATIC_IMPORT_CANDIDATE_MEMBERS = diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NonStaticImportTest.java similarity index 71% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NonStaticImportTest.java index b37ed611ef..709db94f21 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NonStaticImportTest.java @@ -3,56 +3,54 @@ import static org.assertj.core.api.Assertions.assertThat; import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -final class BadStaticImportTest { - private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(BadStaticImport.class, getClass()); - private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance(BadStaticImport.class, getClass()); - +final class NonStaticImportTest { @Test void candidateMembersAreNotRedundant() { - assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) - .doesNotContainAnyElementsOf(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES); + assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) + .doesNotContainAnyElementsOf(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES); - assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.values()) - .doesNotContainAnyElementsOf(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS); + assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.values()) + .doesNotContainAnyElementsOf(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS); } @Test void badTypesDontClashWithStaticImportCandidates() { - assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES) + assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES); } @Test void badMembersDontClashWithStaticImportCandidates() { - assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.entries()) + assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.entries()) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.entries()); } @Test void badIdentifiersDontClashWithStaticImportCandidates() { - assertThat(BadStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS) + assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS) .doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.values()); } @Test void identification() { - compilationTestHelper + CompilationTestHelper.newInstance(NonStaticImport.class, getClass()) .addSourceLines( "pkg/B.java", "package pkg;", "", - "public class B {", + "public final class B {", " public int MIN = 1;", "", " public static class INSTANCE {}", "}") .addSourceLines( - "A.java", + "pkg/A.java", + "package pkg;", + "", "import static com.google.common.collect.ImmutableList.copyOf;", "import static java.lang.Integer.MIN_VALUE;", "import static java.time.Clock.systemUTC;", @@ -67,8 +65,10 @@ void identification() { "import com.google.common.collect.ImmutableList;", "import com.google.common.collect.ImmutableSet;", "import java.time.Clock;", + "import java.time.Instant;", + "import java.time.ZoneOffset;", "import java.util.Locale;", - "import pkg.B;", + "import java.util.Optional;", "import pkg.B.INSTANCE;", "", "class A {", @@ -80,20 +80,23 @@ void identification() { " Clock.systemUTC();", "", " // BUG: Diagnostic contains:", - " Object o1 = empty();", + " Optional optional1 = empty();", + " Optional optional2 = Optional.empty();", "", " // BUG: Diagnostic contains:", - " Object l1 = copyOf(ImmutableList.of());", - " Object l2 = ImmutableList.copyOf(ImmutableList.of());", + " ImmutableList list1 = copyOf(ImmutableList.of());", + " ImmutableList list2 = ImmutableList.copyOf(ImmutableList.of());", "", " // BUG: Diagnostic contains:", - " Locale lo1 = ROOT;", - " Locale lo2 = Locale.ROOT;", + " Locale locale1 = ROOT;", + " Locale locale2 = Locale.ROOT;", "", " // BUG: Diagnostic contains:", - " Object c1 = MIN;", + " Instant instant1 = MIN;", + " Instant instant2 = Instant.MIN;", " // BUG: Diagnostic contains:", - " Object c2 = MAX;", + " Instant instant3 = MAX;", + " Instant instant4 = Instant.MAX;", "", " // BUG: Diagnostic contains:", " ImmutableSet.of(min(ImmutableSet.of()));", @@ -102,23 +105,23 @@ void identification() { " // Not flagged because identifier is variable", " Object lBuilder = ImmutableList.of(builder);", "", - " // Not flagged because member of type is not a candidate", + " // Not flagged because member of type is not a candidate.", " Locale lo3 = ENGLISH;", "", - " // Not flagged because member of type is exempted", - " Object utc = UTC;", + " // Not flagged because member of type is exempted.", + " ZoneOffset utc = UTC;", "", - " // Not flagged because method is not statically imported", + " // 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 x1 = MIN_VALUE;", + " // 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 x2 = new B().MIN;", + " // Not flagged because identifier is not statically imported.", + " Integer i2 = new B().MIN;", "", - " // Not flagged because identifier is not statically imported", + " // Not flagged because identifier is not statically imported.", " Object inst = new INSTANCE();", " }", "", @@ -129,7 +132,7 @@ void identification() { @Test void replacement() { - refactoringTestHelper + BugCheckerRefactoringTestHelper.newInstance(NonStaticImport.class, getClass()) .addInputLines( "A.java", "import static com.google.common.collect.ImmutableList.copyOf;", @@ -143,14 +146,17 @@ void replacement() { "import com.google.common.collect.ImmutableList;", "import com.google.common.collect.ImmutableSet;", "import java.time.Clock;", + "import java.time.Instant;", "import java.util.Locale;", + "import java.util.Optional;", "", "class A {", " void m() {", " systemUTC();", " Clock.systemUTC();", "", - " Object o1 = empty();", + " Optional o1 = empty();", + " Optional o2 = Optional.empty();", "", " Object l1 = copyOf(ImmutableList.of());", " Object l2 = ImmutableList.copyOf(ImmutableList.of());", @@ -158,8 +164,8 @@ void replacement() { " Locale lo1 = ROOT;", " Locale lo2 = Locale.ROOT;", "", - " Object c1 = MIN;", - " Object c2 = MAX;", + " Instant i1 = MIN;", + " Instant i2 = MAX;", "", " ImmutableSet.of(min(ImmutableSet.of()));", " }", @@ -179,7 +185,8 @@ void replacement() { " Clock.systemUTC();", " Clock.systemUTC();", "", - " Object o1 = Optional.empty();", + " Optional o1 = Optional.empty();", + " Optional o2 = Optional.empty();", "", " Object l1 = ImmutableList.copyOf(ImmutableList.of());", " Object l2 = ImmutableList.copyOf(ImmutableList.of());", @@ -187,12 +194,12 @@ void replacement() { " Locale lo1 = Locale.ROOT;", " Locale lo2 = Locale.ROOT;", "", - " Object c1 = Instant.MIN;", - " Object c2 = Instant.MAX;", + " Instant i1 = Instant.MIN;", + " Instant i2 = Instant.MAX;", "", " ImmutableSet.of(Collections.min(ImmutableSet.of()));", " }", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } } From 3f8825b839df7272cf2c1bbfb129b719909c8a30 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 19 Jan 2023 15:49:42 +0100 Subject: [PATCH 09/20] Suggestions pt. 2 --- .../bugpatterns/NonStaticImport.java | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) 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 faaeea55d5..4d3c7d69ec 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 @@ -4,6 +4,7 @@ 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 java.util.Objects.requireNonNull; import static tech.picnic.errorprone.bugpatterns.StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; @@ -116,31 +117,31 @@ public NonStaticImport() {} @Override public Description matchIdentifier(IdentifierTree tree, VisitorState state) { - if (!isMatch(tree, state)) { + Symbol symbol = ASTHelpers.getSymbol(tree); + if (!isMatch(symbol, state)) { return Description.NO_MATCH; } - Symbol symbol = ASTHelpers.getSymbol(tree); - SuggestedFix.Builder builder = - SuggestedFix.builder().removeStaticImport(getImportToRemove(symbol)); + SuggestedFix.Builder builder = SuggestedFix.builder(); String replacement = SuggestedFixes.qualifyType(state, builder, symbol.getEnclosingElement()) + "."; - builder.prefixWith(tree, replacement); - return describeMatch(tree, builder.build()); + + return describeMatch( + tree, + builder + .removeStaticImport(getImportToRemove(symbol)) + .prefixWith(tree, replacement) + .build()); } - @SuppressWarnings("NullAway") private static String getImportToRemove(Symbol symbol) { return String.join( - ".", symbol.getEnclosingElement().getQualifiedName(), symbol.getSimpleName()); + ".", + requireNonNull(symbol.getEnclosingElement()).getQualifiedName(), + symbol.getSimpleName()); } - private static boolean isMatch(IdentifierTree tree, VisitorState state) { - Symbol symbol = ASTHelpers.getSymbol(tree); - if (symbol == null) { - return false; - } - + private static boolean isMatch(Symbol symbol, VisitorState state) { Symbol enclosingSymbol = symbol.getEnclosingElement(); if (enclosingSymbol == null || enclosingSymbol.kind != TYP) { return false; From 34e9f07287912c8961e78b3f1f6e31a0f7ff4769 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 19 Jan 2023 15:56:30 +0100 Subject: [PATCH 10/20] Use `symbol.owner` --- .../errorprone/bugpatterns/NonStaticImport.java | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) 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 4d3c7d69ec..52c30cb66e 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 @@ -4,7 +4,6 @@ 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 java.util.Objects.requireNonNull; import static tech.picnic.errorprone.bugpatterns.StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; @@ -123,8 +122,7 @@ public Description matchIdentifier(IdentifierTree tree, VisitorState state) { } SuggestedFix.Builder builder = SuggestedFix.builder(); - String replacement = - SuggestedFixes.qualifyType(state, builder, symbol.getEnclosingElement()) + "."; + String replacement = SuggestedFixes.qualifyType(state, builder, symbol.owner) + "."; return describeMatch( tree, @@ -135,15 +133,12 @@ public Description matchIdentifier(IdentifierTree tree, VisitorState state) { } private static String getImportToRemove(Symbol symbol) { - return String.join( - ".", - requireNonNull(symbol.getEnclosingElement()).getQualifiedName(), - symbol.getSimpleName()); + return String.join(".", symbol.owner.getQualifiedName(), symbol.getSimpleName()); } private static boolean isMatch(Symbol symbol, VisitorState state) { - Symbol enclosingSymbol = symbol.getEnclosingElement(); - if (enclosingSymbol == null || enclosingSymbol.kind != TYP) { + Symbol enclosingSymbol = symbol.owner; + if (enclosingSymbol.kind != TYP) { return false; } From 0d3a3293ffbcda9a66cccae0dd20b4b56b2e445e Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 20 Jan 2023 08:10:52 +0100 Subject: [PATCH 11/20] Re-introduce null-check --- .../tech/picnic/errorprone/bugpatterns/NonStaticImport.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 52c30cb66e..157841ee29 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 @@ -138,7 +138,7 @@ private static String getImportToRemove(Symbol symbol) { private static boolean isMatch(Symbol symbol, VisitorState state) { Symbol enclosingSymbol = symbol.owner; - if (enclosingSymbol.kind != TYP) { + if (enclosingSymbol == null || enclosingSymbol.kind != TYP) { return false; } From 28e7bec7cb0e571282263d14eabeb8f7ae267d7d Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 20 Jan 2023 08:46:41 +0100 Subject: [PATCH 12/20] Make sure JDK 11 doesnt crash --- .../tech/picnic/errorprone/bugpatterns/NonStaticImport.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 157841ee29..d625f62187 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 @@ -117,7 +117,7 @@ public NonStaticImport() {} @Override public Description matchIdentifier(IdentifierTree tree, VisitorState state) { Symbol symbol = ASTHelpers.getSymbol(tree); - if (!isMatch(symbol, state)) { + if (symbol == null || !isMatch(symbol, state)) { return Description.NO_MATCH; } From 98c3bf5a1f8005b6c56d8c06352b3268330deef3 Mon Sep 17 00:00:00 2001 From: ccernat Date: Tue, 14 Feb 2023 15:53:49 +0100 Subject: [PATCH 13/20] Add more tests --- .../bugpatterns/NonStaticImportTest.java | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) 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 709db94f21..1e4ad62520 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 @@ -47,11 +47,13 @@ void identification() { "", " public static class INSTANCE {}", "}") + .addSourceLines("MAX_VALUE.java", "", "public final class MAX_VALUE {}") .addSourceLines( "pkg/A.java", "package pkg;", "", "import static com.google.common.collect.ImmutableList.copyOf;", + "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;", @@ -123,9 +125,20 @@ void identification() { "", " // Not flagged because identifier is not statically imported.", " Object inst = new INSTANCE();", + "", + " Integer from = 12;", + " // Not flagged because identifier is not statically imported.", + " Integer i3 = from;", + "", + " // Not flagged because identifier does not belong to a type.", + " MAX_VALUE maxValue = new MAX_VALUE();", " }", "", - " void create() {}", + " void create() {", + " Integer MIN_VALUE = 12;", + " // Not flagged because identifier is not statically imported.", + " Integer i1 = MIN_VALUE;", + " }", "}") .doTest(); } From cd134d61e1c11ad575c4b53cf83e8e43a84812ad Mon Sep 17 00:00:00 2001 From: ccernat Date: Tue, 14 Feb 2023 20:24:26 +0100 Subject: [PATCH 14/20] Add more identifiers --- .../picnic/errorprone/bugpatterns/NonStaticImport.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 d625f62187..ab4c6b80e5 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 @@ -29,9 +29,7 @@ /** 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. -// - `java.time.Clock`. // - Several other `java.time` classes. -// - Likely any of `*.{ZERO, ONE, MIX, MAX, MIN_VALUE, MAX_VALUE}`. @AutoService(BugChecker.class) @BugPattern( summary = "Identifier should not be statically imported", @@ -109,7 +107,10 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM "newBuilder", "newInstance", "of", - "valueOf"); + "ONE", + "parse", + "valueOf", + "ZERO"); /** Instantiates a new {@link NonStaticImport} instance. */ public NonStaticImport() {} From 81aae7443a0955de02181f0bd6f17f49084609b6 Mon Sep 17 00:00:00 2001 From: ccernat Date: Tue, 14 Feb 2023 20:26:46 +0100 Subject: [PATCH 15/20] Rename variable --- .../tech/picnic/errorprone/bugpatterns/NonStaticImport.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 ab4c6b80e5..72042e9dd5 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 @@ -138,8 +138,8 @@ private static String getImportToRemove(Symbol symbol) { } private static boolean isMatch(Symbol symbol, VisitorState state) { - Symbol enclosingSymbol = symbol.owner; - if (enclosingSymbol == null || enclosingSymbol.kind != TYP) { + Symbol owner = symbol.owner; + if (owner == null || owner.kind != TYP) { return false; } @@ -151,7 +151,7 @@ private static boolean isMatch(Symbol symbol, VisitorState state) { return false; } - String qualifiedTypeName = enclosingSymbol.getQualifiedName().toString(); + String qualifiedTypeName = owner.getQualifiedName().toString(); return !isExempted(qualifiedTypeName, identifierName) && isCandidate(qualifiedTypeName, identifierName); } From f03925c78e0ea6193e45a0ee5393ac9374f21441 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 29 Jun 2023 13:36:14 +0200 Subject: [PATCH 16/20] Improve testing setup and make tweaks --- .../bugpatterns/NonStaticImport.java | 63 +++++++++---------- .../errorprone/bugpatterns/StaticImport.java | 18 +++--- .../bugpatterns/NonStaticImportTest.java | 63 ++++++++----------- 3 files changed, 64 insertions(+), 80 deletions(-) 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 72042e9dd5..0d32a167fb 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 1713eef60a..9cee4c243a 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 1e4ad62520..ef4d46ae5f 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;", From bad3b60dddd3811815843c21fa4647878659d0bc Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 14 Oct 2023 16:22:31 +0200 Subject: [PATCH 17/20] 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 7c12e0c3e9..f268bbeab3 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 0d32a167fb..4933543240 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}. + *

    + *
  • Types listed by {@link #NON_STATIC_IMPORT_CANDIDATE_TYPES} and members listed by {@link + * #NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should be omitted from this collection. + *
  • This collection 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}. + *

    + *
  • Identifiers listed by {@link StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS} should be + * mutually exclusive with identifiers listed here. + *
  • This list should contain 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 9cee4c243a..020e9fd922 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 ef4d46ae5f..982a87f942 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 6aca45a901..bcfb097d39 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()) From e103c3c96250014ef68218fdf5c1afac11cd5a7b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 15 Oct 2023 18:28:00 +0200 Subject: [PATCH 18/20] Simplify tests --- .../bugpatterns/NonStaticImport.java | 1 + .../errorprone/bugpatterns/StaticImport.java | 1 + .../bugpatterns/NonStaticImportTest.java | 79 ++++++++----------- 3 files changed, 37 insertions(+), 44 deletions(-) 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 4933543240..ff93c479ff 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 @@ -34,6 +34,7 @@ * A {@link BugChecker} that flags static imports of type members that should *not* be statically * imported. */ +// XXX: This check is closely linked to `StaticImport`. Consider merging the two. // 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 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 020e9fd922..3adb431a9c 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 @@ -30,6 +30,7 @@ import java.util.Optional; /** A {@link BugChecker} that flags type members that can and should be statically imported. */ +// XXX: This check is closely linked to `NonStaticImport`. Consider merging the two. // XXX: Tricky cases: // - `org.springframework.http.HttpStatus` (not always an improvement, and `valueOf` must // certainly be excluded) 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 982a87f942..86ef74d4e6 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 @@ -38,21 +38,13 @@ void candidateIdentifiersDoNotClash() { @Test void identification() { CompilationTestHelper.newInstance(NonStaticImport.class, getClass()) - .addSourceLines( - "pkg/B.java", - "package pkg;", - "", - "public final class B {", - " public int MIN = 1;", - "", - " public static class INSTANCE {}", - "}") - .addSourceLines("MAX_VALUE.java", "", "public final class MAX_VALUE {}") .addSourceLines( "pkg/A.java", "package pkg;", "", "// BUG: Diagnostic contains:", + "import static com.google.common.base.Strings.nullToEmpty;", + "// BUG: Diagnostic contains:", "import static com.google.common.collect.ImmutableList.copyOf;", "// BUG: Diagnostic contains:", "import static java.lang.Integer.MAX_VALUE;", @@ -62,6 +54,8 @@ void identification() { "import static java.time.Clock.systemUTC;", "// BUG: Diagnostic contains:", "import static java.time.Instant.MIN;", + "// BUG: Diagnostic contains:", + "import static java.time.ZoneOffset.SHORT_IDS;", "import static java.time.ZoneOffset.UTC;", "// BUG: Diagnostic contains:", "import static java.util.Collections.min;", @@ -70,55 +64,50 @@ void identification() { "import static java.util.Locale.ROOT;", "// BUG: Diagnostic contains:", "import static java.util.Optional.empty;", + "import static pkg.A.WithMethodThatIsSelectivelyFlagged.list;", "", "import com.google.common.collect.ImmutableList;", "import com.google.common.collect.ImmutableSet;", - "import java.time.Clock;", "import java.time.Instant;", "import java.time.ZoneOffset;", "import java.util.Locale;", - "import java.util.Optional;", - "import pkg.B.INSTANCE;", + "import java.util.Map;", + "import pkg.A.Wrapper.ZERO;", "", "class A {", " private Integer MIN_VALUE = 12;", "", " void m() {", + " nullToEmpty(null);", + " copyOf(ImmutableList.of());", + " int max = MAX_VALUE;", + " int min = MIN_VALUE;", " systemUTC();", - " Clock.systemUTC();", - " ZoneOffset utcIsExempted = UTC;", - "", - " Optional optional1 = empty();", - " Optional optional2 = Optional.empty();", - "", - " ImmutableList list1 = copyOf(ImmutableList.of());", - " ImmutableList list2 = ImmutableList.copyOf(ImmutableList.of());", - "", - " Locale locale1 = ROOT;", - " Locale locale2 = Locale.ROOT;", - " Locale isNotACandidate = ENGLISH;", - "", - " Instant instant1 = MIN;", - " Instant instant2 = Instant.MIN;", - "", - " ImmutableSet.of(min(ImmutableSet.of()));", - "", - " Object builder = null;", - " ImmutableList list = ImmutableList.of(builder);", + " Instant minInstant = MIN;", + " Map shortIds = SHORT_IDS;", + " ZoneOffset utc = UTC;", + " min(ImmutableSet.of());", + " Locale english = ENGLISH;", + " Locale root = ROOT;", + " empty();", + "", + " list();", + " new ZERO();", + " }", "", - " Integer refersToMemberVariable = MIN_VALUE;", - " Integer minIsNotStatic = new B().MIN;", - " Object regularImport = new INSTANCE();", - " MAX_VALUE maxValue = new MAX_VALUE();", + " static final class WithMethodThatIsSelectivelyFlagged {", + " static ImmutableList list() {", + " return ImmutableList.of();", + " }", + " }", "", - " Integer from = 12;", - " Integer i1 = from;", + " static final class Wrapper {", + " static final class ZERO {}", " }", "}") .doTest(); } - // XXX: Test multiple replacements of the same import. @Test void replacement() { BugCheckerRefactoringTestHelper.newInstance(NonStaticImport.class, getClass()) @@ -161,8 +150,9 @@ void replacement() { " }", "", " private static final class WithCustomConstant {", - " private static final int MIN = 42;", - " private static final int MAX = MIN;", + " private static final Instant MIN = Instant.EPOCH;", + " private static final Instant OTHER = MIN;", + " private static final Instant OTHER_MAX = MAX;", " }", "}") .addOutputLines( @@ -196,8 +186,9 @@ void replacement() { " }", "", " private static final class WithCustomConstant {", - " private static final int MIN = 42;", - " private static final int MAX = MIN;", + " private static final Instant MIN = Instant.EPOCH;", + " private static final Instant OTHER = MIN;", + " private static final Instant OTHER_MAX = Instant.MAX;", " }", "}") .doTest(TestMode.TEXT_MATCH); From a70444bccca52d659f9a67b7e18213bc1de6a0b6 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 25 Oct 2023 18:08:00 +0200 Subject: [PATCH 19/20] Suggestions --- .../bugpatterns/NonStaticImport.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) 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 ff93c479ff..2a3d593227 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 @@ -78,7 +78,6 @@ public final class NonStaticImport extends BugChecker implements CompilationUnit static final ImmutableSetMultimap NON_STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() .put("com.google.common.base.Predicates", "contains") - .put("com.mongodb.client.model.Filters", "empty") .putAll( "java.util.Collections", "addAll", @@ -92,7 +91,6 @@ public final class NonStaticImport extends BugChecker implements CompilationUnit "sort", "swap") .put("java.util.Locale", "ROOT") - .put("java.util.Optional", "empty") .putAll("java.util.regex.Pattern", "compile", "matches", "quote") .put("org.springframework.http.MediaType", "ALL") .build(); @@ -114,6 +112,7 @@ public final class NonStaticImport extends BugChecker implements CompilationUnit "builder", "copyOf", "create", + "empty", "from", "getDefaultInstance", "INSTANCE", @@ -138,7 +137,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s getUndesiredStaticImports(tree, state); if (!undesiredStaticImports.isEmpty()) { - replaceUndesiredStaticImportUsages(tree, state, undesiredStaticImports); + replaceUndesiredStaticImportUsages(tree, undesiredStaticImports, state); for (UndesiredStaticImport staticImport : undesiredStaticImports.values()) { state.reportMatch( @@ -159,13 +158,14 @@ private static ImmutableTable getUndesire 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)) { + String memberIdentifier = memberSelectTree.getIdentifier().toString(); + if (shouldNotBeStaticallyImported(type, memberIdentifier)) { imports.put( type, - member, + memberIdentifier, new AutoValue_NonStaticImport_UndesiredStaticImport( - importTree, SuggestedFix.builder().removeStaticImport(type + '.' + member))); + importTree, + SuggestedFix.builder().removeStaticImport(type + '.' + memberIdentifier))); } } } @@ -182,8 +182,8 @@ private static boolean shouldNotBeStaticallyImported(String type, String member) private static void replaceUndesiredStaticImportUsages( CompilationUnitTree tree, - VisitorState state, - ImmutableTable undesiredStaticImports) { + ImmutableTable undesiredStaticImports, + VisitorState state) { new TreeScanner<@Nullable Void, @Nullable Void>() { @Override public @Nullable Void visitIdentifier(IdentifierTree node, @Nullable Void unused) { From 6fea4d84b16b3955de834fc2cfcdafe86ade56f0 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 26 Oct 2023 10:47:18 +0200 Subject: [PATCH 20/20] Revert `s/memberIdentifier/member/` --- .../picnic/errorprone/bugpatterns/NonStaticImport.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) 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 2a3d593227..4c84992619 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 @@ -158,14 +158,13 @@ private static ImmutableTable getUndesire if (importTree.isStatic() && qualifiedIdentifier instanceof MemberSelectTree) { MemberSelectTree memberSelectTree = (MemberSelectTree) qualifiedIdentifier; String type = SourceCode.treeToString(memberSelectTree.getExpression(), state); - String memberIdentifier = memberSelectTree.getIdentifier().toString(); - if (shouldNotBeStaticallyImported(type, memberIdentifier)) { + String member = memberSelectTree.getIdentifier().toString(); + if (shouldNotBeStaticallyImported(type, member)) { imports.put( type, - memberIdentifier, + member, new AutoValue_NonStaticImport_UndesiredStaticImport( - importTree, - SuggestedFix.builder().removeStaticImport(type + '.' + memberIdentifier))); + importTree, SuggestedFix.builder().removeStaticImport(type + '.' + member))); } } }