[
https://issues.apache.org/jira/browse/GROOVY-12097?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18090782#comment-18090782
]
ASF GitHub Bot commented on GROOVY-12097:
-----------------------------------------
Copilot commented on code in PR #2620:
URL: https://github.com/apache/groovy/pull/2620#discussion_r3456796943
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java:
##########
@@ -417,6 +418,47 @@ protected void assignToArray(final Expression parent,
final Expression receiver,
rhsValueLoader.visit(controller.getAcg()); // assignment
expression value
}
+ /**
+ * Evaluates the given expression and stores its value in a fresh temporary
+ * variable, returning a loader for that variable. Used to evaluate the
+ * receiver and index of a subscript assignment ahead of the right-hand
side
+ * (GROOVY-12097) while leaving the operand stack clean.
+ */
+ private VariableSlotLoader evaluateIntoTemporary(final Expression
expression, final String name) {
+ AsmClassGenerator acg = controller.getAcg();
+ OperandStack operandStack = controller.getOperandStack();
+ CompileStack compileStack = controller.getCompileStack();
+ expression.visit(acg);
+ ClassNode type = operandStack.getTopOperand();
+ if (type.isGenericsPlaceHolder() ||
GenericsUtils.hasPlaceHolders(type)) {
+ type = controller.getTypeChooser().resolveType(expression,
controller.getClassNode());
+ }
+ int index = compileStack.defineTemporaryVariable(name, type, true);
+ return new VariableSlotLoader(type, index, operandStack);
+ }
+
+ /**
+ * Returns {@code true} if {@code target} occurs (by identity) somewhere
within
+ * {@code container}. This detects synthetic assignments whose right-hand
side
+ * reuses the left-hand side node, such as the rewrites of {@code a[i] op=
b} and
+ * {@code a[i] ?= b}, where the receiver and index are evaluated as part
of the RHS.
+ */
Review Comment:
The Javadoc for `isReferencedWithin` currently reads like it works for any
`Expression` target, but the visitor only checks `BinaryExpression` nodes (and
the method is only correct when `target` is a `BinaryExpression`). Consider
tightening the wording so it accurately reflects the implementation.
> a[index] = expr evaluates RHS before index, breaking left-to-right evaluation
> order
> -----------------------------------------------------------------------------------
>
> Key: GROOVY-12097
> URL: https://issues.apache.org/jira/browse/GROOVY-12097
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Priority: Major
>
> bq. AI read: The intent of GROOVY-2556 was to stop double-evaluating the
> object/index in compound subscript assignments (x[i] += v) — the first hunk
> of that commit. Reordering plain = was an unintended side effect of the
> "related problems" cleanup.
> GROOVY-2556 fixed some cases involved array expressions with side effects but
> also changed the index value on the LHS of an assignment. The following Java
> and Groovy program gave the same result prior to the change (Groovy 1.5.2ish)
> but the behavior below since then:
> {code:groovy}
> public class C {
> public static void main(String[] args) {
> int[] a = new int[]{-1, -1, -1, -1};
> int x = 0;
> int y = 3;
> a[x] = x++;
> a[y] = --y;
> System.out.println("[" + a[0] + "," + a[1] + "," + a[2] + "," + a[3] +
> "]");
> // Groovy: [-1,0,2,-1], Java: [0,-1,-1,2]
> }
> }
> {code}
> JLS Links for comparison with Java (Java SE 21 JLS):
> * §15.7 (left-to-right):
> https://docs.oracle.com/javase/specs/jls/se21/html/jls-15.html#jls-15.7
> * §15.26.1 (the array-assignment order):
> https://docs.oracle.com/javase/specs/jls/se21/html/jls-15.html#jls-15.26.1
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
