Improve the way we return values from modules

msimacek · msimacek · commit db4fd0e8c377 · 2022-08-02T16:20:57.000+02:00
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testImportAs.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testImportAs.co
@@ -7,4 +7,6 @@ Disassembly of <module>:
 000000         9 POP_TOP
 000000        10 IMPORT_FROM                       2    (c)
 000000        12 STORE_NAME                        3    (d)
-000000        14 RETURN_VALUE
+000000        14 POP_TOP
+000000        15 LOAD_NONE
+000000        16 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testImportFrom.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testImportFrom.co
@@ -6,4 +6,6 @@ Disassembly of <module>:
 000000         8 STORE_NAME                        1    (sqrt)
 000000        10 IMPORT_FROM                       2    (sin)
 000000        12 STORE_NAME                        3    (sine)
-000000        14 RETURN_VALUE
+000000        14 POP_TOP
+000000        15 LOAD_NONE
+000000        16 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testLoadClassDefRef.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testLoadClassDefRef.co
@@ -4,7 +4,7 @@ Disassembly of <module>:
 000030         5 LOAD_NAME                         0    (f)
 000032         7 LOAD_BYTE_O                       1
 000030         9 CALL_FUNCTION                     1
-000030        11 RETURN_VALUE
+000000        11 RETURN_VALUE
 
 Disassembly of f:
 000015         0 LOAD_BUILD_CLASS
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExcept.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExcept.co
@@ -47,4 +47,4 @@ Disassembly of <module>:
 000121   >>   78 LOAD_NAME                         0    (print)
 000127        80 LOAD_STRING                       4    ('after')
 000121        82 CALL_FUNCTION                     1
-000121        84 RETURN_VALUE
+000104        84 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExceptBare.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExceptBare.co
@@ -37,4 +37,4 @@ Disassembly of <module>:
 000109   >>   59 LOAD_NAME                         0    (print)
 000115        61 LOAD_STRING                       4    ('after')
 000109        63 CALL_FUNCTION                     1
-000109        65 RETURN_VALUE
+000088        65 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExceptElse.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExceptElse.co
@@ -52,4 +52,4 @@ Disassembly of <module>:
 000143   >>   87 LOAD_NAME                         0    (print)
 000149        89 LOAD_STRING                       5    ('after')
 000143        91 CALL_FUNCTION                     1
-000143        93 RETURN_VALUE
+000129        93 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExceptElseFinally.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExceptElseFinally.co
@@ -68,4 +68,4 @@ Disassembly of <module>:
 000171       112 LOAD_NAME                         0    (print)
 000177       114 LOAD_STRING                       6    ('after')
 000171       116 CALL_FUNCTION                     1
-000171       118 RETURN_VALUE
+000154       118 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExceptFinally.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryExceptFinally.co
@@ -63,4 +63,4 @@ Disassembly of <module>:
 000149       103 LOAD_NAME                         0    (print)
 000155       105 LOAD_STRING                       5    ('after')
 000149       107 CALL_FUNCTION                     1
-000149       109 RETURN_VALUE
+000132       109 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryFinally.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testTryFinally.co
@@ -21,4 +21,4 @@ Disassembly of <module>:
 000064        32 LOAD_NAME                         0    (print)
 000070        34 LOAD_STRING                       3    ('after')
 000064        36 CALL_FUNCTION                     1
-000064        38 RETURN_VALUE
+000047        38 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testWith.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testWith.co
@@ -18,4 +18,4 @@ Disassembly of <module>:
 000062        27 LOAD_NAME                         0    (print)
 000068        29 LOAD_STRING                       3    ('after')
 000062        31 CALL_FUNCTION                     1
-000062        33 RETURN_VALUE
+000016        33 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testWithMultiple.co b/graalpython/com.oracle.graal.python.test/testData/goldenFiles/CompilerTests/testWithMultiple.co
@@ -25,4 +25,4 @@ Disassembly of <module>:
 000080        37 LOAD_NAME                         0    (print)
 000086        39 LOAD_STRING                       4    ('after')
 000080        41 CALL_FUNCTION                     1
-000080        43 RETURN_VALUE
+000016        43 RETURN_VALUE
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/Compiler.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/Compiler.java
@@ -154,23 +154,23 @@
 import com.oracle.graal.python.pegparser.sst.AliasTy;
 import com.oracle.graal.python.pegparser.sst.ArgTy;
 import com.oracle.graal.python.pegparser.sst.ArgumentsTy;
+import com.oracle.graal.python.pegparser.sst.BoolOpTy;
+import com.oracle.graal.python.pegparser.sst.CmpOpTy;
 import com.oracle.graal.python.pegparser.sst.ComprehensionTy;
 import com.oracle.graal.python.pegparser.sst.ConstantValue;
 import com.oracle.graal.python.pegparser.sst.ExceptHandlerTy;
 import com.oracle.graal.python.pegparser.sst.ExprContextTy;
 import com.oracle.graal.python.pegparser.sst.ExprTy;
-import com.oracle.graal.python.pegparser.sst.BoolOpTy;
-import com.oracle.graal.python.pegparser.sst.CmpOpTy;
-import com.oracle.graal.python.pegparser.sst.PatternTy;
-import com.oracle.graal.python.pegparser.sst.MatchCaseTy;
-import com.oracle.graal.python.pegparser.sst.WithItemTy;
-import com.oracle.graal.python.pegparser.sst.UnaryOpTy;
 import com.oracle.graal.python.pegparser.sst.KeywordTy;
+import com.oracle.graal.python.pegparser.sst.MatchCaseTy;
 import com.oracle.graal.python.pegparser.sst.ModTy;
