Introduce `StreamMapTo{Double,Int,Long}Sum` Refaster rules #497

giovannizotta · 2023-02-12T18:47:00Z

I came across this pattern during a code review:
.map(Stock::getQuantity).reduce(0, Integer::sum);

This felt more readable to me:
.mapToInt(Stock::getQuantity).sum();

It can also be extended to Long and Double. Let me know what you think! 🙂

Suggested commit message:

Introduce `StreamMapTo{Double,Int,Long}Sum` Refaster rules (#497)

As well as a new `IsLambdaExpressionOrMethodReference` matcher.

github-actions · 2023-02-12T18:48: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

Adding the Double and Long variants makes sense to me :).

I added a commit; suggested commit message:

Introduce `StreamMapTo{Double,Int,Long}Sum` Refaster rules (#497)

Stephan202 · 2023-02-12T19:38:43Z

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

@@ -379,4 +380,16 @@ boolean after(Stream<T> stream) {
      return stream.allMatch(e -> test(e));
    }
  }
+
+  static final class StreamMapReduceIntSum<T> {


We try to name rules after the code in the @AfterTemplate, which in this case would yield:

Suggested change

static final class StreamMapReduceIntSum<T> {

static final class StreamMapToIntSum<T> {

(I was wondering whether this rule should be in the IntStreamRules class, but since IntStream is only an intermediate type, I suppose not.)

I was wondering the same, and came to the same conclusion 🙂

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

github-actions · 2023-02-12T20:01:40Z

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-02-12T20:09:50Z

Forgot to say: congrats and thanks for your first Error Prone Support contribution 🎉

github-actions · 2023-02-13T22:07:01Z

Surviving mutants in this change: 11
Killed mutants in this change: 0

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsLambdaExpressionOrMethodReference`	11	0

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

giovannizotta · 2023-02-13T22:09:25Z

Thank you! 😊

I added the Matcher, but I couldn't figure out how to make a test for it from these tests. Do you have any pointers? 😅

rickie · 2023-02-15T09:43:10Z

I found a related example that we used during my thesis project: google/error-prone@master...PicnicSupermarket:error-prone:rossendrijver/return_type#diff-cd9d301320. The Matcher used there did some slightly different things and was "hacked together" a little 😬.

I would write the Matcher to be in the same setup/style as the other matchers. Having a Matcher DELEGATE like this:

  private static final Matcher<ExpressionTree> DELEGATE =
      anyOf(isSameType(CHAR_TYPE), isSameType(Character.class));

The DELEGATE can also invoke a method that you would have in IsLambdaExpressionOrMethodReference where you can do the analysis.

The IsLambdaExpressionOrMethodReferenceTest would have the following things;

Test code just like the other Matcher tests.
A TestMatcher that uses the AbstractTestMatcher to validate against the DELEGATE.

This comment combined with the things we discussed offline should be a pointer 😄.

giovannizotta · 2023-02-15T10:28:30Z

Thank you for your time Rick! That's definitely a nice pointer, I'll take a look at it when I have some time :)

giovannizotta · 2023-02-18T20:17:58Z

I'm having fun! 😄
This is the current state of the matcher, which I think resembles what you had in mind.

However, I am having a bit of trouble testing it in the same way as the other Matchers (like IsCharacter) are tested. Those are testing simple one-liners, while in this case, if I want to test that actual instances of Function<T, {Double, Integer, Long}> are matched, I need a multi-line test.

If I try something like this, it doesn't work because the matcher searches for matches in the whole test method, not only the .map(function).reduce(0, Integer::sum) part. Indeed, you can see that of course the matcher matches lambda expressions and method references, but not those that we are interested in.

Do you have any idea on how to proceed? I bet there's a way to be able to handle this by taking a look at, for example, ErrorProneTestHelperSourceFormatTest, but I do not completely understand what's going on. I pushed a commit with the test disabled. Thank you in advance for your time! 🙂

github-actions · 2023-02-18T20:19:48Z

Surviving mutants in this change: 10
Killed mutants in this change: 0

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsLambdaExpressionOrMethodReference`	10	0

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Good question about how to test this stuff. The trick is so split the code over multiple lines. That said, this does indeed highlight a weakness of this kind of test: it's easy to over-match.

I rebased and added a commit with some improvements. Not yet done reviewing, but thought it'd be nice to show this intermediate state :)

(Functionally I think this now looks very good; will look a bit into minimizing the test without impacting coverage.)

Stephan202 · 2023-02-19T09:28:09Z

...ne-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestInput.java

@@ -138,4 +139,28 @@ ImmutableSet<Boolean> testStreamAllMatch() {
  boolean testStreamAllMatch2() {
    return Stream.of("foo").noneMatch(s -> !s.isBlank());
  }
+
+  ImmutableSet<Integer> testStreamMapToIntSum() {
+    Function<String, Integer> parseIntFunction = (String s) -> Integer.parseInt(s);


Just because it's shorter:

Suggested change

Function<String, Integer> parseIntFunction = (String s) -> Integer.parseInt(s);

Function<String, Integer> parseIntFunction = Integer::parseInt;

Stephan202 · 2023-02-19T09:29:49Z

.../main/java/tech/picnic/errorprone/refaster/matchers/IsLambdaExpressionOrMethodReference.java

+  public static <T extends Tree> Matcher<T> isMethodReference() {
+    return (tree, state) ->
+        tree instanceof MemberReferenceTree
+            && ((MemberReferenceTree) tree).getMode().equals(INVOKE);


We can also allow ::new, so the mode check can be omitted :)

Makes sense. I was thinking about the specific usage of this PR, but extending it to ::new it makes it more reusable indeed :)

Stephan202 · 2023-02-19T09:37:40Z

.../main/java/tech/picnic/errorprone/refaster/matchers/IsLambdaExpressionOrMethodReference.java

+public final class IsLambdaExpressionOrMethodReference implements Matcher<ExpressionTree> {
+  private static final long serialVersionUID = 1L;
+
+  private static final Matcher<ExpressionTree> DELEGATE =


While indeed usually we introduce such a DELEGATE constant for performance reasons, in this case there's no efficiency gain over inlining the logic.

github-actions · 2023-02-19T10:00:30Z

Looks good. All 5 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsLambdaExpressionOrMethodReference`	0	5

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Added one more commit. Very nice contribution @giovannizotta! 💪

Updated suggested commit message:

Introduce `StreamMapTo{Double,Int,Long}Sum` Refaster rules (#497)

As well as a new `IsLambdaExpressionOrMethodReference` matcher.

github-actions · 2023-02-19T10:48:58Z

Looks good. All 5 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsLambdaExpressionOrMethodReference`	0	5

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

giovannizotta · 2023-02-19T11:02:10Z

Thank you @Stephan202 for the cleanup! It was the first time I dove into these concepts but I really enjoyed it, even though you ended up doing most of the work 😄

Also nice to see the intermediate state to know how to solve this kind of issue the next time :)

Stephan202 · 2023-02-19T11:08:19Z

Rest assured: this workflow is how it goes every time 😄. It's in the nature of the project: tricky concepts, a large and unfamiliar API, and some rather opinionated maintainers ;)

If you're up for it, don't hesitate to contribute other changes if/when you observe them during code review 🚀.

github-actions · 2023-02-21T06:34:48Z

Looks good. All 5 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsLambdaExpressionOrMethodReference`	0	5

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

@Stephan202, I'm curious, any particular reason to not add Javadoc for the Refaster rules?

Added a commit with some suggestions.

Nice contribution @giovannizotta, cool stuff 🚀 !

rickie · 2023-02-21T07:38:19Z

...t/java/tech/picnic/errorprone/refaster/matchers/IsLambdaExpressionOrMethodReferenceTest.java

+            "",
+            "  Predicate<String> positive1() {",
+            "    // BUG: Diagnostic contains:",
+            "    return str -> true;",


I think it is also nice to have a lambda case with { }, just for completeness.

rickie · 2023-02-21T08:10:16Z

...e-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestOutput.java

+    Function<String, Double> parseDoubleFunction = Double::parseDouble;
+    return ImmutableSet.of(
+        Stream.of(1).mapToDouble(i -> i * 2.0).sum(),
+        Stream.of("1").mapToDouble(Double::parseDouble).sum(),


We generally use a different number or string per test case, see also further up in this file :). So let's change this.

github-actions · 2023-02-21T08:14:10Z

Looks good. All 5 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.refaster.matchers.IsLambdaExpressionOrMethodReference`	0	5

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-02-21T15:17:20Z

@Stephan202, I'm curious, any particular reason to not add Javadoc for the Refaster rules?

Nope, just that the preceding rules also don't have any. :)

Stephan202 reviewed Feb 12, 2023

View reviewed changes

Stephan202 added this to the 0.9.0 milestone Feb 12, 2023

giovannizotta changed the title ~~Introduce StreamMapReduceIntSum Refaster rule~~ Introduce StreamMapTo{Double,Int,Long}Sum Refaster rules Feb 12, 2023

giovannizotta added the WIP Work in progress label Feb 13, 2023

Stephan202 force-pushed the giovannizotta/map-reduce-int-sum branch from 19dc1a2 to eaea479 Compare February 19, 2023 09:58

Stephan202 reviewed Feb 19, 2023

View reviewed changes

Stephan202 approved these changes Feb 19, 2023

View reviewed changes

giovannizotta removed the WIP Work in progress label Feb 20, 2023

rickie self-requested a review February 20, 2023 17:58

rickie added the new feature label Feb 20, 2023

giovannizotta and others added 6 commits February 21, 2023 07:32

Introduce StreamMapReduceIntSum Refaster rule

4150307

Suggestions

b6137dc

Annotate mapper with @matches(IsLambdaExpressionOrMethodReference.class)

63cf484

Refactor IsLambdaExpressionOrMethodReference with a delegate

1d94f39

Suggestions

1614214

Simplify test

3d73f2e

rickie force-pushed the giovannizotta/map-reduce-int-sum branch from 214a0c7 to 3d73f2e Compare February 21, 2023 06:32

Suggestions

5228e4e

rickie approved these changes Feb 21, 2023

View reviewed changes

rickie merged commit 5bb1dd1 into master Feb 21, 2023

rickie deleted the giovannizotta/map-reduce-int-sum branch February 21, 2023 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `StreamMapTo{Double,Int,Long}Sum` Refaster rules #497

Introduce `StreamMapTo{Double,Int,Long}Sum` Refaster rules #497

giovannizotta commented Feb 12, 2023 •

edited

Loading

github-actions bot commented Feb 12, 2023

Stephan202 left a comment

Stephan202 Feb 12, 2023

Stephan202 Feb 12, 2023

giovannizotta Feb 13, 2023

github-actions bot commented Feb 12, 2023

Stephan202 commented Feb 12, 2023

github-actions bot commented Feb 13, 2023

giovannizotta commented Feb 13, 2023

rickie commented Feb 15, 2023 •

edited

Loading

giovannizotta commented Feb 15, 2023

giovannizotta commented Feb 18, 2023

github-actions bot commented Feb 18, 2023

Stephan202 left a comment

Stephan202 Feb 19, 2023

Stephan202 Feb 19, 2023

giovannizotta Feb 19, 2023

Stephan202 Feb 19, 2023

github-actions bot commented Feb 19, 2023

Stephan202 left a comment

github-actions bot commented Feb 19, 2023

giovannizotta commented Feb 19, 2023

Stephan202 commented Feb 19, 2023

github-actions bot commented Feb 21, 2023

rickie left a comment

rickie Feb 21, 2023 •

edited

Loading

rickie Feb 21, 2023

github-actions bot commented Feb 21, 2023

Stephan202 commented Feb 21, 2023

	static final class StreamMapReduceIntSum<T> {
	static final class StreamMapToIntSum<T> {

	Function<String, Integer> parseIntFunction = (String s) -> Integer.parseInt(s);
	Function<String, Integer> parseIntFunction = Integer::parseInt;

Introduce StreamMapTo{Double,Int,Long}Sum Refaster rules #497

Introduce StreamMapTo{Double,Int,Long}Sum Refaster rules #497

Conversation

giovannizotta commented Feb 12, 2023 • edited Loading

github-actions bot commented Feb 12, 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

github-actions bot commented Feb 12, 2023

Stephan202 commented Feb 12, 2023

github-actions bot commented Feb 13, 2023

giovannizotta commented Feb 13, 2023

rickie commented Feb 15, 2023 • edited Loading

giovannizotta commented Feb 15, 2023

giovannizotta commented Feb 18, 2023

github-actions bot commented Feb 18, 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 Feb 19, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 19, 2023

giovannizotta commented Feb 19, 2023

Stephan202 commented Feb 19, 2023

github-actions bot commented Feb 21, 2023

rickie left a comment

Choose a reason for hiding this comment

rickie Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2023

Stephan202 commented Feb 21, 2023

Introduce `StreamMapTo{Double,Int,Long}Sum` Refaster rules #497

Introduce `StreamMapTo{Double,Int,Long}Sum` Refaster rules #497

giovannizotta commented Feb 12, 2023 •

edited

Loading

rickie commented Feb 15, 2023 •

edited

Loading

rickie Feb 21, 2023 •

edited

Loading