Make CollectionUtils guarantee mutable return value. (#11832)

Previously, they used Collectors.toList() which did not provide any such guarantee in its API docs, but behind-the-scenes was implemented as returning a mutable collection.

As such, there's likely triplea code that depends on that implementation detail. This change makes that implementation detail an API guarantee.

Also cleans up a couple code places that were copying the result and a few other clean ups.

game-app/game-core/src/main/java/games/strategy/triplea/delegate/MoveDelegate.java

@@ -395,7 +395,7 @@ static Change giveBonusMovementToUnits(
         final Predicate<Unit> givesBonusUnit =
             Matches.alliedUnit(player).and(Matches.unitCanGiveBonusMovementToThisUnit(u));
         final Collection<Unit> givesBonusUnits =
-            new ArrayList<>(CollectionUtils.getMatches(t.getUnits(), givesBonusUnit));
+            CollectionUtils.getMatches(t.getUnits(), givesBonusUnit);
         if (Matches.unitIsSea().test(u)) {
           final Predicate<Unit> givesBonusUnitLand = givesBonusUnit.and(Matches.unitIsLand());
           final Set<Territory> neighbors =

game-app/game-core/src/test/java/games/strategy/triplea/delegate/MoveDelegateTest.java

@@ -289,7 +289,7 @@ void testCant2StepBlitzWithNonBlitzingUnits() {
   }
 
   @Test
-  void testCantBlitzNuetral() {
+  void testCantBlitzNeutral() {
     final IntegerMap<UnitType> map = new IntegerMap<>();
     map.put(armour, 2);
     final Route route = new Route(equatorialAfrica, westAfrica, algeria);
@@ -710,19 +710,16 @@ void testAirCantLandInConquered() {
     // move carriers to ensure they can't go anywhere
     route = new Route(congoSeaZone, westAfricaSea, northAtlantic);
     Collection<Unit> units =
-        new ArrayList<>(
-            CollectionUtils.getMatches(
-                gameData.getMap().getTerritory(congoSeaZone.toString()).getUnits(),
-                Matches.unitIsCarrier()));
+        CollectionUtils.getMatches(
+            gameData.getMap().getTerritory(congoSeaZone.toString()).getUnits(),
+            Matches.unitIsCarrier());
     results = delegate.move(units, route);
     assertValid(results);
     // move carriers to ensure they can't go anywhere
     route = new Route(redSea, eastMediteranean, blackSea);
     units =
-        new ArrayList<>(
-            CollectionUtils.getMatches(
-                gameData.getMap().getTerritory(redSea.toString()).getUnits(),
-                Matches.unitIsCarrier()));
+        CollectionUtils.getMatches(
+            gameData.getMap().getTerritory(redSea.toString()).getUnits(), Matches.unitIsCarrier());
     results = delegate.move(units, route);
     assertValid(results);
     // make sure the place cant use it to land
@@ -989,8 +986,6 @@ void testReloadTransportAfterRetreatAllied() {
     // Get the defending land units that and their number
     retreatingLandUnits.addAll(
         karelia.getUnitCollection().getMatches(Matches.isUnitAllied(british)));
-    final List<Unit> defendingLandUnits = new ArrayList<>();
-    final int defendingLandSizeInt = defendingLandUnits.size();
     // Set up the battles and the dependent battles
     final IBattle inBalticSeaZone =
         gameData.getBattleDelegate().getBattleTracker().getPendingBattle(balticSeaZone);
@@ -1025,10 +1020,8 @@ void testReloadTransportAfterRetreatAllied() {
     final OffensiveGeneralRetreat offensiveGeneralRetreat =
         new OffensiveGeneralRetreat((BattleState) inBalticSeaZone, (BattleActions) inBalticSeaZone);
     offensiveGeneralRetreat.execute(mock(ExecutionStack.class), bridge);
-    // Get the total number of units that should be left
-    final int postCountInt = preCountInt - retreatingLandSizeInt;
     // Compare the number of units in Finland to begin with the number after retreating
-    assertEquals(defendingLandSizeInt, postCountInt);
+    assertEquals(preCountInt, retreatingLandSizeInt);
   }
 
   @Test
@@ -1052,11 +1045,8 @@ void testReloadTransportAfterDyingAllied() {
     final List<Unit> retreatingLandUnits =
         new ArrayList<>(karelia.getUnitCollection().getMatches(Matches.isUnitAllied(russians)));
     final int retreatingLandSizeInt = retreatingLandUnits.size();
-    // Get the defending land units that and their number
-    final List<Unit> defendingLandUnits = new ArrayList<>();
     retreatingLandUnits.addAll(
         karelia.getUnitCollection().getMatches(Matches.isUnitAllied(british)));
-    final int defendingLandSizeInt = defendingLandUnits.size();
     // Set up the battles and the dependent battles
     final IBattle inBalticSeaZone =
         gameData.getBattleDelegate().getBattleTracker().getPendingBattle(balticSeaZone);
@@ -1091,10 +1081,8 @@ void testReloadTransportAfterDyingAllied() {
     final OffensiveGeneralRetreat offensiveGeneralRetreat =
         new OffensiveGeneralRetreat((BattleState) inBalticSeaZone, (BattleActions) inBalticSeaZone);
     offensiveGeneralRetreat.execute(mock(ExecutionStack.class), bridge);
-    // Get the total number of units that should be left
-    final int postCountInt = preCountInt - retreatingLandSizeInt;
     // Compare the number of units in Finland to begin with the number after retreating
-    assertEquals(defendingLandSizeInt, postCountInt);
+    assertEquals(preCountInt, retreatingLandSizeInt);
   }
 
   @Test

lib/java-extras/src/main/java/org/triplea/java/collections/CollectionUtils.java

@@ -14,6 +14,7 @@
 import java.util.List;
 import java.util.Set;
 import java.util.function.Predicate;
+import java.util.stream.Collector;
 import java.util.stream.Collectors;
 import java.util.stream.StreamSupport;
 import javax.annotation.Nullable;
@@ -56,6 +57,8 @@ public static <T> int countMatches(final Iterable<T> it, final Predicate<T> pred
   /**
    * Returns all elements in the specified collection that match the specified predicate.
    *
+   * <p>Returns a mutable list with distinct storage from `collection`.
+   *
    * @param collection The collection whose elements are to be matched.
    * @param predicate The predicate with which to test each element.
    * @return A collection of all elements that match the specified predicate.
@@ -65,13 +68,15 @@ public static <T> List<T> getMatches(
     checkNotNull(collection);
     checkNotNull(predicate);
 
-    return collection.stream().filter(predicate).collect(Collectors.toList());
+    return collection.stream().filter(predicate).collect(toArrayList());
   }
 
   /**
    * Returns the elements in the specified collection, up to the specified limit, that match the
    * specified predicate.
    *
+   * <p>Returns a mutable list with distinct storage from `collection`.
+   *
    * @param collection The collection whose elements are to be matched.
    * @param max The maximum number of elements in the returned collection.
    * @param predicate The predicate with which to test each element.
@@ -84,10 +89,14 @@ public static <T> List<T> getNMatches(
     checkArgument(max >= 0, "max must not be negative");
     checkNotNull(predicate);
 
-    return collection.stream().filter(predicate).limit(max).collect(Collectors.toList());
+    return collection.stream().filter(predicate).limit(max).collect(toArrayList());
   }
 
-  /** return a such that a exists in c1 and a exists in c2. Always returns a new collection. */
+  /**
+   * Returns a such that a exists in c1 and a exists in c2. Always returns a new collection.
+   *
+   * <p>Returns a mutable list with distinct storage from `collection`.
+   */
   public static <T> List<T> intersection(
       final Collection<T> collection1, final Collection<T> collection2) {
     if (collection1 == null
@@ -98,10 +107,14 @@ public static <T> List<T> intersection(
     }
     final Collection<T> c2 =
         (collection2 instanceof Set) ? collection2 : ImmutableSet.copyOf(collection2);
-    return collection1.stream().distinct().filter(c2::contains).collect(Collectors.toList());
+    return collection1.stream().distinct().filter(c2::contains).collect(toArrayList());
   }
 
-  /** Returns a such that a exists in c1 but not in c2. Always returns a new collection. */
+  /**
+   * Returns a such that a exists in c1 but not in c2. Always returns a new collection.
+   *
+   * <p>Returns a mutable list with distinct storage from `collection`.
+   */
   public static <T> List<T> difference(
       final Collection<T> collection1, final Collection<T> collection2) {
     if (collection1 == null || collection1.isEmpty()) {
@@ -113,7 +126,7 @@ public static <T> List<T> difference(
 
     final Collection<T> c2 =
         (collection2 instanceof Set) ? collection2 : ImmutableSet.copyOf(collection2);
-    return collection1.stream().distinct().filter(not(c2::contains)).collect(Collectors.toList());
+    return collection1.stream().distinct().filter(not(c2::contains)).collect(toArrayList());
   }
 
   /**
@@ -134,6 +147,8 @@ public static <T> boolean haveEqualSizeAndEquivalentElements(
   /**
    * Creates a sorted, mutable collection containing the specified elements that will maintain its
    * sort order according to the specified comparator as elements are added or removed.
+   *
+   * <p>Returns a mutable collection with distinct storage from `elements`.
    */
   public static <T> Collection<T> createSortedCollection(
       final Collection<T> elements, final @Nullable Comparator<T> comparator) {
@@ -145,4 +160,9 @@ public static <T> Collection<T> createSortedCollection(
   public static <T> T getAny(final Iterable<T> elements) {
     return elements.iterator().next();
   }
+
+  /** Like Collectors.toList() but guarantees that the returned object is a mutable ArrayList. */
+  public static <T> Collector<T, ?, List<T>> toArrayList() {
+    return Collectors.toCollection(ArrayList::new);
+  }
 }

lib/java-extras/src/test/java/org/triplea/java/collections/CollectionUtilsTest.java

@@ -1,14 +1,18 @@
 package org.triplea.java.collections;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.triplea.java.collections.CollectionUtils.countMatches;
 import static org.triplea.java.collections.CollectionUtils.getMatches;
 import static org.triplea.java.collections.CollectionUtils.getNMatches;
 import static org.triplea.java.collections.CollectionUtils.haveEqualSizeAndEquivalentElements;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.function.Predicate;
@@ -45,6 +49,26 @@ void shouldFilterOutNonMatchingElementsAndReturnAllMatches() {
       assertEquals(List.of(-1, 1), getMatches(input, IS_ZERO.negate()), "some match");
       assertEquals(List.of(-1, 0, 1), getMatches(input, ALWAYS), "all match");
     }
+
+    @Test
+    void returnsDistinctInstanceWithDifferenceStorage() {
+      final Collection<Integer> input = new ArrayList<>(List.of(-1, 0, 1));
+      final List<Integer> result = getMatches(input, ALWAYS);
+      assertThat(result, equalTo(input));
+      assertThat(result, not(sameInstance(input)));
+      // Modifying input shouldn't change result.
+      input.add(5);
+      assertThat(result, not(equalTo(input)));
+    }
+
+    @Test
+    void returnsMutableInstance() {
+      final Collection<Integer> input = List.of(-1, 0, 1);
+      final List<Integer> result = getMatches(input, IS_ZERO.negate());
+      assertThat(result, equalTo(List.of(-1, 1)));
+      result.add(5);
+      assertThat(result, equalTo(List.of(-1, 1, 5)));
+    }
   }
 
   @Nested