Introduce Guava `Preconditions` Refaster rules #292

werli · 2022-10-12T09:37:58Z

This set of Guava Preconditions Refaster rules is non-exhaustive, but covers the most common use case for rewrites to Preconditions#check{Argument,State}().

I had pointed these four use cases out during code review multiple times by now, so adding a rule already. 👍

These templates only affect simple if constructs, not if-else. I think this should be part of another set of checks that removes the else-branch when the if-branch's responsibility is only throwing an exception (or returning something).

Stephan202

Very nice! Pushed a commit with some tweaks in line with "how we do it elsewhere", particularly in terms of naming.

The format string variants require special handling and are perhaps better deferred to another ticket (though I have some ideas around that; can write some stuff about that after the meetings I have now).

I would suggest also adding the other simple cases, though; don't mind doing those later :)

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

werli · 2022-10-12T11:27:29Z

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

+    @BeforeTemplate
+    void before(boolean condition, String message) {
+      if (condition) {
+        throw new IllegalArgumentException(message);
+      }
+    }


Opening a thread w/ @Stephan202's message:

The format string variants require special handling and are perhaps better deferred to another ticket (though I have some ideas around that; can write some stuff about that after the meetings I have now).

I also thought of leaving many of those as follow up. 👍

One thought on this however. Wouldn't e.g. a String#format() call already partially be handled by the case I had pointed out here? As a result, we'd get

checkArgument(!condition, String.format(message, obj1));

which I'd then expect to be re-written by another Preconditions rule. Curious to hear the thinking.

Indeed! The philosophy of the Refaster rules defined in this repository are that they define the smallest/most generic possible improvement, so that in unison they improve the largest amount of code. So what you propose is the way to go.

There's one caveat here: not all String.format calls can be replaced. What I suspect we need is a Matcher that identifies valid Strings.lenientFormat format strings; then we can instruct Refaster to rewrite only eligible expressions using @Matches(IsLenientFormatTemplateString.class). (Best name TBD.)

^ It'll be more "scalable" to handle the String.format logic as a BugChecker, so let's defer that to a separate PR.

Stephan202

Added another commit.

Stephan202 · 2022-10-12T13:50:22Z

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

+    @BeforeTemplate
+    void before(boolean condition, String message) {
+      if (condition) {
+        throw new IllegalArgumentException(message);
+      }
+    }


Indeed! The philosophy of the Refaster rules defined in this repository are that they define the smallest/most generic possible improvement, so that in unison they improve the largest amount of code. So what you propose is the way to go.

There's one caveat here: not all String.format calls can be replaced. What I suspect we need is a Matcher that identifies valid Strings.lenientFormat format strings; then we can instruct Refaster to rewrite only eligible expressions using @Matches(IsLenientFormatTemplateString.class). (Best name TBD.)

werli · 2022-10-12T14:48:41Z

Latest commits LGTM @Stephan202, thanks 👍

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

Stephan202

Rebased and added one more commit.

Suggested commit message:

Introduce Guava `Preconditions` Refaster rules (#292)

Stephan202 · 2022-10-15T08:49:24Z

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

+    @BeforeTemplate
+    void before(boolean condition, String message) {
+      if (condition) {
+        throw new IllegalArgumentException(message);
+      }
+    }


^ It'll be more "scalable" to handle the String.format logic as a BugChecker, so let's defer that to a separate PR.

Stephan202

Added :)

Stephan202 · 2022-10-15T10:27:05Z

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