+import com.oracle.graal.python.pegparser.sst.PatternTy;
 import com.oracle.graal.python.pegparser.sst.SSTNode;
 import com.oracle.graal.python.pegparser.sst.SSTreeVisitor;
 import com.oracle.graal.python.pegparser.sst.StmtTy;
 import com.oracle.graal.python.pegparser.sst.TypeIgnoreTy;
+import com.oracle.graal.python.pegparser.sst.UnaryOpTy;
+import com.oracle.graal.python.pegparser.sst.WithItemTy;
 import com.oracle.graal.python.pegparser.tokenizer.SourceRange;
 import com.oracle.graal.python.util.PythonUtils;
 import com.oracle.truffle.api.memory.ByteArraySupport;
@@ -213,26 +213,8 @@ public CompilationUnit compile(ModTy mod, EnumSet<Flags> flags, int optimization
         enterScope("<module>", CompilationScope.Module, mod);
         mod.accept(this);
         CompilationUnit topUnit = unit;
-        Block lastBlock = unit.currentBlock;
-        if (!lastBlock.isReturn()) {
-            boolean addNone = !(mod instanceof ModTy.Expression);
-            if (lastBlock.instr.size() > 0 && lastBlock.instr.get(lastBlock.instr.size() - 1).opcode == POP_TOP) {
-                /*
-                 * To support interop eval we need to return the value of the last statement even if
-                 * we're in file mode.
-                 */
-                lastBlock.instr.remove(lastBlock.instr.size() - 1);
-                addNone = false;
-            }
-            if (addNone) {
-                // add a none return at the end
-                addOp(LOAD_NONE);
-            }
-            addOp(RETURN_VALUE);
-        }
         exitScope();
         return topUnit;
-        // return (!(mod instanceof ModTy.Expression));
     }
 
     private void parseFuture(StmtTy[] modBody) {
@@ -855,28 +837,49 @@ private void makeClosure(CodeUnit code, int makeFunctionFlags) {
 
     // visiting
 
-    private void visitBody(StmtTy[] stmts) {
-        if (unit.scope.isModule() && stmts.length > 0) {
-            /*
-             * Set current line number to the line number of first statement. This way line number
-             * for SETUP_ANNOTATIONS will always coincide with the line number of first "real"
-             * statement in module. If body is empty, then lineno will be set later in assemble.
-             */
-            setLocation(stmts[0]);
-        }
-        if (containsAnnotations(stmts)) {
-            addOp(SETUP_ANNOTATIONS);
-        }
-        int i = 0;
-        TruffleString docstring = getDocstring(stmts);
-        if (docstring != null) {
-            i++;
-            StmtTy.Expr stmt = (StmtTy.Expr) stmts[0];
-            stmt.value.accept(this);
-            addNameOp("__doc__", ExprContextTy.Store);
+    private void visitBody(StmtTy[] stmts, boolean returnValue) {
+        if (stmts != null) {
+            if (unit.scope.isModule() && stmts.length > 0) {
+                /*
+                 * Set current line number to the line number of first statement. This way line
+                 * number for SETUP_ANNOTATIONS will always coincide with the line number of first
+                 * "real" statement in module. If body is empty, then lineno will be set later in
+                 * assemble.
+                 */
+                setLocation(stmts[0]);
+            }
+            if (containsAnnotations(stmts)) {
+                addOp(SETUP_ANNOTATIONS);
+            }
+            if (stmts.length > 0) {
+                int i = 0;
+                TruffleString docstring = getDocstring(stmts);
+                if (docstring != null) {
+                    i++;
+                    StmtTy.Expr stmt = (StmtTy.Expr) stmts[0];
+                    stmt.value.accept(this);
+                    addNameOp("__doc__", ExprContextTy.Store);
+                }
+                for (; i < stmts.length - 1; i++) {
+                    stmts[i].accept(this);
+                }
+                /*
+                 * To support interop eval we need to return the value of the last statement even if
+                 * we're in file mode. Also used when parsing with arguments.
+                 */
+                StmtTy lastStatement = stmts[stmts.length - 1];
+                if (returnValue && lastStatement instanceof StmtTy.Expr) {
+                    ((StmtTy.Expr) lastStatement).value.accept(this);
+                    addOp(RETURN_VALUE);
+                    return;
+                } else {
+                    lastStatement.accept(this);
+                }
+            }
         }
-        for (; i < stmts.length; i++) {
-            stmts[i].accept(this);
+        if (returnValue) {
+            addOp(LOAD_NONE);
+            addOp(RETURN_VALUE);
         }
     }
 
@@ -1846,7 +1849,9 @@ public Void visit(KeywordTy node) {
 
     @Override
     public Void visit(ModTy.Expression node) {
-        return node.body.accept(this);
+        node.body.accept(this);
+        addOp(RETURN_VALUE);
+        return null;
     }
 
     @Override
@@ -1860,14 +1865,15 @@ public Void visit(ModTy.Interactive node) {
             addOp(SETUP_ANNOTATIONS);
         }
         interactive = true;
-        return visitSequence(node.body);
+        visitSequence(node.body);
+        addOp(LOAD_NONE);
+        addOp(RETURN_VALUE);
+        return null;
     }
 
     @Override
     public Void visit(ModTy.Module node) {
-        if (node.body != null) {
-            visitBody(node.body);
-        }
+        visitBody(node.body, true);
         return null;
     }
 
@@ -2077,7 +2083,7 @@ public Void visit(StmtTy.ClassDef node) {
         addOp(LOAD_STRING, addObject(unit.constants, toTruffleStringUncached(unit.qualName)));
         addNameOp("__qualname__", ExprContextTy.Store);
 
-        visitBody(node.body);
+        visitBody(node.body, false);
 
         if (unit.scope.needsClassClosure()) {
             int idx = unit.cellvars.get("__class__");
