Copilot commented on code in PR #2489:
URL: https://github.com/apache/groovy/pull/2489#discussion_r3142776415
##########
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java:
##########
@@ -1374,6 +1406,298 @@ private boolean
typeCheckMultipleAssignmentAndContinue(final Expression leftExpr
return true;
}
+ /**
+ * GEP-20 rest binding under static checking. Handles both list-literal
RHS and
+ * declared-type RHS (List, array, String, Range, Set, Iterator, Stream —
anything with a
+ * typed {@code getAt(int)} or {@code iterator()} inferable element type).
The rest
+ * variable's inferred type tracks the runtime dispatch in
+ * {@code MultipleAssignmentSupport.tailRest}: {@code List<T>} for Path A
receivers
+ * (those that support {@code getAt(IntRange)}); {@code Stream<T>} for
{@code Stream}
+ * RHS (Path B', tail rest only); {@code Iterator<T>} for any other Path B
receiver
+ * (tail rest only). Head and middle rest reject Path B and Path B'
receivers as a
+ * static type error.
+ */
+ private boolean typeCheckMultipleAssignmentRest(final Expression
leftExpression, final Expression rightExpression) {
+ if (rightExpression instanceof ListExpression values) {
+ return typeCheckRestAgainstListLiteral((TupleExpression)
leftExpression, values, rightExpression);
+ }
+ // Any RHS with a statically-resolvable getAt(int) (or known shape via
declared type)
+ // routes through the declared-type path; this covers variables,
properties, method
+ // calls, constants (e.g. String literals), closure invocations, etc.
+ return typeCheckRestAgainstDeclaredType((TupleExpression)
leftExpression, rightExpression);
+ }
+
+ private boolean typeCheckRestAgainstListLiteral(final TupleExpression
tuple, final ListExpression values, final Expression rightExpression) {
+ List<Expression> tupleExpressions = tuple.getExpressions();
+ List<Expression> valueExpressions = values.getExpressions();
+ int tupleSize = tupleExpressions.size();
+ int valueSize = valueExpressions.size();
+
+ int restIndex = restIndexOf(tuple);
+ int fixedAfter = tupleSize - restIndex - 1;
+
+ // Per GEP-20: lower sizes are forgiving (getAt(int) returns null for
OOB,
+ // getAt(IntRange) returns empty for inverted ranges). Skip the strict
size check.
+
+ // Fixed slots before the rest binding: pairwise from index 0.
+ for (int i = 0; i < restIndex; i++) {
+ if (i >= valueSize) break;
+ ClassNode valueType = getType(valueExpressions.get(i));
+ ClassNode targetType = getType(tupleExpressions.get(i));
+ if (!isAssignableTo(valueType, targetType)) {
+ addStaticTypeError("Cannot assign value of type " +
prettyPrintType(valueType) + " to variable of type " +
prettyPrintType(targetType), rightExpression);
+ return false;
+ }
+ storeType(tupleExpressions.get(i), valueType);
+ }
+
+ // Fixed slots after the rest binding: anchored to the right of the
value list.
+ for (int j = 0; j < fixedAfter; j++) {
+ int lhsIdx = restIndex + 1 + j;
+ int rhsIdx = valueSize - fixedAfter + j;
+ if (rhsIdx < 0 || rhsIdx >= valueSize) continue;
+ ClassNode valueType = getType(valueExpressions.get(rhsIdx));
+ ClassNode targetType = getType(tupleExpressions.get(lhsIdx));
+ if (!isAssignableTo(valueType, targetType)) {
+ addStaticTypeError("Cannot assign value of type " +
prettyPrintType(valueType) + " to variable of type " +
prettyPrintType(targetType), rightExpression);
+ return false;
+ }
+ storeType(tupleExpressions.get(lhsIdx), valueType);
+ }
+
+ // Rest position: container type derived from the LUB of absorbed
value types.
+ int restFromIdx = Math.min(restIndex, valueSize);
+ int restToIdx = Math.max(restFromIdx, valueSize - fixedAfter);
+ Expression restExpr = tupleExpressions.get(restIndex);
+ ClassNode elementType;
+ if (restToIdx > restFromIdx) {
+ elementType = getType(valueExpressions.get(restFromIdx));
+ for (int k = restFromIdx + 1; k < restToIdx; k++) {
+ elementType = lowestUpperBound(elementType,
getType(valueExpressions.get(k)));
+ }
+ } else {
+ // Rest absorbs zero values (e.g. `def (Integer a, List<Integer>
*t, Integer b) = [1, 2]`).
+ // Default to OBJECT_TYPE, but if the rest binder is typed with a
single generic
+ // argument, use that as the element type so `List<Integer> *t`
doesn't get rejected
+ // as `List<Object>` not assignable to `List<Integer>`.
+ elementType = OBJECT_TYPE;
+ if (restExpr instanceof Variable v) {
+ ClassNode declared = v.getOriginType();
+ if (declared != null && !ClassHelper.isDynamicTyped(declared)
&& !declared.equals(OBJECT_TYPE)) {
+ GenericsType[] gts = declared.getGenericsTypes();
+ if (gts != null && gts.length == 1) {
+ elementType = getCombinedBoundType(gts[0]);
+ }
+ }
+ }
+ }
+ // Generics can't take primitives; box (int → Integer) so
List<Integer> matches.
+ elementType = ClassHelper.getWrapper(elementType);
+ ClassNode restType =
GenericsUtils.makeClassSafeWithGenerics(LIST_TYPE, new
GenericsType(elementType));
+ if (!checkRestAssignability(restExpr, restType, rightExpression))
return false;
+ storeType(restExpr, restType);
+
+ return true;
+ }
+
+ private boolean typeCheckRestAgainstDeclaredType(final TupleExpression
tuple, final Expression rightExpression) {
+ ClassNode rhsType =
Optional.ofNullable(getType(rightExpression)).orElseGet(rightExpression::getType);
+ ClassNode elementType = inferComponentType(rhsType, int_TYPE);
+ if (elementType == null) {
+ // Non-indexable RHS — let positional surface the existing
rejection.
+ return typeCheckMultipleAssignmentPositional(tuple,
rightExpression);
+ }
+
+ List<Expression> tupleExpressions = tuple.getExpressions();
+ int tupleSize = tupleExpressions.size();
+ int restIndex = restIndexOf(tuple);
+ boolean tailRest = restIndex == tupleSize - 1;
+ boolean pathA = rhsSupportsPathA(rhsType);
+
+ if (!tailRest && !pathA) {
+ addStaticTypeError("Head or middle rest binding requires an
indexable right-hand side; "
+ + prettyPrintType(rhsType) + " does not support
getAt(IntRange)", rightExpression);
+ return false;
+ }
+
+ // Non-rest positions all share the element type derived from
getAt(int).
+ for (int i = 0; i < tupleSize; i++) {
+ if (i == restIndex) continue;
+ ClassNode targetType = getType(tupleExpressions.get(i));
+ if (!isAssignableTo(elementType, targetType)) {
+ addStaticTypeError("Cannot assign value of type " +
prettyPrintType(elementType) + " to variable of type " +
prettyPrintType(targetType), rightExpression);
+ return false;
+ }
+ storeType(tupleExpressions.get(i), elementType);
+ }
+
+ // Rest position: List<T> for Path A, Stream<T> for Stream RHS (tail
rest only),
+ // Iterator<T> for any other Path B receiver (tail rest only).
+ // Box primitives so List<Integer>/Stream<Integer>/Iterator<Integer>
matches.
+ ClassNode container;
+ if (pathA) {
+ container = LIST_TYPE;
+ } else if (GeneralUtils.isOrImplements(rhsType, STREAM_TYPE)) {
+ container = STREAM_TYPE;
+ } else {
+ container = Iterator_TYPE;
+ }
+ ClassNode restType =
GenericsUtils.makeClassSafeWithGenerics(container, new
GenericsType(ClassHelper.getWrapper(elementType)));
Review Comment:
In `typeCheckRestAgainstDeclaredType`, Path A currently infers the rest
binder type as `List<T>` for all indexable RHS types. This is incorrect for
`CharSequence`/`String`/`GString` RHS: `MultipleAssignmentSupport.tailRest`
delegates to `getAt(IntRange)` which returns a `CharSequence` (or `String` for
`GString`), not a `List`. This can cause wrong inferred types under
STC/`@CompileStatic` and false negatives for typed rest binders like `def (c,
String *t) = 'hello'`.
Consider special-casing `CharSequence`/`GString` (and possibly `String`) so
the inferred rest type matches the `getAt(IntRange)` return type for those
receivers (instead of forcing `List<T>`), while keeping `List<T>` for
list/array/range Path A receivers.
##########
src/main/java/org/codehaus/groovy/runtime/MultipleAssignmentSupport.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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() {}
+
+ /**
+ * Resolve the value for a tail rest binding ({@code *t}) at declarator
index
+ * {@code fromIndex}. Picks Path A (slice via {@code getAt(IntRange)})
when the
+ * RHS is a list/string/array; Path B' (a fresh {@code Stream} wrapping the
+ * already-advanced iterator) when the RHS is a {@code Stream}; otherwise
Path B
+ * (returns the already-advanced iterator).
+ *
+ * <p>For Path B' the new Stream is sequential and reports no spliterator
+ * characteristics — capturing the original characteristics would require
reading
+ * the spliterator before the head was extracted, which is mutually
exclusive
+ * with the iterator path the head uses. The {@code onClose} handler of
the new
+ * Stream delegates to {@code rhs.close()} so that a typed source such as
+ * {@code Files.lines(p)} can be closed via the rest binder.</p>
+ *
+ * <p>Primitive streams ({@code IntStream}, {@code LongStream},
+ * {@code DoubleStream}) are not subtypes of {@code Stream<T>} and
therefore
+ * fall through to Path B; 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);
+ }
+ return seq;
+ }
+
+ /**
+ * Resolve the value for a head or middle rest binding. Requires a sized,
indexable RHS
+ * (List, CharSequence, or array); for other RHS types this routes through
the MOP so that
+ * non-indexable sources (iterators, streams, sets) fail fast with a
+ * {@code MissingMethodException} — preventing silent materialisation of
unbounded sources.
Review Comment:
The Javadoc for `nonTailRestSlice` says non-indexable sources like streams
fail fast with `MissingMethodException`, but `Stream` actually has
`getAt(IntRange)` (see `StreamGroovyMethods`) and in the head/middle-rest case
the negative end index will typically trigger an `IllegalArgumentException` for
a reverse range instead. Please adjust the documentation to match the actual
failure modes (e.g., MissingMethodException for truly non-indexable RHS like
Set/Iterator, and IllegalArgumentException for Stream reverse ranges).
```suggestion
* (List, CharSequence, or array); for other RHS types this routes
through the MOP.
* Truly non-indexable sources such as iterators and sets typically fail
with a
* {@code MissingMethodException}, preventing silent materialisation of
unbounded sources.
* Streams are a special case: Groovy provides {@code getAt(IntRange)}
for
* {@link java.util.stream.Stream}, so in head/middle-rest scenarios an
inverted or otherwise
* unsupported range may instead fail with an {@code
IllegalArgumentException}.
```
--
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]
