Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend StreamRules Refaster rule collection #593

Merged
merged 6 commits into from
Apr 26, 2023
Merged

Extend StreamRules Refaster rule collection #593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3fe9b10
578-refaster-rule-that-replaces-redundant-streamcollect-calls
Apr 23, 2023
f362d89
578 Fix issue where output test (parser?) expects the same imports fr…
Apr 23, 2023
e9c07d4
Suggestions
Stephan202 Apr 24, 2023
19cf5b6
Add new SonarCloud suppressions
Stephan202 Apr 25, 2023
df3d2bf
Forgot one
Stephan202 Apr 25, 2023
c826335
Man man man
Stephan202 Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 172 additions & 5 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,20 @@
import static java.util.Comparator.naturalOrder;
import static java.util.Comparator.reverseOrder;
import static java.util.function.Predicate.not;
import static java.util.stream.Collectors.counting;
import static java.util.stream.Collectors.filtering;
import static java.util.stream.Collectors.flatMapping;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.mapping;
import static java.util.stream.Collectors.maxBy;
import static java.util.stream.Collectors.minBy;
import static java.util.stream.Collectors.reducing;
import static java.util.stream.Collectors.summarizingDouble;
import static java.util.stream.Collectors.summarizingInt;
import static java.util.stream.Collectors.summarizingLong;
import static java.util.stream.Collectors.summingDouble;
import static java.util.stream.Collectors.summingInt;
import static java.util.stream.Collectors.summingLong;

