@wip bugfixes

both in the BC->IR compiler and in parsing/printing

currently fixing node IDs parsed and printed outside of the CFG they were defined in.

src/main/java/org/prlprg/bc/BcInstr.java

@@ -934,7 +934,7 @@ public BcOp op() {
     }
   }
 
-  record BaseGuard(ConstPool.Idx<LangSXP> expr, @LabelName("baseGuardFail") BcLabel ifFail)
+  record BaseGuard(ConstPool.Idx<LangSXP> expr, @LabelName("baseGuardAfter") BcLabel ifFail)
       implements BcInstr {
     @Override
     public BcOp op() {

src/main/java/org/prlprg/bc2ir/CFGCompiler.java

@@ -320,27 +320,36 @@ private CFGCompiler(Bc bc, CFG cfg, boolean isPromise, Module module) {
 
   /** Actually compile {@code bc} into {@code cfg}. */
   void doCompile() {
-    var code = bc.code();
-
-    // Put BBs at jump destinations.
-    // We will add more BBs when compiling some instructions, but IR in those BBs will be specific
-    // to the instruction, whereas labels in instructions may jump to within the IR of previously-
-    // compiled instructions. To insert a BB later that jumps to previously-compiled IR: it's
-    // possible, but adds complexity, because we have to remember which bytecode positions
-    // correspond to which IR positions at these positions, and move already-compiled IR into the
-    // new BB. It's easier to insert these BBs first, then we don't have to move IR and we only need
-    // to track the current bytecode and IR position.
-    for (var i = 0; i < code.size(); i++) {
-      addBcLabelBBs(code.get(i));
-    }
+    try {
+      var code = bc.code();
+
+      // Put BBs at jump destinations.
+      // We will add more BBs when compiling some instructions, but IR in those BBs will be specific
+      // to the instruction, whereas labels in instructions may jump to within the IR of previously-
+      // compiled instructions. To insert a BB later that jumps to previously-compiled IR: it's
+      // possible, but adds complexity, because we have to remember which bytecode positions
+      // correspond to which IR positions at these positions, and move already-compiled IR into the
+      // new BB. It's easier to insert these BBs first, then we don't have to move IR and we only need
+      // to track the current bytecode and IR position.
+      for (var i = 0; i < code.size(); i++) {
+        addBcLabelBBs(code.get(i));
+      }
 
-    // Add instructions to BBs, including jumps which connect them.
-    // Also, move to the BB at each label.
-    for (bcPos = 0; bcPos < code.size(); bcPos++) {
-      if (bcPos != 0 && bbByLabel.containsKey(bcPos)) {
-        moveTo(bbByLabel.get(bcPos));
+      // Add instructions to BBs, including jumps which connect them.
+      // Also, move to the BB at each label.
+      for (bcPos = 0; bcPos < code.size(); bcPos++) {
+        if (bcPos != 0 && bbByLabel.containsKey(bcPos)) {
+          var nextBb = bbByLabel.get(bcPos);
+          if (cursor.bb().jump() == null) {
+            cursor.insert(new JumpData.Goto(nextBb));
+          }
+          moveTo(nextBb);
+        }
+        addBcInstrIrInstrs(code.get(bcPos));
       }
-      addBcInstrIrInstrs(code.get(bcPos));
+    } catch (Throwable e) {
+      throw e instanceof CFGCompilerException e1 ? e1 :
+          new CFGCompilerException("uncaught exception: " + e, bc, bcPos, cursor, e);
     }
   }
 
@@ -667,7 +676,7 @@ case Or(var ast) ->
               new StmtData.LOr(Optional.of(get(ast)), pop(RValue.class), pop(RValue.class), env));
       case Not(var ast) ->
           pushInsert(new StmtData.Not(Optional.of(get(ast)), pop(RValue.class), env));
-      case DotsErr() -> pushInsert(new StmtData.Error("'...' used in an incorrect context", env));
+      case DotsErr() -> insert(new StmtData.Error("'...' used in an incorrect context", env));
       case StartAssign(var _), EndAssign(var _) -> throw failUnsupported("complex assignment");
       case StartSubset(var ast, var after) ->
           compileStartDispatch(Dispatch.Type.SUBSET, ast, after);
@@ -1037,8 +1046,8 @@ case BaseGuard(var expr, var after) -> {
         var base = cursor.insert(new StmtData.LdFun(fun, StaticEnv.BASE));
         var guard = cursor.insert(new StmtData.Eq(Optional.of(get(expr)), sym, base, env));
 
-        var safeBb = cfg.addBB();
-        var fallbackBb = cfg.addBB();
+        var safeBb = cfg.addBB("baseGuardSafe");
+        var fallbackBb = cfg.addBB("baseGuardFail");
         var afterBb = bbAt(after);
         cursor.insert(new JumpData.Branch(guard, safeBb, fallbackBb));
 
@@ -1161,9 +1170,8 @@ private void compileCall(LangSXP ast) {
                       ? new StmtData.NamedCall(ast, fun, names, args, env, compileFrameState())
                       : new StmtData.Call(ast, fun, args, env, compileFrameState());
               case Call.Fun.Builtin(var fun) ->
-                  isNamed
-                      ? new StmtData.Error("Name in builtin call", env)
-                      : new StmtData.CallBuiltin(ast, fun, args, env);
+                  // Even if `isNamed` is true, R just ignores the names.
+                  new StmtData.CallBuiltin(ast, fun, args, env);
             });
     push(callInstr);
   }
@@ -1190,14 +1198,15 @@ private FrameState compileFrameState() {
    * {@link #bbAt(BcLabel)} leaves the stack unchanged since it's still in the oly block.
    */
   private void moveTo(BB bb) {
-    cursor.moveToStart(bb);
-
     // Make sure this block has phis representing the arguments currently on the stack, then replace
     // the stack with those phis (this is how to convert a stack-based bytecode to an SSA-form IR).
     addPhiInputsForStack(bb);
     // Then replace the stack with those phis.
     stack.clear();
     stack.addAll(bb.phis());
+
+    // Finally, actually move the cursor to the BB.
+    cursor.moveToStart(bb);
   }
 
   /**
@@ -1221,6 +1230,11 @@ private void addPhiInputsForStack(BB bb) {
   private void addPhiInputsForStack(BB bb, BB incoming) {
     // Ensure the BB has phis for each stack value.
     if (bbsWithPhis.add(bb)) {
+      // Add phis for all of the nodes on the stack.
+      for (var node : stack) {
+        bb.addPhi(node.getClass());
+      }
+    } else {
       // Already added phis,
       // but sanity-check that they're still same type and amount as the nodes on the stack.
       var phis = bb.phis().iterator();
@@ -1235,7 +1249,7 @@ private void addPhiInputsForStack(BB bb, BB incoming) {
                   + i);
         }
         var phi = phis.next();
-        if (phi.getClass() != stack.get(i).getClass()) {
+        if (!phi.nodeClass().isInstance(stack.get(i))) {
           throw fail(
               "BB stack mismatch: "
                   + bb.id()
@@ -1255,17 +1269,12 @@ private void addPhiInputsForStack(BB bb, BB incoming) {
         }
         throw fail(
             "BB stack mismatch: "
-                + bb
+                + bb.id()
                 + " has too many phis; expected "
                 + stack.size()
                 + " but got "
                 + i);
       }
-    } else {
-      // Add phis for all of the nodes on the stack.
-      for (var node : stack) {
-        bb.addPhi(node.getClass());
-      }
     }
 
     // Add an input to each phi, for the corresponding stack value and the current BB.

src/main/java/org/prlprg/bc2ir/CFGCompilerException.java

@@ -1,5 +1,6 @@
 package org.prlprg.bc2ir;
 
+import javax.annotation.Nullable;
 import org.prlprg.bc.Bc;
 import org.prlprg.ir.cfg.BB;
 import org.prlprg.ir.cfg.CFG;
@@ -18,7 +19,11 @@ public class CFGCompilerException extends RuntimeException {
   private final CFGCursor irPos;
 
   CFGCompilerException(String message, Bc bc, int bcPos, CFGCursor irPos) {
-    super(message);
+    this(message, bc, bcPos, irPos, null);
+  }
+
+  CFGCompilerException(String message, Bc bc, int bcPos, CFGCursor irPos, @Nullable Throwable cause) {
+    super(message, cause);
     this.bc = bc;
     this.bcPos = bcPos;
     this.irPos = irPos;
@@ -50,6 +55,6 @@ public String getMessage() {
         + " in:\n\n"
         + irPos.cfg()
         + "\n\n"
-        + bc;
+        + bc.code();
   }
 }

src/main/java/org/prlprg/ir/cfg/CFGEdit.java

@@ -235,6 +235,7 @@ public RemovePhis apply(CFG cfg) {
     public Iterable<? extends InsertPhi<?>> subEdits() {
       return phis.stream()
           .map(
+              // Sometimes this line fails to compile. Clean/recompile and it should magically pass.
               a -> new InsertPhi<>(bbId, a.id(), a.nodeClass(), ImmutableList.copyOf(a.inputIds())))
           .toList();
     }

src/main/java/org/prlprg/ir/cfg/CFGParsePrint.java

@@ -110,36 +110,39 @@ private void printBB(BB bb, Printer p) {
 
     p.print(bb.id());
 
-    // Phis
-    if (!bb.phis().isEmpty()) {
-      w.write("(\n    ");
-      var first = true;
-      for (var phi : bb.phis()) {
-        if (first) {
-          first = false;
-        } else {
-          w.write(",\n    ");
-        }
-        bbP.print(phi);
+    w.runIndented(() -> {
+      // Phis
+      if (!bb.phis().isEmpty()) {
+        w.runIndented(() -> {
+          w.write("(\n");
+          var first = true;
+          for (var phi : bb.phis()) {
+            if (first) {
+              first = false;
+            } else {
+              w.write(",\n");
+            }
+            bbP.print(phi);
+          }
+          w.write(')');
+        });
       }
-      w.write(')');
-    }
 
-    // Instructions (statements and jump)
-    w.write(":\n");
-    // Statements
-    for (var stmt : bb.stmts()) {
+      // Instructions (statements and jump)
+      w.write(":\n");
+      // Statements
+      for (var stmt : bb.stmts()) {
+        bbP.print(stmt);
+        w.write(";\n");
+      }
+      // Jump
       w.write("  ");
-      bbP.print(stmt);
-      w.write(";\n");
-    }
-    // Jump
-    w.write("  ");
-    if (bb.jump() != null) {
-      bbP.print(bb.jump());
-    } else {
-      w.write("null");
-    }
+      if (bb.jump() != null) {
+        bbP.print(bb.jump());
+      } else {
+        w.write("null");
+      }
+    });
     w.write(";\n");
   }
 }
@@ -169,7 +172,7 @@ private Phi<?> parsePhi(Parser p) {
 
     var inputP = p.withContext(dataContext);
     var inputs = new ArrayList<Phi.Input<?>>();
-    s.assertAndSkip(" = φ(");
+    s.assertAndSkip("= φ(");
     while (!s.trySkip(')')) {
       inputs.add(inputP.parse(Phi.Input.class));
       if (s.trySkip(')')) {

src/main/java/org/prlprg/ir/cfg/Instr.java

@@ -59,7 +59,7 @@ public sealed interface Instr extends InstrOrPhi permits Jump, Stmt {
   static <I extends Instr> void mutateArgs(I instr, InstrData<I> newArgs) {
     if (!((InstrImpl<?>) instr).canReplaceDataWith(newArgs)) {
       throw new IllegalArgumentException(
-          "Incompatible data for replacement: " + instr + " -> " + newArgs);
+          "incompatible data for replacement: " + instr + " -> " + newArgs);
     }
 
     var wasEmpty = false;
@@ -191,7 +191,7 @@ public final ImmutableList<Node> args() {
   private void computeArgs() {
     // Reflectively get all Node record components
     if (!(data instanceof Record r)) {
-      throw new AssertionError("InstrData must be a record");
+      throw new AssertionError("`InstrData` must be a record");
     }
     var components = data.getClass().getRecordComponents();
 
@@ -239,7 +239,7 @@ protected final void unsafeReplaceInData(String argTypeStr, Object old, Object r
     // and throw an exception if `old` isn't present or `replacement` is the wrong type.
     // Also, build an array of new arguments, where `old` is replaced with `replacement`.
     if (!(data instanceof Record r)) {
-      throw new AssertionError("InstrData must be a record");
+      throw new AssertionError("`InstrData` must be a record");
     }
     var cls = Classes.classOf(data);
     var components = cls.getRecordComponents();
@@ -256,7 +256,7 @@ protected final void unsafeReplaceInData(String argTypeStr, Object old, Object r
 
         if (!cmp.getType().isInstance(replacement)) {
           throw new IllegalArgumentException(
-              "Replacement "
+              "replacement "
                   + argTypeStr
                   + " is of wrong type: required "
                   + cmp.getType().getSimpleName()
@@ -273,7 +273,7 @@ protected final void unsafeReplaceInData(String argTypeStr, Object old, Object r
 
           if (!elemClass.isInstance(replacement)) {
             throw new IllegalArgumentException(
-                "Replacement "
+                "replacement "
                     + argTypeStr
                     + " (in optional) is of wrong type: required "
                     + cmp.getType().getSimpleName()
@@ -293,7 +293,7 @@ protected final void unsafeReplaceInData(String argTypeStr, Object old, Object r
 
             if (!elemClass.isInstance(replacement)) {
               throw new IllegalArgumentException(
-                  "Replacement "
+                  "replacement "
                       + argTypeStr
                       + " (in collection) is of wrong type: required "
                       + cmp.getType().getSimpleName()
@@ -309,7 +309,7 @@ protected final void unsafeReplaceInData(String argTypeStr, Object old, Object r
 
               if (!elemClass.isInstance(replacement)) {
                 throw new IllegalArgumentException(
-                    "Replacement "
+                    "replacement "
                         + argTypeStr
                         + " (in optional in collection) is of wrong type: required "
                         + cmp.getType().getSimpleName()
@@ -350,7 +350,7 @@ public final void replaceInArgs(Node old, Node replacement) {
    */
   private static Class<?> optionalOrCollectionComponentElementClass(RecordComponent cmp) {
     assert cmp.getType() == Optional.class || cmp.getType() == ImmutableList.class
-        : "InstrData has Collection component which isn't an ImmutableList";
+        : "`InstrData` has `Collection` component which isn't an `ImmutableList`";
     if (cmp.getGenericType() instanceof ParameterizedType p
         && p.getActualTypeArguments().length == 1) {
       if (p.getActualTypeArguments()[0] instanceof ParameterizedType innerP
@@ -363,7 +363,7 @@ private static Class<?> optionalOrCollectionComponentElementClass(RecordComponen
       }
     }
     throw new AssertionError(
-        "InstrData has Collection component which isn't a straightforward generic type, don't know how to handle: "
+        "`InstrData` has `Collection` component which isn't a straightforward generic type, don't know how to handle: "
             + cmp.getGenericType());
   }
 
@@ -376,7 +376,7 @@ private void computeEffects() {
     var clazz = data().getClass();
     effects =
         mergeComputed(
-            "Effects",
+            "effects",
             data().computeEffects(),
             computeEffectsFromAnnotation(
                 clazz.getAnnotation(EffectsAre.class),
@@ -403,14 +403,14 @@ private void computeEffects() {
     throw new UnreachableError();
   }
 
-  protected static <T> T mergeComputed(
+  protected <T> T mergeComputed(
       String desc, @Nullable T fromOverride, @Nullable T fromAnnotation) {
     if (fromOverride == null && fromAnnotation == null) {
-      throw new AssertionError(desc + " must be computed either by annotation or by override");
+      throw new AssertionError(desc + " must be computed either by annotation or by override.\nIn `" + data.getClass().getSimpleName() + "`");
     }
     if (fromOverride != null && fromAnnotation != null) {
       throw new AssertionError(
-          desc + " must be computed either by annotation or by override, not both");
+          desc + " must be computed either by annotation or by override, not both.\nIn `" + data.getClass().getSimpleName() + "`");
     }
     return fromOverride != null ? fromOverride : fromAnnotation;
   }

src/main/java/org/prlprg/ir/cfg/Phi.java

@@ -392,12 +392,12 @@ public N setInput(BB incomingBB, N node) {
 
     if (oldNode == null) {
       throw new IllegalArgumentException(
-          "Incoming BB not a predecessor: " + incomingBB + " (node=" + node + ")");
+          "Incoming BB not a predecessor of " + id + "'s BB: " + incomingBB.id() + " (node = " + node.id() + ")");
     }
     if (!nodeClass.isInstance(node)) {
       throw new IllegalArgumentException(
-          "Tried to add an input of incompatible type: "
-              + node
+          "Tried to add an input of incompatible type to " + id + ": "
+              + node.id()
               + " ("
               + node.getClass().getSimpleName()
               + ") is not an instance of the phi's node class "