WIP: Fix try/catch catches more than it should jqlang#1859

Rename the FORK_OPT opcode to TRY_BEGIN, add a TRY_END opcode, and wrap
errors when raising through a TRY_END so that they will not be caught by
the matching TRY_BEGIN.

src/compile.c

@@ -1074,26 +1074,34 @@ block gen_try_handler(block handler) {
 
 block gen_try(block exp, block handler) {
   /*
-   * Produce something like:
-   *  FORK_OPT <address of handler>
+   * Produce:
+   *  TRY_BEGIN <address of TRY_END>
    *  <exp>
+   *  TRY_END
+   */
+  exp = BLOCK(exp, inst_block(inst_new(TRY_END)));
+  exp = BLOCK(gen_op_target(TRY_BEGIN, exp), exp);
+
+  /*
+   * then
+   *
    *  JUMP <end of handler>
    *  <handler>
    *
-   * If this is not an internal try/catch, then catch and re-raise
-   * internal errors to prevent them from leaking.
+   * if there's a handler.
    *
-   * The handler will only execute if we backtrack to the FORK_OPT with
-   * an error (exception).  If <exp> produces no value then FORK_OPT
-   * will backtrack (propagate the `empty`, as it were.  If <exp>
-   * produces a value then we'll execute whatever bytecode follows this
-   * sequence.
+   * The handler will only execute if we backtrack to the TRY_BEGIN with
+   * an error (exception) and the exception occurred in the original try
+   * block expression and not from any opcodes past the TRY_END.  See
+   * jq_next().
+   *
+   * If <exp> produces no value then TRY_BEGIN will backtrack, propagating
+   * the `empty`.  If <exp> produces a value then we'll execute whatever
+   * bytecode follows this sequence.
    */
-  if (!handler.first && !handler.last)
-    // A hack to deal with `.` as the handler; we could use a real NOOP here
-    handler = BLOCK(gen_op_simple(DUP), gen_op_simple(POP), handler);
-  exp = BLOCK(exp, gen_op_target(JUMP, handler));
-  return BLOCK(gen_op_target(FORK_OPT, exp), exp, handler);
+  if (block_is_noop(handler))
+    return exp;
+  return BLOCK(exp, gen_op_target(JUMP, handler), handler);
 }
 
 block gen_label(const char *label, block exp) {
@@ -1265,6 +1273,7 @@ static int compile(struct bytecode* bc, block b, struct locfile* lf, jv args, jv
   int errors = 0;
   int pos = 0;
   int var_frame_idx = 0;
+  int try_block_id = 0;
   bc->nsubfunctions = 0;
   errors += expand_call_arglist(&b, args, env);
   b = BLOCK(b, gen_op_simple(RET));
@@ -1288,7 +1297,10 @@ static int compile(struct bytecode* bc, block b, struct locfile* lf, jv args, jv
       curr->imm.intval = var_frame_idx++;
       localnames = jv_array_append(localnames, jv_string(curr->symbol));
     }
-
+    if (curr->op == TRY_BEGIN) {
+      assert(curr->imm.target && curr->imm.target->op == TRY_END);
+      curr->imm.target->imm.intval = curr->imm.intval = ++try_block_id;
+    }
     if (curr->op == CLOSURE_CREATE) {
       assert(curr->bound_by == curr);
       curr->imm.intval = bc->nsubfunctions++;
@@ -1384,6 +1396,10 @@ static int compile(struct bytecode* bc, block b, struct locfile* lf, jv args, jv
       assert(curr->imm.target->bytecode_pos > pos); // only forward branches
       code[pos] = curr->imm.target->bytecode_pos - (pos + 1);
       pos++;
+      if (curr->op == TRY_BEGIN)
+        code[pos++] = curr->imm.intval;
+    } else if (curr->op == TRY_END) {
+      code[pos++] = curr->imm.intval;
     } else if (op->length > 1) {
       assert(0 && "codegen not implemented for this operation");
     }

src/execute.c

@@ -441,6 +441,7 @@ jv jq_next(jq_state *jq) {
 
   int raising;
   int backtracking = !jq->initial_execution;
+
   jq->initial_execution = 0;
   assert(jv_get_kind(jq->error) == JV_KIND_NULL);
   assert(jq->parent == 0 || jq->start_limit >= jq->stk.limit);
@@ -910,15 +911,72 @@ jv jq_next(jq_state *jq) {
       break;
     }
 
-    case FORK_OPT:
+    case TRY_BEGIN:
+      stack_save(jq, pc - 1, stack_get_pos(jq));
+      pc++; // skip handler offset this time
+      break;
+
+    case TRY_END:
+      stack_save(jq, pc - 1, stack_get_pos(jq));
+      break;
+
+    case ON_BACKTRACK(TRY_BEGIN): {
+      if (!raising) {
+        // `try EXP ...` backtracked here (no value, `empty`), so we backtrack more
+        jv_free(stack_pop(jq));
+        goto do_backtrack;
+      }
+
+      /*
+       * We're looking for errors of the form {__jq:<error-value>}, and if we
+       * find one then, if it's of the form {__jq:{...}} then we'll unwrap and
+       * re-raise, else just re-raise it.
+       *
+       * On backtrack through TRY_END we'll wrap error values as {__jq:<ev>} so
+       * that we can hit this handling here and not catch those errors here.
+       */
+      jv e = jv_invalid_get_msg(jv_copy(jq->error));
+      jv ev = jv_invalid();
+      if (jv_get_kind(e) == JV_KIND_OBJECT && jv_is_valid((ev = jv_get(jv_copy(e), jv_string("__jq"))))) {
+        if (jv_get_kind(ev) == JV_KIND_OBJECT)
+          set_error(jq, jv_invalid_with_msg(JV_OBJECT(jv_string("__jq"), jv_copy(ev))));
+        else
+          set_error(jq, jv_invalid_with_msg(jv_copy(e)));
+        jv_free(ev);
+        jv_free(e);
+        goto do_backtrack;
+      }
+      jv_free(ev);
+      jv_free(e);
+
+      /*
+       * The error isn't for the form {__jq:...}, so we must catch it in this
+       * TRY_BEGIN.
+       *
+       * Jump to the try/catch handler.
+       */
+      uint16_t offset = *pc++;
+      jv_free(stack_pop(jq)); // free the input
+      stack_push(jq, jv_invalid_get_msg(jq->error));  // push the error's message
+      jq->error = jv_null();
+      pc += offset;
+      break;
+    }
+    case ON_BACKTRACK(TRY_END):
+      // Wrap the error so the matching TRY_BEGIN doesn't catch it
+      if (raising)
+        set_error(jq,
+                  jv_invalid_with_msg(JV_OBJECT(jv_string("__jq"),
+                                                jv_invalid_get_msg(jv_copy(jq->error)))));
+      goto do_backtrack;
+
     case DESTRUCTURE_ALT:
     case FORK: {
       stack_save(jq, pc - 1, stack_get_pos(jq));
       pc++; // skip offset this time
       break;
     }
 
-    case ON_BACKTRACK(FORK_OPT):
     case ON_BACKTRACK(DESTRUCTURE_ALT): {
       if (jv_is_valid(jq->error)) {
         // `try EXP ...` backtracked here (no value, `empty`), so we backtrack more

src/opcode_list.h

@@ -11,9 +11,10 @@ OP(STORE_GLOBAL, GLOBAL, 0, 0)
 OP(INDEX, NONE,     2, 1)
 OP(INDEX_OPT, NONE,     2, 1)
 OP(EACH,  NONE,     1, 1)
-OP(EACH_OPT,  NONE,     1, 1)
+OP(EACH_OPT,  NONE, 1, 1)
 OP(FORK,  BRANCH,   0, 0)
-OP(FORK_OPT,  BRANCH,   0, 0)
+OP(TRY_BEGIN,  BRANCH,   0, 0)
+OP(TRY_END,  NONE,   0, 0)
 OP(JUMP,  BRANCH,   0, 0)
 OP(JUMP_F,BRANCH,   1, 0)
 OP(BACKTRACK, NONE, 0, 0)

src/parser.c

@@ -2576,7 +2576,7 @@ YYLTYPE yylloc = yyloc_default;
   case 19:
 #line 396 "src/parser.y" /* yacc.c:1646  */
     {
-  //$$ = BLOCK(gen_op_target(FORK_OPT, $2), $2, $4);
+  //$$ = BLOCK(gen_op_target(TRY_BEGIN, $2), $2, $4);
   (yyval.blk) = gen_try((yyvsp[-2].blk), gen_try_handler((yyvsp[0].blk)));
 }
 #line 2583 "src/parser.c" /* yacc.c:1646  */
@@ -2585,7 +2585,7 @@ YYLTYPE yylloc = yyloc_default;
   case 20:
 #line 400 "src/parser.y" /* yacc.c:1646  */
     {
-  //$$ = BLOCK(gen_op_target(FORK_OPT, $2), $2, gen_op_simple(BACKTRACK));
+  //$$ = BLOCK(gen_op_target(TRY_BEGIN, $2), $2, gen_op_simple(BACKTRACK));
   (yyval.blk) = gen_try((yyvsp[0].blk), gen_op_simple(BACKTRACK));
 }
 #line 2592 "src/parser.c" /* yacc.c:1646  */

src/parser.y

@@ -394,11 +394,11 @@ Term "as" Patterns '|' Exp {
 } |
 
 "try" Exp "catch" Exp {
-  //$$ = BLOCK(gen_op_target(FORK_OPT, $2), $2, $4);
+  //$$ = BLOCK(gen_op_target(TRY_BEGIN, $2), $2, $4);
   $$ = gen_try($2, gen_try_handler($4));
 } |
 "try" Exp {
-  //$$ = BLOCK(gen_op_target(FORK_OPT, $2), $2, gen_op_simple(BACKTRACK));
+  //$$ = BLOCK(gen_op_target(TRY_BEGIN, $2), $2, gen_op_simple(BACKTRACK));
   $$ = gen_try($2, gen_op_simple(BACKTRACK));
 } |
 "try" Exp "catch" error {