import com.google.common.collect.Streams;
import com.google.errorprone.refaster.Refaster;
Expand All @@ -16,8 +29,12 @@
import com.google.errorprone.refaster.annotation.UseImportPolicy;
import java.util.Arrays;
import java.util.Comparator;
import java.util.DoubleSummaryStatistics;
import java.util.IntSummaryStatistics;
import java.util.LongSummaryStatistics;
import java.util.Objects;
import java.util.Optional;
import java.util.function.BinaryOperator;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.function.ToDoubleFunction;
Expand Down Expand Up @@ -263,9 +280,12 @@ boolean after(Stream<T> stream) {

static final class StreamMin<T> {
@BeforeTemplate
@SuppressWarnings("java:S4266" /* This violation will be rewritten. */)
Optional<T> before(Stream<T> stream, Comparator<? super T> comparator) {
return Refaster.anyOf(
stream.max(comparator.reversed()), stream.sorted(comparator).findFirst());
stream.max(comparator.reversed()),
stream.sorted(comparator).findFirst(),
stream.collect(minBy(comparator)));
}

@AfterTemplate
Expand All @@ -289,9 +309,12 @@ Optional<T> after(Stream<T> stream) {

static final class StreamMax<T> {
@BeforeTemplate
@SuppressWarnings("java:S4266" /* This violation will be rewritten. */)
Optional<T> before(Stream<T> stream, Comparator<? super T> comparator) {
return Refaster.anyOf(
stream.min(comparator.reversed()), Streams.findLast(stream.sorted(comparator)));
stream.min(comparator.reversed()),
Streams.findLast(stream.sorted(comparator)),
stream.collect(maxBy(comparator)));
}

@AfterTemplate
Expand Down Expand Up @@ -389,7 +412,13 @@ boolean after(Stream<T> stream) {

static final class StreamMapToIntSum<T> {
@BeforeTemplate
int before(
@SuppressWarnings("java:S4266" /* This violation will be rewritten. */)
long before(Stream<T> stream, ToIntFunction<T> mapper) {
return stream.collect(summingInt(mapper));
}

@BeforeTemplate
int before2(
Stream<T> stream,
@Matches(IsLambdaExpressionOrMethodReference.class) Function<? super T, Integer> mapper) {
return stream.map(mapper).reduce(0, Integer::sum);
Expand All @@ -403,7 +432,13 @@ int after(Stream<T> stream, ToIntFunction<T> mapper) {

static final class StreamMapToDoubleSum<T> {
@BeforeTemplate
double before(
@SuppressWarnings("java:S4266" /* This violation will be rewritten. */)
double before(Stream<T> stream, ToDoubleFunction<T> mapper) {
return stream.collect(summingDouble(mapper));
}

@BeforeTemplate
double before2(
Stream<T> stream,
@Matches(IsLambdaExpressionOrMethodReference.class) Function<? super T, Double> mapper) {
return stream.map(mapper).reduce(0.0, Double::sum);
Expand All @@ -417,7 +452,13 @@ static final class StreamMapToDoubleSum<T> {

static final class StreamMapToLongSum<T> {
@BeforeTemplate
long before(
@SuppressWarnings("java:S4266" /* This violation will be rewritten. */)
long before(Stream<T> stream, ToLongFunction<T> mapper) {
return stream.collect(summingLong(mapper));
}

@BeforeTemplate
long before2(
Stream<T> stream,
@Matches(IsLambdaExpressionOrMethodReference.class) Function<? super T, Long> mapper) {
return stream.map(mapper).reduce(0L, Long::sum);
Expand All @@ -428,4 +469,130 @@ long after(Stream<T> stream, ToLongFunction<T> mapper) {
return stream.mapToLong(mapper).sum();
}
}

static final class StreamMapToIntSummaryStatistics<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@BeforeTemplate
IntSummaryStatistics before(Stream<T> stream, ToIntFunction<T> mapper) {
return stream.collect(summarizingInt(mapper));
}

@AfterTemplate
IntSummaryStatistics after(Stream<T> stream, ToIntFunction<T> mapper) {
return stream.mapToInt(mapper).summaryStatistics();
}
}

static final class StreamMapToDoubleSummaryStatistics<T> {
@BeforeTemplate
DoubleSummaryStatistics before(Stream<T> stream, ToDoubleFunction<T> mapper) {
return stream.collect(summarizingDouble(mapper));
}

@AfterTemplate
DoubleSummaryStatistics after(Stream<T> stream, ToDoubleFunction<T> mapper) {
return stream.mapToDouble(mapper).summaryStatistics();
}
}

static final class StreamMapToLongSummaryStatistics<T> {
@BeforeTemplate
LongSummaryStatistics before(Stream<T> stream, ToLongFunction<T> mapper) {
return stream.collect(summarizingLong(mapper));
}

@AfterTemplate
LongSummaryStatistics after(Stream<T> stream, ToLongFunction<T> mapper) {
return stream.mapToLong(mapper).summaryStatistics();
}
}

static final class StreamCount<T> {
@BeforeTemplate
@SuppressWarnings("java:S4266" /* This violation will be rewritten. */)
long before(Stream<T> stream) {
return stream.collect(counting());
}

@AfterTemplate
long after(Stream<T> stream) {
return stream.count();
}
}

static final class StreamReduce<T> {
@BeforeTemplate
@SuppressWarnings("java:S4266" /* This violation will be rewritten. */)
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);
}
}

static final class StreamReduceWithIdentity<T> {
@BeforeTemplate
@SuppressWarnings("java:S4266" /* This violation will be rewritten. */)
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 StreamFilterCollect<T, R> {
@BeforeTemplate
R before(
Stream<T> stream, Predicate<? super T> predicate, Collector<? super T, ?, R> collector) {
return stream.collect(filtering(predicate, collector));
}

@AfterTemplate
R after(
Stream<T> stream, Predicate<? super T> predicate, Collector<? super T, ?, R> collector) {
return stream.filter(predicate).collect(collector);
}
}

static final class StreamMapCollect<T, U, R> {
@BeforeTemplate
@SuppressWarnings("java:S4266" /* This violation will be rewritten. */)
R before(
Stream<T> stream,
Function<? super T, ? extends U> mapper,
Collector<? super U, ?, R> collector) {
return stream.collect(mapping(mapper, collector));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@AfterTemplate
R after(
Stream<T> stream,
Function<? super T, ? extends U> mapper,
Collector<? super U, ?, R> collector) {
return stream.map(mapper).collect(collector);
}
}

static final class StreamFlatMapCollect<T, U, R> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😋.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you 🙃

@BeforeTemplate
R before(
Stream<T> stream,
Function<? super T, ? extends Stream<? extends U>> mapper,
Collector<? super U, ?, R> collector) {
return stream.collect(flatMapping(mapper, collector));
}

@AfterTemplate
R after(
Stream<T> stream,
Function<? super T, ? extends Stream<? extends U>> mapper,
Collector<? super U, ?, R> collector) {
return stream.flatMap(mapper).collect(collector);
}
}
}
98 changes: 86 additions & 12 deletions ...contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,29 @@
package tech.picnic.errorprone.refasterrules;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.util.Comparator.comparingInt;
import static java.util.Comparator.reverseOrder;
import static java.util.function.Predicate.not;
import static java.util.stream.Collectors.counting;
import static java.util.stream.Collectors.filtering;
import static java.util.stream.Collectors.flatMapping;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.mapping;
import static java.util.stream.Collectors.maxBy;
import static java.util.stream.Collectors.minBy;
import static java.util.stream.Collectors.reducing;
import static java.util.stream.Collectors.summarizingDouble;
import static java.util.stream.Collectors.summarizingInt;
import static java.util.stream.Collectors.summarizingLong;
import static java.util.stream.Collectors.summingDouble;
import static java.util.stream.Collectors.summingInt;
import static java.util.stream.Collectors.summingLong;

import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import java.util.DoubleSummaryStatistics;
import java.util.IntSummaryStatistics;
import java.util.LongSummaryStatistics;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Function;
Expand All @@ -17,7 +34,23 @@
final class StreamRulesTest implements RefasterRuleCollectionTestCase {
@Override
public ImmutableSet<Object> elidedTypesAndStaticImports() {
return ImmutableSet.of(Objects.class, Streams.class, not(null));
return ImmutableSet.of(
Objects.class,
Streams.class,
counting(),
filtering(null, null),
flatMapping(null, null),
mapping(null, null),
maxBy(null),
minBy(null),
not(null),
reducing(null),
summarizingDouble(null),
summarizingInt(null),
summarizingLong(null),
summingDouble(null),
summingInt(null),
summingLong(null));
}

String testJoining() {
Expand Down Expand Up @@ -90,7 +123,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(minBy(comparingInt(String::length))));
}

ImmutableSet<Optional<String>> testStreamMinNaturalOrder() {
Expand All @@ -101,7 +135,8 @@ ImmutableSet<Optional<String>> testStreamMinNaturalOrder() {
ImmutableSet<Optional<String>> testStreamMax() {
return ImmutableSet.of(
Stream.of("foo").min(comparingInt(String::length).reversed()),
Streams.findLast(Stream.of("bar").sorted(comparingInt(String::length))));
Streams.findLast(Stream.of("bar").sorted(comparingInt(String::length))),
Stream.of("baz").collect(maxBy(comparingInt(String::length))));
}

ImmutableSet<Optional<String>> testStreamMaxNaturalOrder() {
Expand Down Expand Up @@ -143,24 +178,63 @@ boolean testStreamAllMatch2() {
ImmutableSet<Integer> testStreamMapToIntSum() {
Function<String, Integer> parseIntFunction = Integer::parseInt;
return ImmutableSet.of(
Stream.of(1).map(i -> i * 2).reduce(0, Integer::sum),
Stream.of("2").map(Integer::parseInt).reduce(0, Integer::sum),
Stream.of("3").map(parseIntFunction).reduce(0, Integer::sum));
Stream.of("1").collect(summingInt(Integer::parseInt)),
Stream.of(2).map(i -> i * 2).reduce(0, Integer::sum),
Stream.of("3").map(Integer::parseInt).reduce(0, Integer::sum),
Stream.of("4").map(parseIntFunction).reduce(0, Integer::sum));
}

ImmutableSet<Double> testStreamMapToDoubleSum() {
Function<String, Double> parseDoubleFunction = Double::parseDouble;
return ImmutableSet.of(
Stream.of(1).map(i -> i * 2.0).reduce(0.0, Double::sum),
Stream.of("2").map(Double::parseDouble).reduce(0.0, Double::sum),
Stream.of("3").map(parseDoubleFunction).reduce(0.0, Double::sum));
Stream.of("1").collect(summingDouble(Double::parseDouble)),
Stream.of(2).map(i -> i * 2.0).reduce(0.0, Double::sum),
Stream.of("3").map(Double::parseDouble).reduce(0.0, Double::sum),
Stream.of("4").map(parseDoubleFunction).reduce(0.0, Double::sum));
}

ImmutableSet<Long> testStreamMapToLongSum() {
Function<String, Long> parseLongFunction = Long::parseLong;
return ImmutableSet.of(
Stream.of(1).map(i -> i * 2L).reduce(0L, Long::sum),
Stream.of("2").map(Long::parseLong).reduce(0L, Long::sum),
Stream.of("3").map(parseLongFunction).reduce(0L, Long::sum));
Stream.of("1").collect(summingLong(Long::parseLong)),
Stream.of(2).map(i -> i * 2L).reduce(0L, Long::sum),
Stream.of("3").map(Long::parseLong).reduce(0L, Long::sum),
Stream.of("4").map(parseLongFunction).reduce(0L, Long::sum));
}

IntSummaryStatistics testStreamMapToIntSummaryStatistics() {
return Stream.of("1").collect(summarizingInt(Integer::parseInt));
}

DoubleSummaryStatistics testStreamMapToDoubleSummaryStatistics() {
return Stream.of("1").collect(summarizingDouble(Double::parseDouble));
}

LongSummaryStatistics testStreamMapToLongSummaryStatistics() {
return Stream.of("1").collect(summarizingLong(Long::parseLong));
}

Long testStreamCount() {
return Stream.of(1).collect(counting());
}

Optional<Integer> testStreamReduce() {
return Stream.of(1).collect(reducing(Integer::sum));
}

Integer testStreamReduceWithIdentity() {
return Stream.of(1).collect(reducing(0, Integer::sum));
}

ImmutableSet<String> testStreamFilterCollect() {
return Stream.of("1").collect(filtering(String::isEmpty, toImmutableSet()));
}

ImmutableSet<Integer> testStreamMapCollect() {
return Stream.of("1").collect(mapping(Integer::parseInt, toImmutableSet()));
}

ImmutableSet<Integer> testStreamFlatMapCollect() {
return Stream.of(1).collect(flatMapping(n -> Stream.of(n, n), toImmutableSet()));
}
}
Loading