Copilot commented on code in PR #2489: URL: https://github.com/apache/groovy/pull/2489#discussion_r3141799364
########## src/main/java/org/codehaus/groovy/runtime/MultipleAssignmentSupport.java: ########## @@ -0,0 +1,99 @@ +/* + * 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; + +/** + * 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, else Path B (returns the already-advanced iterator). + * + * @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()) { + if (fromIndex >= Array.getLength(rhs)) { + return Array.newInstance(rhs.getClass().getComponentType(), 0); + } + IntRange range = new IntRange(true, fromIndex, -1); + return InvokerHelper.invokeMethod(rhs, "getAt", range); + } Review Comment: In `tailRest`, the array short-circuit returns a new empty array when `fromIndex` is beyond the array length. However `getAt(IntRange)` for arrays returns a `List` (see `ArrayGroovyMethods#getAt(T[], IntRange)`), so this makes the rest binder’s runtime type depend on whether the slice is empty. This can break typed rest binders (e.g. `List<T> *t`) and is inconsistent with the non-empty array slice case. Consider returning an empty `List` (or delegating to `getAt` for an empty range) to keep array rest slices consistently `List`-typed. ########## src/main/java/org/codehaus/groovy/runtime/MultipleAssignmentSupport.java: ########## @@ -0,0 +1,99 @@ +/* + * 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; + +/** + * 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, else Path B (returns the already-advanced iterator). + * + * @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()) { + if (fromIndex >= Array.getLength(rhs)) { + return Array.newInstance(rhs.getClass().getComponentType(), 0); + } + 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); 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. + * + * <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.</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) { + if (rhs instanceof CharSequence) return ""; + if (rhs.getClass().isArray()) return Array.newInstance(rhs.getClass().getComponentType(), 0); Review Comment: `nonTailRestSlice` returns a new empty array when the computed slice is empty/inverted and the RHS is an array. But for non-empty slices, the implementation delegates to `getAt(IntRange)`, which returns a `List` for arrays. This means callers can get `List` vs array depending on slice length, which is surprising and can break head/middle rest destructuring comparisons (e.g. expecting `[]`). Consider returning `Collections.emptyList()` for arrays here (to match `getAt(IntRange)` behavior) or otherwise making the return type consistent across empty and non-empty slices. ```suggestion if (rhs.getClass().isArray()) return Collections.emptyList(); ``` ########## src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java: ########## @@ -1374,6 +1406,275 @@ 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 is {@code List<T>} for Path A receivers (those that support + * {@code getAt(IntRange)}) and {@code Iterator<T>} for Path B receivers — matching the + * runtime dispatch in {@code MultipleAssignmentSupport.tailRest}. Head and middle rest + * reject 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); + 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 { + elementType = OBJECT_TYPE; + } + // 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)); + Expression restExpr = tupleExpressions.get(restIndex); + if (!checkRestAssignability(restExpr, restType, rightExpression)) return false; + storeType(restExpr, restType); Review Comment: When the absorbed rest slice is empty (`restToIdx <= restFromIdx`), the code forces `elementType` to `Object` and thus infers `restType` as `List<Object>`. For typed rest binders (e.g. `List<Integer> *m`), `checkRestAssignability` will then fail even though an empty slice should be safely assignable to any `List<T>`. Consider inferring the rest element type from the RHS list’s overall element LUB (or from the declared rest generic type when present) instead of defaulting to `Object` for empty slices. ########## src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java: ########## @@ -1374,6 +1406,275 @@ 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 is {@code List<T>} for Path A receivers (those that support + * {@code getAt(IntRange)}) and {@code Iterator<T>} for Path B receivers — matching the + * runtime dispatch in {@code MultipleAssignmentSupport.tailRest}. Head and middle rest + * reject 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); + 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 { + elementType = OBJECT_TYPE; + } + // 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)); + Expression restExpr = tupleExpressions.get(restIndex); + 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); + } Review Comment: `typeCheckRestAgainstDeclaredType` infers `elementType` via `inferComponentType(rhsType, int_TYPE)`, which only checks for a resolvable `getAt(int)`. This will reject RHS types that are Path B (iterator-based) but don’t have `getAt(int)` (e.g. `java.util.stream.Stream`), despite the method’s Javadoc claiming Stream support. Consider basing element-type inference on Path A/B: use `inferComponentType(rhsType, int_TYPE)` for Path A receivers and fall back to `inferComponentType(rhsType, null)` (iterator-based) for Path B receivers (or as a fallback when `getAt(int)` inference returns null). -- 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]
