From efbde936dc023901cc309fa20b9fc0c515a76ab4 Mon Sep 17 00:00:00 2001
From: Rick Ossendrijver <rick.ossendrijver@gmail.com>
Date: Wed, 10 Aug 2022 14:09:36 +0200
Subject: [PATCH] Only suggest bulk removal operations on sets (#186)

If a collection may contain duplicates, then removing a single occurrence of
each element in some other set is not equivalent to removing all such
occurrences.
---
 .../CollectionTemplates.java                  | 22 ++++++++++---------
 .../CollectionTemplatesTestInput.java         | 11 +++++-----
 .../CollectionTemplatesTestOutput.java        |  9 ++++----
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/CollectionTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/CollectionTemplates.java
index e8dfc4e9..55f8a100 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/CollectionTemplates.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/CollectionTemplates.java
@@ -15,6 +15,7 @@ import java.util.List;
 import java.util.NavigableSet;
 import java.util.Optional;
 import java.util.Queue;
+import java.util.Set;
 import java.util.SortedSet;
 import java.util.function.IntFunction;
 import java.util.stream.Stream;
@@ -129,31 +130,32 @@ final class CollectionTemplates {
     }
   }
 
-  static final class CollectionRemoveAllFromCollectionBlock<T, S extends T> {
+  static final class SetRemoveAllCollection<T, S extends T> {
     @BeforeTemplate
-    void before(Collection<T> removeTo, Collection<S> elementsToRemove) {
-      elementsToRemove.forEach(removeTo::remove);
+    void before(Set<T> removeFrom, Collection<S> elementsToRemove) {
+      elementsToRemove.forEach(removeFrom::remove);
     }
 
     @BeforeTemplate
-    void before2(Collection<T> removeTo, Collection<S> elementsToRemove) {
+    void before2(Set<T> removeFrom, Collection<S> elementsToRemove) {
       for (T element : elementsToRemove) {
-        removeTo.remove(element);
+        removeFrom.remove(element);
       }
     }
 
     // XXX: This method is identical to `before2` except for the loop type. Make Refaster smarter so
-    // that this is supported out of the box.
+    // that this is supported out of the box. After doing so, also drop the `S extends T` type
+    // constraint; ideally this check applies to any `S`.
     @BeforeTemplate
-    void before3(Collection<T> removeTo, Collection<S> elementsToRemove) {
+    void before3(Set<T> removeFrom, Collection<S> elementsToRemove) {
       for (S element : elementsToRemove) {
-        removeTo.remove(element);
+        removeFrom.remove(element);
       }
     }
 
     @AfterTemplate
-    void after(Collection<T> removeTo, Collection<S> elementsToRemove) {
-      removeTo.removeAll(elementsToRemove);
+    void after(Set<T> removeFrom, Collection<S> elementsToRemove) {
+      removeFrom.removeAll(elementsToRemove);
     }
   }
 
diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/CollectionTemplatesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/CollectionTemplatesTestInput.java
index ac952b70..33421b1e 100644
--- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/CollectionTemplatesTestInput.java
+++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/CollectionTemplatesTestInput.java
@@ -6,6 +6,7 @@ import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.Optional;
@@ -52,13 +53,13 @@ final class CollectionTemplatesTest implements RefasterTemplateTestCase {
     return Iterables.removeAll(new ArrayList<>(), ImmutableSet.of("foo"));
   }
 
-  void testCollectionRemoveAllFromCollectionBlock() {
-    ImmutableSet.of("foo").forEach(new ArrayList<>()::remove);
-    for (Number element : ImmutableSet.of(1)) {
-      new ArrayList<Number>().remove(element);
+  void testSetRemoveAllCollection() {
+    ImmutableSet.of("foo").forEach(new HashSet<>()::remove);
+    for (Number element : ImmutableList.of(1)) {
+      new HashSet<Number>().remove(element);
     }
     for (Integer element : ImmutableSet.of(2)) {
-      new ArrayList<Number>().remove(element);
+      new HashSet<Number>().remove(element);
     }
   }
 
diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/CollectionTemplatesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/CollectionTemplatesTestOutput.java
index 51807141..0ae34afd 100644
--- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/CollectionTemplatesTestOutput.java
+++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/CollectionTemplatesTestOutput.java
@@ -6,6 +6,7 @@ import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.Optional;
@@ -48,10 +49,10 @@ final class CollectionTemplatesTest implements RefasterTemplateTestCase {
     return new ArrayList<>().removeAll(ImmutableSet.of("foo"));
   }
 
-  void testCollectionRemoveAllFromCollectionBlock() {
-    new ArrayList<>().removeAll(ImmutableSet.of("foo"));
-    new ArrayList<Number>().removeAll(ImmutableSet.of(1));
-    new ArrayList<Number>().removeAll(ImmutableSet.of(2));
+  void testSetRemoveAllCollection() {
+    new HashSet<>().removeAll(ImmutableSet.of("foo"));
+    new HashSet<Number>().removeAll(ImmutableList.of(1));
+    new HashSet<Number>().removeAll(ImmutableSet.of(2));
   }
 
   ArrayList<String> testNewArrayListFromCollection() {