Skip to content

Commit

Permalink
Introduce additional Refaster rules to ComparatorRules (#388)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Stephan202
Stephan202 authored Dec 7, 2022
1 parent ae327d8 commit d427e29
Show file tree
Hide file tree
Showing 4 changed files with 218 additions and 45 deletions.
129 changes: 105 additions & 24 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.google.errorprone.refaster.Refaster;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import com.google.errorprone.refaster.annotation.Repeated;
import com.google.errorprone.refaster.annotation.UseImportPolicy;
import java.util.Arrays;
import java.util.Collections;
Expand All @@ -24,6 +25,7 @@
import java.util.function.ToDoubleFunction;
import java.util.function.ToIntFunction;
import java.util.function.ToLongFunction;
import java.util.stream.Stream;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;

/** Refaster rules related to expressions dealing with {@link Comparator}s. */
Expand All @@ -37,7 +39,10 @@ static final class NaturalOrder<T extends Comparable<? super T>> {
@BeforeTemplate
Comparator<T> before() {
return Refaster.anyOf(
comparing(Refaster.anyOf(identity(), v -> v)), Comparator.<T>reverseOrder().reversed());
T::compareTo,
comparing(Refaster.anyOf(identity(), v -> v)),
Collections.<T>reverseOrder(reverseOrder()),
Comparator.<T>reverseOrder().reversed());
}

@AfterTemplate
Expand All @@ -51,7 +56,10 @@ Comparator<T> after() {
static final class ReverseOrder<T extends Comparable<? super T>> {
@BeforeTemplate
Comparator<T> before() {
return Comparator.<T>naturalOrder().reversed();
return Refaster.anyOf(
Collections.reverseOrder(),
Collections.<T>reverseOrder(naturalOrder()),
Comparator.<T>naturalOrder().reversed());
}

@AfterTemplate
Expand Down Expand Up @@ -189,15 +197,54 @@ Comparator<T> after(Comparator<T> cmp) {
}
}

/** Prefer {@link Comparable#compareTo(Object)}} over more verbose alternatives. */
static final class CompareTo<T extends Comparable<? super T>> {
@BeforeTemplate
int before(T value1, T value2) {
return Refaster.anyOf(
Comparator.<T>naturalOrder().compare(value1, value2),
Comparator.<T>reverseOrder().compare(value2, value1));
}

@AfterTemplate
int after(T value1, T value2) {
return value1.compareTo(value2);
}
}

/**
* Avoid unnecessary creation of a {@link Stream} to determine the minimum of a known collection
* of values.
*/
static final class MinOfVarargs<T> {
@BeforeTemplate
@SuppressWarnings("StreamOfArray" /* In practice individual values are provided. */)
T before(@Repeated T value, Comparator<T> cmp) {
return Stream.of(Refaster.asVarargs(value)).min(cmp).orElseThrow();
}

@AfterTemplate
T after(@Repeated T value, Comparator<T> cmp) {
return Collections.min(Arrays.asList(value), cmp);
}
}

/** Prefer {@link Comparators#min(Comparable, Comparable)}} over more verbose alternatives. */
static final class MinOfPairNaturalOrder<T extends Comparable<? super T>> {
@BeforeTemplate
T before(T value1, T value2) {
return Collections.min(
Refaster.anyOf(
Arrays.asList(value1, value2),
ImmutableList.of(value1, value2),
ImmutableSet.of(value1, value2)));
return Refaster.anyOf(
value1.compareTo(value2) <= 0 ? value1 : value2,
value1.compareTo(value2) > 0 ? value2 : value1,
value2.compareTo(value1) < 0 ? value2 : value1,
value2.compareTo(value1) >= 0 ? value1 : value2,
Comparators.min(value1, value2, naturalOrder()),
Comparators.max(value1, value2, reverseOrder()),
Collections.min(
Refaster.anyOf(
Arrays.asList(value1, value2),
ImmutableList.of(value1, value2),
ImmutableSet.of(value1, value2))));
}

@AfterTemplate
Expand All @@ -212,12 +259,17 @@ T after(T value1, T value2) {
static final class MinOfPairCustomOrder<T> {
@BeforeTemplate
T before(T value1, T value2, Comparator<T> cmp) {
return Collections.min(
Refaster.anyOf(
Arrays.asList(value1, value2),
ImmutableList.of(value1, value2),
ImmutableSet.of(value1, value2)),
cmp);
return Refaster.anyOf(
cmp.compare(value1, value2) <= 0 ? value1 : value2,
cmp.compare(value1, value2) > 0 ? value2 : value1,
cmp.compare(value2, value1) < 0 ? value2 : value1,
cmp.compare(value2, value1) >= 0 ? value1 : value2,
Collections.min(
Refaster.anyOf(
Arrays.asList(value1, value2),
ImmutableList.of(value1, value2),
ImmutableSet.of(value1, value2)),
cmp));
}

@AfterTemplate
Expand All @@ -226,15 +278,39 @@ T after(T value1, T value2, Comparator<T> cmp) {
}
}

/**
* Avoid unnecessary creation of a {@link Stream} to determine the maximum of a known collection
* of values.
*/
static final class MaxOfVarargs<T> {
@BeforeTemplate
@SuppressWarnings("StreamOfArray" /* In practice individual values are provided. */)
T before(@Repeated T value, Comparator<T> cmp) {
return Stream.of(Refaster.asVarargs(value)).max(cmp).orElseThrow();
}

@AfterTemplate
T after(@Repeated T value, Comparator<T> cmp) {
return Collections.max(Arrays.asList(value), cmp);
}
}

/** Prefer {@link Comparators#max(Comparable, Comparable)}} over more verbose alternatives. */
static final class MaxOfPairNaturalOrder<T extends Comparable<? super T>> {
@BeforeTemplate
T before(T value1, T value2) {
return Collections.max(
Refaster.anyOf(
Arrays.asList(value1, value2),
ImmutableList.of(value1, value2),
ImmutableSet.of(value1, value2)));
return Refaster.anyOf(
value1.compareTo(value2) >= 0 ? value1 : value2,
value1.compareTo(value2) < 0 ? value2 : value1,
value2.compareTo(value1) > 0 ? value2 : value1,
value2.compareTo(value1) <= 0 ? value1 : value2,
Comparators.max(value1, value2, naturalOrder()),
Comparators.min(value1, value2, reverseOrder()),
Collections.max(
Refaster.anyOf(
Arrays.asList(value1, value2),
ImmutableList.of(value1, value2),
ImmutableSet.of(value1, value2))));
}

@AfterTemplate
Expand All @@ -249,12 +325,17 @@ T after(T value1, T value2) {
static final class MaxOfPairCustomOrder<T> {
@BeforeTemplate
T before(T value1, T value2, Comparator<T> cmp) {
return Collections.max(
Refaster.anyOf(
Arrays.asList(value1, value2),
ImmutableList.of(value1, value2),
ImmutableSet.of(value1, value2)),
cmp);
return Refaster.anyOf(
cmp.compare(value1, value2) >= 0 ? value1 : value2,
cmp.compare(value1, value2) < 0 ? value2 : value1,
cmp.compare(value2, value1) > 0 ? value2 : value1,
cmp.compare(value2, value1) <= 0 ? value1 : value2,
Collections.max(
Refaster.anyOf(
Arrays.asList(value1, value2),
ImmutableList.of(value1, value2),
ImmutableSet.of(value1, value2)),
cmp));
}

@AfterTemplate
Expand Down
3 changes: 3 additions & 0 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ Stream<T> after(T object) {
* Prefer {@link Arrays#stream(Object[])} over {@link Stream#of(Object[])}, as the former is
* clearer.
*/
// XXX: Introduce a `Matcher` that identifies `Refaster.asVarargs(...)` invocations and annotate
// the `array` parameter as `@NotMatches(IsRefasterAsVarargs.class)`. Then elsewhere
// `@SuppressWarnings("StreamOfArray")` annotations can be dropped.
static final class StreamOfArray<T> {
@BeforeTemplate
Stream<T> before(T[] array) {
Expand Down
64 changes: 55 additions & 9 deletions ...rib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,30 +4,42 @@
import static java.util.Comparator.reverseOrder;
import static java.util.function.Function.identity;

import com.google.common.collect.Comparators;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.function.BinaryOperator;
import java.util.stream.Stream;
import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;

final class ComparatorRulesTest implements RefasterRuleCollectionTestCase {
@Override
public ImmutableSet<?> elidedTypesAndStaticImports() {
return ImmutableSet.of(
Arrays.class, Collections.class, ImmutableList.class, ImmutableSet.class, identity());
Arrays.class,
Collections.class,
ImmutableList.class,
ImmutableSet.class,
Stream.class,
identity());
}

ImmutableSet<Comparator<String>> testNaturalOrder() {
return ImmutableSet.of(
String::compareTo,
Comparator.comparing(identity()),
Comparator.comparing(s -> s),
Collections.<String>reverseOrder(reverseOrder()),
Comparator.<String>reverseOrder().reversed());
}

Comparator<String> testReverseOrder() {
return Comparator.<String>naturalOrder().reversed();
ImmutableSet<Comparator<String>> testReverseOrder() {
return ImmutableSet.of(
Collections.reverseOrder(),
Collections.<String>reverseOrder(naturalOrder()),
Comparator.<String>naturalOrder().reversed());
}

ImmutableSet<Comparator<String>> testCustomComparator() {
Expand Down Expand Up @@ -77,32 +89,66 @@ ImmutableSet<Comparator<String>> testThenComparingNaturalOrder() {
Comparator.<String>naturalOrder().thenComparing(s -> s));
}

ImmutableSet<Integer> testCompareTo() {
return ImmutableSet.of(
Comparator.<String>naturalOrder().compare("foo", "bar"),
Comparator.<String>reverseOrder().compare("baz", "qux"));
}

int testMinOfVarargs() {
return Stream.of(1, 2).min(naturalOrder()).orElseThrow();
}

ImmutableSet<String> testMinOfPairNaturalOrder() {
return ImmutableSet.of(
"a".compareTo("b") <= 0 ? "a" : "b",
"a".compareTo("b") > 0 ? "b" : "a",
"a".compareTo("b") < 0 ? "a" : "b",
"a".compareTo("b") >= 0 ? "b" : "a",
Comparators.min("a", "b", naturalOrder()),
Comparators.max("a", "b", reverseOrder()),
Collections.min(Arrays.asList("a", "b")),
Collections.min(ImmutableList.of("a", "b")),
Collections.min(ImmutableSet.of("a", "b")));
}

ImmutableSet<Object> testMinOfPairCustomOrder() {
return ImmutableSet.of(
Collections.min(Arrays.asList(new Object(), new Object()), (a, b) -> -1),
Collections.min(ImmutableList.of(new Object(), new Object()), (a, b) -> 0),
Collections.min(ImmutableSet.of(new Object(), new Object()), (a, b) -> 1));
Comparator.comparingInt(String::length).compare("a", "b") <= 0 ? "a" : "b",
Comparator.comparingInt(String::length).compare("a", "b") > 0 ? "b" : "a",
Comparator.comparingInt(String::length).compare("a", "b") < 0 ? "a" : "b",
Comparator.comparingInt(String::length).compare("a", "b") >= 0 ? "b" : "a",
Collections.min(Arrays.asList("a", "b"), (a, b) -> -1),
Collections.min(ImmutableList.of("a", "b"), (a, b) -> 0),
Collections.min(ImmutableSet.of("a", "b"), (a, b) -> 1));
}

int testMaxOfVarargs() {
return Stream.of(1, 2).max(naturalOrder()).orElseThrow();
}

ImmutableSet<String> testMaxOfPairNaturalOrder() {
return ImmutableSet.of(
"a".compareTo("b") >= 0 ? "a" : "b",
"a".compareTo("b") < 0 ? "b" : "a",
"a".compareTo("b") > 0 ? "a" : "b",
"a".compareTo("b") <= 0 ? "b" : "a",
Comparators.max("a", "b", naturalOrder()),
Comparators.min("a", "b", reverseOrder()),
Collections.max(Arrays.asList("a", "b")),
Collections.max(ImmutableList.of("a", "b")),
Collections.max(ImmutableSet.of("a", "b")));
}

ImmutableSet<Object> testMaxOfPairCustomOrder() {
return ImmutableSet.of(
Collections.max(Arrays.asList(new Object(), new Object()), (a, b) -> -1),
Collections.max(ImmutableList.of(new Object(), new Object()), (a, b) -> 0),
Collections.max(ImmutableSet.of(new Object(), new Object()), (a, b) -> 1));
Comparator.comparingInt(String::length).compare("a", "b") >= 0 ? "a" : "b",
Comparator.comparingInt(String::length).compare("a", "b") < 0 ? "b" : "a",
Comparator.comparingInt(String::length).compare("a", "b") > 0 ? "a" : "b",
Comparator.comparingInt(String::length).compare("a", "b") <= 0 ? "b" : "a",
Collections.max(Arrays.asList("a", "b"), (a, b) -> -1),
Collections.max(ImmutableList.of("a", "b"), (a, b) -> 0),
Collections.max(ImmutableSet.of("a", "b"), (a, b) -> 1));
}

BinaryOperator<String> testComparatorsMin() {
Expand Down
Loading

0 comments on commit d427e29

Please sign in to comment.