Extend `StreamRules` Refaster rule collection #593

mohamedsamehsalah · 2023-04-23T21:41:38Z

Replace redundant stream#collect calls.

Extend `StreamRules` Refaster rule collection (#593)

All changes suggested by SonarCloud's java:S4266 rule are now covered.

See https://sonarcloud.io/organizations/picnic-technologies/rules?open=java%3AS4266&rule_key=java%3AS4266

Fixes #578.

stream.collect(minBy(comparator)) -> stream.min(comparator)
stream.collect(maxBy(comparator)) -> stream.max(comparator)
stream.collect(summingInt(mapper)) -> stream.mapToInt(mapper).sum()
stream.collect(summingDouble(mapper)) -> stream.mapToDouble(mapper).sum()
stream.collect(summingLong(mapper)) -> stream.mapToLong(mapper).sum()
stream.collect(counting()) -> stream.count()
stream.collect(reducing(identity, accumulator)) -> stream.reduce(identity, accumulator)
stream.collect(reducing(accumulator)) -> stream.reduce(accumulator)
stream.collect(mapping(mapper, collector)) -> stream.map(mapper).collect(collector)

Fun fact: Most of this list was suggested by co-pilot 🧠 :

github-actions · 2023-04-23T21:43:08Z

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.

mohamedsamehsalah · 2023-04-23T21:58:45Z

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

@@ -90,7 +91,8 @@ ImmutableSet<Boolean> testStreamIsNotEmpty() {
  ImmutableSet<Optional<String>> testStreamMin() {
    return ImmutableSet.of(
        Stream.of("foo").max(comparingInt(String::length).reversed()),
-        Stream.of("bar").sorted(comparingInt(String::length)).findFirst());
+        Stream.of("bar").sorted(comparingInt(String::length)).findFirst(),
+        Stream.of("baz").collect(java.util.stream.Collectors.minBy(comparingInt(String::length))));


Not sure about this...
For some reason, test output expcts to have the same imports are input test 😕.

While running fmt:format removes the expected imported packages.

Yep, that's why we have that funny elidedTypesAndStaticImports method at the top :).

mohamedsamehsalah · 2023-04-23T22:00:37Z

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

+    @BeforeTemplate
+    long before(
+        Stream<T> stream,
+        @Matches(IsLambdaExpressionOrMethodReference.class) ToIntFunction<T> mapper) {


I could not Generalise Function<? super T, Integer> mapper from StreamMapToIntSum....

Ditto StreamMapToDoubleSum & StreamMapToLongSum

Yeah, so what we usually do here is to combine the @BeforeTemplates, even though some will have a parameter type that doesn't match the @AfterTemplate. In this case that's already true for the existing rule, so we can add the extra @BeforeTemplate to it. Two things to note:

None of the new rules should have @Matches(IsLambdaExpressionOrMethodReference.class). That matcher is only present on the existing rules precisely because the parameter type mismatch :)

The compiler may want about a potential ambiguity between the two before methods; this we can avoid using a rename.

Thank you so much for the clarification. 🚀

One question though
should'nt

....Function<? super T, Integer> mapper) { return stream.collect(summingInt(t -> mapper.apply(t).intValue())); }

be equal to

....ToIntFunction<T> mapper) { return stream.collect(summingInt(mapper)); }

The tests fail but the compiler aint complaining 😓 ...

Explained offline 😄

Stephan202

Thanks @mohamedsamehsalah! I added a commit.

We have some unwritten fuzzy rule about the order in which we place Refaster rules inside the enclosing class, based on which some of these new rules should perhaps be moved up. But since eventually we'll likely sort the rules lexicographically (or according to some other more objective algorithm), I'm fine with merging this as-is.

Suggested commit message:

