RSPEC-4034 - "Stream" call chains should be simplified when possible

[yeikel]

 private static boolean isTypeAllowed(String type) {
        return Set.of("types").stream().filter(type::contains).collect(Collectors.toList()).isEmpty();
    }

 private static boolean isTypeAllowed(String type) {
        return Set.of("types").stream().stream().filter(type::contains).count() == 0
    }

 private static boolean isTypeAllowed(String type) {
        return Set.of("types").stream().noneMatch(type::contains);
    }

More examples : https://rules.sonarsource.com/java/RSPEC-4034

[timtebeek]

This sounds like a good candidate for the more refaster-like before/after templates that Knut is working on.

[yeikel]

Another example :

-                        .filter(e -> {
+                        .anyMatch(e -> {
                             return true;
-                        })
-                        .findFirst()
-                        .isPresent());
+                        }));

[knutwannheden]

This sounds like a good candidate for the more refaster-line before/after templates that Knut is working on.

Indeed. At the moment the Refaster integration doesn't yet offer support for parameters using generics, otherwise something like this would be possible:

    @BeforeTemplate
    <T> boolean before(Stream<T> stream, Predicate<T> predicate) {
        return stream.filter(predicate).collect(Collectors.toList()).isEmpty();
    }

    @AfterTemplate
    <T> boolean after(Stream<T> stream, Predicate<T> predidcate) {
        return stream.noneMatch(predidcate);
    }

or possibly even like this (not sure what Refaster would do here, as the third parameter isn't referenced in the after template):

    @BeforeTemplate
    <T> boolean before(Stream<T> stream, Predicate<T> predidcate, Collector<? super T, ?, ? extends Collection<T>> collector) {
        return stream.filter(predidcate).collect(collector).isEmpty();
    }

    @AfterTemplate
    <T> boolean after(Stream<T> stream, Predicate<T> predidcate, Collector<? super T, ?, ? extends Collection<T>> collector) {
        return stream.noneMatch(predidcate);
    }

[Stephan202]

not sure what Refaster would do here, as the third parameter isn't referenced in the after template

That's okay; it works. Error Prone Support has a Refaster test framework to validate this, so I filed PicnicSupermarket/error-prone-support#605. (This specific case is covered by the combination of StreamIsEmpty and StreamNoneMatch.)

[knutwannheden]

Looks great and thanks for explaining what Refaster does with situations like this one.

[knutwannheden]

I tested your templates and it works beautifully. I was a bit surprised that Refaster apparently only applies the rules one at the time. So when I started out with the Set.of("types").stream().filter(type::contains).collect(Collectors.toList()).isEmpty() from the description it took me two compilation attempts to reach the desired output. This is an area where OpenRewrite could offer an advantage.

After that second step error-prone-support also proposes a third one, which I think doesn't really add any value and wouldn't be expected in projects not using Guava. It wants me to change Set.of("types").stream().noneMatch(type::contains) to ImmutableSet.of("types").stream().noneMatch(type::contains). But since the Set#of() factory methods already return immutable collections, that doesn't really offer any advantage. But this is a detail really and I suppose I could as a user easily configure that I am not interested in Guava-related checks.

[Stephan202]

I tested your templates and it works beautifully.

:tada:

I was a bit surprised that Refaster apparently only applies the rules one at the time.

Indeed, this is comparatively costly, more so because we try to define "atomic" rewrite rules. I have seen cases where a conversion from one common type of expression to another got decomposed into four more generic rules. Picnic-internally we have a patch.sh script that runs this kind of operation until a fixed point.

But this is a detail really and I suppose I could as a user easily configure that I am not interested in Guava-related checks.

Indeed! In some cases the Refaster rules encode Picnic-internal guidelines, though we do try to strike a balance. For Refaster we allow name-based exclusions, and there are separate rules for Guava types; so skipping these is possible (the website shows the relevant regex for each rule; combining them is left as an exercise to the reader :upside_down_face:). And I imagine that an OpenRewrite bridge would offer similar functionality.

NB: Some of our Error Prone bug checkers use this class to control whether to trigger or what kind of fix to suggest, depending on what's on the classpath. Something I've also been thinking of adding to our Refaster wrapper. But... time :sweat_smile: