Fix layout ranking for conditional loops with break and return (#127)

spcl · Dec 18, 2023 · f4662ae · f4662ae
1 parent e633724
commit f4662ae
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 27 deletions.
diff --git a/src/layouter/state_machine/sm_layouter.ts b/src/layouter/state_machine/sm_layouter.ts
@@ -360,21 +360,25 @@ export class SMLayouter {
                 throw new Error('Failed to determine exit.');
         }
 
-        // TODO: Not sure about this, this may fail in situations where the loop
-        // is executed conditionally and contains breaks and returns.
-        let subtr = 0;
-        for (const s of exitCandidates)
-            subtr += this.allDom.get(s)?.size ?? 0;
-        const exitRank = rank + (
-            (this.allDom.get(node)?.size ?? 0) - subtr
-        );
-        for (const s of exitCandidates)
-            q.push([s, exitRank]);
-
-        // Add all successors that are not the exit candidate.
-        for (const n of successors)
-            if (!exitCandidates.has(n))
+        if (exitCandidates.size > 1) {
+            throw new Error('Undetermined number of possible exit states.');
+        } else if (exitCandidates.size === 1) {
+            let loopSize = 0;
+            const exitNode = Array.from(exitCandidates)[0];
+            for (const s of successors) {
+                if (s !== exitNode) {
+                    loopSize += (this.allDom.get(s)?.size ?? 0) + 1;
+                    q.push([s, rank + 1]);
+                }
+            }
+            const exitRank = rank + loopSize + 1;
+            for (const s of exitCandidates)
+                q.push([s, exitRank]);
+        } else {
+            // Add all successors that are not the exit candidate.
+            for (const n of successors)
                 q.push([n, rank + 1]);
+        }
     }
 
     /**
@@ -386,20 +390,16 @@ export class SMLayouter {
         const q: [string, number][] = [[this.startNode, 0]];
 
         const scopes: [ScopeType, number, number][] = [];
-        const visited = new Set<string>();
         const rankings = new Map<string, number>();
 
         while (q.length > 0) {
             const [node, rank] = q.shift()!;
-            if (!visited.has(node)) {
-                const backedges = this.backedgesDstDict.get(node) ?? new Set();
+            if (rankings.has(node)) {
+                rankings.set(node, Math.max(rankings.get(node)!, rank));
+            } else {
+                rankings.set(node, rank);
 
-                // Assign the rank for the current node (passed along in the
-                // queue).
-                if (rankings.has(node))
-                    rankings.set(node, Math.max(rankings.get(node)!, rank));
-                else
-                    rankings.set(node, rank);
+                const backedges = this.backedgesDstDict.get(node) ?? new Set();
 
                 // Gather all successors that are not reached through backedges.
                 const successors: string[] = [];
@@ -443,8 +443,6 @@ export class SMLayouter {
                 } else {
                     this.propagate(node, successors, rank, q, scopes);
                 }
-
-                visited.add(node);
             }
         }
 

diff --git a/tests/unit/layouter/state_machine/sm_layouter.test.ts b/tests/unit/layouter/state_machine/sm_layouter.test.ts
@@ -218,7 +218,7 @@ function testSelfLoop(): void {
     expect(graph2.get('3')?.rank).toBe(3);
 }
 
-function testMultiInvertedSkipLops(): void {
+function testMultiInvertedSkipLoops(): void {
     const graph = new DiGraph<SMLayouterNode, SMLayouterEdge>();
 
     // Construct graph.
@@ -274,6 +274,36 @@ function testMultiInvertedSkipLops(): void {
     expect(graph.get('17')?.rank).toBe(9);
 }
 
+function testSkipLoopWithBreakReturn(): void {
+    const graph = new DiGraph<SMLayouterNode, SMLayouterEdge>();
+
+    // Construct graph.
+
+    constructEdge(graph, '0', '1');
+    constructEdge(graph, '1', '2');
+    constructEdge(graph, '1', '6');
+    constructEdge(graph, '2', '3');
+    constructEdge(graph, '2', '6');
+    constructEdge(graph, '3', '4');
+    constructEdge(graph, '3', '6');
+    constructEdge(graph, '4', '5');
+    constructEdge(graph, '4', '7');
+    constructEdge(graph, '5', '2');
+    constructEdge(graph, '6', '7');
+
+    const layouter = new SMLayouter(graph);
+    layouter.doLayout();
+
+    expect(graph.get('0')?.rank).toBe(0);
+    expect(graph.get('1')?.rank).toBe(1);
+    expect(graph.get('2')?.rank).toBe(2);
+    expect(graph.get('3')?.rank).toBe(3);
+    expect(graph.get('4')?.rank).toBe(4);
+    expect(graph.get('5')?.rank).toBe(5);
+    expect(graph.get('6')?.rank).toBe(6);
+    expect(graph.get('7')?.rank).toBe(7);
+}
+
 describe('Test vertical state machine layout ranking', () => {
     test('Basic branching', testBasicBranching);
     test('Nested branching', testNestedBranching);
@@ -284,7 +314,11 @@ describe('Test vertical state machine layout ranking', () => {
     test('Test self loops', testSelfLoop);
     test(
         'Test multiple inverted loops with skip edges',
-        testMultiInvertedSkipLops
+        testMultiInvertedSkipLoops
+    );
+    test(
+        'Test conditionally skipped loop with break and returns',
+        testSkipLoopWithBreakReturn
     );
 });