Merge pull request #294 from Pat-Lafon/main

Miscellaneous fixes

.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

.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:

.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

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;

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:
 }

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;
 # }
 #
-
-

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

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.
+   */
+  getLastInstr(): bril.Instruction | undefined {
+    if (!this.curFunction) {
+      return undefined
+    }
+
+    if (!this.curFunction.instrs) {
+      return undefined
+    }
+
+    if (this.curFunction.instrs.length === 0) {
+      return undefined
+    }
+
+    const last_instr : bril.Instruction | bril.Label = this.curFunction.instrs[this.curFunction.instrs.length - 1];
+
+    if ('label' in last_instr) {
+      return undefined
+    }
+
+    return last_instr
+  }
+
   /**
    * Generate an unused variable name.
    */

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":

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| {

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();
 

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;

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:
+}

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;
+    }
+}

ts2bril.ts

@@ -145,20 +145,20 @@ 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 {
           let decl = call.parent as ts.VariableDeclaration;
           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.getLastInstr()?.op === "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]}`;
     }