Introduce `BugCheckerRules` Refaster rule collection #526

Ptijohn · 2023-03-10T09:35:26Z

Suggested commit message:

Introduce `BugCheckerRule` refaster rules (#526)

Ptijohn · 2023-03-10T09:36:22Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java

+
+/** Refaster rules related to BugChecker classes. */
+@OnlineDocumentation
+final class BugCheckerRules {


Honestly not sure about the reach of this class 🤔
I could also limit it to BugCheckerRefactoringTestHelper, but then it's also quite closed.
That would also impact docs and all.

Yeah, good question. In the past we have dodged questions such as this one by locating single rules in AssortedRules. I'll add an extra rule so that the new rule isn't feeling lonely ;)

Hahaha, yeah I saw this class and hesitated, but felt like doing this was cleaner ;)
Thanks for the added rule 💪

github-actions · 2023-03-10T09:37:59Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Added a commit. Suggested commit message:

Introduce `BugCheckerRules` Refaster rule collection (#526)

Stephan202 · 2023-03-12T10:23:31Z

error-prone-contrib/pom.xml

@@ -37,7 +37,6 @@
        <dependency>
            <groupId>${groupId.error-prone}</groupId>
            <artifactId>error_prone_test_helpers</artifactId>
-            <scope>test</scope>


We don't want to pull in test dependencies at runtime; this should become provided.

Stephan202 · 2023-03-12T10:34:37Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java

+   * Drop {@link BugCheckerRefactoringTestHelper#setFixChooser(FixChooser)} when set to the default
+   * {@link FixChoosers#FIRST}.
+   */
+  static final class SetFixChooserDefault {


I guess that with our upcoming automated Refaster rule naming scheme this class would be named BugCheckerRefactoringTestHelperIdentity. (Due to side-effects this isn't fully accurate, but alas.) We can extend this rule to cover .setImportOrder("static-first").

Stephan202 · 2023-03-12T10:41:55Z

...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/BugCheckerRulesTestInput.java

+  }
+
+  BugCheckerRefactoringTestHelper testSetFixChooserDefault() {
+    return BugCheckerRefactoringTestHelper.newInstance(StringCaseLocaleUsage.class, getClass())


Suggested change

return BugCheckerRefactoringTestHelper.newInstance(StringCaseLocaleUsage.class, getClass())

return BugCheckerRefactoringTestHelper.newInstance(BugChecker.class, getClass())

Stephan202 · 2023-03-12T10:59:17Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java

+
+/** Refaster rules related to BugChecker classes. */
+@OnlineDocumentation
+final class BugCheckerRules {


Yeah, good question. In the past we have dodged questions such as this one by locating single rules in AssortedRules. I'll add an extra rule so that the new rule isn't feeling lonely ;)

github-actions · 2023-03-12T11:12:45Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-03-12T11:13:35Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java

+  // XXX: This rule assumes that the full source code is specified as a single string, e.g. using a
+  // text block. Support for multi-line source code input would require a `BugChecker`
+  // implementation instead.


Right now this rule won't match, but after #198 it'll be relevant.

Thanks @Stephan202 💪

rickie

Added a commit with Javadoc tweak and rebased.

Maybe we can consider adding something for BugCheckerRefactoringTestHelper#setArgs for the case where we pass an ImmutableList and have only one string. Could be nicer to drop immutableList in that case. If the setArgs is called with multiple strings we can opt for the ImmutableList variant. WDYT? (out of scope in any case though)

Apart from that, it looks really good 👍🏻, nice to see that it drops a few of these occurrences 😄.

Thanks for picking up @Ptijohn 🚀 !

rickie · 2023-03-22T17:29:38Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java

+  private BugCheckerRules() {}
+
+  /**
+   * Avoid calling {@link BugCheckerRefactoringTestHelper#setFixChooser(FixChooser)} with the


We should update this to also account for setImportOrder. Will propose something :).

github-actions · 2023-03-22T17:32:18Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-03-22T17:51:38Z

Changes LGTM 👍

W.r.t. setArgs: yes, that one is on my list of things to do. That case is not really Refaster-compatible (using @Repeated won't work); instead I'm thinking of introducing a bug checker that simplifies unary methods calls with an ImmutableList.of/Arrays.asList (or similar) argument, for which a varargs overload exists.

rickie · 2023-03-22T17:56:23Z

Nice, sounds like a good idea 👍🏻 . Rebased, will merge once 🟢.

github-actions · 2023-03-22T17:58:02Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Ptijohn commented Mar 10, 2023

View reviewed changes

Ptijohn requested a review from rickie March 10, 2023 09:36

Stephan202 approved these changes Mar 12, 2023

View reviewed changes

Stephan202 reviewed Mar 12, 2023

View reviewed changes

Stephan202 added this to the 0.9.0 milestone Mar 12, 2023

rickie force-pushed the bdiederichs/fix-choosers-first branch from 74a3a4d to 68fac51 Compare March 22, 2023 17:30

rickie approved these changes Mar 22, 2023

View reviewed changes

rickie added the new feature label Mar 22, 2023

Bastien Diederichs and others added 5 commits March 22, 2023 18:56

Introduce rule to drop default setFixChooser

f9c69dd

Apply the actual rule

8277605

Clean doc

23c51d1

Suggestions

fc064c5

Suggestions

7e05c73

rickie force-pushed the bdiederichs/fix-choosers-first branch from 68fac51 to 7e05c73 Compare March 22, 2023 17:56

rickie changed the title ~~Introduce BugCheckerRule refaster rules~~ Introduce BugCheckerRules Refaster rule collection Mar 23, 2023

rickie merged commit 8f1d1df into master Mar 23, 2023

rickie deleted the bdiederichs/fix-choosers-first branch March 23, 2023 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `BugCheckerRules` Refaster rule collection #526

Introduce `BugCheckerRules` Refaster rule collection #526

Ptijohn commented Mar 10, 2023 •

edited

Loading

Ptijohn Mar 10, 2023

Stephan202 Mar 12, 2023

Ptijohn Mar 13, 2023

github-actions bot commented Mar 10, 2023

Stephan202 left a comment

Stephan202 Mar 12, 2023

Stephan202 Mar 12, 2023

Stephan202 Mar 12, 2023

Stephan202 Mar 12, 2023

github-actions bot commented Mar 12, 2023

Stephan202 Mar 12, 2023

Ptijohn Mar 13, 2023

rickie left a comment

rickie Mar 22, 2023

github-actions bot commented Mar 22, 2023

Stephan202 commented Mar 22, 2023

rickie commented Mar 22, 2023

github-actions bot commented Mar 22, 2023

	return BugCheckerRefactoringTestHelper.newInstance(StringCaseLocaleUsage.class, getClass())
	return BugCheckerRefactoringTestHelper.newInstance(BugChecker.class, getClass())

Introduce BugCheckerRules Refaster rule collection #526

Introduce BugCheckerRules Refaster rule collection #526

Conversation

Ptijohn commented Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 10, 2023

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

github-actions bot commented Mar 12, 2023

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

Choose a reason for hiding this comment

github-actions bot commented Mar 22, 2023

Stephan202 commented Mar 22, 2023

rickie commented Mar 22, 2023

github-actions bot commented Mar 22, 2023

Introduce `BugCheckerRules` Refaster rule collection #526

Introduce `BugCheckerRules` Refaster rule collection #526

Ptijohn commented Mar 10, 2023 •

edited

Loading