From 7301d9ff06fc9cb7b5dfba2f89c42f8da286df06 Mon Sep 17 00:00:00 2001 From: Bastien Diederichs Date: Thu, 2 Mar 2023 12:22:25 +0100 Subject: [PATCH 1/5] Introduce `ImmutableMapFilter{Keys,Values}` Refaster rules --- .../refasterrules/ImmutableMapRules.java | 43 +++++++++++++++++++ .../ImmutableMapRulesTestInput.java | 12 ++++++ .../ImmutableMapRulesTestOutput.java | 10 +++++ 3 files changed, 65 insertions(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index aa68506b83..43689c5dac 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -6,6 +6,7 @@ import static java.util.Collections.singletonMap; import static java.util.function.Function.identity; +import com.google.common.base.Predicate; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import com.google.common.collect.Streams; @@ -315,6 +316,48 @@ ImmutableMap after(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4, K k5, V } } + /** + * Prefer {@link Maps#filterKeys(Map, Predicate)} over streaming entries of a map and filtering + * the keys. + */ + abstract static class ImmutableMapFilterKeys { + @Placeholder(allowsIdentity = true) + abstract boolean keyFilter(@MayOptionallyUse K element); + + @BeforeTemplate + ImmutableMap before(ImmutableMap input) { + return input.entrySet().stream() + .filter(entry -> keyFilter(entry.getKey())) + .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); + } + + @AfterTemplate + ImmutableMap after(ImmutableMap input) { + return ImmutableMap.copyOf(Maps.filterKeys(input, k -> keyFilter(k))); + } + } + + /** + * Prefer {@link Maps#filterValues(Map, Predicate)} over streaming entries of a map and filtering + * the values. + */ + abstract static class ImmutableMapFilterValues { + @Placeholder(allowsIdentity = true) + abstract boolean valueFilter(@MayOptionallyUse V element); + + @BeforeTemplate + ImmutableMap before(ImmutableMap input) { + return input.entrySet().stream() + .filter(entry -> valueFilter(entry.getValue())) + .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); + } + + @AfterTemplate + ImmutableMap after(ImmutableMap input) { + return ImmutableMap.copyOf(Maps.filterValues(input, v -> valueFilter(v))); + } + } + // XXX: Add a rule for this: // Maps.transformValues(streamOfEntries.collect(groupBy(fun)), ImmutableMap::copyOf) // -> diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java index b4cd5f950e..d062ee3a44 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java @@ -107,4 +107,16 @@ Map testImmutableMapOf4() { Map testImmutableMapOf5() { return Map.of("k1", "v1", "k2", "v2", "k3", "v3", "k4", "v4", "k5", "v5"); } + + ImmutableMap testImmutableMapFilterKeys() { + return ImmutableMap.of("k1", "v1", "k2", "v2").entrySet().stream() + .filter(entry -> entry.getKey().equals("k1")) + .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); + } + + ImmutableMap testImmutableMapFilterValues() { + return ImmutableMap.of("k1", "v1", "k2", "v2").entrySet().stream() + .filter(entry -> entry.getValue().equals("v1")) + .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); + } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java index 4c1ddf948f..beaacbe4ce 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java @@ -90,4 +90,14 @@ Map testImmutableMapOf4() { Map testImmutableMapOf5() { return ImmutableMap.of("k1", "v1", "k2", "v2", "k3", "v3", "k4", "v4", "k5", "v5"); } + + ImmutableMap testImmutableMapFilterKeys() { + return ImmutableMap.copyOf( + Maps.filterKeys(ImmutableMap.of("k1", "v1", "k2", "v2"), k -> k.equals("k1"))); + } + + ImmutableMap testImmutableMapFilterValues() { + return ImmutableMap.copyOf( + Maps.filterValues(ImmutableMap.of("k1", "v1", "k2", "v2"), v -> v.equals("v1"))); + } } From 5797e3bc6e906eaf66a13b1c158359f41d888070 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 2 Mar 2023 16:08:36 +0100 Subject: [PATCH 2/5] Suggestions --- .../refasterrules/ImmutableMapRules.java | 36 +++++++++---------- .../ImmutableMapRulesTestInput.java | 12 +++---- .../ImmutableMapRulesTestOutput.java | 10 +++--- 3 files changed, 28 insertions(+), 30 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index 43689c5dac..c4ec6fe5e4 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -317,44 +317,44 @@ ImmutableMap after(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4, K k5, V } /** - * Prefer {@link Maps#filterKeys(Map, Predicate)} over streaming entries of a map and filtering - * the keys. + * Prefer creation of an immutable submap using {@link Maps#filterKeys(Map, Predicate)} over more + * contrived alternatives. */ - abstract static class ImmutableMapFilterKeys { + abstract static class ImmutableMapCopyOfMapsFilterKeys { @Placeholder(allowsIdentity = true) - abstract boolean keyFilter(@MayOptionallyUse K element); + abstract boolean keyFilter(@MayOptionallyUse K key); @BeforeTemplate - ImmutableMap before(ImmutableMap input) { - return input.entrySet().stream() - .filter(entry -> keyFilter(entry.getKey())) + ImmutableMap before(ImmutableMap map) { + return map.entrySet().stream() + .filter(e -> keyFilter(e.getKey())) .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); } @AfterTemplate - ImmutableMap after(ImmutableMap input) { - return ImmutableMap.copyOf(Maps.filterKeys(input, k -> keyFilter(k))); + ImmutableMap after(ImmutableMap map) { + return ImmutableMap.copyOf(Maps.filterKeys(map, k -> keyFilter(k))); } } /** - * Prefer {@link Maps#filterValues(Map, Predicate)} over streaming entries of a map and filtering - * the values. + * Prefer creation of an immutable submap using {@link Maps#filterValues(Map, Predicate)} over + * more contrived alternatives. */ - abstract static class ImmutableMapFilterValues { + abstract static class ImmutableMapCopyOfMapsFilterValues { @Placeholder(allowsIdentity = true) - abstract boolean valueFilter(@MayOptionallyUse V element); + abstract boolean valueFilter(@MayOptionallyUse V value); @BeforeTemplate - ImmutableMap before(ImmutableMap input) { - return input.entrySet().stream() - .filter(entry -> valueFilter(entry.getValue())) + ImmutableMap before(ImmutableMap map) { + return map.entrySet().stream() + .filter(e -> valueFilter(e.getValue())) .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); } @AfterTemplate - ImmutableMap after(ImmutableMap input) { - return ImmutableMap.copyOf(Maps.filterValues(input, v -> valueFilter(v))); + ImmutableMap after(ImmutableMap map) { + return ImmutableMap.copyOf(Maps.filterValues(map, v -> valueFilter(v))); } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java index d062ee3a44..3db671a7a9 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestInput.java @@ -108,15 +108,15 @@ Map testImmutableMapOf5() { return Map.of("k1", "v1", "k2", "v2", "k3", "v3", "k4", "v4", "k5", "v5"); } - ImmutableMap testImmutableMapFilterKeys() { - return ImmutableMap.of("k1", "v1", "k2", "v2").entrySet().stream() - .filter(entry -> entry.getKey().equals("k1")) + ImmutableMap testImmutableMapCopyOfMapsFilterKeys() { + return ImmutableMap.of("foo", 1).entrySet().stream() + .filter(entry -> entry.getKey().length() > 1) .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); } - ImmutableMap testImmutableMapFilterValues() { - return ImmutableMap.of("k1", "v1", "k2", "v2").entrySet().stream() - .filter(entry -> entry.getValue().equals("v1")) + ImmutableMap testImmutableMapCopyOfMapsFilterValues() { + return ImmutableMap.of("foo", 1).entrySet().stream() + .filter(entry -> entry.getValue() > 0) .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java index beaacbe4ce..1b135e32de 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableMapRulesTestOutput.java @@ -91,13 +91,11 @@ Map testImmutableMapOf5() { return ImmutableMap.of("k1", "v1", "k2", "v2", "k3", "v3", "k4", "v4", "k5", "v5"); } - ImmutableMap testImmutableMapFilterKeys() { - return ImmutableMap.copyOf( - Maps.filterKeys(ImmutableMap.of("k1", "v1", "k2", "v2"), k -> k.equals("k1"))); + ImmutableMap testImmutableMapCopyOfMapsFilterKeys() { + return ImmutableMap.copyOf(Maps.filterKeys(ImmutableMap.of("foo", 1), k -> k.length() > 1)); } - ImmutableMap testImmutableMapFilterValues() { - return ImmutableMap.copyOf( - Maps.filterValues(ImmutableMap.of("k1", "v1", "k2", "v2"), v -> v.equals("v1"))); + ImmutableMap testImmutableMapCopyOfMapsFilterValues() { + return ImmutableMap.copyOf(Maps.filterValues(ImmutableMap.of("foo", 1), v -> v > 0)); } } From 1f6b2b74ce3bf3dada99369ff52f939ed676ba29 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 3 Mar 2023 08:58:13 +0100 Subject: [PATCH 3/5] Drop now obsolete comment --- .../picnic/errorprone/refasterrules/ImmutableMapRules.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index c4ec6fe5e4..e3f2fb5662 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -366,9 +366,4 @@ ImmutableMap after(ImmutableMap map) { // map.entrySet().stream().filter(keyPred).forEach(mapBuilder::put) // -> // mapBuilder.putAll(Maps.filterKeys(map, pred)) - // - // map.entrySet().stream().filter(entry -> - // pred(entry.getKey())).collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)) - // -> - // ImmutableMap.copyOf(Maps.filterKeys(map, pred)) } From e7a5eae8ee3b6da13fe75b3ef9c95d76cb75cf0d Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 3 Mar 2023 08:59:50 +0100 Subject: [PATCH 4/5] Unrelated typo --- .../tech/picnic/errorprone/refasterrules/ImmutableMapRules.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index e3f2fb5662..dbd8571572 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -127,7 +127,7 @@ ImmutableMap after(Iterable> } /** - * Don't map a a stream's elements to map entries, only to subsequently collect them into an + * Don't map a stream's elements to map entries, only to subsequently collect them into an * {@link ImmutableMap}. The collection can be performed directly. */ abstract static class StreamOfMapEntriesToImmutableMap { From f2a961d7f084595e3896a295084d0cdd37c5cfa9 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 3 Mar 2023 09:02:42 +0100 Subject: [PATCH 5/5] Format ofcourse ;) --- .../picnic/errorprone/refasterrules/ImmutableMapRules.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index dbd8571572..9e5096bbe8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -127,8 +127,8 @@ ImmutableMap after(Iterable> } /** - * Don't map a stream's elements to map entries, only to subsequently collect them into an - * {@link ImmutableMap}. The collection can be performed directly. + * Don't map a stream's elements to map entries, only to subsequently collect them into an {@link + * ImmutableMap}. The collection can be performed directly. */ abstract static class StreamOfMapEntriesToImmutableMap { @Placeholder(allowsIdentity = true)