Preserve Jass return paths after non-exiting loops (#1586)

Author: Frotty

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/ControlFlowGraph.java

@@ -40,9 +40,16 @@ public static final class Node {
     private final Reference2ObjectOpenHashMap<ImVarargLoop, Node> varargLoopEnd = new Reference2ObjectOpenHashMap<>();
     private final ObjectArrayList<Node> nodeList = new ObjectArrayList<>();
     private final ImStmts body;
+    private final boolean loopsMayFallThrough;
 
     public ControlFlowGraph(ImStmts stmts) {
+        this(stmts, false);
+    }
+
+    /** Conservative loop exits for Jass liveness: its validator requires returns after non-exiting loops too. */
+    public ControlFlowGraph(ImStmts stmts, boolean loopsMayFallThrough) {
         this.body = stmts;
+        this.loopsMayFallThrough = loopsMayFallThrough;
         // a light hint helps the first growth step avoid rehash
         nodes.trim(0);
         buildCfg(stmts);
@@ -120,6 +127,7 @@ private void buildCfg(ImStmts stmts) {
                 buildCfg(body);
                 if (!body.isEmpty()) addSuccessor(current, getNode(body.get(0)));
                 Node endloopNode = getEndloopNode(imLoop);
+                if (loopsMayFallThrough) addSuccessor(current, endloopNode);
                 nodeList.add(endloopNode);
                 addAllSuccessors(endloopNode, getSuccessorList(imLoop, i));
             } else if (s instanceof ImVarargLoop l) {

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalMerger.java

@@ -51,9 +51,8 @@ private void optimizeFunctions(List<ImFunction> functions) {
 
     void optimizeFunc(ImFunction func) {
         LivenessAnalysis liveness = analyzeLiveness(func);
-        // The liveness is that of the code which a path reaches: a local which only code no path reaches reads (after
-        // a loop which nothing leaves, say) is live nowhere, and eliminateDeadCode removes the assignments to it. That
-        // code goes too, so that no read is left with no assignment before it, which pjass rejects.
+        // Remove unreachable reads together with their assignments. On Jass the graph conservatively lets loops
+        // fall through: pjass requires a return after a non-exiting loop, and its operands must stay initialised.
         List<ImStmt> unreachable = liveness.cfg.unreachableStatements();
         if (!unreachable.isEmpty()) {
             for (ImStmt s : unreachable) {
@@ -399,7 +398,9 @@ public Map<ImStmt, Set<ImVar>> calculateLiveness(ImFunction func) {
 
     private LivenessAnalysis analyzeLiveness(ImFunction func) {
         // 1. Build Control Flow Graph
-        ControlFlowGraph cfg = new ControlFlowGraph(func.getBody());
+        boolean needsJassReturn = translator != null && !translator.isLuaTarget()
+            && !(func.getReturnType() instanceof ImVoid);
+        ControlFlowGraph cfg = new ControlFlowGraph(func.getBody(), needsJassReturn);
         final List<Node> nodes = cfg.getNodes();
         final int N = nodes.size();
 

de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java

@@ -2725,6 +2725,50 @@ public void garbageRemovalAnalysesAgainWhenABlizzardGlobalLosesItsLastRead() {
                 "    testSuccess()"));
     }
 
+    /** Issue #1577: pjass requires a trailing return even when the loop only exits by returning. */
+    @Test
+    public void localOptimizationsKeepJassReturnAfterNonExitingLoop() {
+        for (boolean lua : new boolean[]{false, true}) {
+            TestConfig config = test().localOptimizations().executeProg();
+            if (lua) config.testLua(true);
+            config.compilationUnits(
+                compilationUnit("returningLoop.j",
+                    "function returningLoop takes integer n returns integer",
+                    "    local integer result = n + 7",
+                    "    loop",
+                    "        if n > 0 then",
+                    "            return result",
+                    "        endif",
+                    "        set n = n + 1",
+                    "    endloop",
+                    "    return result",
+                    "endfunction",
+                    "function nestedReturningLoop takes integer n returns string",
+                    "    local string result = \"done\"",
+                    "    if n >= 0 then",
+                    "        loop",
+                    "            loop",
+                    "                exitwhen n > 0",
+                    "                set n = n + 1",
+                    "            endloop",
+                    "            if n > 0 then",
+                    "                return result",
+                    "            endif",
+                    "        endloop",
+                    "    else",
+                    "        return \"negative\"",
+                    "    endif",
+                    "    return result",
+                    "endfunction"),
+                compilationUnit("test.wurst",
+                    "package test",
+                    "native testSuccess()",
+                    "init",
+                    "    if returningLoop(0) == 7 and nestedReturningLoop(0) == \"done\" and nestedReturningLoop(-1) == \"negative\"",
+                    "        testSuccess()"));
+        }
+    }
+
     /**
      * The only read of `a` is after a loop which always returns, so it is never reached. The optimisations do not
      * leave that read with no assignment before it, which pjass rejects.