From 30476f924769cda9e7eefb9c7e6b1c0da372d013 Mon Sep 17 00:00:00 2001
From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com>
Date: Tue, 5 Sep 2023 13:54:25 -0400
Subject: [PATCH 1/8] Update to actions/checkout v4

---
 .github/workflows/build.yaml | 8 ++++----
 .github/workflows/docs.yaml  | 2 +-
 .github/workflows/rust.yaml  | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
index 5171817f1..624737bdc 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -12,7 +12,7 @@ jobs:
   test:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: denoland/setup-deno@v1
         with:
@@ -37,7 +37,7 @@ jobs:
 
       - name: Install Turnt
         # run: pip install turnt  # Use instead if pip turnt version >= 1.7
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
         with:
           repository: cucapra/turnt
           path: './turnt'
@@ -53,7 +53,7 @@ jobs:
   check:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: denoland/setup-deno@v1
         with:
@@ -81,7 +81,7 @@ jobs:
   style:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - uses: TrueBrain/actions-flake8@master
         with:
           path: bril-txt
diff --git a/.github/workflows/docs.yaml b/.github/workflows/docs.yaml
index 9e31d16d8..ed8fdc0b8 100644
--- a/.github/workflows/docs.yaml
+++ b/.github/workflows/docs.yaml
@@ -12,7 +12,7 @@ jobs:
   build:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - name: Setup mdBook
         uses: peaceiris/actions-mdbook@v1
         with:
diff --git a/.github/workflows/rust.yaml b/.github/workflows/rust.yaml
index f679ddb4e..e3c395660 100644
--- a/.github/workflows/rust.yaml
+++ b/.github/workflows/rust.yaml
@@ -39,7 +39,7 @@ jobs:
           - "cd brilift && cargo build --release && make rt.o && make test TURNTARGS=-v"
           - "cd brilift && cargo build --release && make rt.o && make benchmark TURNTARGS=-v"
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - uses: dtolnay/rust-toolchain@v1
         with:
           toolchain: stable
@@ -60,7 +60,7 @@ jobs:
 
       - name: Install Turnt
         # run: pip install turnt  # Use instead if pip turnt version >= 1.7
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
         with:
           repository: cucapra/turnt
           path: './turnt'
@@ -82,7 +82,7 @@ jobs:
       matrix:
         path: ["brilirs/Cargo.toml", "bril-rs/Cargo.toml", "bril-rs/bril2json/Cargo.toml", "bril-rs/brild/Cargo.toml", "brilift/Cargo.toml", "bril-rs/rs2bril/Cargo.toml", "bril-rs/brillvm/Cargo.toml"]
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - uses: dtolnay/rust-toolchain@v1
         with:
           toolchain: stable

From 175dd765b86efd3d12bff9bfcb275167aa68dee2 Mon Sep 17 00:00:00 2001
From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com>
Date: Sat, 9 Sep 2023 10:34:43 -0400
Subject: [PATCH 2/8] Bril2json off by one error for initial row

---
 bril-rs/bril2json/src/lib.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/bril-rs/bril2json/src/lib.rs b/bril-rs/bril2json/src/lib.rs
