GROOVY-11288: use operand stack (no temp variable) for simple assignment

apache · Oct 10, 2024 · 49057f0 · 49057f0
1 parent cdb69c9
commit 49057f0
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 27 deletions.
diff --git a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
@@ -1372,8 +1372,9 @@ private void storeThisInstanceField(final FieldExpression expression) {
             operandStack.doGroovyCast(field.getOriginType());
             operandStack.box();
             mv.visitVarInsn(ALOAD, 0);
+            operandStack.push(controller.getClassNode());
             mv.visitFieldInsn(GETFIELD, getFieldOwnerName(field), field.getName(), BytecodeHelper.getTypeDescription(type));
-            mv.visitInsn(SWAP);
+            operandStack.swap();
             mv.visitMethodInsn(INVOKEVIRTUAL, "groovy/lang/Reference", "set", "(Ljava/lang/Object;)V", false);
         } else {
             // rhs is normal value, set normal value
@@ -1383,23 +1384,23 @@ private void storeThisInstanceField(final FieldExpression expression) {
             operandStack.swap();
             mv.visitFieldInsn(PUTFIELD, getFieldOwnerName(field), field.getName(), BytecodeHelper.getTypeDescription(type));
         }
+        operandStack.remove(2);
     }
 
     private void storeStaticField(final FieldExpression expression) {
         MethodVisitor mv = controller.getMethodVisitor();
         FieldNode field = expression.getField();
-        ClassNode type = field.getType();
 
         OperandStack operandStack = controller.getOperandStack();
         operandStack.doGroovyCast(field);
 
         if (field.isHolder() && !controller.isInGeneratedFunctionConstructor()) {
             operandStack.box();
-            mv.visitFieldInsn(GETSTATIC, getFieldOwnerName(field), field.getName(), BytecodeHelper.getTypeDescription(type));
+            mv.visitFieldInsn(GETSTATIC, getFieldOwnerName(field), field.getName(), BytecodeHelper.getTypeDescription(field.getType()));
             mv.visitInsn(SWAP);
             mv.visitMethodInsn(INVOKEVIRTUAL, "groovy/lang/Reference", "set", "(Ljava/lang/Object;)V", false);
         } else {
-            mv.visitFieldInsn(PUTSTATIC, getFieldOwnerName(field), field.getName(), BytecodeHelper.getTypeDescription(type));
+            mv.visitFieldInsn(PUTSTATIC, getFieldOwnerName(field), field.getName(), BytecodeHelper.getTypeDescription(field.getType()));
         }
 
         operandStack.remove(1);

diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java
@@ -22,7 +22,6 @@
 import org.codehaus.groovy.ast.ClassHelper;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.Variable;
-import org.codehaus.groovy.ast.expr.ArgumentListExpression;
 import org.codehaus.groovy.ast.expr.ArrayExpression;
 import org.codehaus.groovy.ast.expr.BinaryExpression;
 import org.codehaus.groovy.ast.expr.ClassExpression;
@@ -368,21 +367,14 @@ public void eval(final BinaryExpression expression) {
         }
     }
 
-    @Deprecated
-    protected void assignToArray(final Expression parent, final Expression receiver, final Expression index, final Expression rhsValueLoader) {
-        assignToArray(parent, receiver, index, rhsValueLoader, false);
-    }
-
     protected void assignToArray(final Expression parent, final Expression receiver, final Expression index, final Expression rhsValueLoader, final boolean safe) {
         // let's replace this assignment to a subscript operator with a method call
-        // e.g. x[5] = 10
-        // -> (x, [], 5), =, 10
-        // -> methodCall(x, "putAt", [5, 10])
-        ArgumentListExpression ae = new ArgumentListExpression(index, rhsValueLoader);
-        controller.getInvocationWriter().makeCall(parent, receiver, constX("putAt"), ae, InvocationWriter.invokeMethod, safe, false, false);
-        controller.getOperandStack().pop();
-        // return value of assignment
-        rhsValueLoader.visit(controller.getAcg());
+        // e.g. x[5] = 10 --> ScriptBytecodeAdapter.invokeMethod(senderClass, x, "putAt", [5, 10])
+        controller.getInvocationWriter().makeCall(parent, receiver, constX("putAt"), args(index, rhsValueLoader), InvocationWriter.invokeMethod, safe, false, false);
+        controller.getOperandStack().pop(); // method return value
+
+        if (!Boolean.TRUE.equals(parent.getNodeMetaData("GROOVY-11288")))
+            rhsValueLoader.visit(controller.getAcg()); // assignment expression value
     }
 
     public void evaluateElvisEqual(final BinaryExpression expression) {
@@ -393,6 +385,7 @@ public void evaluateElvisEqual(final BinaryExpression expression) {
                 Token.newSymbol(ASSIGN, expression.getOperation().getStartLine(), expression.getOperation().getStartColumn()),
                 rhs
         );
+        assignment.copyNodeMetaData(expression);
         evaluateEqual(assignment, false);
     }
 