Add additional Refaster rules to `StreamRules` (#593)

All changes suggested by SonarCloud's java:S4266 rule are now covered.

See https://sonarcloud.io/organizations/picnic-technologies/rules?open=java%3AS4266&rule_key=java%3AS4266

Fixes #578.

Stephan202 · 2023-04-24T04:38:14Z

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

+    @BeforeTemplate
+    long before(
+        Stream<T> stream,
+        @Matches(IsLambdaExpressionOrMethodReference.class) ToIntFunction<T> mapper) {


Yeah, so what we usually do here is to combine the @BeforeTemplates, even though some will have a parameter type that doesn't match the @AfterTemplate. In this case that's already true for the existing rule, so we can add the extra @BeforeTemplate to it. Two things to note:

None of the new rules should have @Matches(IsLambdaExpressionOrMethodReference.class). That matcher is only present on the existing rules precisely because the parameter type mismatch :)

The compiler may want about a potential ambiguity between the two before methods; this we can avoid using a rename.

Stephan202 · 2023-04-24T04:40:32Z

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

+  static final class StreamReduce<T> {
+    @BeforeTemplate
+    T before(Stream<T> stream, T identity, BinaryOperator<T> accumulator) {
+      return stream.collect(reducing(identity, accumulator));
+    }
+
+    @AfterTemplate
+    T after(Stream<T> stream, T identity, BinaryOperator<T> accumulator) {
+      return stream.reduce(identity, accumulator);
+    }
+  }
+
+  static final class StreamReduceOptional<T> {
+    @BeforeTemplate
+    Optional<T> before(Stream<T> stream, BinaryOperator<T> accumulator) {
+      return stream.collect(reducing(accumulator));
+    }
+
+    @AfterTemplate
+    Optional<T> after(Stream<T> stream, BinaryOperator<T> accumulator) {
+      return stream.reduce(accumulator);
+    }
+  }


I think that so far we used the shorter/simpler rule name for the shorter/simpler code. That would change the names of these rules to StreamReduceAccumulator and StreamReduceWithIdentity, respectively.

Stephan202 · 2023-04-24T04:41:21Z

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

@@ -90,7 +91,8 @@ ImmutableSet<Boolean> testStreamIsNotEmpty() {
  ImmutableSet<Optional<String>> testStreamMin() {
    return ImmutableSet.of(
        Stream.of("foo").max(comparingInt(String::length).reversed()),
-        Stream.of("bar").sorted(comparingInt(String::length)).findFirst());
+        Stream.of("bar").sorted(comparingInt(String::length)).findFirst(),
+        Stream.of("baz").collect(java.util.stream.Collectors.minBy(comparingInt(String::length))));


Yep, that's why we have that funny elidedTypesAndStaticImports method at the top :).

Stephan202 · 2023-04-24T04:45:10Z

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

+        Stream<T> stream,
+        @Matches(IsLambdaExpressionOrMethodReference.class) Function<? super T, ? extends U> mapper,
+        Collector<? super U, A, R> collector) {
+      return stream.collect(mapping(mapper, collector));


While missing from SonarCloud's overview, we can add similar rules for filtering, flatMapping and summarizing*.

Stephan202 · 2023-04-24T05:24:15Z

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

+    }
+  }
+
+  static final class StreamMap<T, U, A, R> {


Our naming strategy would call for StreamMapCollect.

github-actions · 2023-04-24T05:26:27Z

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-04-25T06:35:05Z

Rebased and resolved trivial conflicts.

github-actions · 2023-04-25T06:36:04Z

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-04-25T09:19:40Z

^ New SonarCloud violations. I'll have a look.

github-actions · 2023-04-25T09:39:53Z

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 two commits. @rickie I think this one is ready for your eyes :)

Stephan202 · 2023-04-25T09:40:07Z

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

