From c87972f23662e4939b116dd54a99546386ae46a0 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 7 Dec 2021 10:46:27 +0100 Subject: [PATCH 1/4] PSM-1165 `StaticImportCheck` support exempting methods --- .../bugpatterns/StaticImportCheck.java | 52 +++++++++++++++++-- .../bugpatterns/StaticImportCheckTest.java | 46 ++++++++++++++++ 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java index 5a5eb98208..9c9abe5a0e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java @@ -1,5 +1,7 @@ package tech.picnic.errorprone.bugpatterns; +import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT; +import static com.sun.source.tree.Tree.Kind.METHOD_INVOCATION; import static java.util.Objects.requireNonNull; import com.google.auto.service.AutoService; @@ -19,14 +21,18 @@ 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.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol; import java.util.Optional; -/** A {@link BugChecker} which flags methods that can and should be statically imported. */ +/** + * A {@link BugChecker} which flags methods and constants that can and should be statically + * imported. + */ // XXX: Tricky cases: -// - `org.springframework.http.MediaType` (do except for `ALL`?) // - `org.springframework.http.HttpStatus` (not always an improvement, and `valueOf` must // certainly be excluded) // - `com.google.common.collect.Tables` @@ -42,7 +48,7 @@ @AutoService(BugChecker.class) @BugPattern( name = "StaticImport", - summary = "Method should be statically imported", + summary = "Method or constant should be statically imported", linkType = LinkType.NONE, severity = SeverityLevel.SUGGESTION, tags = StandardTags.SIMPLIFICATION) @@ -84,6 +90,7 @@ public final class StaticImportCheck extends BugChecker implements MemberSelectT "org.springframework.format.annotation.DateTimeFormat.ISO", "org.springframework.http.HttpHeaders", "org.springframework.http.HttpMethod", + "org.springframework.http.MediaType", "org.testng.Assert", "reactor.function.TupleUtils"); @@ -123,9 +130,28 @@ public final class StaticImportCheck extends BugChecker implements MemberSelectT .putAll("com.google.common.collect.Comparators", "emptiesFirst", "emptiesLast") .build(); + @VisibleForTesting + static final ImmutableSetMultimap STATIC_IMPORT_EXEMPTION_METHODS = + ImmutableSetMultimap.builder() + .put("com.mongodb.client.model.Filters", "empty") + .put("org.springframework.http.MediaType", "ALL") + .build(); + + private static final ImmutableSet BAD_STATIC_IDENTIFIERS = + ImmutableSet.of( + "builder", + "create", + "copyOf", + "from", + "getDefaultInstance", + "INSTANCE", + "newBuilder", + "of", + "valueOf"); + @Override public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) { - if (!isCandidate(state)) { + if (!isCandidate(state) || isExempted(state)) { return Description.NO_MATCH; } @@ -155,6 +181,24 @@ private static boolean isCandidate(VisitorState state) { } } + private static boolean isExempted(VisitorState state) { + Tree currentTree = state.getPath().getLeaf(); + Tree parentTree = state.getPath().getParentPath().getLeaf(); + if (currentTree.getKind() != MEMBER_SELECT && parentTree.getKind() != METHOD_INVOCATION) { + return false; + } + + Symbol symbol = + currentTree.getKind() == MEMBER_SELECT + ? ASTHelpers.getSymbol(currentTree) + : ASTHelpers.getSymbol(parentTree); + + String methodName = symbol.name.toString(); + return STATIC_IMPORT_EXEMPTION_METHODS.get(symbol.owner.toString()).stream() + .anyMatch(methodName::equals) + || BAD_STATIC_IDENTIFIERS.contains(methodName); + } + private static Optional getCandidateSimpleName(StaticImportInfo importInfo) { String canonicalName = importInfo.canonicalName(); return importInfo diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java index c45c08f75b..515d15ad19 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java @@ -18,6 +18,15 @@ void candidateMethodsAreNotRedundant() { .doesNotContainAnyElementsOf(StaticImportCheck.STATIC_IMPORT_CANDIDATE_CLASSES); } + @Test + void exemptionsAreListedAsCandidates() { + assertThat(StaticImportCheck.STATIC_IMPORT_EXEMPTION_METHODS.keySet()) + .allMatch( + e -> + StaticImportCheck.STATIC_IMPORT_CANDIDATE_CLASSES.contains(e) + || StaticImportCheck.STATIC_IMPORT_CANDIDATE_METHODS.containsKey(e)); + } + @Test void identification() { compilationTestHelper @@ -38,6 +47,7 @@ void identification() { "import java.util.function.Predicate;", "import org.springframework.boot.test.context.SpringBootTest;", "import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;", + "import org.springframework.http.MediaType;", "", "class A {", " void m() {", @@ -75,6 +85,11 @@ void identification() { " Object e1 = WebEnvironment.RANDOM_PORT;", " Object e2 = RANDOM_PORT;", "", + " // Not flagged because `MediaType.ALL` is exempted.", + " MediaType type1 = MediaType.ALL;", + " // BUG: Diagnostic contains:", + " MediaType type2 = MediaType.APPLICATION_JSON;", + "", " Optional.empty();", " }", "", @@ -100,6 +115,7 @@ void replacement() { "import org.springframework.format.annotation.DateTimeFormat.ISO;", "import org.springframework.boot.test.context.SpringBootTest;", "import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;", + "import org.springframework.http.MediaType;", "", "class A {", " void m1() {", @@ -117,6 +133,11 @@ void replacement() { " Objects.requireNonNull(\"bar\");", "", " Object o = StandardCharsets.UTF_8;", + "", + " ImmutableSet.of(MediaType.TEXT_HTML,", + " MediaType.APPLICATION_XHTML_XML,", + " MediaType.valueOf(\"image/webp\"),", + " MediaType.ALL);", " }", "", " void m2(", @@ -144,6 +165,8 @@ void replacement() { "import static org.springframework.format.annotation.DateTimeFormat.ISO.DATE;", "import static org.springframework.format.annotation.DateTimeFormat.ISO.DATE_TIME;", "import static org.springframework.format.annotation.DateTimeFormat.ISO.TIME;", + "import static org.springframework.http.MediaType.APPLICATION_XHTML_XML;", + "import static org.springframework.http.MediaType.TEXT_HTML;", "", "import com.google.common.base.Predicates;", "import com.google.common.collect.ImmutableMap;", @@ -155,6 +178,7 @@ void replacement() { "import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;", "import org.springframework.format.annotation.DateTimeFormat;", "import org.springframework.format.annotation.DateTimeFormat.ISO;", + "import org.springframework.http.MediaType;", "", "class A {", " void m1() {", @@ -172,6 +196,11 @@ void replacement() { " requireNonNull(\"bar\");", "", " Object o = UTF_8;", + "", + " ImmutableSet.of(TEXT_HTML,", + " APPLICATION_XHTML_XML,", + " MediaType.valueOf(\"image/webp\"),", + " MediaType.ALL);", " }", "", " void m2(", @@ -189,4 +218,21 @@ void replacement() { "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } + + @Test + void exemptions() { + refactoringTestHelper + .addInputLines( + "in/A.java", + "import org.springframework.http.MediaType;", + "", + "class A { ", + " void exemptions() {", + " MediaType mediaType1 = MediaType.ALL;", + " MediaType mediaType2 = MediaType.valueOf(\"\");", + " }", + "}") + .expectUnchanged() + .doTest(); + } } From 1c3b13b3d64a70206da96bbbbac970fb55ed8c93 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 7 Dec 2021 12:31:30 +0100 Subject: [PATCH 2/4] PSM-1165 `StaticImportCheck` handle `package-info.java` files --- .../tech/picnic/errorprone/bugpatterns/StaticImportCheck.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java index 9c9abe5a0e..cc73da727b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java @@ -2,6 +2,7 @@ import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT; import static com.sun.source.tree.Tree.Kind.METHOD_INVOCATION; +import static com.sun.source.tree.Tree.Kind.PACKAGE; import static java.util.Objects.requireNonNull; import com.google.auto.service.AutoService; @@ -184,7 +185,8 @@ private static boolean isCandidate(VisitorState state) { private static boolean isExempted(VisitorState state) { Tree currentTree = state.getPath().getLeaf(); Tree parentTree = state.getPath().getParentPath().getLeaf(); - if (currentTree.getKind() != MEMBER_SELECT && parentTree.getKind() != METHOD_INVOCATION) { + if ((currentTree.getKind() != MEMBER_SELECT && parentTree.getKind() != METHOD_INVOCATION) + || parentTree.getKind() == PACKAGE) { return false; } From 626d740d7a3bef9c0a05b4c3ec2294d5e0695d69 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 2 Jan 2022 17:07:36 +0100 Subject: [PATCH 3/4] PSM-1165 Suggestions --- .../bugpatterns/StaticImportCheck.java | 61 +++++++++++-------- .../bugpatterns/StaticImportCheckTest.java | 46 ++++++++------ 2 files changed, 60 insertions(+), 47 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java index cc73da727b..96b6fcf60c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java @@ -1,8 +1,5 @@ package tech.picnic.errorprone.bugpatterns; -import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT; -import static com.sun.source.tree.Tree.Kind.METHOD_INVOCATION; -import static com.sun.source.tree.Tree.Kind.PACKAGE; import static java.util.Objects.requireNonNull; import com.google.auto.service.AutoService; @@ -26,7 +23,7 @@ import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; -import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; import java.util.Optional; /** @@ -49,15 +46,19 @@ @AutoService(BugChecker.class) @BugPattern( name = "StaticImport", - summary = "Method or constant should be statically imported", + summary = "Identifier should be statically imported", linkType = LinkType.NONE, severity = SeverityLevel.SUGGESTION, tags = StandardTags.SIMPLIFICATION) public final class StaticImportCheck extends BugChecker implements MemberSelectTreeMatcher { private static final long serialVersionUID = 1L; + /** + * Types whose members should be statically imported, unless exempted by {@link + * #STATIC_IMPORT_EXEMPTED_MEMBERS} or {@link #STATIC_IMPORT_EXEMPTED_IDENTIFIERS}. + */ @VisibleForTesting - static final ImmutableSet STATIC_IMPORT_CANDIDATE_CLASSES = + static final ImmutableSet STATIC_IMPORT_CANDIDATE_TYPES = ImmutableSet.of( "com.google.common.base.Preconditions", "com.google.common.base.Predicates", @@ -95,8 +96,9 @@ public final class StaticImportCheck extends BugChecker implements MemberSelectT "org.testng.Assert", "reactor.function.TupleUtils"); + /** Type members that should be statically imported. */ @VisibleForTesting - static final ImmutableSetMultimap STATIC_IMPORT_CANDIDATE_METHODS = + static final ImmutableSetMultimap STATIC_IMPORT_CANDIDATE_MEMBERS = ImmutableSetMultimap.builder() .putAll( "com.google.common.collect.ImmutableListMultimap", @@ -131,14 +133,27 @@ public final class StaticImportCheck extends BugChecker implements MemberSelectT .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. + */ @VisibleForTesting - static final ImmutableSetMultimap STATIC_IMPORT_EXEMPTION_METHODS = + static final ImmutableSetMultimap STATIC_IMPORT_EXEMPTED_MEMBERS = ImmutableSetMultimap.builder() .put("com.mongodb.client.model.Filters", "empty") .put("org.springframework.http.MediaType", "ALL") .build(); - private static final ImmutableSet BAD_STATIC_IDENTIFIERS = + /** + * 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", @@ -152,7 +167,7 @@ public final class StaticImportCheck extends BugChecker implements MemberSelectT @Override public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) { - if (!isCandidate(state) || isExempted(state)) { + if (!isCandidateContext(state) || !isCandidate(tree)) { return Description.NO_MATCH; } @@ -167,7 +182,7 @@ public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) .orElse(Description.NO_MATCH); } - private static boolean isCandidate(VisitorState state) { + private static boolean isCandidateContext(VisitorState state) { Tree parentTree = requireNonNull(state.getPath().getParentPath(), "MemberSelectTree lacks enclosing node") .getLeaf(); @@ -182,23 +197,15 @@ private static boolean isCandidate(VisitorState state) { } } - private static boolean isExempted(VisitorState state) { - Tree currentTree = state.getPath().getLeaf(); - Tree parentTree = state.getPath().getParentPath().getLeaf(); - if ((currentTree.getKind() != MEMBER_SELECT && parentTree.getKind() != METHOD_INVOCATION) - || parentTree.getKind() == PACKAGE) { + private static boolean isCandidate(MemberSelectTree tree) { + String identifier = tree.getIdentifier().toString(); + if (STATIC_IMPORT_EXEMPTED_IDENTIFIERS.contains(identifier)) { return false; } - Symbol symbol = - currentTree.getKind() == MEMBER_SELECT - ? ASTHelpers.getSymbol(currentTree) - : ASTHelpers.getSymbol(parentTree); - - String methodName = symbol.name.toString(); - return STATIC_IMPORT_EXEMPTION_METHODS.get(symbol.owner.toString()).stream() - .anyMatch(methodName::equals) - || BAD_STATIC_IDENTIFIERS.contains(methodName); + Type type = ASTHelpers.getType(tree.getExpression()); + return type != null + && !STATIC_IMPORT_EXEMPTED_MEMBERS.containsEntry(type.toString(), identifier); } private static Optional getCandidateSimpleName(StaticImportInfo importInfo) { @@ -208,8 +215,8 @@ private static Optional getCandidateSimpleName(StaticImportInfo importIn .toJavaUtil() .filter( name -> - STATIC_IMPORT_CANDIDATE_CLASSES.contains(canonicalName) - || STATIC_IMPORT_CANDIDATE_METHODS.containsEntry(canonicalName, name)); + STATIC_IMPORT_CANDIDATE_TYPES.contains(canonicalName) + || STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(canonicalName, name)); } private static Optional tryStaticImport( diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java index 515d15ad19..fe172bf2f6 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java @@ -3,6 +3,7 @@ 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; @@ -14,17 +15,20 @@ public final class StaticImportCheckTest { @Test void candidateMethodsAreNotRedundant() { - assertThat(StaticImportCheck.STATIC_IMPORT_CANDIDATE_METHODS.keySet()) - .doesNotContainAnyElementsOf(StaticImportCheck.STATIC_IMPORT_CANDIDATE_CLASSES); + assertThat(StaticImportCheck.STATIC_IMPORT_CANDIDATE_MEMBERS.keySet()) + .doesNotContainAnyElementsOf(StaticImportCheck.STATIC_IMPORT_CANDIDATE_TYPES); } @Test - void exemptionsAreListedAsCandidates() { - assertThat(StaticImportCheck.STATIC_IMPORT_EXEMPTION_METHODS.keySet()) - .allMatch( - e -> - StaticImportCheck.STATIC_IMPORT_CANDIDATE_CLASSES.contains(e) - || StaticImportCheck.STATIC_IMPORT_CANDIDATE_METHODS.containsKey(e)); + void exemptedMembersAreNotVacuous() { + assertThat(StaticImportCheck.STATIC_IMPORT_EXEMPTED_MEMBERS.keySet()) + .isSubsetOf(StaticImportCheck.STATIC_IMPORT_CANDIDATE_TYPES); + } + + @Test + void exemptedMembersAreNotRedundant() { + assertThat(StaticImportCheck.STATIC_IMPORT_EXEMPTED_MEMBERS.values()) + .doesNotContainAnyElementsOf(StaticImportCheck.STATIC_IMPORT_EXEMPTED_IDENTIFIERS); } @Test @@ -86,9 +90,9 @@ void identification() { " Object e2 = RANDOM_PORT;", "", " // Not flagged because `MediaType.ALL` is exempted.", - " MediaType type1 = MediaType.ALL;", + " MediaType t1 = MediaType.ALL;", " // BUG: Diagnostic contains:", - " MediaType type2 = MediaType.APPLICATION_JSON;", + " MediaType t2 = MediaType.APPLICATION_JSON;", "", " Optional.empty();", " }", @@ -134,10 +138,11 @@ void replacement() { "", " Object o = StandardCharsets.UTF_8;", "", - " ImmutableSet.of(MediaType.TEXT_HTML,", - " MediaType.APPLICATION_XHTML_XML,", - " MediaType.valueOf(\"image/webp\"),", - " MediaType.ALL);", + " ImmutableSet.of(", + " MediaType.ALL,", + " MediaType.APPLICATION_XHTML_XML,", + " MediaType.TEXT_HTML,", + " MediaType.valueOf(\"image/webp\"));", " }", "", " void m2(", @@ -197,10 +202,11 @@ void replacement() { "", " Object o = UTF_8;", "", - " ImmutableSet.of(TEXT_HTML,", - " APPLICATION_XHTML_XML,", - " MediaType.valueOf(\"image/webp\"),", - " MediaType.ALL);", + " ImmutableSet.of(", + " MediaType.ALL,", + " APPLICATION_XHTML_XML,", + " TEXT_HTML,", + " MediaType.valueOf(\"image/webp\"));", " }", "", " void m2(", @@ -216,7 +222,7 @@ void replacement() { " @SpringBootTest(webEnvironment = RANDOM_PORT)", " final class Test {}", "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + .doTest(TestMode.TEXT_MATCH); } @Test @@ -233,6 +239,6 @@ void exemptions() { " }", "}") .expectUnchanged() - .doTest(); + .doTest(TestMode.TEXT_MATCH); } } From fdd738cb9d9b81597177b31b617e43881392cf1d Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 3 Jan 2022 05:55:14 +0100 Subject: [PATCH 4/4] PSM-1134 Drop excess test --- .../bugpatterns/StaticImportCheckTest.java | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java index fe172bf2f6..0a0485ade5 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java @@ -224,21 +224,4 @@ void replacement() { "}") .doTest(TestMode.TEXT_MATCH); } - - @Test - void exemptions() { - refactoringTestHelper - .addInputLines( - "in/A.java", - "import org.springframework.http.MediaType;", - "", - "class A { ", - " void exemptions() {", - " MediaType mediaType1 = MediaType.ALL;", - " MediaType mediaType2 = MediaType.valueOf(\"\");", - " }", - "}") - .expectUnchanged() - .doTest(TestMode.TEXT_MATCH); - } }