@@ -485,6 +478,15 @@ public void evaluateEqual(final BinaryExpression expression, final boolean defin
             }
         }
 
+        // GROOVY-11288: get value from the stack
+        if (singleAssignment && !returnRightValue
+                && !(leftExpression instanceof BinaryExpression)) {
+            compileStack.pushLHS(true);
+            leftExpression.visit(acg);
+            compileStack.popLHS();
+            return;
+        }
+
         int rhsValueId = compileStack.defineTemporaryVariable("$rhs", rhsType, true);
         // TODO: if RHS is already a VariableSlotLoader, then skip creating a new one
         Expression rhsValueLoader = new VariableSlotLoader(rhsType, rhsValueId, operandStack);

diff --git a/...a/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java b/...a/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
@@ -334,19 +334,16 @@ protected void assignToArray(final Expression enclosing, final Expression receiv
             }
 
             // replace assignment to a subscript operator with a method call
-            // e.g. x[5] = 10 -> methodCall(x, "putAt", [5, 10])
+            // e.g. x[5] = 10 --> x.putAt(5, 10)
             MethodCallExpression call = callX(receiver, "putAt", args(subscript, rhsValueLoader));
             call.setSafe(safe);
             call.setSourcePosition(enclosing);
 
-            OperandStack operandStack = controller.getOperandStack();
-            int height = operandStack.getStackLength();
             call.visit(controller.getAcg());
-            operandStack.pop();
-            operandStack.remove(operandStack.getStackLength() - height);
+            controller.getOperandStack().pop(); // method return value
 
             if (!Boolean.TRUE.equals(enclosing.getNodeMetaData("GROOVY-11288")))
-                rhsValueLoader.visit(controller.getAcg()); // re-load result value
+                rhsValueLoader.visit(controller.getAcg()); // assignment expression value
         }
     }
 }
diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompilationTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompilationTest.groovy
@@ -172,7 +172,7 @@ final class StaticCompilationTest extends AbstractBytecodeTestCase {
     }
 
     // GROOVY-11288
-    void testSimpleAssignment0() {
+    void testSingleAssignment2() {
         assert compile(method: 'm', '''@groovy.transform.CompileStatic
             class C {
                 int i
@@ -184,8 +184,6 @@ final class StaticCompilationTest extends AbstractBytecodeTestCase {
                 'L0',
                 'LINENUMBER 5',
                 'ICONST_1',
-                'ISTORE 1',
-                'ILOAD 1',
                 'ALOAD 0',
                 'SWAP',
                 'PUTFIELD C.i',
@@ -195,6 +193,48 @@ final class StaticCompilationTest extends AbstractBytecodeTestCase {
             ])
     }
 
+    // GROOVY-11288
+    void testSingleAssignment3() {
+        assert compile(method: 'm', '''@groovy.transform.CompileStatic
+            class C {
+                int i
+                void m() {
+                    i ?= 1
+                }
+            }
+        ''').hasStrictSequence([
+                'L0',
+                'LINENUMBER 5',
+                'ALOAD 0',
+                'GETFIELD C.i',
+                'DUP',
+                'IFEQ L1',
+                'ICONST_1',
+                'GOTO L2',
+                'L1',
+                'FRAME SAME1 I',
+                'ICONST_0',
+                'L2',
+                'FRAME FULL [C] [I I]',
+                'IFEQ L3',
+                'GOTO L4',
+                'L3',
+                'FRAME SAME1 I',
+                'POP',
+                'ICONST_1',
+                'L4',
+                'FRAME SAME1 I',
+                // store and load temp var 1 gone
+                'ALOAD 0',
+                'SWAP',
+                'PUTFIELD C.i',
+                // load temp var 1 and pop gone
+                'L5',
+                'LINENUMBER 6',
+                'RETURN'
+            ])
+    }
+
     // GROOVY-11288
     void testSubscriptAssignment1() {
         assert compile(method: 'm', '''@groovy.transform.CompileStatic