Skip to content

Commit

Permalink
Extend ImmutableMapRules Refaster rule collection (#1488)
Browse files Browse the repository at this point in the history 
Resolves #1223.
  • Loading branch information
@Stephan202
Stephan202 authored Jan 3, 2025
1 parent 9493f2d commit 19d3ba0
Show file tree
Hide file tree
Showing 8 changed files with 273 additions and 76 deletions.
2 changes: 1 addition & 1 deletion ...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpringMvcAnnotation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public final class SpringMvcAnnotation extends BugChecker implements AnnotationT
.put("PATCH", "PatchMapping")
.put("POST", "PostMapping")
.put("PUT", "PutMapping")
.build();
.buildOrThrow();

/** Instantiates a new {@link SpringMvcAnnotation} instance. */
public SpringMvcAnnotation() {}
Expand Down
76 changes: 64 additions & 12 deletions ...r-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.google.errorprone.refaster.annotation.Matches;
import com.google.errorprone.refaster.annotation.MayOptionallyUse;
import com.google.errorprone.refaster.annotation.Placeholder;
import com.google.errorprone.refaster.annotation.Repeated;
import com.google.errorprone.refaster.annotation.UseImportPolicy;
import java.util.Collection;
import java.util.Iterator;
Expand Down Expand Up @@ -44,12 +45,28 @@ ImmutableMap.Builder<K, V> after() {
}
}

/**
* Prefer {@link ImmutableMap.Builder#buildOrThrow()} over the less explicit {@link
* ImmutableMap.Builder#build()}.
*/
static final class ImmutableMapBuilderBuildOrThrow<K, V> {
@BeforeTemplate
ImmutableMap<K, V> before(ImmutableMap.Builder<K, V> builder) {
return builder.build();
}

@AfterTemplate
ImmutableMap<K, V> after(ImmutableMap.Builder<K, V> builder) {
return builder.buildOrThrow();
}
}

/** Prefer {@link ImmutableMap#of(Object, Object)} over more contrived alternatives. */
static final class EntryToImmutableMap<K, V> {
@BeforeTemplate
ImmutableMap<K, V> before(Map.Entry<? extends K, ? extends V> entry) {
return Refaster.anyOf(
ImmutableMap.<K, V>builder().put(entry).build(),
ImmutableMap.<K, V>builder().put(entry).buildOrThrow(),
Stream.of(entry).collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)));
}

Expand Down Expand Up @@ -104,16 +121,17 @@ ImmutableMap<K, V> after(
/** Prefer {@link ImmutableMap#copyOf(Iterable)} over more contrived alternatives. */
static final class EntryIterableToImmutableMap<K, V> {
@BeforeTemplate
ImmutableMap<K, V> before(Map<? extends K, ? extends V> iterable) {
Map<K, V> before(Map<? extends K, ? extends V> iterable) {
return Refaster.anyOf(
ImmutableMap.copyOf(iterable.entrySet()),
ImmutableMap.<K, V>builder().putAll(iterable).build());
ImmutableMap.<K, V>builder().putAll(iterable).buildOrThrow(),
Map.copyOf(iterable));
}

@BeforeTemplate
ImmutableMap<K, V> before(Iterable<? extends Map.Entry<? extends K, ? extends V>> iterable) {
return Refaster.anyOf(
ImmutableMap.<K, V>builder().putAll(iterable).build(),
ImmutableMap.<K, V>builder().putAll(iterable).buildOrThrow(),
Streams.stream(iterable).collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)));
}