@@ -263,9 +280,12 @@ boolean after(Stream<T> stream) {

  static final class StreamMin<T> {
    @BeforeTemplate
+    @SuppressWarnings("java:S1155" /* This violation will be rewritten. */)


Here and below: we could introduce a separate method to further reduce this suppression's scope, but not sure that this is worth it.

Don't think that is worth it indeed.

github-actions · 2023-04-25T09:41:05Z

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.

github-actions · 2023-04-25T09:42:51Z

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.

rickie

Some questions and considerations :).

In general, looks good! Curious to see the impact 😄.

rickie · 2023-04-25T11:46:13Z

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

@@ -263,9 +280,12 @@ boolean after(Stream<T> stream) {

  static final class StreamMin<T> {
    @BeforeTemplate
+    @SuppressWarnings("java:S1155" /* This violation will be rewritten. */)


Don't think that is worth it indeed.

rickie · 2023-04-25T12:01:55Z

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

@@ -428,4 +469,130 @@ long after(Stream<T> stream, ToLongFunction<T> mapper) {
      return stream.mapToLong(mapper).sum();
    }
  }
+
+  static final class StreamMapToIntSummaryStatistics<T> {


I'd suspect *SummaryStatistics rules to be at the end as they are not directly on Stream 🤔.

So the ordering is not lexicographical but also not based on the order of Stream itself 🤔. filter, map and flatMap things are first in that class.

If you want to reorder things based on a formula that we can eventually automate/auto-enforce: go for it 😄

rickie · 2023-04-25T12:13:10Z

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

+    }
+  }
+
+  static final class StreamFlatMapCollect<T, U, R> {


As Stream#collect and Stream#reduce both have two (or three) overloads, we could consider to add an extra type suffix, but we can also defer till at some point we have the name generator 😋.

Up to you 🙃

rickie · 2023-04-25T12:20:04Z

Well, okay let's leave it for now. Just wanted to make sure we are aware of the choice we were implicitly making 😄.

rickie · 2023-04-25T14:10:43Z

Pinged @werli on Slack and I made some suggestions to the commit message from the PR description.

werli

Nothing to add from my side after @Stephan202 & @rickie's suggestions. 👍

Thx @mohamedsamehsalah 💪

…om input tests for java.util.stream.Collectors while the mvn fmt:format does not :passive-aggressive-smile:

rickie · 2023-04-26T06:05:37Z

Rebased, will merge once 🟢 .

github-actions · 2023-04-26T06:07:13Z

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.

sonarqubecloud · 2023-04-26T06:07:30Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

mohamedsamehsalah added this to the 0.10.0 milestone Apr 23, 2023

mohamedsamehsalah linked an issue Apr 23, 2023 that may be closed by this pull request

Refaster rule that replaces redundant Stream#collect(..) calls #578

Closed

3 tasks

mohamedsamehsalah commented Apr 23, 2023

View reviewed changes

mohamedsamehsalah requested review from Stephan202, werli and rickie April 23, 2023 22:01

Stephan202 approved these changes Apr 24, 2023

View reviewed changes

rickie added the new feature label Apr 24, 2023

Stephan202 force-pushed the 578-refaster-rule-that-replaces-redundant-streamcollect-calls branch from 78e60cf to b8b6adc Compare April 25, 2023 06:34

Stephan202 changed the title ~~578-refaster-rule-that-replaces-redundant-streamcollect-calls~~ Add additional Refaster rules to StreamRules Apr 25, 2023

Stephan202 force-pushed the 578-refaster-rule-that-replaces-redundant-streamcollect-calls branch from b8b6adc to 653403b Compare April 25, 2023 09:38

Stephan202 reviewed Apr 25, 2023

View reviewed changes

rickie reviewed Apr 25, 2023

View reviewed changes

rickie approved these changes Apr 25, 2023

View reviewed changes

werli approved these changes Apr 25, 2023

View reviewed changes

mohamedsamehsalah and others added 4 commits April 26, 2023 08:05

578-refaster-rule-that-replaces-redundant-streamcollect-calls

3fe9b10

578 Fix issue where output test (parser?) expects the same imports fr…

f362d89

…om input tests for java.util.stream.Collectors while the mvn fmt:format does not :passive-aggressive-smile:

Suggestions

e9c07d4

Add new SonarCloud suppressions

19cf5b6

Stephan202 added 2 commits April 26, 2023 08:05

Forgot one

df3d2bf

Man man man

c826335

rickie force-pushed the 578-refaster-rule-that-replaces-redundant-streamcollect-calls branch from 9b6a0e6 to c826335 Compare April 26, 2023 06:05

rickie changed the title ~~Add additional Refaster rules to StreamRules~~ Extend StreamRules Refaster rule collection Apr 26, 2023

rickie merged commit 32d50ab into master Apr 26, 2023

rickie deleted the 578-refaster-rule-that-replaces-redundant-streamcollect-calls branch April 26, 2023 07:07

Extend StreamRules Refaster rule collection #593

Extend StreamRules Refaster rule collection #593

Conversation

mohamedsamehsalah commented Apr 23, 2023 • edited by rickie Loading

github-actions bot commented Apr 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedsamehsalah Apr 23, 2023 • 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

Stephan202 left a comment • edited by rickie 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

Choose a reason for hiding this comment

github-actions bot commented Apr 24, 2023

Stephan202 commented Apr 25, 2023

github-actions bot commented Apr 25, 2023

Stephan202 commented Apr 25, 2023

github-actions bot commented Apr 25, 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

github-actions bot commented Apr 25, 2023

github-actions bot commented Apr 25, 2023

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

rickie commented Apr 25, 2023

rickie commented Apr 25, 2023

werli left a comment

Choose a reason for hiding this comment

rickie commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

sonarqubecloud bot commented Apr 26, 2023

Extend `StreamRules` Refaster rule collection #593

Extend `StreamRules` Refaster rule collection #593

mohamedsamehsalah commented Apr 23, 2023 •

edited by rickie

Loading

mohamedsamehsalah Apr 23, 2023 •

edited

Loading

Stephan202 left a comment •

edited by rickie

Loading