Copilot commented on code in PR #2558:
URL: https://github.com/apache/groovy/pull/2558#discussion_r3297866065
##########
src/main/java/org/codehaus/groovy/runtime/StreamGroovyMethods.java:
##########
@@ -692,16 +722,32 @@ public static <T> T[] toArray(final Stream<? extends T>
self, final Class<T> typ
}
/**
- * Accumulates the elements of stream into a new List.
+ * Accumulates the elements of stream into a new mutable List.
+ * <p>
+ * <strong>Note:</strong> since JDK 16, {@link Stream} has a native
+ * {@code toList()} method that returns an <em>unmodifiable</em> list.
+ * Java instance methods take precedence over GDK extensions in both
+ * {@code @CompileStatic} and dynamic dispatch, so {@code stream.toList()}
+ * now resolves to the native call and yields an immutable result —
+ * <em>not</em> a fresh {@code ArrayList} as it did pre-JDK 16.
+ * Direct callers of this extension method are pointed at the explicit
Review Comment:
The Javadoc here states that pre-JDK 16 this extension returned a “fresh
ArrayList” and that this method returns a “new mutable List”.
Collectors.toList() doesn’t specify ArrayList nor mutability, so this is
stronger than the implementation contract. Either weaken the wording (e.g.
“typically mutable”) or switch implementation to a collector that guarantees
ArrayList/mutability.
##########
src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java:
##########
@@ -15870,6 +15911,23 @@ public static <T> Set<T> toSet(Iterator<T> self) {
return answer;
}
+ /**
+ * Convert an iterator to an immutable Set. The iterator will become
+ * exhausted of elements after making this conversion. Returns the
+ * canonical empty set ({@code Set.of()}) when the iterator is empty.
+ *
+ * @param self an iterator
+ * @return an immutable Set of the elements from the iterator
+ * @since 6.0.0
+ */
+ public static <T> Set<T> toImmutableSet(Iterator<T> self) {
+ Set<T> answer = new HashSet<>();
+ while (self.hasNext()) {
+ answer.add(self.next());
+ }
+ return Set.copyOf(answer);
Review Comment:
Same issue as toImmutableList(Iterator): Set.copyOf rejects null elements,
so this will throw NullPointerException if the iterator yields null. Either
document the restriction or use an unmodifiable wrapper over a copied Set to
keep behavior consistent with other Groovy “immutable” helpers.
##########
src/main/java/org/codehaus/groovy/runtime/StreamGroovyMethods.java:
##########
@@ -49,6 +50,35 @@ public class StreamGroovyMethods {
private StreamGroovyMethods() {
}
+ // ---- Cached Collectors -------------------------------------------------
+ // Collector instances are stateless configuration objects; the mutable
+ // accumulator (e.g. ArrayList) is freshly produced by the Supplier on each
+ // collect() invocation. So sharing the Collector across calls and threads
+ // is safe — and saves a per-call allocation that the JDK never elides.
+ // Same trick Eclipse Collections' Collectors2 uses.
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private static final Collector TO_LIST = Collectors.toList();
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private static final Collector TO_SET = Collectors.toSet();
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private static final Collector TO_UNMODIFIABLE_SET =
Collectors.toUnmodifiableSet();
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private static <T> Collector<T, ?, List<T>> toListCollector() {
+ return (Collector) TO_LIST;
+ }
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private static <T> Collector<T, ?, Set<T>> toSetCollector() {
+ return (Collector) TO_SET;
+ }
Review Comment:
Collectors.toList()/toSet() don’t guarantee mutability or concrete
collection type (per JDK contract), but later methods/Javadoc promise “mutable”
results (e.g. toMutableList/toMutableSet and the updated toList(Stream) docs).
If the API intends to guarantee mutability, consider using
Collectors.toCollection(ArrayList::new) / Collectors.toCollection(HashSet::new)
(and caching those collectors) instead of Collectors.toList()/toSet().
##########
src/main/java/org/codehaus/groovy/runtime/ArrayGroovyMethods.java:
##########
@@ -9961,6 +9961,117 @@ public static <T> List<T> toList(T[] self) {
return new ArrayList<>(Arrays.asList(self));
}
+
//--------------------------------------------------------------------------
+ // toImmutableList
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self a boolean array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static List<Boolean> toImmutableList(boolean[] self) {
+ return List.copyOf(toList(self));
+ }
Review Comment:
ArrayGroovyMethods adds toImmutableList overloads but provides no
groovyTestCase examples for them (while toImmutableSet does). Consider adding
at least one inline groovyTestCase verifying immutability and expected contents
(and null handling for object arrays if supported) to prevent regressions.
##########
src/main/java/org/codehaus/groovy/runtime/ParallelCollectionExtensions.java:
##########
@@ -52,6 +53,17 @@
*/
public class ParallelCollectionExtensions {
+ // Cached Collector: stateless config object; the per-call mutable
ArrayList
+ // is produced fresh by the Supplier inside collect(), so sharing this
+ // instance across threads is safe and saves a per-call allocation.
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private static final Collector TO_LIST = Collectors.toList();
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private static <T> Collector<T, ?, List<T>> toListCollector() {
+ return (Collector) TO_LIST;
+ }
Review Comment:
This class comment says the collector’s per-call accumulator is a “mutable
ArrayList”, but Collectors.toList() doesn’t guarantee ArrayList (or even
mutability) by contract. If you rely on a mutable ArrayList result, use
Collectors.toCollection(ArrayList::new) (can still be cached) or adjust the
comment to avoid overstating guarantees.
##########
src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java:
##########
@@ -15689,6 +15689,23 @@ public static <T> List<T> toList(Iterator<T> self) {
return answer;
}
+ /**
+ * Convert an iterator to an immutable List. The iterator will become
+ * exhausted of elements after making this conversion. Returns the
+ * canonical empty list ({@code List.of()}) when the iterator is empty.
+ *
+ * @param self an iterator
+ * @return an immutable List of the elements from the iterator
+ * @since 6.0.0
+ */
+ public static <T> List<T> toImmutableList(Iterator<T> self) {
+ List<T> answer = new ArrayList<>();
+ while (self.hasNext()) {
+ answer.add(self.next());
+ }
+ return List.copyOf(answer);
+ }
Review Comment:
List.copyOf/Set.copyOf reject null elements, which means these new
toImmutableList/toImmutableSet methods will throw NullPointerException if the
Iterator/Iterable contains nulls. That differs from existing
asImmutable(List/Set) which allows null values. Please either document the
null-element restriction clearly or implement immutability via unmodifiable
wrappers over copies to preserve Groovy’s typical null-tolerant semantics.
##########
src/main/java/org/codehaus/groovy/runtime/ArrayGroovyMethods.java:
##########
@@ -10111,6 +10222,148 @@ public static <T> Set<T> toSet(T[] self) {
return DefaultGroovyMethods.toSet(Arrays.asList(self));
}
+
//--------------------------------------------------------------------------
+ // toImmutableSet
+
+ /**
+ * Converts this array to an immutable Set, with each unique element added
to the set.
+ * <pre class="language-groovy groovyTestCase">
+ * boolean[] array = [true, false, true]
+ * Set expected = [true, false]
+ * assert array.toImmutableSet() == expected
+ * </pre>
+ *
+ * @param self a boolean array
+ * @return An immutable set containing the unique contents of this array.
+ * @since 6.0.0
+ */
+ public static Set<Boolean> toImmutableSet(boolean[] self) {
+ return Set.copyOf(toSet(self));
+ }
+
+ /**
+ * Converts this array to an immutable Set, with each unique element added
to the set.
+ * <pre class="language-groovy groovyTestCase">
+ * byte[] array = [1, 2, 3, 2, 1]
+ * Set expected = [1, 2, 3]
+ * assert array.toImmutableSet() == expected
+ * </pre>
+ *
+ * @param self a byte array
+ * @return An immutable set containing the unique contents of this array.
+ * @since 6.0.0
+ */
+ public static Set<Byte> toImmutableSet(byte[] self) {
+ return Set.copyOf(toSet(self));
+ }
+
+ /**
+ * Converts this array to an immutable Set, with each unique element added
to the set.
+ * <pre class="language-groovy groovyTestCase">
+ * char[] array = 'xyzzy'.chars
+ * Set expected = ['x', 'y', 'z']
+ * assert array.toImmutableSet() == expected
+ * </pre>
+ *
+ * @param self a char array
+ * @return An immutable set containing the unique contents of this array.
+ * @since 6.0.0
+ */
+ public static Set<Character> toImmutableSet(char[] self) {
+ return Set.copyOf(toSet(self));
+ }
+
+ /**
+ * Converts this array to an immutable Set, with each unique element added
to the set.
+ * <pre class="language-groovy groovyTestCase">
+ * short[] array = [1, 2, 3, 2, 1]
+ * Set expected = [1, 2, 3]
+ * assert array.toImmutableSet() == expected
+ * </pre>
+ *
+ * @param self a short array
+ * @return An immutable set containing the unique contents of this array.
+ * @since 6.0.0
+ */
+ public static Set<Short> toImmutableSet(short[] self) {
+ return Set.copyOf(toSet(self));
+ }
+
+ /**
+ * Converts this array to an immutable Set, with each unique element added
to the set.
+ * <pre class="language-groovy groovyTestCase">
+ * int[] array = [1, 2, 3, 2, 1]
+ * Set expected = [1, 2, 3]
+ * assert array.toImmutableSet() == expected
+ * </pre>
+ *
+ * @param self an int array
+ * @return An immutable set containing the unique contents of this array.
+ * @since 6.0.0
+ */
+ public static Set<Integer> toImmutableSet(int[] self) {
+ return Set.copyOf(toSet(self));
+ }
+
+ /**
+ * Converts this array to an immutable Set, with each unique element added
to the set.
+ * <pre class="language-groovy groovyTestCase">
+ * long[] array = [1, 2, 3, 2, 1]
+ * Set expected = [1, 2, 3]
+ * assert array.toImmutableSet() == expected
+ * </pre>
+ *
+ * @param self a long array
+ * @return An immutable set containing the unique contents of this array.
+ * @since 6.0.0
+ */
+ public static Set<Long> toImmutableSet(long[] self) {
+ return Set.copyOf(toSet(self));
+ }
+
+ /**
+ * Converts this array to an immutable Set, with each unique element added
to the set.
+ * <pre class="language-groovy groovyTestCase">
+ * float[] array = [1.0f, 2.0f, 3.0f, 2.0f, 1.0f]
+ * Set expected = [1.0f, 2.0f, 3.0f]
+ * assert array.toImmutableSet() == expected
+ * </pre>
+ *
+ * @param self a float array
+ * @return An immutable set containing the unique contents of this array.
+ * @since 6.0.0
+ */
+ public static Set<Float> toImmutableSet(float[] self) {
+ return Set.copyOf(toSet(self));
+ }
+
+ /**
+ * Converts this array to an immutable Set, with each unique element added
to the set.
+ * <pre class="language-groovy groovyTestCase">
+ * double[] array = [1.0d, 2.0d, 3.0d, 2.0d, 1.0d]
+ * Set expected = [1.0d, 2.0d, 3.0d]
+ * assert array.toImmutableSet() == expected
+ * </pre>
+ *
+ * @param self a double array
+ * @return An immutable set containing the unique contents of this array.
+ * @since 6.0.0
+ */
+ public static Set<Double> toImmutableSet(double[] self) {
+ return Set.copyOf(toSet(self));
+ }
+
+ /**
+ * Converts this array to an immutable Set, with each unique element added
to the set.
+ *
+ * @param self an object array
+ * @return An immutable set containing the unique contents of this array.
+ * @since 6.0.0
+ */
+ public static <T> Set<T> toImmutableSet(T[] self) {
+ return Set.copyOf(toSet(self));
Review Comment:
Set.copyOf(...) throws NullPointerException if any element is null. Since
toSet(T[]) can include nulls, this toImmutableSet(T[]) will fail for arrays
containing null. Please document this behavior or change implementation to an
unmodifiable wrapper over a copy if nulls should be allowed.
##########
src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java:
##########
@@ -15689,6 +15689,23 @@ public static <T> List<T> toList(Iterator<T> self) {
return answer;
}
+ /**
+ * Convert an iterator to an immutable List. The iterator will become
+ * exhausted of elements after making this conversion. Returns the
+ * canonical empty list ({@code List.of()}) when the iterator is empty.
+ *
+ * @param self an iterator
+ * @return an immutable List of the elements from the iterator
+ * @since 6.0.0
+ */
+ public static <T> List<T> toImmutableList(Iterator<T> self) {
+ List<T> answer = new ArrayList<>();
+ while (self.hasNext()) {
+ answer.add(self.next());
+ }
+ return List.copyOf(answer);
Review Comment:
The Javadoc guarantees returning the “canonical empty list (List.of())” for
an empty iterator, but the implementation always allocates an ArrayList and
List.copyOf only *may* return a shared empty instance. Either remove the
canonical-instance guarantee from the docs or add a fast-path that returns
List.of() when the iterator is empty.
##########
src/main/java/org/codehaus/groovy/runtime/StreamGroovyMethods.java:
##########
@@ -740,6 +786,115 @@ public static <T> Set<T> toSet(final Stream<T> self) {
* @since 2.5.0
*/
public static <T> Set<T> toSet(final BaseStream<T, ? extends BaseStream>
self) {
- return stream(self.iterator()).collect(Collectors.toSet());
+ return stream(self.iterator()).collect(toSetCollector());
+ }
+
+ /**
+ * Accumulates the elements of stream into a new mutable List.
+ * <p>
+ * Explicit alternative to the native {@link Stream#toList()} (which
+ * returns an <em>unmodifiable</em> list since JDK 16). Use this when
+ * you need to add to, remove from, or sort the returned list.
+ *
+ * @param self the stream
+ * @param <T> the type of element
+ * @return a new mutable {@code java.util.List} instance
+ *
+ * @since 6.0.0
+ */
+ public static <T> List<T> toMutableList(final Stream<T> self) {
+ return self.collect(toListCollector());
+ }
+
+ /**
+ * Accumulates the elements of stream into a new mutable List.
+ * <p>
+ * Explicit-mutability alias for {@link #toList(BaseStream)}, symmetric
with
+ * {@link #toMutableList(Stream)}.
+ *
+ * @param self the {@code java.util.stream.BaseStream}
+ * @param <T> the type of element
+ * @return a new mutable {@code java.util.List} instance
+ *
+ * @since 6.0.0
+ */
+ public static <T> List<T> toMutableList(final BaseStream<T, ? extends
BaseStream> self) {
+ return stream(self.iterator()).collect(toListCollector());
+ }
+
+ /**
+ * Accumulates the elements of stream into a new mutable Set.
+ * <p>
+ * Explicit-mutability alias for {@link #toSet(Stream)}. Provided
defensively
+ * so user intent stays unambiguous if a future JDK release adds a native
+ * {@code Stream.toSet()} (which would shadow the GDK extension the way
+ * native {@link Stream#toList()} did in JDK 16).
+ *
+ * @param self the stream
+ * @param <T> the type of element
+ * @return a new mutable {@code java.util.Set} instance
+ *
+ * @since 6.0.0
+ */
+ public static <T> Set<T> toMutableSet(final Stream<T> self) {
+ return self.collect(toSetCollector());
+ }
+
+ /**
+ * Accumulates the elements of stream into a new mutable Set.
+ * <p>
+ * Explicit-mutability alias for {@link #toSet(BaseStream)}, symmetric with
+ * {@link #toMutableSet(Stream)}.
+ *
+ * @param self the {@code java.util.stream.BaseStream}
+ * @param <T> the type of element
+ * @return a new mutable {@code java.util.Set} instance
+ *
+ * @since 6.0.0
+ */
+ public static <T> Set<T> toMutableSet(final BaseStream<T, ? extends
BaseStream> self) {
+ return stream(self.iterator()).collect(toSetCollector());
+ }
Review Comment:
StreamGroovyMethods introduces new API surface
(toMutableList/toMutableSet/toImmutableList/toImmutableSet) but there are no
inline groovyTestCase examples asserting mutability vs immutability. Adding
minimal Javadoc assertions (e.g. add() succeeds/fails) would clarify the
contract, especially given the JDK 16+ Stream#toList shadowing behavior.
##########
src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java:
##########
@@ -15689,6 +15689,23 @@ public static <T> List<T> toList(Iterator<T> self) {
return answer;
}
+ /**
+ * Convert an iterator to an immutable List. The iterator will become
+ * exhausted of elements after making this conversion. Returns the
+ * canonical empty list ({@code List.of()}) when the iterator is empty.
+ *
+ * @param self an iterator
+ * @return an immutable List of the elements from the iterator
+ * @since 6.0.0
+ */
+ public static <T> List<T> toImmutableList(Iterator<T> self) {
+ List<T> answer = new ArrayList<>();
+ while (self.hasNext()) {
+ answer.add(self.next());
+ }
+ return List.copyOf(answer);
+ }
Review Comment:
New public GDK methods toImmutableList/toImmutableSet (Iterator/Iterable)
don’t currently have an inline groovyTestCase block (or other visible tests)
validating key behavior like immutability (UnsupportedOperationException on
mutation), empty-input behavior, and null-element handling (allowed vs NPE).
Since Groovy runs these Javadoc examples as tests, adding such a block would
lock in the intended contract.
##########
src/main/java/org/codehaus/groovy/runtime/ArrayGroovyMethods.java:
##########
@@ -9961,6 +9961,117 @@ public static <T> List<T> toList(T[] self) {
return new ArrayList<>(Arrays.asList(self));
}
+
//--------------------------------------------------------------------------
+ // toImmutableList
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self a boolean array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static List<Boolean> toImmutableList(boolean[] self) {
+ return List.copyOf(toList(self));
+ }
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self a byte array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static List<Byte> toImmutableList(byte[] self) {
+ return List.copyOf(toList(self));
+ }
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self a char array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static List<Character> toImmutableList(char[] self) {
+ return List.copyOf(toList(self));
+ }
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self a short array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static List<Short> toImmutableList(short[] self) {
+ return List.copyOf(toList(self));
+ }
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self an int array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static List<Integer> toImmutableList(int[] self) {
+ return List.copyOf(toList(self));
+ }
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self a long array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static List<Long> toImmutableList(long[] self) {
+ return List.copyOf(toList(self));
+ }
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self a float array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static List<Float> toImmutableList(float[] self) {
+ return List.copyOf(toList(self));
+ }
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self a double array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static List<Double> toImmutableList(double[] self) {
+ return List.copyOf(toList(self));
+ }
+
+ /**
+ * Converts this array to an immutable List of the same size, with each
+ * element added to the list.
+ *
+ * @param self an object array
+ * @return An immutable list containing the contents of this array.
+ * @since 6.0.0
+ */
+ public static <T> List<T> toImmutableList(T[] self) {
+ return List.of(self);
Review Comment:
List.of(...) throws NullPointerException if any array element is null. If
Groovy intends to allow nulls in arrays converted to immutable lists
(consistent with toList and asImmutable), consider using an unmodifiable
wrapper over a copied list instead; otherwise, please document that null
elements are not supported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
