Fix try/catch catches more than it should #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.

Now a `try exp catch handler` expression generates code like:

    TRY_BEGIN handler
    <exp>
    TRY_END
    JUMP past_handler
    handler: <handler>
    past_handler:
    ...

On backtrack through TRY_BEGIN it just backtracks.

If anything past the whole thing raises when <exp> produced a value,
then the TRY_END will catch the error, wrap it in another, and
backtrack.  The TRY_BEGIN will see a wrapped error and then it will
unwrap and re-raise the error.

If <exp> raises, then TRY_BEGIN will catch the error and jump to the
handler, but the TRY_BEGIN will not stack_save() in that case, so on
raise/backtrack the TRY_BEGIN will not execute again (nor will the
TRY_END).

Author: nicowilliams

src/compile.c

@@ -1034,43 +1034,40 @@ block gen_cond(block cond, block iftrue, block iffalse) {
                               BLOCK(gen_op_simple(POP), iffalse)));
 }
 
-block gen_try_handler(block handler) {
-  // Quite a pain just to hide jq's internal errors.
-  return gen_cond(// `if type=="object" and .__jq
-                  gen_and(gen_call("_equal",
-                                   BLOCK(gen_lambda(gen_const(jv_string("object"))),
-                                         gen_lambda(gen_call("type", gen_noop())))),
-                          BLOCK(gen_subexp(gen_const(jv_string("__jq"))),
-                                gen_noop(),
-                                gen_op_simple(INDEX))),
-                  // `then error`
-                  gen_call("error", gen_noop()),
-                  // `else HANDLER end`
-                  handler);
-}
-
 block gen_try(block exp, block handler) {
   /*
-   * Produce something like:
-   *  FORK_OPT <address of handler>
+   * Produce:
+   *
+   *  TRY_BEGIN handler
    *  <exp>
-   *  JUMP <end of handler>
-   *  <handler>
+   *  TRY_END
+   *  JUMP past_handler
+   *  handler: <handler>
+   *  past_handler:
    *
-   * If this is not an internal try/catch, then catch and re-raise
-   * internal errors to prevent them from leaking.
+   * If <exp> backtracks then TRY_BEGIN will backtrack.
    *
-   * 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.
+   * If <exp> produces a value then we'll execute whatever bytecode follows
+   * this sequence.  If that code raises an exception, then TRY_END will wrap
+   * and re-raise that exception, and TRY_BEGIN will unwrap and re-raise the
+   * exception (see jq_next()).
+   *
+   * If <exp> raises then the TRY_BEGIN will see a non-wrapped exception and
+   * will jump to the handler (note the TRY_END will not execute in this case),
+   * and if the handler produces any values, then we'll execute whatever
+   * bytecode follows this sequence.  Note that TRY_END will not execute in
+   * this case, so if the handler raises an exception, or code past the handler
+   * raises an exception, then that exception won't be wrapped and re-raised,
+   * and the TRY_BEGIN will not catch it because it does not stack_save() when
+   * it branches to the handler.
    */
-  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))
+    handler = BLOCK(gen_op_simple(DUP), gen_op_simple(POP));
+
+  block jump = gen_op_target(JUMP, handler);
+  return BLOCK(gen_op_target(TRY_BEGIN, jump), exp, gen_op_simple(TRY_END),
+               jump, handler);
 }
 
 block gen_label(const char *label, block exp) {

src/compile.h

@@ -59,7 +59,6 @@ block gen_destructure(block var, block matcher, block body);
 block gen_destructure_alt(block matcher);
 
 block gen_cond(block cond, block iftrue, block iffalse);
-block gen_try_handler(block handler);
 block gen_try(block exp, block handler);
 block gen_label(const char *label, block exp);
 

src/execute.c

@@ -346,6 +346,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);
   while (1) {
@@ -806,15 +807,66 @@ 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 ...` -- EXP backtracked (e.g., EXP was `empty`), so we
+         * backtrack more:
+         */
+        jv_free(stack_pop(jq));
+        goto do_backtrack;
+      }
+
+      /*
+       * Else `(try EXP ... ) | EXP2` raised an error.
+       *
+       * If the error was wrapped in another error, then that means EXP2 raised
+       * the error.  We unwrap it and re-raise it as it wasn't raised by EXP.
+       *
+       * See commentary in gen_try().
+       */
+      jv e = jv_invalid_get_msg(jv_copy(jq->error));
+      if (!jv_is_valid(e) && jv_invalid_has_msg(jv_copy(e))) {
+        set_error(jq, e);
+        goto do_backtrack;
+      }
+      jv_free(e);
+
+      /*
+       * Else we caught an error containing a non-error value, so we jump to
+       * the handler.
+       *
+       * See commentary in gen_try().
+       */
+      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_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)