GROOVY-12098: Pre/post increment on array element double-evaluates th…

src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java

@@ -1059,28 +1059,39 @@ 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();
         ClassNode expressionType = operandStack.getTopOperand();
         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();
 
         // 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);
 

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'
+        '''
+    }
+}