Prefer `Mono#fromSupplier` over `Mono#fromCallable` where possible #232

Stephan202 · 2022-09-10T13:50:48Z

Suggested commit message:

Prefer `Mono#fromSupplier` over `Mono#fromCallable` where possible (#232)

This rule is implemented using a Refaster template, relying on the new
`ThrowsCheckedException` matcher.

While there, introduce `AbstractTestChecker` to simplify the test setup for
Refaster `Matcher`s. This base class flags all `ExpressionTree`s matched by the
`Matcher` under test.

rickie

This significantly improves our setup, very cool!

Also nice Refaster template 🚀.

Have some questions and considerations :).

rickie · 2022-09-12T12:33:12Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractTestChecker.java

+import com.sun.source.util.TreeScanner;
+import javax.annotation.Nullable;
+
+abstract class AbstractTestChecker extends BugChecker implements CompilationUnitTreeMatcher {


To make clear the checker is solely focused on matching ExpressionTrees, should we either (1) change the name or (2) add some Javadoc explaining this?

Hmm, maybe AbstractMatcherTestChecker. Will add some documentation.

rickie · 2022-09-12T12:37:52Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractTestChecker.java

+      public Void visitImport(ImportTree node, @Nullable Void unused) {
+        /*
+         * We're not interested in matching import statements. While components of these
+         * can be `ExpressionTree`s.


Should we do this to be consistent with the method below?

Suggested change

* can be `ExpressionTree`s.

* can be `ExpressionTree`s, they will never be matched by Refaster.

rickie · 2022-09-12T12:42:23Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractTestChecker.java

+    new TreeScanner<Void, Void>() {
+      @Nullable
+      @Override
+      public Void scan(Tree tree, @Nullable Void p) {


Suggested change

public Void scan(Tree tree, @Nullable Void p) {

public Void scan(Tree tree, @Nullable Void unused) {

The name p was most likely choosen over unused on purpose, but still wanted to ask, technically it's "unused" because p is Void... Does it make sense to let it be consistent with the visitImport one and also call it unused?

Ah, it's named p after the method in the TreeScanner superclass. First it had another name, but then IDEA complained. But I see that IDEA handles unused specially, which makes sense. No strong opinion either way, so will change.

rickie · 2022-09-12T12:46:11Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractTestChecker.java

+import com.sun.source.util.TreeScanner;
+import javax.annotation.Nullable;
+
+abstract class AbstractTestChecker extends BugChecker implements CompilationUnitTreeMatcher {


Is there anything against moving this to refaster-test-support?

People might want to use this AbstractTestChecker for their own Matchers as well.

I'd defer such a move for two reasons:

This class isn't truly Refaster- specific.

As-is the class isn't part of the public API, thus allowing us to change it more freely in the future.

rickie · 2022-09-12T12:52:59Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsArrayTest.java

-    @Override
-    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
-      return new IsArray().matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH;
+    // XXX: This is false positive reported by CheckStyle. See


Suggested change

// XXX: This is false positive reported by CheckStyle. See

// XXX: This is a false positive reported by Checkstyle. See

Right?

It's funny, you usually write Github instead of GitHub and for Checkstyle it's the other way around 😉.

rickie · 2022-09-12T13:59:05Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/ThrowsCheckedException.java

+    }
+  }
+
+  private static boolean containsCheckedException(Collection<Type> types, VisitorState state) {


Should this method be above the other one because it comes before getThrownTypes? "Call-down" 😄 ?

In either case we would call down, but getThrownTypes is invoked before containsCheckedException, so I'd say the current order is "correct".

This rule is implemented using a Refaster template, relying on the new `ThrowsCheckedException` matcher. While there, introduce `AbstractTestChecker` to simplify the test setup for Refaster `Matcher`s. This base class flags all `ExpressionTree`s matched by the `Matcher` under test.

Stephan202

Rebased and added a commit. Tnx for the review!

Stephan202 · 2022-09-12T17:53:21Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/ThrowsCheckedException.java

+    }
+  }
+
+  private static boolean containsCheckedException(Collection<Type> types, VisitorState state) {


In either case we would call down, but getThrownTypes is invoked before containsCheckedException, so I'd say the current order is "correct".

Stephan202 · 2022-09-12T17:54:39Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractTestChecker.java

+import com.sun.source.util.TreeScanner;
+import javax.annotation.Nullable;
+
+abstract class AbstractTestChecker extends BugChecker implements CompilationUnitTreeMatcher {


I'd defer such a move for two reasons:

This class isn't truly Refaster- specific.

As-is the class isn't part of the public API, thus allowing us to change it more freely in the future.

Stephan202 · 2022-09-12T18:07:46Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractTestChecker.java

+import com.sun.source.util.TreeScanner;
+import javax.annotation.Nullable;
+
+abstract class AbstractTestChecker extends BugChecker implements CompilationUnitTreeMatcher {


Hmm, maybe AbstractMatcherTestChecker. Will add some documentation.

Stephan202 · 2022-09-12T18:27:32Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/AbstractTestChecker.java

+    new TreeScanner<Void, Void>() {
+      @Nullable
+      @Override
+      public Void scan(Tree tree, @Nullable Void p) {


Ah, it's named p after the method in the TreeScanner superclass. First it had another name, but then IDEA complained. But I see that IDEA handles unused specially, which makes sense. No strong opinion either way, so will change.

rickie

Changes LGTM!

Suggested commit message is 👍🏻.

EnricSala

Nice! From now on will be able to stop checking all Mono.fromCallable(... for checked exceptions 😄

Didn't review the ThrowsCheckedException matcher because it escapes my knowledge and I have no time to learn it right now, but looks cool :)

rickie · 2022-09-15T07:41:16Z

Thanks for the review @EnricSala 😄.

Stephan202 added this to the 0.2.1 milestone Sep 10, 2022

Stephan202 requested a review from rickie September 10, 2022 13:50

Stephan202 marked this pull request as ready for review September 10, 2022 13:57

Stephan202 mentioned this pull request Sep 10, 2022

Introduce Refaster templates for AbstractComparableAsserts #225

Merged

rickie added the new feature label Sep 12, 2022

rickie requested changes Sep 12, 2022

View reviewed changes

Stephan202 and others added 3 commits September 12, 2022 19:56

Suggestions

83629ce

Address feedback

6f199a3

Stephan202 force-pushed the sschroevers/prefer-mono-from-supplier branch from 0cd0e3a to 6f199a3 Compare September 12, 2022 19:21

Stephan202 commented Sep 12, 2022

View reviewed changes

rickie approved these changes Sep 13, 2022

View reviewed changes

rickie requested a review from EnricSala September 14, 2022 06:26

EnricSala approved these changes Sep 15, 2022

View reviewed changes

rickie merged commit 62fe10f into master Sep 15, 2022

rickie deleted the sschroevers/prefer-mono-from-supplier branch September 15, 2022 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer `Mono#fromSupplier` over `Mono#fromCallable` where possible #232

Prefer `Mono#fromSupplier` over `Mono#fromCallable` where possible #232

Stephan202 commented Sep 10, 2022 •

edited by rickie

rickie left a comment

rickie Sep 12, 2022

Stephan202 Sep 12, 2022

rickie Sep 12, 2022

rickie Sep 12, 2022

Stephan202 Sep 12, 2022

rickie Sep 12, 2022

Stephan202 Sep 12, 2022

rickie Sep 12, 2022

rickie Sep 12, 2022

Stephan202 Sep 12, 2022

Stephan202 left a comment

Stephan202 Sep 12, 2022

Stephan202 Sep 12, 2022

Stephan202 Sep 12, 2022

Stephan202 Sep 12, 2022

rickie left a comment

EnricSala left a comment

rickie commented Sep 15, 2022

	* can be `ExpressionTree`s.
	* can be `ExpressionTree`s, they will never be matched by Refaster.

	public Void scan(Tree tree, @Nullable Void p) {
	public Void scan(Tree tree, @Nullable Void unused) {

	// XXX: This is false positive reported by CheckStyle. See
	// XXX: This is a false positive reported by Checkstyle. See

Prefer Mono#fromSupplier over Mono#fromCallable where possible #232

Prefer Mono#fromSupplier over Mono#fromCallable where possible #232

Conversation

Stephan202 commented Sep 10, 2022 • edited by rickie

rickie 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

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

rickie left a comment

Choose a reason for hiding this comment

EnricSala left a comment

Choose a reason for hiding this comment

rickie commented Sep 15, 2022

Prefer `Mono#fromSupplier` over `Mono#fromCallable` where possible #232

Prefer `Mono#fromSupplier` over `Mono#fromCallable` where possible #232

Stephan202 commented Sep 10, 2022 •

edited by rickie