Copilot commented on code in PR #2558:
URL: https://github.com/apache/groovy/pull/2558#discussion_r3314893463
##########
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:
`toImmutableSet` uses `Set.copyOf(answer)`, which throws
`NullPointerException` if the iterator contains null. This differs from
Groovy’s existing “immutable” semantics (unmodifiable copy that can contain
null) and the Javadoc doesn’t mention the restriction. Consider creating an
unmodifiable copy that preserves nulls, or document the null rejection
explicitly.
##########
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:
`toImmutableList(T[] self)` delegates to `List.of(self)`, which throws
`NullPointerException` if any array element is null. Since `toList(T[])` (and
Groovy collections generally) allow nulls, this is a behavioral gotcha that’s
not documented. Consider returning an unmodifiable copy that permits nulls, or
document the null-element restriction.
##########
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:
`toImmutableSet(T[] self)` uses `Set.copyOf(toSet(self))`, which throws
`NullPointerException` if the array contains null. That restriction isn’t
documented and differs from Groovy’s usual “immutable == unmodifiable copy”
behavior which permits nulls. Consider using an unmodifiable wrapper around a
defensive copy to preserve nulls, or document the null rejection explicitly.
##########
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());
+ }
+
+ /**
+ * Accumulates the elements of stream into an immutable List.
+ *
+ * @param self the {@code java.util.stream.BaseStream}
+ * @param <T> the type of element
+ * @return an immutable {@code java.util.List} instance
+ *
+ * @since 6.0.0
+ */
+ public static <T> List<T> toImmutableList(final BaseStream<T, ? extends
BaseStream> self) {
+ return stream(self.iterator()).toList();
+ }
+
+ /**
+ * Accumulates the elements of stream into an immutable Set.
+ * <p>
+ * No native {@code Stream.toSet()} exists, so this provides the immutable
+ * counterpart to {@link #toSet(Stream)}.
+ *
+ * @param self the stream
+ * @param <T> the type of element
+ * @return an immutable {@code java.util.Set} instance
+ *
+ * @since 6.0.0
+ */
+ public static <T> Set<T> toImmutableSet(final Stream<T> self) {
+ return self.collect(toUnmodifiableSetCollector());
+ }
Review Comment:
`toImmutableSet(Stream)` uses `Collectors.toUnmodifiableSet()`, which throws
`NullPointerException` if the stream contains null elements. Streams and Groovy
collections can contain nulls, and the Javadoc currently doesn’t mention this
restriction. Consider using an unmodifiable wrapper around a copied mutable set
to preserve nulls, or document that null elements are rejected.
##########
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:
`toImmutableList` uses `List.copyOf(answer)`, which throws
`NullPointerException` if the iterator contains any null elements. Groovy’s
existing “immutable” helpers (e.g. `asImmutable(List)`/`asImmutable(Set)`)
create an unmodifiable copy and allow nulls; this new method’s null-hostile
behavior is surprising and undocumented. Consider using an unmodifiable wrapper
around a defensive copy (or explicitly document that null elements are
rejected).
##########
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());
+ }
Review Comment:
`toMutableSet` promises a “mutable Set” but it delegates to
`Collectors.toSet()`, which does not guarantee mutability or a specific Set
implementation. If mutability is part of the contract, consider using a
collector like `Collectors.toCollection(LinkedHashSet::new)`/`HashSet::new`
(and caching it) so callers don’t get an unexpectedly unmodifiable set under a
different JDK implementation.
##########
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:
For an empty iterator, this implementation still allocates an `ArrayList`
(and then discards it) before returning the canonical empty immutable list.
Consider a fast-path `if (!self.hasNext()) return List.of();` to avoid the
intermediate allocation for the common empty case.
##########
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
+ * replacements; see the deprecation note.
*
* @param self the stream
* @param <T> the type of element
- * @return a new {@code java.util.List} instance
+ * @return a new mutable {@code java.util.List} instance
*
* @since 2.5.0
+ *
+ * @deprecated since 6.0.0; the native {@link Stream#toList()}
(JDK 16+)
+ * shadows this and returns an <em>unmodifiable</em> list. If
+ * you need a mutable list, call {@link
#toMutableList(Stream)};
+ * otherwise use {@code stream.toList()} directly (faster than
+ * this extension because it skips the {@code Collectors}
path).
*/
+ @Deprecated(since = "6.0.0")
public static <T> List<T> toList(final Stream<T> self) {
- return self.collect(Collectors.toList());
+ return self.collect(toListCollector());
}
Review Comment:
This method (and `toMutableList`) promises a “mutable List” but it currently
delegates to `Collectors.toList()`, whose JDK contract explicitly makes no
guarantees about mutability/implementation type. If Groovy intends to guarantee
mutability here, consider switching the collector to
`Collectors.toCollection(ArrayList::new)` (and caching that collector) to make
the contract explicit and future-proof.
##########
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:
Similarly, for an empty iterator this always allocates a `HashSet` before
returning the canonical empty immutable set. A `if (!self.hasNext()) return
Set.of();` fast-path would avoid the extra allocation for the empty case.
--
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]
