From 19149bb3522da2c1299ca9d659435e94cd7ed6d1 Mon Sep 17 00:00:00 2001 From: Paul King Date: Tue, 23 Jun 2026 13:59:30 +1000 Subject: [PATCH] GROOVY-12098: Pre/post increment on array element double-evaluates the receiver --- .../classgen/asm/BinaryExpressionHelper.java | 65 +++++-- src/test/groovy/bugs/Groovy12098.groovy | 170 ++++++++++++++++++ 2 files changed, 221 insertions(+), 14 deletions(-) create mode 100644 src/test/groovy/bugs/Groovy12098.groovy diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java index 9e1265128e9..c6a5419864e 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java @@ -1059,12 +1059,23 @@ private void evaluateNotInstanceof(final BinaryExpression expression) { ); } + // Holds the temporaries created for a subscript expression's receiver and index so + // that a read-modify-write such as a[i]++ reuses them for the getAt (read) and the + // putAt (write) instead of evaluating the receiver (or index) expression twice. + private static final class SubscriptTemps { + final VariableSlotLoader receiver, index; + SubscriptTemps(final VariableSlotLoader receiver, final VariableSlotLoader index) { + this.receiver = receiver; + this.index = index; + } + } + private void evaluatePostfixMethod(final int op, final String method, final Expression expression, final Expression orig) { CompileStack compileStack = controller.getCompileStack(); OperandStack operandStack = controller.getOperandStack(); // load Expressions - VariableSlotLoader usesSubscript = loadWithSubscript(expression); + SubscriptTemps subscript = loadWithSubscript(expression); // save copy for later operandStack.dup(); @@ -1072,7 +1083,7 @@ private void evaluatePostfixMethod(final int op, final String method, final Expr int tempIdx = compileStack.defineTemporaryVariable("postfix_" + method, expressionType, true); // execute method - execMethodAndStoreForSubscriptOperator(op, method, expression, usesSubscript, orig); + execMethodAndStoreForSubscriptOperator(op, method, expression, subscript, orig); // remove the result of the method call operandStack.pop(); @@ -1080,7 +1091,7 @@ private void evaluatePostfixMethod(final int op, final String method, final Expr // reload saved value operandStack.load(expressionType, tempIdx); compileStack.removeVar(tempIdx); - if (usesSubscript != null) compileStack.removeVar(usesSubscript.getIndex()); + removeSubscriptTemps(subscript); } /** @@ -1119,36 +1130,62 @@ public void evaluatePrefixMethod(final PrefixExpression expression) { private void evaluatePrefixMethod(final int op, final String method, final Expression expression, final Expression orig) { // load expressions - VariableSlotLoader usesSubscript = loadWithSubscript(expression); + SubscriptTemps subscript = loadWithSubscript(expression); // execute method - execMethodAndStoreForSubscriptOperator(op, method, expression, usesSubscript, orig); + execMethodAndStoreForSubscriptOperator(op, method, expression, subscript, orig); // new value is already on stack, so nothing to do here - if (usesSubscript != null) controller.getCompileStack().removeVar(usesSubscript.getIndex()); + removeSubscriptTemps(subscript); } - private VariableSlotLoader loadWithSubscript(final Expression expression) { + /** + * Removes the subscript receiver/index temporaries created by + * {@link #loadWithSubscript} in reverse order of definition (LIFO). + */ + private void removeSubscriptTemps(final SubscriptTemps subscript) { + if (subscript != null) { + CompileStack compileStack = controller.getCompileStack(); + compileStack.removeVar(subscript.index.getIndex()); + compileStack.removeVar(subscript.receiver.getIndex()); + } + } + + private SubscriptTemps loadWithSubscript(final Expression expression) { AsmClassGenerator acg = controller.getAcg(); // if we have a BinaryExpression, check if it is with subscription if (expression instanceof BinaryExpression bexp) { if (bexp.getOperation().getType() == LEFT_SQUARE_BRACKET) { + OperandStack operandStack = controller.getOperandStack(); + CompileStack compileStack = controller.getCompileStack(); + + // GROOVY-12098: evaluate the receiver once (and before the index, to keep + // left-to-right order) and store it, so the read (getAt below) and the + // later write (putAt) reuse the value instead of re-evaluating the receiver. + Expression receiver = bexp.getLeftExpression(); + receiver.visit(acg); + ClassNode receiverType = operandStack.getTopOperand(); + if (receiverType.isGenericsPlaceHolder() || GenericsUtils.hasPlaceHolders(receiverType)) { + receiverType = controller.getTypeChooser().resolveType(receiver, controller.getClassNode()); + } + int receiverId = compileStack.defineTemporaryVariable("$receiver", receiverType, true); + VariableSlotLoader receiverExpression = new VariableSlotLoader(receiverType, receiverId, operandStack); + // right expression is the subscript expression // we store the result of the subscription on the stack Expression subscript = bexp.getRightExpression(); subscript.visit(acg); - OperandStack operandStack = controller.getOperandStack(); ClassNode subscriptType = operandStack.getTopOperand(); if (subscriptType.isGenericsPlaceHolder() || GenericsUtils.hasPlaceHolders(subscriptType)) { subscriptType = controller.getTypeChooser().resolveType(bexp, controller.getClassNode()); } - int id = controller.getCompileStack().defineTemporaryVariable("$subscript", subscriptType, true); + int id = compileStack.defineTemporaryVariable("$subscript", subscriptType, true); VariableSlotLoader subscriptExpression = new VariableSlotLoader(subscriptType, id, operandStack); - BinaryExpression rewrite = binX(bexp.getLeftExpression(), bexp.getOperation(), subscriptExpression); + BinaryExpression rewrite = binX(receiverExpression, bexp.getOperation(), subscriptExpression); rewrite.copyNodeMetaData(bexp); rewrite.setSourcePosition(bexp); rewrite.visit(acg); - return subscriptExpression; + return new SubscriptTemps(receiverExpression, subscriptExpression); } } @@ -1157,11 +1194,11 @@ private VariableSlotLoader loadWithSubscript(final Expression expression) { return null; } - private void execMethodAndStoreForSubscriptOperator(final int op, String method, final Expression expression, final VariableSlotLoader usesSubscript, final Expression orig) { + private void execMethodAndStoreForSubscriptOperator(final int op, String method, final Expression expression, final SubscriptTemps subscript, final Expression orig) { writePostOrPrefixMethod(op, method, expression, orig); // we need special code for arrays to store the result (like for a[1]++) - if (usesSubscript != null) { + if (subscript != null) { BinaryExpression be = (BinaryExpression) expression; CompileStack compileStack = controller.getCompileStack(); OperandStack operandStack = controller.getOperandStack(); @@ -1170,7 +1207,7 @@ private void execMethodAndStoreForSubscriptOperator(final int op, String method, BytecodeExpression methodResultLoader = new VariableSlotLoader(methodResultType, resultIdx, operandStack); // execute the assignment, this will leave the right side (here the method call result) on the stack - assignToArray(be, be.getLeftExpression(), usesSubscript, methodResultLoader, be.isSafe()); + assignToArray(be, subscript.receiver, subscript.index, methodResultLoader, be.isSafe()); compileStack.removeVar(resultIdx); diff --git a/src/test/groovy/bugs/Groovy12098.groovy b/src/test/groovy/bugs/Groovy12098.groovy new file mode 100644 index 00000000000..8e85d2f1382 --- /dev/null +++ b/src/test/groovy/bugs/Groovy12098.groovy @@ -0,0 +1,170 @@ +/* + * 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 bugs + +import org.junit.jupiter.api.Test + +import static groovy.test.GroovyAssert.assertScript + +/** + * A pre/post increment or decrement of an array (or list) element, e.g. {@code a[i]++}, + * must evaluate the receiver and the index exactly once -- reusing them for both the + * read (getAt) and the write (putAt) -- and in left-to-right order (receiver, then index). + * Previously the receiver was evaluated twice and the index was evaluated before it. + */ +final class Groovy12098 { + + @Test + void testPostfixEvaluatesReceiverAndIndexOnceInOrder() { + assertScript ''' + def order = [] + def data = [10, 20, 30] + def receiver = { order << 'receiver'; data } + def index = { order << 'index'; 1 } + receiver()[index()]++ + assert order == ['receiver', 'index'] + assert data == [10, 21, 30] + ''' + } + + @Test + void testPrefixEvaluatesReceiverAndIndexOnceInOrder() { + assertScript ''' + def order = [] + def data = [10, 20, 30] + def receiver = { order << 'receiver'; data } + def index = { order << 'index'; 1 } + ++receiver()[index()] + assert order == ['receiver', 'index'] + assert data == [10, 21, 30] + ''' + } + + @Test + void testDecrementEvaluatesReceiverAndIndexOnceInOrder() { + assertScript ''' + def order = [] + def data = [10, 20, 30] + def receiver = { order << 'receiver'; data } + def index = { order << 'index'; 1 } + receiver()[index()]-- + --receiver()[index()] + assert order == ['receiver', 'index', 'receiver', 'index'] + assert data == [10, 18, 30] + ''' + } + + // A receiver that is not referentially transparent (a fresh value each call) must be + // evaluated once, otherwise the read and the write would target different objects. + @Test + void testNonIdempotentReceiverEvaluatedOnce() { + assertScript ''' + int calls = 0 + def receiver = { calls += 1; [10, 20, 30] as int[] } + receiver()[1]++ + assert calls == 1 + ''' + } + + // The index subexpression's side effect happens exactly once. + @Test + void testIndexSideEffectHappensOnce() { + assertScript ''' + int[] a = [10, 20, 30] + int i = 0 + a[i++]++ + assert a == [11, 20, 30] as int[] + assert i == 1 + ''' + } + + @Test + void testValuesForArray() { + assertScript ''' + int[] a = [10, 20, 30] + assert a[1]++ == 20 && a == [10, 21, 30] as int[] + assert ++a[1] == 22 && a == [10, 22, 30] as int[] + assert a[1]-- == 22 && a == [10, 21, 30] as int[] + assert --a[1] == 20 && a == [10, 20, 30] as int[] + ''' + } + + @Test + void testValuesForList() { + assertScript ''' + def a = [10, 20, 30] + assert a[1]++ == 20 && a == [10, 21, 30] + assert ++a[1] == 22 && a == [10, 22, 30] + ''' + } + + @Test + void testCompileStaticPostfixEvaluatesReceiverOnce() { + assertScript ''' + @groovy.transform.CompileStatic + class C { + static int calls = 0 + static int[] data = [10, 20, 30] + static int[] receiver() { calls += 1; data } + static void run() { receiver()[1]++ } + } + C.run() + assert C.calls == 1 + assert C.data == [10, 21, 30] as int[] + ''' + } + + @Test + void testCompileStaticPrefixAndDecrementEvaluateReceiverOnce() { + assertScript ''' + @groovy.transform.CompileStatic + class C { + static int incCalls = 0, decCalls = 0 + static int[] inc = [10, 20, 30] + static int[] dec = [10, 20, 30] + static int[] incReceiver() { incCalls += 1; inc } + static int[] decReceiver() { decCalls += 1; dec } + static void run() { + ++incReceiver()[1] + --decReceiver()[1] + } + } + C.run() + assert C.incCalls == 1 + assert C.decCalls == 1 + assert C.inc == [10, 21, 30] as int[] + assert C.dec == [10, 19, 30] as int[] + ''' + } + + @Test + void testCompileStaticValuesForArray() { + assertScript ''' + @groovy.transform.CompileStatic + String go() { + int[] a = [10, 20, 30] + int i = 0 + int p = a[i++]++ // i -> 1, a[0]: 10 -> 11, p == 10 + int q = ++a[1] // a[1]: 20 -> 21, q == 21 + "p=$p q=$q a=${a.toList()} i=$i" + } + assert go() == 'p=10 q=21 a=[11, 21, 30] i=1' + ''' + } +}