Expand All @@ -139,8 +157,6 @@ abstract static class StreamOfMapEntriesToImmutableMap<E, K, V> {
@Placeholder(allowsIdentity = true)
abstract V valueFunction(@MayOptionallyUse E element);

// XXX: We could add variants in which the entry is created some other way, but we have another
// rule that covers canonicalization to `Map.entry`.
@BeforeTemplate
ImmutableMap<K, V> before(Stream<E> stream) {
return stream
Expand Down Expand Up @@ -224,7 +240,11 @@ ImmutableMap<K, V2> after(Map<K, V1> map) {
static final class ImmutableMapOf<K, V> {
@BeforeTemplate
Map<K, V> before() {
return Refaster.anyOf(ImmutableMap.<K, V>builder().build(), emptyMap(), Map.of());
return Refaster.anyOf(
ImmutableMap.<K, V>builder().buildOrThrow(),
ImmutableMap.ofEntries(),
emptyMap(),
Map.of());
}

@AfterTemplate
Expand All @@ -243,7 +263,10 @@ static final class ImmutableMapOf1<K, V> {
@BeforeTemplate
Map<K, V> before(K k1, V v1) {
return Refaster.anyOf(
ImmutableMap.<K, V>builder().put(k1, v1).build(), singletonMap(k1, v1), Map.of(k1, v1));
ImmutableMap.<K, V>builder().put(k1, v1).buildOrThrow(),
ImmutableMap.ofEntries(Map.entry(k1, v1)),
singletonMap(k1, v1),
Map.of(k1, v1));
}

@AfterTemplate
Expand All @@ -261,7 +284,8 @@ ImmutableMap<K, V> after(K k1, V v1) {
static final class ImmutableMapOf2<K, V> {
@BeforeTemplate
Map<K, V> before(K k1, V v1, K k2, V v2) {
return Map.of(k1, v1, k2, v2);
return Refaster.anyOf(
ImmutableMap.ofEntries(Map.entry(k1, v1), Map.entry(k2, v2)), Map.of(k1, v1, k2, v2));
}

@AfterTemplate
Expand All @@ -279,7 +303,9 @@ ImmutableMap<K, V> after(K k1, V v1, K k2, V v2) {
static final class ImmutableMapOf3<K, V> {
@BeforeTemplate
Map<K, V> before(K k1, V v1, K k2, V v2, K k3, V v3) {
return Map.of(k1, v1, k2, v2, k3, v3);
return Refaster.anyOf(
ImmutableMap.ofEntries(Map.entry(k1, v1), Map.entry(k2, v2), Map.entry(k3, v3)),
Map.of(k1, v1, k2, v2, k3, v3));
}

@AfterTemplate
Expand All @@ -299,7 +325,10 @@ ImmutableMap<K, V> after(K k1, V v1, K k2, V v2, K k3, V v3) {
static final class ImmutableMapOf4<K, V> {
@BeforeTemplate
Map<K, V> before(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4) {
return Map.of(k1, v1, k2, v2, k3, v3, k4, v4);
return Refaster.anyOf(
ImmutableMap.ofEntries(
Map.entry(k1, v1), Map.entry(k2, v2), Map.entry(k3, v3), Map.entry(k4, v4)),
Map.of(k1, v1, k2, v2, k3, v3, k4, v4));
}

@AfterTemplate
Expand All @@ -319,7 +348,14 @@ ImmutableMap<K, V> after(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4) {
static final class ImmutableMapOf5<K, V> {
@BeforeTemplate
Map<K, V> before(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4, K k5, V v5) {
return Map.of(k1, v1, k2, v2, k3, v3, k4, v4, k5, v5);
return Refaster.anyOf(
ImmutableMap.ofEntries(
Map.entry(k1, v1),
Map.entry(k2, v2),
Map.entry(k3, v3),
Map.entry(k4, v4),
Map.entry(k5, v5)),
Map.of(k1, v1, k2, v2, k3, v3, k4, v4, k5, v5));
}

@AfterTemplate
Expand Down Expand Up @@ -370,6 +406,22 @@ ImmutableMap<K, V> after(Map<K, V> map) {
}
}

/**
* Prefer {@link ImmutableMap#ofEntries(Map.Entry[])} over alternatives that don't communicate the
* immutability of the resulting map at the type level.
*/
static final class ImmutableMapOfEntries<K, V> {
@BeforeTemplate
Map<K, V> before(@Repeated Map.Entry<? extends K, ? extends V> entries) {
return Map.ofEntries(entries);
}

@AfterTemplate
ImmutableMap<K, V> after(@Repeated Map.Entry<? extends K, ? extends V> entries) {
return ImmutableMap.ofEntries(entries);
}
}

// XXX: Add a rule for this:
// Maps.transformValues(streamOfEntries.collect(groupBy(fun)), ImmutableMap::copyOf)
// ->
Expand Down
10 changes: 5 additions & 5 deletions ...e-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ ImmutableSortedMap.Builder<K, V> after() {
static final class EmptyImmutableSortedMap<K extends Comparable<? super K>, V> {
@BeforeTemplate
ImmutableSortedMap<K, V> before() {
return ImmutableSortedMap.<K, V>naturalOrder().build();
return ImmutableSortedMap.<K, V>naturalOrder().buildOrThrow();
}

@AfterTemplate
Expand All @@ -89,7 +89,7 @@ ImmutableSortedMap<K, V> after() {
static final class PairToImmutableSortedMap<K extends Comparable<? super K>, V> {
@BeforeTemplate
ImmutableSortedMap<K, V> before(K key, V value) {
return ImmutableSortedMap.<K, V>naturalOrder().put(key, value).build();
return ImmutableSortedMap.<K, V>naturalOrder().put(key, value).buildOrThrow();
}

@AfterTemplate
Expand All @@ -105,7 +105,7 @@ static final class EntryToImmutableSortedMap<K extends Comparable<? super K>, V>
@BeforeTemplate
ImmutableSortedMap<K, V> before(Map.Entry<? extends K, ? extends V> entry) {
return Refaster.anyOf(
ImmutableSortedMap.<K, V>naturalOrder().put(entry).build(),
ImmutableSortedMap.<K, V>naturalOrder().put(entry).buildOrThrow(),
Stream.of(entry)
.collect(
toImmutableSortedMap(naturalOrder(), Map.Entry::getKey, Map.Entry::getValue)));
Expand All @@ -126,15 +126,15 @@ ImmutableMap<K, V> before(Map<? extends K, ? extends V> iterable) {
return Refaster.anyOf(
ImmutableSortedMap.copyOf(iterable, naturalOrder()),
ImmutableSortedMap.copyOf(iterable.entrySet()),
ImmutableSortedMap.<K, V>naturalOrder().putAll(iterable).build());
ImmutableSortedMap.<K, V>naturalOrder().putAll(iterable).buildOrThrow());
}

@BeforeTemplate
ImmutableSortedMap<K, V> before(
Iterable<? extends Map.Entry<? extends K, ? extends V>> iterable) {
return Refaster.anyOf(
ImmutableSortedMap.copyOf(iterable, naturalOrder()),
ImmutableSortedMap.<K, V>naturalOrder().putAll(iterable).build(),
ImmutableSortedMap.<K, V>naturalOrder().putAll(iterable).buildOrThrow(),
Streams.stream(iterable)
.collect(
toImmutableSortedMap(
Expand Down
59 changes: 45 additions & 14 deletions ...b/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,13 @@ ImmutableMap.Builder<String, Integer> testImmutableMapBuilder() {
return new ImmutableMap.Builder<>();
}

ImmutableMap<Object, Object> testImmutableMapBuilderBuildOrThrow() {
return ImmutableMap.builder().build();
}

ImmutableSet<ImmutableMap<String, Integer>> testEntryToImmutableMap() {
return ImmutableSet.of(
ImmutableMap.<String, Integer>builder().put(Map.entry("foo", 1)).build(),
ImmutableMap.<String, Integer>builder().put(Map.entry("foo", 1)).buildOrThrow(),
Stream.of(Map.entry("foo", 1))
.collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)));
}
Expand All @@ -51,13 +55,14 @@ ImmutableSet<ImmutableMap<Integer, Integer>> testIterableToImmutableMap() {
ImmutableMap.copyOf(Maps.asMap(ImmutableSet.of(10), Integer::valueOf)));
}

ImmutableSet<ImmutableMap<String, Integer>> testEntryIterableToImmutableMap() {
ImmutableSet<Map<String, Integer>> testEntryIterableToImmutableMap() {
return ImmutableSet.of(
ImmutableMap.copyOf(ImmutableMap.of("foo", 1).entrySet()),
ImmutableMap.<String, Integer>builder().putAll(ImmutableMap.of("foo", 1)).build(),
ImmutableMap.<String, Integer>builder().putAll(ImmutableMap.of("foo", 1)).buildOrThrow(),
Map.copyOf(ImmutableMap.of("foo", 1)),
ImmutableMap.<String, Integer>builder()
.putAll(ImmutableMap.of("foo", 1).entrySet())
.build(),
.buildOrThrow(),
ImmutableMap.of("foo", 1).entrySet().stream()
.collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)),
Streams.stream(Iterables.cycle(Map.entry("foo", 1)))
Expand Down Expand Up @@ -100,32 +105,51 @@ ImmutableSet<ImmutableMap<String, Integer>> testTransformMapValuesToImmutableMap

ImmutableSet<Map<String, String>> testImmutableMapOf() {
return ImmutableSet.of(
ImmutableMap.<String, String>builder().build(),
ImmutableMap.<String, String>builder().buildOrThrow(),
ImmutableMap.ofEntries(),
Collections.<String, String>emptyMap(),
Map.<String, String>of());
}

ImmutableSet<Map<String, String>> testImmutableMapOf1() {
return ImmutableSet.of(
ImmutableMap.<String, String>builder().put("k1", "v1").build(),
ImmutableMap.<String, String>builder().put("k1", "v1").buildOrThrow(),
ImmutableMap.ofEntries(Map.entry("k1", "v1")),
Collections.singletonMap("k1", "v1"),
Map.of("k1", "v1"));
}

Map<String, String> testImmutableMapOf2() {
return Map.of("k1", "v1", "k2", "v2");
ImmutableSet<Map<String, String>> testImmutableMapOf2() {
return ImmutableSet.of(
ImmutableMap.ofEntries(Map.entry("k1", "v1"), Map.entry("k2", "v2")),
Map.of("k1", "v1", "k2", "v2"));
}

Map<String, String> testImmutableMapOf3() {
return Map.of("k1", "v1", "k2", "v2", "k3", "v3");
ImmutableSet<Map<String, String>> testImmutableMapOf3() {
return ImmutableSet.of(
ImmutableMap.ofEntries(Map.entry("k1", "v1"), Map.entry("k2", "v2"), Map.entry("k3", "v3")),
Map.of("k1", "v1", "k2", "v2", "k3", "v3"));
}

Map<String, String> testImmutableMapOf4() {
return Map.of("k1", "v1", "k2", "v2", "k3", "v3", "k4", "v4");
ImmutableSet<Map<String, String>> testImmutableMapOf4() {
return ImmutableSet.of(
ImmutableMap.ofEntries(
Map.entry("k1", "v1"),
Map.entry("k2", "v2"),
Map.entry("k3", "v3"),
Map.entry("k4", "v4")),
Map.of("k1", "v1", "k2", "v2", "k3", "v3", "k4", "v4"));
}

Map<String, String> testImmutableMapOf5() {
return Map.of("k1", "v1", "k2", "v2", "k3", "v3", "k4", "v4", "k5", "v5");
ImmutableSet<Map<String, String>> testImmutableMapOf5() {
return ImmutableSet.of(
ImmutableMap.ofEntries(
Map.entry("k1", "v1"),
Map.entry("k2", "v2"),
Map.entry("k3", "v3"),
Map.entry("k4", "v4"),
Map.entry("k5", "v5")),
Map.of("k1", "v1", "k2", "v2", "k3", "v3", "k4", "v4", "k5", "v5"));
}

ImmutableMap<String, Integer> testImmutableMapCopyOfMapsFilterKeys() {
Expand All @@ -139,4 +163,11 @@ ImmutableMap<String, Integer> testImmutableMapCopyOfMapsFilterValues() {
.filter(entry -> entry.getValue() > 0)
.collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
}

ImmutableSet<Map<String, Integer>> testImmutableMapOfEntries() {
return ImmutableSet.of(
Map.ofEntries(),
Map.ofEntries(Map.entry("foo", 1)),
Map.ofEntries(Map.entry("bar", 2), Map.entry("baz", 3)));
}
}
Loading

0 comments on commit 19d3ba0

Please sign in to comment.