Fix try/catch catches more than it should #1859

Close #1885, #2140, #2011, #2220, #2485, #2073

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

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

@@ -348,6 +348,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) {
@@ -808,7 +809,59 @@ 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));
@@ -816,7 +869,6 @@ jv jq_next(jq_state *jq) {
       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.y

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

tests/jq.test

@@ -1834,3 +1834,58 @@ null
 debug
 1
 1
+
+# try/catch catches more than it should #1859
+"foo" | try ((try . catch "caught too much") | error) catch "caught just right"
+null
+"caught just right"
+
+.[]|(try (if .=="hi" then . else error end) catch empty) | "\(.) there!"
+["hi","ho"]
+"hi there!"
+
+try (["hi","ho"]|.[]|(try . catch (if .=="ho" then "BROKEN"|error else empty end)) | if .=="ho" then error else "\(.) there!" end) catch "caught outside \(.)"
+null
+"hi there!"
+"caught outside ho"
+
+.[]|(try . catch (if .=="ho" then "BROKEN"|error else empty end)) | if .=="ho" then error else "\(.) there!" end
+["hi","ho"]
+"hi there!"
+
+try (try error catch "inner catch \(.)") catch "outer catch \(.)"
+"foo"
+"inner catch foo"
+
+try ((try error catch "inner catch \(.)")|error) catch "outer catch \(.)"
+"foo"
+"outer catch inner catch foo"
+
+# Also #1859, but from #1885
+first(.?,.?)
+null
+null
+
+# Also #1859, but from #2140
+{foo: "bar"} | .foo |= .?
+null
+{"foo": "bar"}
+
+# Also #1859, but from #2220
+. |= try 2
+1
+2
+
+. |= try 2 catch 3
+1
+2
+
+.[] |= try tonumber
+["1", "2a", "3", 4]
+[1, 3, 4]
+
+# Also 1859, but from 2073
+any(keys[]|tostring?;true)
+{"a":"1","b":"2","c":"3"}
+true
+