Make implicit break/continue explicit in ReactiveFunction

Previously when converting from HIR -> ReactiveFunction we elided break/continue 
terminals in places where control would implicitly transfer to the 
break/continue target and therefore nothing has to be emitted. The one downside 
of this approach is that it makes scope analysis a bit trickier. We want to 
close scopes once we see an instruction id past the end of the scope's range, 
but these implicit breaks were causing us to miss some instruction ids. We 
compensated for this, but it's helpful to keep the representation explicit and 
discard these terminals later in codegen.

compiler/forget/src/HIR/HIR.ts

@@ -68,10 +68,23 @@ export type ReactiveValueBlock = {
 };
 
 export type ReactiveStatement =
-  | { kind: "instruction"; instruction: ReactiveInstruction }
-  | { kind: "terminal"; terminal: ReactiveTerminal; label: BlockId | null }
+  | ReactiveInstructionStatement
+  | ReactiveTerminalStatement
   | ReactiveScopeBlock;
 
+export type ReactiveInstructionStatement = {
+  kind: "instruction";
+  instruction: ReactiveInstruction;
+};
+
+export type ReactiveTerminalStatement<
+  Tterminal extends ReactiveTerminal = ReactiveTerminal
+> = {
+  kind: "terminal";
+  terminal: Tterminal;
+  label: BlockId | null;
+};
+
 export type ReactiveInstruction = {
   id: InstructionId;
   lvalue: LValue | null;
@@ -80,40 +93,67 @@ export type ReactiveInstruction = {
 };
 
 export type ReactiveTerminal =
-  | { kind: "break"; label: BlockId | null; id: InstructionId | null }
-  | { kind: "continue"; label: BlockId | null; id: InstructionId }
-  | { kind: "return"; value: Place | null; id: InstructionId }
-  | { kind: "throw"; value: Place; id: InstructionId }
-  | {
-      kind: "switch";
-      test: Place;
-      cases: Array<{
-        test: Place | null;
-        block: ReactiveBlock | void;
-      }>;
-      id: InstructionId;
-    }
-  | {
-      kind: "while";
-      test: ReactiveValueBlock;
-      loop: ReactiveBlock;
-      id: InstructionId;
-    }
-  | {
-      kind: "for";
-      init: ReactiveValueBlock;
-      test: ReactiveValueBlock;
-      update: ReactiveValueBlock;
-      loop: ReactiveBlock;
-      id: InstructionId;
-    }
-  | {
-      kind: "if";
-      test: Place;
-      consequent: ReactiveBlock;
-      alternate: ReactiveBlock | null;
-      id: InstructionId;
-    };
+  | ReactiveBreakTerminal
+  | ReactiveContinueTerminal
+  | ReactiveReturnTerminal
+  | ReactiveThrowTerminal
+  | ReactiveSwitchTerminal
+  | ReactiveWhileTerminal
+  | ReactiveForTerminal
+  | ReactiveIfTerminal;
+
+export type ReactiveBreakTerminal = {
+  kind: "break";
+  label: BlockId | null;
+  id: InstructionId | null;
+  implicit: boolean;
+};
+export type ReactiveContinueTerminal = {
+  kind: "continue";
+  label: BlockId | null;
+  id: InstructionId;
+  implicit: boolean;
+};
+export type ReactiveReturnTerminal = {
+  kind: "return";
+  value: Place | null;
+  id: InstructionId;
+};
+export type ReactiveThrowTerminal = {
+  kind: "throw";
+  value: Place;
+  id: InstructionId;
+};
+export type ReactiveSwitchTerminal = {
+  kind: "switch";
+  test: Place;
+  cases: Array<{
+    test: Place | null;
+    block: ReactiveBlock | void;
+  }>;
+  id: InstructionId;
+};
+export type ReactiveWhileTerminal = {
+  kind: "while";
+  test: ReactiveValueBlock;
+  loop: ReactiveBlock;
+  id: InstructionId;
+};
+export type ReactiveForTerminal = {
+  kind: "for";
+  init: ReactiveValueBlock;
+  test: ReactiveValueBlock;
+  update: ReactiveValueBlock;
+  loop: ReactiveBlock;
+  id: InstructionId;
+};
+export type ReactiveIfTerminal = {
+  kind: "if";
+  test: Place;
+  consequent: ReactiveBlock;
+  alternate: ReactiveBlock | null;
+  id: InstructionId;
+};
 
 /**
  * A function lowered to HIR form, ie where its body is lowered to an HIR control-flow graph

compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts

@@ -15,10 +15,15 @@ import {
   InstructionValue,
   Place,
   ReactiveBlock,
-  ReactiveStatement,
   ReactiveValueBlock,
 } from "../HIR";
-import { HIRFunction, ReactiveFunction } from "../HIR/HIR";
+import {
+  HIRFunction,
+  ReactiveBreakTerminal,
+  ReactiveContinueTerminal,
+  ReactiveFunction,
+  ReactiveTerminalStatement,
+} from "../HIR/HIR";
 import todo from "../Utils/todo";
 import { assertExhaustive } from "../Utils/utils";
 
@@ -180,7 +185,7 @@ class Driver {
             const break_ = this.visitBreak(case_.block, null);
             if (
               index === 0 &&
-              break_ === null &&
+              break_.terminal.implicit &&
               case_.block === terminal.fallthrough &&
               case_.test === null
             ) {
@@ -383,6 +388,9 @@ class Driver {
         }
         break;
       }
+      case "error": {
+        invariant(false, "Unexpected error terminal");
+      }
       default: {
         assertExhaustive(terminal, "Unexpected terminal");
       }
@@ -434,27 +442,30 @@ class Driver {
   visitBreak(
     block: BlockId,
     id: InstructionId | null
-  ): ReactiveStatement | null {
+  ): ReactiveTerminalStatement<ReactiveBreakTerminal> {
     const target = this.cx.getBreakTarget(block);
     if (target === null) {
-      // TODO: we should always have a target
-      return null;
+      invariant(false, "Expected a break target");
     }
     switch (target.type) {
       case "implicit": {
-        return null;
+        return {
+          kind: "terminal",
+          terminal: { kind: "break", label: null, id, implicit: true },
+          label: null,
+        };
       }
       case "labeled": {
         return {
           kind: "terminal",
-          terminal: { kind: "break", label: target.block, id },
+          terminal: { kind: "break", label: target.block, id, implicit: false },
           label: null,
         };
       }
       case "unlabeled": {
         return {
           kind: "terminal",
-          terminal: { kind: "break", label: null, id },
+          terminal: { kind: "break", label: null, id, implicit: false },
           label: null,
         };
       }
@@ -467,7 +478,10 @@ class Driver {
     }
   }
 
-  visitContinue(block: BlockId, id: InstructionId): ReactiveStatement | null {
+  visitContinue(
+    block: BlockId,
+    id: InstructionId
+  ): ReactiveTerminalStatement<ReactiveContinueTerminal> {
     const target = this.cx.getContinueTarget(block);
     invariant(
       target !== null,
@@ -475,19 +489,28 @@ class Driver {
     );
     switch (target.type) {
       case "implicit": {
-        return null;
+        return {
+          kind: "terminal",
+          terminal: { kind: "continue", label: null, id, implicit: true },
+          label: null,
+        };
       }
       case "labeled": {
         return {
           kind: "terminal",
-          terminal: { kind: "continue", label: target.block, id },
+          terminal: {
+            kind: "continue",
+            label: target.block,
+            id,
+            implicit: false,
+          },
           label: null,
         };
       }
       case "unlabeled": {
         return {
           kind: "terminal",
-          terminal: { kind: "continue", label: null, id },
+          terminal: { kind: "continue", label: null, id, implicit: false },
           label: null,
         };
       }

compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts

@@ -104,6 +104,9 @@ function codegenBlock(cx: Context, block: ReactiveBlock): t.BlockStatement {
       }
       case "terminal": {
         const statement = codegenTerminal(cx, item.terminal);
+        if (statement === null) {
+          break;
+        }
         if (item.label !== null) {
           statements.push(
             t.labeledStatement(
@@ -232,9 +235,15 @@ function codegenReactiveScope(
   statements.push(t.ifStatement(testCondition, computationBlock, memoBlock));
 }
 
-function codegenTerminal(cx: Context, terminal: ReactiveTerminal): t.Statement {
+function codegenTerminal(
+  cx: Context,
+  terminal: ReactiveTerminal
+): t.Statement | null {
   switch (terminal.kind) {
     case "break": {
+      if (terminal.implicit) {
+        return null;
+      }
       return t.breakStatement(
         terminal.label !== null
           ? t.identifier(codegenLabel(terminal.label))
@@ -242,6 +251,9 @@ function codegenTerminal(cx: Context, terminal: ReactiveTerminal): t.Statement {
       );
     }
     case "continue": {
+      if (terminal.implicit) {
+        return null;
+      }
       return t.continueStatement(
         terminal.label !== null
           ? t.identifier(codegenLabel(terminal.label))

compiler/forget/src/__tests__/fixtures/hir/conditional-break.expect.md

@@ -56,22 +56,6 @@ function Component(props) {
   return a;
 }
 
-/**
- * props.b *does* influence `a`
- */
-function Component(props) {
-  const a = [];
-  a.push(props.a);
-  label: {
-    if (props.b) {
-      break label;
-    }
-    a.push(props.c);
-  }
-  a.push(props.d);
-  return a;
-}
-
 ```
 
 ## Code
@@ -202,19 +186,4 @@ function Component(props) {
 }
 
 ```
-## Code
-
-```javascript
-function Component(props) {
-  const a = [];
-  a.push(props.a);
-  if (props.b) {
-    a.push(props.d);
-    return a;
-  }
-
-  a.push(props.c);
-}
-
-```
       

compiler/forget/src/__tests__/fixtures/hir/conditional-break.js

@@ -51,19 +51,3 @@ function Component(props) {
   a.push(props.d);
   return a;
 }
-
-/**
- * props.b *does* influence `a`
- */
-function Component(props) {
-  const a = [];
-  a.push(props.a);
-  label: {
-    if (props.b) {
-      break label;
-    }
-    a.push(props.c);
-  }
-  a.push(props.d);
-  return a;
-}

compiler/forget/src/__tests__/fixtures/hir/error.conditional-break-labeled.expect.md

@@ -20,19 +20,11 @@ function Component(props) {
 
 ```
 
-## Code
 
-```javascript
-function Component(props) {
-  const a = [];
-  a.push(props.a);
-  if (props.b) {
-    a.push(props.d);
-    return a;
-  }
-
-  a.push(props.c);
-}
+## Error
 
 ```
+Expected a break target
+```
+          
       

compiler/forget/src/__tests__/fixtures/hir/error.ssa-for-of.expect.md

@@ -15,12 +15,11 @@ function foo(cond) {
 
 ```
 
-## Code
 
-```javascript
-function foo(cond) {
-  const items = [];
-}
+## Error
 
 ```
+Expected a break target
+```
+          
       

compiler/forget/src/__tests__/fixtures/hir/inverted-if.expect.md

@@ -2,13 +2,14 @@
 ## Input
 
 ```javascript
-function foo(a, b, c) {
+function foo(a, b, c, d) {
   let y = [];
   label: if (a) {
     if (b) {
       y.push(c);
       break label;
     }
+    y.push(d);
   }
   return y;
 }
@@ -18,27 +19,32 @@ function foo(a, b, c) {
 ## Code
 
 ```javascript
-function foo(a, b, c) {
+function foo(a, b, c, d) {
   const $ = React.useMemoCache();
   const c_0 = $[0] !== a;
   const c_1 = $[1] !== b;
   const c_2 = $[2] !== c;
+  const c_3 = $[3] !== d;
   let y;
-  if (c_0 || c_1 || c_2) {
+  if (c_0 || c_1 || c_2 || c_3) {
     y = [];
 
-    if (a) {
+    bb1: if (a) {
       if (b) {
         y.push(c);
+        break bb1;
       }
+
+      y.push(d);
     }
 
     $[0] = a;
     $[1] = b;
     $[2] = c;
-    $[3] = y;
+    $[3] = d;
+    $[4] = y;
   } else {
-    y = $[3];
+    y = $[4];
   }
 
   return y;

compiler/forget/src/__tests__/fixtures/hir/inverted-if.js

@@ -1,10 +1,11 @@
-function foo(a, b, c) {
+function foo(a, b, c, d) {
   let y = [];
   label: if (a) {
     if (b) {
       y.push(c);
       break label;
     }
+    y.push(d);
   }
   return y;
 }