`StaticImportCheck` use static imports for `ImportPolicy` usages #45

Badbond · 2022-03-15T15:49:34Z

Adds a candidate to StaticImportCheck to statically import ImportPolicy usages. Causes @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS) to be changed to @UseImportPolicy(STATIC_IMPORT_ALWAYS).

Self applied to this repository using:

git grep 'ImportPolicy.STATIC_IMPORT_ALWAYS' | cut -d ":" -f 1 | xargs -n1 | sort -u | xargs sed -i '2i import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;'
git grep 'ImportPolicy.STATIC_IMPORT_ALWAYS' | cut -d ":" -f 1 | xargs -n1 | sort -u | xargs sed -i 's/@UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)/@UseImportPolicy(STATIC_IMPORT_ALWAYS)/'
IntelliJ reformatting to rearrange the imports and remove unused ones.

Badbond · 2022-03-15T15:50:33Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJTemplates.java

@@ -2213,7 +2213,7 @@ void after(Predicate<T> predicate, T object) {
  //        }
  //
  //        @AfterTemplate
-  //        @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)
+  //        @UseImportPolicy(STATIC_IMPORT_ALWAYS)


Would be cool if EP could do this as well @rickie 😜

rickie

Good idea to add this one to StaticImport. Left some suggestions 😄.

rickie · 2022-03-17T08:49:34Z

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

@@ -97,6 +99,8 @@ void identification() {
            "    Optional.empty();",
            "  }",
            "",
+            "  // BUG: Diagnostic contains:",
+            "  @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)",


I feel that we are now mixing two test cases in one thing. The toImmutableMultiset() is required for code above:
" // Not flagged because we define #toImmutableMultiset below.",
Therefore I'd suggest to leave the method as is and not annotate it.

We could make a new method in this test and annotate that method with @UseImportPolicy.
I'll push a suggestion in a sec.

Normally UseImportPolicy should only be used in the context of Refaster. Here we can see that (when running Refaster) it is required to have @AfterTemplate present when we use UseImportPolicy. Now in this case it is, strictly speaking, not a problem (because RequiredAnnotation is only checked on Refaster templates at runtime), nevertheless it seems better to add the @AfterTemplate here as well.

You are right, I was making it myself a bit too easy here (also for below) 😅 Makes sense to make the test case best represent actual usage 👍 Thanks for committing the suggestion!

rickie · 2022-03-17T09:08:33Z

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

@@ -122,6 +128,7 @@ void replacement() {
            "import org.springframework.http.MediaType;",
            "",
            "class A {",
+            "  @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)",


A few questions / suggestions;

I'm not a fan of adding this method on the big method that contains many test cases. It feels like we are "mixing up" things.
Also, especially for people that do not know the STATIC_IMPORT_ALWAYS, it could be quite confusing to see this annotation here on a class where we are testing static imports.

Not 100% sure about whether this is an improvement, but we could decide to replace STATIC_IMPORT_ALWAYS by a different ImportPolicy (like IMPORT_TOP_LEVEL) to prevent possible confusion. WDYT?

As explained in the other comment, only using @UseImportPolicy is, strictly speaking, not valid. It would make sense to add @AfterTemplate.

Lastly, I have some doubts about adding tests for ImportPolicy in the replacement() test. Again, because of the fact that we only should use this in the case of Refaster templates. Therefore it feels a bit like this test case doesn't really belong here. Perhaps, for this one case, we could decide that testing this in identification() is "enough". Another argument in favor of this could be that not all cases from identification are also in replacement and the other way around.

Lastly, I have some doubts about adding tests for ImportPolicy in the replacement() test. Again, because of the fact that we only should use this in the case of Refaster templates. Therefore it feels a bit like this test case doesn't really belong here. Perhaps, for this one case, we could decide that testing this in identification() is "enough". Another argument in favor of this could be that not all cases from identification are also in replacement and the other way around.

Not completely following you there, as there are also existing tests in replacement() for static imports e.g. for Arguments.arguments and Objects.requireNonNull. Therefore, I did it for consistency and full test coverage. But I can imagine that we don't want to test every static import case this way. Would be good to have at least one static import replacement test-case for a static import on an annotation, however (did not check whether already exists). Otherwise, I'm also okay with removing this one.

Therefore, I did it for consistency and full test coverage.

Yes, in most cases we add tests in both methods, so normally when introducing a static import candidate, it would indeed make total sense, so I fully understand why it is in the PR 😄. For this case, because it is a special Refaster thingy, I think we could approach a bit different than normally.

did not check whether already exists

There exists one, see here.

Not completely following you there

Did the extra explanation help? 😄, let me know, happy to discuss.

With the reasoning that this is implicitly tested by self-check in this PR, I agree that we can remove the replacement() testcase. Also used a different import in identification() as suggested as STATIC_IMPORT_ALWAYS could cause confusion in a StaticImportCheckTest due to equal terminology.

rickie

Nice improvement 💪🏻 .

Suggested commit message:

Require static import of `com.google.errorprone.refaster.ImportPolicy`

mussatto

LGTM :)

Stephan202

Rebased and added a tiny commit. Tweaked suggested commit message:

Require static import of `com.google.errorprone.refaster.ImportPolicy` members (#45)

Stephan202 · 2022-04-10T11:24:55Z

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

+            "  // BUG: Diagnostic contains:",
+            "  @UseImportPolicy(ImportPolicy.IMPORT_TOP_LEVEL)",
+            "  @AfterTemplate",
+            "  void m2() {}",


Since it's "fake" anyway, we can just explain the context by giving the method a better name :)

Suggested change

" // BUG: Diagnostic contains:",

" @UseImportPolicy(ImportPolicy.IMPORT_TOP_LEVEL)",

" @AfterTemplate",

" void m2() {}",

" // BUG: Diagnostic contains:",

" @UseImportPolicy(ImportPolicy.IMPORT_TOP_LEVEL)",

" void refasterAfterTemplate() {}",

Why not try to be close to the "real deal" as discussed here?

We try to make examples realistic right?

How realistic is realistic enough? See for example m2 and m3 in replacement, which use @DateTimeFormat on methods without any other annotations that would make Spring actually invoke it.

Ah I see, thanks for the explanation, current state is 👍🏻 :).

rickie · 2022-04-11T08:28:01Z

Rebased.

rickie · 2022-04-12T07:50:36Z

Changes LGTM, WDYT @Badbond ? :)

Badbond · 2022-04-12T07:55:21Z

LGTM as well 🚀

Badbond requested review from Stephan202 and rickie March 15, 2022 15:49

Badbond commented Mar 15, 2022

View reviewed changes

rickie requested a review from mussatto March 17, 2022 08:23

rickie requested changes Mar 17, 2022

View reviewed changes

Badbond requested a review from rickie March 18, 2022 08:56

rickie approved these changes Mar 19, 2022

View reviewed changes

mussatto approved these changes Mar 22, 2022

View reviewed changes

Stephan202 force-pushed the pdsoels/static-import-policies branch from 3cf1a2c to fa08ac3 Compare April 10, 2022 11:26

Stephan202 approved these changes Apr 10, 2022

View reviewed changes

Stephan202 added this to the 0.1.0 milestone Apr 10, 2022

Stephan202 mentioned this pull request Apr 10, 2022

StreamTemplates Add template to prefer joining without delimiter #44

Merged

Badbond and others added 6 commits April 11, 2022 09:54

Add ImportPolicy static import

5830d3a

Self apply static import of ImportPolicy.STATIC_IMPORT_ALWAYS

816af07

Suggestions for the StaticImport tests

f03258b

Remove testcase in replacement test and use different import

880f102

Suggestion

03b1a57

Post rebase fix

d4b46a0

rickie force-pushed the pdsoels/static-import-policies branch from fa08ac3 to d4b46a0 Compare April 11, 2022 08:27

Stephan202 merged commit 9192245 into master Apr 12, 2022

Stephan202 deleted the pdsoels/static-import-policies branch April 12, 2022 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`StaticImportCheck` use static imports for `ImportPolicy` usages #45

`StaticImportCheck` use static imports for `ImportPolicy` usages #45

Badbond commented Mar 15, 2022 •

edited by rickie

Loading

Badbond Mar 15, 2022 •

edited

Loading

rickie left a comment

rickie Mar 17, 2022

Badbond Mar 17, 2022 •

edited

Loading

rickie Mar 17, 2022

Badbond Mar 17, 2022

rickie Mar 17, 2022

Badbond Mar 18, 2022

rickie left a comment

mussatto left a comment

Stephan202 left a comment

Stephan202 Apr 10, 2022

rickie Apr 11, 2022 •

edited

Loading

Stephan202 Apr 11, 2022

rickie Apr 11, 2022

rickie commented Apr 11, 2022

rickie commented Apr 12, 2022

Badbond commented Apr 12, 2022

StaticImportCheck use static imports for ImportPolicy usages #45

StaticImportCheck use static imports for ImportPolicy usages #45

Conversation

Badbond commented Mar 15, 2022 • edited by rickie Loading

Badbond Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Mar 17, 2022 • 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 left a comment

Choose a reason for hiding this comment

mussatto left a 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 Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Apr 11, 2022

rickie commented Apr 12, 2022

Badbond commented Apr 12, 2022

`StaticImportCheck` use static imports for `ImportPolicy` usages #45

`StaticImportCheck` use static imports for `ImportPolicy` usages #45

Badbond commented Mar 15, 2022 •

edited by rickie

Loading

Badbond Mar 15, 2022 •

edited

Loading

Badbond Mar 17, 2022 •

edited

Loading

rickie Apr 11, 2022 •

edited

Loading