index 032a69b67..cb1c122f9 100644
--- a/bril-rs/bril2json/src/lib.rs
+++ b/bril-rs/bril2json/src/lib.rs
@@ -82,17 +82,22 @@ impl Lines {
                 self.new_lines
                     .iter()
                     .enumerate()
+                    //(i+1) because line numbers start at 1
                     .map(|(i, j)| (i + 1, j))
                     .fold(
                         ColRow {
-                            col: index as u64,
+                            // (index + 1) because column numbers start at 1
+                            col: (index + 1) as u64,
+                            // Hard code the first row to be 1
                             row: 1,
                         },
                         |current, (line_num, idx)| {
                             if *idx < index {
                                 ColRow {
+                                    // (line_num + 1) because line numbers start at 1
                                     row: (line_num + 1) as u64,
-                                    col: (index - idx) as u64,
+                                    // column values are kept relative to the previous index
+                                    col: ((index) - idx) as u64,
                                 }
                             } else {
                                 current

From f8e213f0f16d53bcf355dfc4ae585e8026cab3f5 Mon Sep 17 00:00:00 2001
From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com>
Date: Sat, 9 Sep 2023 10:54:11 -0400
Subject: [PATCH 3/8] Stricter cli arg parsing in brili

---
 brili.ts | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/brili.ts b/brili.ts
index 348b36f6c..ff6e2a8b3 100644
--- a/brili.ts
+++ b/brili.ts
@@ -764,7 +764,7 @@ function evalInstr(instr: bril.Instruction, state: State): Action {
     let i = getInt(instr, state.env, 0);
     if (i > 1114111 || i < 0 || (55295 < i && i < 57344)) {
       throw error(`value ${i} cannot be converted to char`);
-    } 
+    }
     let val = String.fromCodePoint(Number(i));
     state.env.set(instr.dest, val);
     return NEXT;
@@ -873,7 +873,12 @@ function parseBool(s: string): boolean {
 
 function parseNumber(s: string): number {
   let f = parseFloat(s);
-  if (!isNaN(f)) {
+  // parseFloat and Number have subtly different behaviors for parsing strings
+    // parseFloat ignores all random garbage after any valid number
+    // Number accepts empty/whitespace only strings and rejects numbers with seperators
+  // Use both and only accept the intersection of the results?
+  let f2 = Number(s);
+  if (!isNaN(f) && f === f2) {
     return f;
   } else {
     throw error(`float argument to main must not be 'NaN'; got ${s}`);
@@ -891,7 +896,8 @@ function parseMainArguments(expected: bril.Argument[], args: string[]) : Env {
     let type = expected[i].type;
     switch (type) {
       case "int":
-        let n: bigint = BigInt(parseInt(args[i]));
+        // https://dev.to/darkmavis1980/you-should-stop-using-parseint-nbf
+        let n: bigint = BigInt(Number(args[i]));
         newEnv.set(expected[i].name, n as Value);
         break;
       case "float":

From db8ea47e1117ed8a65818504c779af3ca67a20a6 Mon Sep 17 00:00:00 2001
From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com>
Date: Sat, 9 Sep 2023 10:54:36 -0400
Subject: [PATCH 4/8] Fix mod_inv.bril

---
 benchmarks/core/mod_inv.bril | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/benchmarks/core/mod_inv.bril b/benchmarks/core/mod_inv.bril
index f5ee57cf6..7f02408f3 100644
--- a/benchmarks/core/mod_inv.bril
+++ b/benchmarks/core/mod_inv.bril
@@ -1,4 +1,4 @@
-# ARGS: 46, 10007
+# ARGS: 46 10007
 @main(n: int, p: int) {
   v0: int = const 2;
   two: int = id v0;
@@ -81,10 +81,8 @@
 #   }
 #   console.log(ans);
 # }
-# 
+#
 # function mod(n: bigint, p: bigint): bigint {
 #   return n - n / p * p;
 # }
 #
-
-

From 05968875d02781b02c7a80b63307160fcde01656 Mon Sep 17 00:00:00 2001
From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com>
Date: Sat, 9 Sep 2023 11:38:42 -0400
Subject: [PATCH 5/8] Give the ts compiler special handling for returns in then
 branches

---
 bril-ts/builder.ts     | 28 +++++++++++++++++++++++++++-
 test/ts/factorial.out  |  2 --
 test/ts/simplified.out | 13 +++++++++++++
 test/ts/simplified.ts  |  8 ++++++++
 ts2bril.ts             | 24 +++++++++++++++---------
 5 files changed, 63 insertions(+), 12 deletions(-)
 create mode 100644 test/ts/simplified.out
 create mode 100644 test/ts/simplified.ts

diff --git a/bril-ts/builder.ts b/bril-ts/builder.ts
index 775948c08..0f68f9924 100644
--- a/bril-ts/builder.ts
+++ b/bril-ts/builder.ts
@@ -67,7 +67,7 @@ export class Builder {
       return this.buildEffect("call", args, [func], undefined);
     }
   }
-  
+
   /**
    * Build a constant instruction. As above, the destination name is optional.
    */
@@ -124,6 +124,32 @@ export class Builder {
     this.curFunction.instrs.push(instr);
   }
 
+  /**
+   * Checks whether the last emitted instruction in the current function is the specified op code.
+   * Useful for checking for terminating instructions.
+   */
+  isLastEmittedEffectOp(op: bril.EffectOpCode): boolean {
+    if (!this.curFunction) {
+      return false
+    }
+
+    if (!this.curFunction.instrs) {
+      return false
+    }
+
+    if (this.curFunction.instrs.length === 0) {
+      return false
+    }
+
+    const last_instr : bril.Instruction | bril.Label = this.curFunction.instrs[this.curFunction.instrs.length - 1];
+
+    if ('label' in last_instr) {
+      return false
+    }
+
+    return last_instr.op === op
+  }
+
   /**
    * Generate an unused variable name.
    */
diff --git a/test/ts/factorial.out b/test/ts/factorial.out
index cf0ff0cdb..b2cd75b60 100644
--- a/test/ts/factorial.out
+++ b/test/ts/factorial.out
@@ -16,9 +16,7 @@
 .then.0:
   v4: int = const 1;
   ret v4;
-  jmp .endif.0;
 .else.0:
-.endif.0:
   v5: int = id x;
   v6: int = id x;
   v7: int = const 1;
diff --git a/test/ts/simplified.out b/test/ts/simplified.out
new file mode 100644
index 000000000..cf4144983
--- /dev/null
+++ b/test/ts/simplified.out
@@ -0,0 +1,13 @@
+@main {
+  v0: int = const 1;
+  call @erronious v0;
+  v1: int = const 0;
+}
+@erronious(x: int): int {
+  v1: bool = const true;
+  br v1 .then.0 .else.0;
+.then.0:
+  v2: int = id x;
+  ret v2;
+.else.0:
+}
diff --git a/test/ts/simplified.ts b/test/ts/simplified.ts
new file mode 100644
index 000000000..6e53b939b
--- /dev/null
+++ b/test/ts/simplified.ts
@@ -0,0 +1,8 @@
+erronious(1n);
+
+// This snippet checks that the ts compiler doesn't try to create an unreachable else branch
+function erronious(x: bigint): bigint {
+    if (true) {
+        return x;
+    }
+}
diff --git a/ts2bril.ts b/ts2bril.ts
index f2a1929ac..7b198015b 100644
--- a/ts2bril.ts
+++ b/ts2bril.ts
@@ -145,7 +145,7 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program {
 
         // Check if effect statement, i.e., a call that is not a subexpression
         if (call.parent.kind === ts.SyntaxKind.ExpressionStatement) {
-          builder.buildCall(callText, 
+          builder.buildCall(callText,
             values.map(v => v.dest));
           return builder.buildInt(0);  // Expressions must produce values
         } else {
@@ -153,12 +153,12 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program {
           let type = brilType(decl, checker);
           let name = (decl.name != undefined) ? decl.name.getText() : undefined;
           return builder.buildCall(
-            callText, 
-            values.map(v => v.dest), 
-            type, 
+            callText,
+            values.map(v => v.dest),
+            type,
             name,
           );
-        } 
+        }
       }
     default:
       throw `unsupported expression kind: ${expr.getText()}`;
@@ -215,12 +215,18 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program {
         // Statement chunks.
         builder.buildLabel(thenLab);
         emit(if_.thenStatement);
-        builder.buildEffect("jmp", [], undefined, [endLab]);
+        const then_branch_terminated = builder.isLastEmittedEffectOp("ret");
+        if (!then_branch_terminated) {
+          builder.buildEffect("jmp", [], undefined, [endLab]);
+        }
         builder.buildLabel(elseLab);
         if (if_.elseStatement) {
           emit(if_.elseStatement);
         }
-        builder.buildLabel(endLab);
+        // The else branch otherwise just falls through without needing a target label
+        if (!then_branch_terminated) {
+          builder.buildLabel(endLab);
+        }
 
         break;
       }
@@ -258,7 +264,7 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program {
         break;
       }
 
-      case ts.SyntaxKind.FunctionDeclaration: 
+      case ts.SyntaxKind.FunctionDeclaration:
         let funcDef = node as ts.FunctionDeclaration;
         if (funcDef.name === undefined) {
           throw `no anonymous functions!`;
@@ -298,7 +304,7 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program {
 
       case ts.SyntaxKind.ImportDeclaration:
         break;
-  
+
       default:
         throw `unhandled TypeScript AST node kind ${ts.SyntaxKind[node.kind]}`;
     }

From 00872a753503591dc36b8b7cb791228084aac5ae Mon Sep 17 00:00:00 2001
From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com>
Date: Sat, 9 Sep 2023 11:41:43 -0400
Subject: [PATCH 6/8] Fix benchmarks with dead jumps

---
 benchmarks/core/bitshift.bril | 2 --
 benchmarks/core/fact.bril     | 2 --
 2 files changed, 4 deletions(-)

diff --git a/benchmarks/core/bitshift.bril b/benchmarks/core/bitshift.bril
index f7a79defa..c3c35fc82 100644
--- a/benchmarks/core/bitshift.bril
+++ b/benchmarks/core/bitshift.bril
@@ -7,7 +7,6 @@
 .then.0:
   v4: int = id x;
   ret v4;
-  jmp .endif.0;
 .else.0:
   v5: int = id x;
   v6: int = id n;
@@ -37,7 +36,6 @@
 .endif.12:
   v22: int = id ans;
   ret v22;
-.endif.0:
 }
 @mod(a: int, b: int): int {
   v0: int = id a;
diff --git a/benchmarks/core/fact.bril b/benchmarks/core/fact.bril
index 52f3f2d49..947725523 100644
--- a/benchmarks/core/fact.bril
+++ b/benchmarks/core/fact.bril
@@ -16,7 +16,6 @@
 .then.0:
   v4: int = const 1;
   ret v4;
-  jmp .endif.0;
 .else.0:
   v5: int = id a;
   v6: int = id a;
@@ -25,5 +24,4 @@
   v9: int = call @fact v8;
   v10: int = mul v5 v9;
   ret v10;
-.endif.0:
 }

From 1a276e5ede3b5177ef6f59da7ba08c9abf28a9e3 Mon Sep 17 00:00:00 2001
From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com>
Date: Sat, 9 Sep 2023 11:47:35 -0400
Subject: [PATCH 7/8] Move main typecheck to typechecking rather than runtime

---
 brilirs/src/check.rs  | 5 +++++
 brilirs/src/interp.rs | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/brilirs/src/check.rs b/brilirs/src/check.rs
index f14a56e12..d115ea4d5 100644
--- a/brilirs/src/check.rs
+++ b/brilirs/src/check.rs
@@ -508,6 +508,11 @@ fn type_check_instruction<'a>(
 }
 
 fn type_check_func(bbfunc: &BBFunction, bbprog: &BBProgram) -> Result<(), PositionalInterpError> {
+  if bbfunc.name == "main" && bbfunc.return_type.is_some() {
+    return Err(InterpError::NonEmptyRetForFunc(bbfunc.name.clone()))
+      .map_err(|e| e.add_pos(bbfunc.pos.clone()));
+  }
+
   let mut env: FxHashMap<&str, &Type> =
     FxHashMap::with_capacity_and_hasher(20, fxhash::FxBuildHasher::default());
   bbfunc.args.iter().for_each(|a| {
diff --git a/brilirs/src/interp.rs b/brilirs/src/interp.rs
index 17c5a5596..de8d5fdab 100644
--- a/brilirs/src/interp.rs
+++ b/brilirs/src/interp.rs
@@ -796,11 +796,6 @@ pub fn execute_main<T: std::io::Write, U: std::io::Write>(
     .map(|i| prog.get(i).unwrap())
     .ok_or(InterpError::NoMainFunction)?;
 
-  if main_func.return_type.is_some() {
-    return Err(InterpError::NonEmptyRetForFunc(main_func.name.clone()))
-      .map_err(|e| e.add_pos(main_func.pos.clone()));
-  }
-
   let mut env = Environment::new(main_func.num_of_vars);
   let heap = Heap::default();
 

From 67ddbe297765c6f840ddbe429df3a0aa80f44271 Mon Sep 17 00:00:00 2001
From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com>
Date: Sun, 10 Sep 2023 17:39:23 -0400
Subject: [PATCH 8/8] generalize to getLastInstr

---
 bril-ts/builder.ts | 12 ++++++------
 ts2bril.ts         |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/bril-ts/builder.ts b/bril-ts/builder.ts
index 0f68f9924..63996077e 100644
--- a/bril-ts/builder.ts
+++ b/bril-ts/builder.ts
@@ -128,26 +128,26 @@ export class Builder {
    * Checks whether the last emitted instruction in the current function is the specified op code.
    * Useful for checking for terminating instructions.
    */
-  isLastEmittedEffectOp(op: bril.EffectOpCode): boolean {
+  getLastInstr(): bril.Instruction | undefined {
     if (!this.curFunction) {
-      return false
+      return undefined
     }
 
     if (!this.curFunction.instrs) {
-      return false
+      return undefined
     }
 
     if (this.curFunction.instrs.length === 0) {
-      return false
+      return undefined
     }
 
     const last_instr : bril.Instruction | bril.Label = this.curFunction.instrs[this.curFunction.instrs.length - 1];
 
     if ('label' in last_instr) {
-      return false
+      return undefined
     }
 
-    return last_instr.op === op
+    return last_instr
   }
 
   /**
diff --git a/ts2bril.ts b/ts2bril.ts
index 7b198015b..b009df1b9 100644
--- a/ts2bril.ts
+++ b/ts2bril.ts
@@ -215,7 +215,7 @@ function emitBril(prog: ts.Node, checker: ts.TypeChecker): bril.Program {
         // Statement chunks.
         builder.buildLabel(thenLab);
         emit(if_.thenStatement);
-        const then_branch_terminated = builder.isLastEmittedEffectOp("ret");
+        const then_branch_terminated = builder.getLastInstr()?.op === "ret";
         if (!then_branch_terminated) {
           builder.buildEffect("jmp", [], undefined, [endLab]);
         }