`StaticImportCheck` support exemption of methods #22

rickie · 2021-12-07T10:16:00Z

While there, update the Javadoc and @BugPattern content to better reflect how the check works.

rickie · 2021-12-07T10:17:03Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java

+  static final ImmutableSetMultimap<String, String> STATIC_IMPORT_EXEMPTION_METHODS =
+      ImmutableSetMultimap.<String, String>builder()
+          .put("com.mongodb.client.model.Filters", "empty")
+          .put("org.springframework.http.MediaType", "ALL")


We could add "valueOf" here as well, to make it clear. However, the case is already in BAD_STATIC_IDENTIFIERS so I left it out.

rickie · 2021-12-07T10:17:39Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java

@@ -189,4 +218,21 @@ void replacement() {
            "}")
        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
  }
+
+  @Test
+  void exemptions() {


The idea is that we can add more exemptions here in the future. If you want to leave it out, it is also fine by me.

Tough one. On the one hand this allows us to test all exceptions, but we're also not testing all inclusions. So would indeed be inclined to leave it out.

rickie · 2021-12-07T10:18:29Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java

@@ -172,6 +196,11 @@ void replacement() {
            "    requireNonNull(\"bar\");",
            "",
            "    Object o = UTF_8;",
+            "",
+            "    ImmutableSet.of(TEXT_HTML,",


Would it be better to do the minimal case here and remove (e.g.) the APPLICATION_XHTML_XML?

rickie · 2021-12-07T11:39:24Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java

+    Tree currentTree = state.getPath().getLeaf();
+    Tree parentTree = state.getPath().getParentPath().getLeaf();
+    if ((currentTree.getKind() != MEMBER_SELECT && parentTree.getKind() != METHOD_INVOCATION)
+        || parentTree.getKind() == PACKAGE) {


A bug was found by the selfcheck of EPS. The problem was that String methodName = symbol.name.toString(); would fail because symbol is null. This only occurred in a package-info.java file. So for that reason this parentTree.getKind() == PACKAGE is added.

Another way of implementing this could be: isPackageInfo(CompilationUnitTree) method.

Hmm, if I remove this check then all tests still pass, so at the very least we need to increase test coverage. But even then:

If I remove this if statement then the -Pself-check build still passes.

IIUC currentTree.getKind() == MEMBER_SELECT is always true 👀

It seems that some/all of this logic belongs in isCandidate, or that these methods should get different names.

rickie · 2021-12-07T11:48:34Z

As the plan is to open source, should we not use ticket numbers here? Or can we do that until we go public?

Stephan202

Rebased and added two commits. Suggested commit message:

Define assorted `StaticImportCheck` exemptions

While there, require that most `org.springframework.http.MediaType` members are
statically imported.

Stephan202 · 2022-01-01T13:16:36Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java

@@ -123,9 +131,28 @@
          .putAll("com.google.common.collect.Comparators", "emptiesFirst", "emptiesLast")
          .build();

+  @VisibleForTesting
+  static final ImmutableSetMultimap<String, String> STATIC_IMPORT_EXEMPTION_METHODS =


To match the grammatical form of existing constants:

Suggested change

static final ImmutableSetMultimap<String, String> STATIC_IMPORT_EXEMPTION_METHODS =

static final ImmutableSetMultimap<String, String> STATIC_IMPORT_EXEMPTED_METHODS =

That said, ALL isn't a method, so I suppose we should use IDENTIFIERS, just like Error Prone's BadImport implementation, or MEMBERS. I guess the latter makes more sense since here we define (type, member) pairs.

Stephan202 · 2022-01-03T03:47:43Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java

+    Tree currentTree = state.getPath().getLeaf();
+    Tree parentTree = state.getPath().getParentPath().getLeaf();
+    if ((currentTree.getKind() != MEMBER_SELECT && parentTree.getKind() != METHOD_INVOCATION)
+        || parentTree.getKind() == PACKAGE) {


Hmm, if I remove this check then all tests still pass, so at the very least we need to increase test coverage. But even then:

If I remove this if statement then the -Pself-check build still passes.

IIUC currentTree.getKind() == MEMBER_SELECT is always true 👀

It seems that some/all of this logic belongs in isCandidate, or that these methods should get different names.

Stephan202 · 2022-01-03T04:07:14Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java

+    Symbol symbol =
+        currentTree.getKind() == MEMBER_SELECT
+            ? ASTHelpers.getSymbol(currentTree)
+            : ASTHelpers.getSymbol(parentTree);


We should be getting the Symbol, but rather the Type.

Instead of the PACKAGE logic above we should likely simply verify that the returned symbol (type) is not null.

Hmm, I am double checking your improvements; as I was doing some things here that were overly difficult compared to what it should be / what it is now.

Stephan202 · 2022-01-03T04:07:28Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java

+    return STATIC_IMPORT_EXEMPTION_METHODS.get(symbol.owner.toString()).stream()
+            .anyMatch(methodName::equals)


There's Multimap#containsEntry :)

Stephan202 · 2022-01-03T04:16:14Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java

@@ -189,4 +218,21 @@ void replacement() {
            "}")
        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);


Suggested change

.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);

.doTest(TestMode.TEXT_MATCH);

Stephan202 · 2022-01-03T04:18:22Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java

+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();


Clearer error message:

Suggested change

.doTest();

.doTest(TestMode.TEXT_MATCH);

(Maybe we should enforce this mode using a separate check, though after fixing this one no "violations" remain.)

Stephan202 · 2022-01-03T04:26:09Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java

+            e ->
+                StaticImportCheck.STATIC_IMPORT_CANDIDATE_CLASSES.contains(e)
+                    || StaticImportCheck.STATIC_IMPORT_CANDIDATE_METHODS.containsKey(e));


This doesn't seem right. Rather the key set should be a subset of STATIC_IMPORT_CANDIDATE_CLASSES. By construction then there won't be overlap with STATIC_IMPORT_CANDIDATE_METHODS.

Additionally, redundancy w.r.t. BAD_STATIC_IDENTIFIERS should also be tested.

Stephan202 · 2022-01-03T04:34:47Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java

@@ -42,7 +49,7 @@
 @AutoService(BugChecker.class)
 @BugPattern(
    name = "StaticImport",
-    summary = "Method should be statically imported",
+    summary = "Method or constant should be statically imported",


Suggested change

summary = "Method or constant should be statically imported",

summary = "Identifier should be statically imported",

Stephan202 · 2022-01-03T04:39:06Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java

@@ -117,6 +133,11 @@ void replacement() {
            "    Objects.requireNonNull(\"bar\");",
            "",
            "    Object o = StandardCharsets.UTF_8;",
+            "",
+            "    ImmutableSet.of(MediaType.TEXT_HTML,",


Let's stick with GJF formatting.

Stephan202 · 2022-01-03T04:54:35Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java

@@ -189,4 +218,21 @@ void replacement() {
            "}")
        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
  }
+
+  @Test
+  void exemptions() {


Tough one. On the one hand this allows us to test all exceptions, but we're also not testing all inclusions. So would indeed be inclined to leave it out.

rickie

Only one question, but the code looks very nice! This allows us to fix many of the remaining XXX's in this class and further improve this BugPattern with improvements we already discussed.

It's interesting to see how you solve the questions I have in the code, very informative/instructive 😄 .

rickie · 2022-01-03T07:57:06Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java

 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


The summary is now updated to have Identifier shouldn't that also be updated here in the Javadoc? @Stephan202

I considered that, but here I was thinking this might be a bit clearer albeit less accurate. (The summary is shown the context of a specific "did you mean" suggestion, in which case the "X or Y" feels a bit awkward.)

Stephan202

Do we want a second review or shall we merge? :)

Stephan202 · 2022-01-03T09:00:15Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java

 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


I considered that, but here I was thinking this might be a bit clearer albeit less accurate. (The summary is shown the context of a specific "did you mean" suggestion, in which case the "X or Y" feels a bit awkward.)

rickie · 2022-01-03T09:05:51Z

I'd say, lets merge 🚀 !

rickie requested a review from Stephan202 December 7, 2021 10:16

rickie commented Dec 7, 2021

View reviewed changes

rickie mentioned this pull request Dec 14, 2021

StaticImportCheck: Add java.util.Collections candidates #23

Merged

rickie added 2 commits January 1, 2022 14:05

PSM-1165 StaticImportCheck support exempting methods

c87972f

PSM-1165 StaticImportCheck handle package-info.java files

1c3b13b

rickie force-pushed the rossendrijver/PSM-1134_static_import_improvements branch from 7af019c to 49d614f Compare January 2, 2022 13:44

rickie changed the title ~~PSM-1165 StaticImportCheck support exemption of methods~~ StaticImportCheck support exemption of methods Jan 2, 2022

Stephan202 added 2 commits January 3, 2022 05:54

PSM-1165 Suggestions

626d740

PSM-1134 Drop excess test

fdd738c

Stephan202 force-pushed the rossendrijver/PSM-1134_static_import_improvements branch from 49d614f to fdd738c Compare January 3, 2022 05:10

Stephan202 approved these changes Jan 3, 2022

View reviewed changes

rickie commented Jan 3, 2022

View reviewed changes

Stephan202 reviewed Jan 3, 2022

View reviewed changes

Stephan202 merged commit c163806 into master Jan 3, 2022

Stephan202 deleted the rossendrijver/PSM-1134_static_import_improvements branch January 3, 2022 09:20

Stephan202 added this to the 0.1.0 milestone Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`StaticImportCheck` support exemption of methods #22

`StaticImportCheck` support exemption of methods #22

rickie commented Dec 7, 2021 •

edited

Loading

rickie Dec 7, 2021

rickie Dec 7, 2021 •

edited

Loading

Stephan202 Jan 3, 2022

rickie Dec 7, 2021

rickie Dec 7, 2021

Stephan202 Jan 3, 2022

rickie commented Dec 7, 2021

Stephan202 left a comment

Stephan202 Jan 1, 2022

Stephan202 Jan 1, 2022

Stephan202 Jan 3, 2022

Stephan202 Jan 3, 2022

rickie Jan 3, 2022

Stephan202 Jan 3, 2022

Stephan202 Jan 3, 2022

Stephan202 Jan 3, 2022

Stephan202 Jan 3, 2022

Stephan202 Jan 3, 2022

Stephan202 Jan 3, 2022

Stephan202 Jan 3, 2022

rickie left a comment •

edited

Loading

rickie Jan 3, 2022

Stephan202 Jan 3, 2022

Stephan202 left a comment

Stephan202 Jan 3, 2022

rickie commented Jan 3, 2022

	static final ImmutableSetMultimap<String, String> STATIC_IMPORT_EXEMPTION_METHODS =
	static final ImmutableSetMultimap<String, String> STATIC_IMPORT_EXEMPTED_METHODS =

		return STATIC_IMPORT_EXEMPTION_METHODS.get(symbol.owner.toString()).stream()
		.anyMatch(methodName::equals)

		@@ -189,4 +218,21 @@ void replacement() {
		"}")
		.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);

	.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
	.doTest(TestMode.TEXT_MATCH);

	summary = "Method or constant should be statically imported",
	summary = "Identifier should be statically imported",

StaticImportCheck support exemption of methods #22

StaticImportCheck support exemption of methods #22

Conversation

rickie commented Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

rickie Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Dec 7, 2021

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Jan 3, 2022

`StaticImportCheck` support exemption of methods #22

`StaticImportCheck` support exemption of methods #22

rickie commented Dec 7, 2021 •

edited

Loading

rickie Dec 7, 2021 •

edited

Loading

rickie left a comment •

edited

Loading