GROOVY-11286: optimize void method return in expression statement (safe)

apache · Oct 14, 2024 · 338929c · 338929c
1 parent 7b070d8
commit 338929c
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 30 deletions.
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java
@@ -563,44 +563,36 @@ public void makeCall(final Expression origin, final Expression receiver, final E
                 ((TemporaryVariableExpression) tmpReceiver).remove(controller);
             }
         } else if (safe && origin instanceof MethodCallExpression) {
-            // wrap call in an IFNULL check
-            MethodVisitor mv = controller.getMethodVisitor();
             CompileStack compileStack = controller.getCompileStack();
             OperandStack operandStack = controller.getOperandStack();
+            MethodVisitor mv = controller.getMethodVisitor();
             int counter = labelCounter.incrementAndGet();
-            // if (receiver != null)
-            ExpressionAsVariableSlot slot = new ExpressionAsVariableSlot(controller, receiver);
+            // (receiver != null) ? receiver.name(args) : null
+            Label ifnull = compileStack.createLocalLabel("ifnull_" + counter);
+            Label nonull = compileStack.createLocalLabel("nonull_" + counter);
+            Label theEnd = compileStack.createLocalLabel("ending_" + counter);
+            var slot = new ExpressionAsVariableSlot(controller, receiver);
             slot.visit(controller.getAcg());
             operandStack.box();
-            Label ifnull = compileStack.createLocalLabel("ifnull_" + counter);
             mv.visitJumpInsn(IFNULL, ifnull);
-            operandStack.remove(1); // receiver consumed by if()
-            Label nonull = compileStack.createLocalLabel("nonull_" + counter);
+            operandStack.remove(1); // receiver consumed
             mv.visitLabel(nonull);
-            MethodCallExpression origMCE = (MethodCallExpression) origin;
-            MethodCallExpression newMCE = callX(
-                    new VariableSlotLoader(slot.getType(), slot.getIndex(), controller.getOperandStack()),
-                    origMCE.getMethodAsString(),
-                    origMCE.getArguments()
-            );
-            MethodNode methodTarget = origMCE.getMethodTarget();
-            newMCE.setImplicitThis(origMCE.isImplicitThis());
-            newMCE.setMethodTarget(methodTarget);
+            var newMCE = (MethodCallExpression) origin.transformExpression((expression) -> expression);
+            newMCE.setObjectExpression(new VariableSlotLoader(slot.getType(), slot.getIndex(), operandStack));
+            newMCE.getObjectExpression().setSourcePosition(((MethodCallExpression) origin).getObjectExpression());
             newMCE.setSafe(false);
-            newMCE.setSourcePosition(origMCE);
-            newMCE.getObjectExpression().setSourcePosition(origMCE.getObjectExpression());
+            int osl = operandStack.getStackLength();
             newMCE.visit(controller.getAcg());
             compileStack.removeVar(slot.getIndex());
-            ClassNode returnType = operandStack.getTopOperand();
-            if (ClassHelper.isPrimitiveType(returnType) && !isPrimitiveVoid(returnType)) {
-                operandStack.box();
+            if (operandStack.getStackLength() > osl) {
+                operandStack.box(); // non-void method
+                mv.visitJumpInsn(GOTO, theEnd);
+                mv.visitLabel(ifnull);
+                mv.visitInsn(ACONST_NULL);
+                mv.visitLabel(theEnd);
+            } else {
+                mv.visitLabel(ifnull);
             }
-            Label endof = compileStack.createLocalLabel("endof_" + counter);
-            mv.visitJumpInsn(GOTO, endof);
-            mv.visitLabel(ifnull);
-            // else { null }
-            mv.visitInsn(ACONST_NULL);
-            mv.visitLabel(endof);
         } else {
             if (origin instanceof AttributeExpression && (adapter == AsmClassGenerator.getField || adapter == AsmClassGenerator.getGroovyObjectField)) {
                 CallSiteWriter callSiteWriter = controller.getCallSiteWriter();

diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticPropertyAccessHelper.java b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticPropertyAccessHelper.java
@@ -110,15 +110,19 @@ public Expression transformExpression(final ExpressionTransformer transformer) {
             call.setSafe(isSafe());
             call.setSpreadSafe(isSpreadSafe());
             call.setImplicitThis(isImplicitThis());
+            call.setGenericsTypes(getGenericsTypes());
             return call;
         }
 
         @Override
         public void visit(final GroovyCodeVisitor visitor) {
-            super.visit(visitor);
             if (visitor instanceof AsmClassGenerator) {
-                // ignore the return of the call
-                ((AsmClassGenerator) visitor).getController().getOperandStack().pop();
+                var os = ((AsmClassGenerator) visitor).getController().getOperandStack();
+                int sl = os.getStackLength();
+                super.visit(visitor);
+                os.popDownTo(sl);
+            } else {
+                super.visit(visitor);
             }
         }
     }

diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompilationTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompilationTest.groovy
@@ -315,6 +315,39 @@ final class StaticCompilationTest extends AbstractBytecodeTestCase {
             ])
     }
 
+    // GROOVY-11286
+    void testVoidMethod3() {
+        assert compile(method: 'm', '''@groovy.transform.CompileStatic
+            void m() {
+                System.out?.print("")
+            }
+        ''').hasStrictSequence([
+                'L0',
+                'LINENUMBER 3',
+                'GETSTATIC java/lang/System.out : Ljava/io/PrintStream;',
+                'DUP',
+                'ASTORE 1',
+                'IFNULL L1',
+                'ALOAD 1',
+                'LDC ""',
+                'INVOKEVIRTUAL java/io/PrintStream.print (Ljava/lang/String;)V',
+                /* replaced all this with 'L1'
+                'ACONST_NULL',
+                'GOTO L2',
+                'L1',
+                'FRAME APPEND [java/io/PrintStream]',
+                'ACONST_NULL',
+                'L2',
+                'FRAME SAME',
+                'POP',
+                */
+                'L1',
+                'LINENUMBER 4',
+                'FRAME APPEND [java/io/PrintStream]',
+                'RETURN'
+            ])
+    }
+
     void testIntLeftShift() {
         assert compile(method: 'm', '''@groovy.transform.CompileStatic
             void m() {