Copilot commented on code in PR #2489: URL: https://github.com/apache/groovy/pull/2489#discussion_r3142882307
########## src/main/java/org/codehaus/groovy/runtime/MultipleAssignmentSupport.java: ########## @@ -0,0 +1,161 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.codehaus.groovy.runtime; + +import groovy.lang.IntRange; + +import java.lang.reflect.Array; +import java.util.Collections; +import java.util.Iterator; +import java.util.List; +import java.util.Spliterator; +import java.util.Spliterators; +import java.util.stream.Stream; +import java.util.stream.StreamSupport; + +/** + * Runtime helpers used by the GEP-20 multi-assignment destructuring bytecode. + * + * <p>These are called from generated bytecode only. User code should not rely on them.</p> + */ +public final class MultipleAssignmentSupport { + + private MultipleAssignmentSupport() {} + + private static final Class<?>[] INT_RANGE_PARAM = { IntRange.class }; + + /** + * Resolve the value for a tail rest binding ({@code *t}) at declarator index + * {@code fromIndex}. The GEP-20 dispatch model has three paths: + * + * <ul> + * <li><b>Path A</b> — {@code Stream<T>}: wrap the already-advanced iterator + * in a fresh sequential Stream and chain {@code onClose} to the + * original. The new Stream reports no spliterator characteristics + * (preserving them would require reading the original spliterator, + * which is mutually exclusive with the iterator path used for head + * extraction).</li> + * <li><b>Path B</b> — receiver with a resolvable {@code getAt(IntRange)}. + * Implemented as fast-path {@code instanceof} branches for + * {@code List}, {@code CharSequence}, and Java arrays (size-aware so + * out-of-bounds {@code fromIndex} returns the canonical empty value + * without calling user code), with an MOP-dispatched fallback for + * any other Path B receiver — {@link java.util.BitSet} (returns + * {@code BitSet}), user custom classes, etc.</li> + * <li><b>Path C</b> — anything else iterable: return the already-advanced + * iterator.</li> + * </ul> + * + * <p>The actual {@code instanceof} checks are ordered for performance + * (List/CharSequence/array fast paths first, then Stream, then MOP + * fallback, then iterator) but the conceptual dispatch order is A, then + * B, then C.</p> + * + * <p>Primitive streams ({@code IntStream}, {@code LongStream}, + * {@code DoubleStream}) are not subtypes of {@code Stream<T>} and therefore + * fall through to Path B (if they expose {@code getAt(IntRange)}) or + * Path C; their elements are boxed by the underlying iterator.</p> + * + * @param rhs the original RHS value + * @param fromIndex the declarator position of the rest binding + * @param seq the iterator that has already yielded the preceding fixed-slot values + */ + public static Object tailRest(final Object rhs, final int fromIndex, final Iterator<?> seq) { + if (rhs instanceof List) { + if (fromIndex >= ((List<?>) rhs).size()) return Collections.emptyList(); Review Comment: In the List fast-path, returning Collections.emptyList() when fromIndex is past the end makes the rest binding immutable. This is inconsistent with Groovy's List range slicing (getAt(EmptyRange) / getAt(Range)) which returns a new, mutable list (often of a similar concrete type). Consider returning a mutable empty list (or delegating to List#getAt with an empty range) so `*t` behaves the same whether it absorbs 0 or >0 elements. ```suggestion if (fromIndex >= ((List<?>) rhs).size()) return new java.util.ArrayList<>(); ``` ########## src/main/java/org/codehaus/groovy/runtime/MultipleAssignmentSupport.java: ########## @@ -0,0 +1,161 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.codehaus.groovy.runtime; + +import groovy.lang.IntRange; + +import java.lang.reflect.Array; +import java.util.Collections; +import java.util.Iterator; +import java.util.List; +import java.util.Spliterator; +import java.util.Spliterators; +import java.util.stream.Stream; +import java.util.stream.StreamSupport; + +/** + * Runtime helpers used by the GEP-20 multi-assignment destructuring bytecode. + * + * <p>These are called from generated bytecode only. User code should not rely on them.</p> + */ +public final class MultipleAssignmentSupport { + + private MultipleAssignmentSupport() {} + + private static final Class<?>[] INT_RANGE_PARAM = { IntRange.class }; + + /** + * Resolve the value for a tail rest binding ({@code *t}) at declarator index + * {@code fromIndex}. The GEP-20 dispatch model has three paths: + * + * <ul> + * <li><b>Path A</b> — {@code Stream<T>}: wrap the already-advanced iterator + * in a fresh sequential Stream and chain {@code onClose} to the + * original. The new Stream reports no spliterator characteristics + * (preserving them would require reading the original spliterator, + * which is mutually exclusive with the iterator path used for head + * extraction).</li> + * <li><b>Path B</b> — receiver with a resolvable {@code getAt(IntRange)}. + * Implemented as fast-path {@code instanceof} branches for + * {@code List}, {@code CharSequence}, and Java arrays (size-aware so + * out-of-bounds {@code fromIndex} returns the canonical empty value + * without calling user code), with an MOP-dispatched fallback for + * any other Path B receiver — {@link java.util.BitSet} (returns + * {@code BitSet}), user custom classes, etc.</li> + * <li><b>Path C</b> — anything else iterable: return the already-advanced + * iterator.</li> + * </ul> + * + * <p>The actual {@code instanceof} checks are ordered for performance + * (List/CharSequence/array fast paths first, then Stream, then MOP + * fallback, then iterator) but the conceptual dispatch order is A, then + * B, then C.</p> + * + * <p>Primitive streams ({@code IntStream}, {@code LongStream}, + * {@code DoubleStream}) are not subtypes of {@code Stream<T>} and therefore + * fall through to Path B (if they expose {@code getAt(IntRange)}) or + * Path C; their elements are boxed by the underlying iterator.</p> + * + * @param rhs the original RHS value + * @param fromIndex the declarator position of the rest binding + * @param seq the iterator that has already yielded the preceding fixed-slot values + */ + public static Object tailRest(final Object rhs, final int fromIndex, final Iterator<?> seq) { + if (rhs instanceof List) { + if (fromIndex >= ((List<?>) rhs).size()) return Collections.emptyList(); + IntRange range = new IntRange(true, fromIndex, -1); + return InvokerHelper.invokeMethod(rhs, "getAt", range); + } + if (rhs instanceof CharSequence) { + if (fromIndex >= ((CharSequence) rhs).length()) return ""; + IntRange range = new IntRange(true, fromIndex, -1); + return InvokerHelper.invokeMethod(rhs, "getAt", range); + } + if (rhs != null && rhs.getClass().isArray()) { + // Match the non-empty branch: ArrayGroovyMethods.getAt(T[], IntRange) returns List<T>, + // so the empty case must be Collections.emptyList() — not an empty T[] — for typed + // rest binders like `List<T> *t` to round-trip consistently. + if (fromIndex >= Array.getLength(rhs)) return Collections.emptyList(); + IntRange range = new IntRange(true, fromIndex, -1); + return InvokerHelper.invokeMethod(rhs, "getAt", range); + } + if (rhs instanceof Stream) { + Spliterator<?> spliterator = Spliterators.spliteratorUnknownSize(seq, 0); + Stream<?> tail = StreamSupport.stream(spliterator, false); + return tail.onClose(((Stream<?>) rhs)::close); + } + // MOP fallback: any other receiver with getAt(IntRange) — BitSet, user classes, etc. + // Picked deliberately AFTER the Stream branch so a Stream RHS never materialises via + // StreamGroovyMethods.getAt(Stream, IntRange). + if (rhs != null && InvokerHelper.getMetaClass(rhs).pickMethod("getAt", INT_RANGE_PARAM) != null) { + IntRange range = new IntRange(true, fromIndex, -1); + return InvokerHelper.invokeMethod(rhs, "getAt", range); + } + return seq; + } + + /** + * Resolve the value for a head or middle rest binding. Requires a sized, indexable RHS + * (List, CharSequence, or array — or any other receiver with a non-reversing + * {@code getAt(IntRange)}); for other RHS types this routes through the MOP so that: + * + * <ul> + * <li>Truly non-indexable sources (iterators, sets, custom Iterables without a + * {@code getAt(IntRange)} extension) fail fast with + * {@code MissingMethodException} — preventing silent materialisation of + * unbounded sources.</li> + * <li>{@code Stream} RHS resolves {@code StreamGroovyMethods.getAt(Stream, IntRange)}, + * which rejects the destructuring slice's reverse range with + * {@code IllegalArgumentException("reverse range")} — also fail-fast, also + * no materialisation. (Head/middle rest from {@code Stream} is rejected at + * compile time under {@code @CompileStatic}; the IAE only surfaces in + * dynamic mode.)</li> + * </ul> + * + * <p>When the RHS is shorter than the sum of fixed slots around the rest, the computed range + * is inverted. DGM's {@code getAt(IntRange)} would reverse an inverted range; for destructuring + * we want an empty slice instead, so this helper short-circuits that case for the sized fast + * paths (List/CharSequence/array). Other indexable receivers are responsible for their own + * bounds handling.</p> + * + * @param rhs the original RHS value + * @param fromIndex positive index where the slice begins + * @param toIndex negative index where the slice ends (inclusive) + */ + public static Object nonTailRestSlice(final Object rhs, final int fromIndex, final int toIndex) { + int size = -1; + if (rhs instanceof List) size = ((List<?>) rhs).size(); + else if (rhs instanceof CharSequence) size = ((CharSequence) rhs).length(); + else if (rhs != null && rhs.getClass().isArray()) size = Array.getLength(rhs); + + if (size >= 0) { + int effectiveTo = toIndex < 0 ? size + toIndex : toIndex; + if (fromIndex > effectiveTo || fromIndex >= size || effectiveTo < 0) { + // Array empty-slice falls through to Collections.emptyList(): the non-empty + // path delegates to getAt(IntRange) which returns List<T> for arrays, so the + // empty path must agree — see tailRest for the same alignment. + if (rhs instanceof CharSequence) return ""; + return Collections.emptyList(); + } Review Comment: nonTailRestSlice returns Collections.emptyList() for sized non-CharSequence RHS when the computed slice is empty/inverted. For List/array receivers, Groovy's getAt(EmptyRange) returns a new mutable list; returning an immutable empty list here makes the rest binding's mutability depend on whether it captured 0 elements. Consider returning a mutable empty list (and ideally matching Groovy's usual empty-slice behavior) to avoid UnsupportedOperationException surprises. -- 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]