+import com.google.errorprone.refaster.annotation.UseImportPolicy;
+
+/** Refaster templates related to statements dealing with {@link Preconditions}. */
+final class PreconditionsRules {


One more thing:

Suggested change

final class PreconditionsRules {

@OnlineDocumentation

final class PreconditionsRules {

rickie

Added a commit.

Nice improvements @werli 🚀 !

Suggested commit message LGTM, will merge soon.

rickie · 2022-10-19T15:01:25Z

...contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestOutput.java

@@ -41,6 +41,10 @@ int testCheckIndex() {
    return Objects.checkIndex(0, 1);
  }

+  void testCheckIndexConditional() {
+    Objects.checkIndex(1, 2);


I think we should add a ImportPolicy for CheckIndex and CheckIndexConditional as well.

We do it for the PreconditionsRules as well.

We internally always prefer that as well.

Even the StaticImport enforces this. However, as that is quite an opinionated check, some people will probably disable this check. Therefore it would be nice to make sure we use the static import for this as well.

JnRouvignac · 2022-10-20T12:36:27Z

Did you consider Java 9's java.util.Objects methods checkFromIndexSize(), checkFromToIndex() and checkIndex()?

rickie · 2022-10-20T14:59:45Z

Hello @JnRouvignac, yes for checkIndex() we have two rules in AssortedRules.java.

Up until now we haven't yet considered checkFromIndexSize or checkFromIndex. Adding rules for that could be interesting though. Are you willing to file a ticket for that? If you could provide some examples there as well, we can discuss adding such a rule. Maybe you would be up for opening a PR for that 😄?

Stephan202 · 2022-10-21T07:13:09Z

Something I just realized (and should have considered earlier): in a small number of cases the rules introduced here can cause functional or performance issues. See google/guava#5927 for an example of the former. An example of the latter would be a positional argument that is expensive to compute; such a computation now happens eagerly/unconditionally.

It might be good to test these changes internally before the next tag. (There's no nice way to add a local @SuppressWarnings for this case; perhaps we should look into a comment-based do-not-replace marker 🤔.)

werli requested a review from rickie October 12, 2022 09:37

Stephan202 reviewed Oct 12, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PreconditionsRules.java Outdated Show resolved Hide resolved

werli commented Oct 12, 2022

View reviewed changes

Stephan202 reviewed Oct 12, 2022

View reviewed changes

werli commented Oct 13, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java Outdated Show resolved Hide resolved

Stephan202 force-pushed the werli/preconditions branch from 946e517 to 0d6a33f Compare October 15, 2022 08:48

Stephan202 approved these changes Oct 15, 2022

View reviewed changes

Stephan202 reviewed Oct 15, 2022

View reviewed changes

werli and others added 7 commits October 15, 2022 13:58

Introduce Guava Preconditions Refaster rules

f28016b

Suggestions

1e34bbc

Some more

3080605

Also cover checkElementIndex

0bf5b9b

Rename before2 -> before if no other before is present

4ae9bb1

Also cover checkPositionIndex

012f8ad

Add @OnlineDocumentation

8d65924

Stephan202 force-pushed the werli/preconditions branch from 0b06825 to 8d65924 Compare October 15, 2022 12:00

Stephan202 added this to the 0.5.0 milestone Oct 15, 2022

Tweaks

b0b4c55

rickie approved these changes Oct 19, 2022

View reviewed changes

rickie added the new feature label Oct 19, 2022

rickie merged commit 68dca32 into master Oct 20, 2022

rickie deleted the werli/preconditions branch October 20, 2022 06:55

rickie pushed a commit that referenced this pull request Oct 20, 2022

Introduce Guava Preconditions Refaster rules (#292)

8e5314c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Guava `Preconditions` Refaster rules #292

Introduce Guava `Preconditions` Refaster rules #292

werli commented Oct 12, 2022

Stephan202 left a comment

werli Oct 12, 2022

Stephan202 Oct 12, 2022

Stephan202 Oct 15, 2022

Stephan202 left a comment

Stephan202 Oct 12, 2022

werli commented Oct 12, 2022 •

edited

Stephan202 left a comment

Stephan202 Oct 15, 2022

Stephan202 left a comment

Stephan202 Oct 15, 2022

rickie left a comment

rickie Oct 19, 2022

JnRouvignac commented Oct 20, 2022

rickie commented Oct 20, 2022 •

edited

Stephan202 commented Oct 21, 2022

	final class PreconditionsRules {
	@OnlineDocumentation
	final class PreconditionsRules {

Introduce Guava Preconditions Refaster rules #292

Introduce Guava Preconditions Refaster rules #292

Conversation

werli commented Oct 12, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

werli Oct 12, 2022

Choose a reason for hiding this comment

Stephan202 Oct 12, 2022

Choose a reason for hiding this comment

Stephan202 Oct 15, 2022

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Oct 12, 2022

Choose a reason for hiding this comment

werli commented Oct 12, 2022 • edited

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Oct 15, 2022

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Oct 15, 2022

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

rickie Oct 19, 2022

Choose a reason for hiding this comment

JnRouvignac commented Oct 20, 2022

rickie commented Oct 20, 2022 • edited

Stephan202 commented Oct 21, 2022

Introduce Guava `Preconditions` Refaster rules #292

Introduce Guava `Preconditions` Refaster rules #292

werli commented Oct 12, 2022 •

edited

rickie commented Oct 20, 2022 •

edited