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

@@ -205,6 +205,8 @@ jv block_const(block b) {
  return jv_copy(b.first->imm.constant);
 }
 
+// Generate an instruction that needs a branch and set the branch to be the
+// last instruction in the target.
 block gen_op_target(opcode op, block target) {
  assert(opcode_describe(op)->flags & OP_HAS_BRANCH);
  assert(target.last);
@@ -213,6 +215,7 @@ block gen_op_target(opcode op, block target) {
  return inst_block(i);
 }
 
+// Like gen_op_target(), but set the target later
 block gen_op_targetlater(opcode op) {
  assert(opcode_describe(op)->flags & OP_HAS_BRANCH);
  inst* i = inst_new(op);
@@ -1057,43 +1060,42 @@ 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>
+ * JUMP try_end
+ * handler: <handler>
+ * JUMP try_end
+ * try_end: TRY_END
  *
- * If this is not an internal try/catch, then catch and re-raise
- * internal errors to prevent them from leaking.
+ * 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().
  *
- * 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 <exp> backtracks then TRY_BEGIN will backtrack.
+ *
+ * If <exp> raises then the TRY_BEGIN will jump to the handler.
+ *
+ * If anything part the TRY_END raises, the error will be wrapped and the
+ * TRY_BEGIN will unwrap and re-raise it.
  */
- 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);
+
+ // First make sure the handler ends in BACKTRACK:
+ if (block_is_noop(handler))
+ handler = gen_op_simple(BACKTRACK);
+
+ // Make the two blocks whose next instruction will be the targets of
+ // the two branches we have (TRY_BEGIN's and JUMP's):
+ block jump = gen_op_target(JUMP, handler);
+ exp = BLOCK(exp, jump);
+ return BLOCK(gen_op_target(TRY_BEGIN, exp), exp, handler, gen_op_simple(TRY_END));
 }
 
 block gen_label(const char *label, block exp) {

src/compile.h

@@ -60,7 +60,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

@@ -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")))) &&
+ jv_get_kind(ev) != JV_KIND_NUMBER) {
+ set_error(jq, jv_invalid_with_msg(jv_copy(ev)));
+ 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.
+ */
+ stack_save(jq, pc - 1, stack_get_pos(jq));
+ 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/jq_test.c

@@ -178,28 +178,69 @@ static void run_jq_tests(jv lib_dirs, int verbose, FILE *testdata, int skip, int
  }
  jq_start(jq, input, verbose ? JQ_DEBUG_TRACE : 0);
 
+ // XXX Use getline(), not fgets()
  while (fgets(buf, sizeof(buf), testdata)) {
+ int expect_error = 0;
+ int ignore_error = 0;
+ jv expected;
+
  lineno++;
  if (skipline(buf)) break;
- jv expected = jv_parse(buf);
+ if ((expect_error = (strncmp(buf, "%%ERROR", sizeof("%%ERROR") - 1) == 0))) {
+ if ((ignore_error = (strncmp(buf, "%%ERROR IGNORE", sizeof("%%ERROR IGNORE") - 1) == 0))) {
+ expected = jv_parse(buf + sizeof("%%ERROR IGNORE") - 1);
+ } else {
+ expected = jv_parse(buf + sizeof("%%ERROR:") - 1);
+ }
+ } else {
+ expected = jv_parse(buf);
+ }
  if (!jv_is_valid(expected)) {
  printf("*** Expected result is invalid on line %u: %s\n", lineno, buf);
  invalid++;
  continue;
  }
  jv actual = jq_next(jq);
- if (!jv_is_valid(actual)) {
+ if (!jv_is_valid(actual) && jv_invalid_has_msg(jv_copy(actual))) {
+ actual = jv_invalid_get_msg(actual);
+ if (!expect_error) {
+ printf("*** Expected non-error ");
+ jv_dump(expected, 0);
+ printf(", but got jq program error ");
+ jv_dump(actual, 0);
+ printf(" for test at line number %u: %s\n", lineno, prog);
+ pass = 0;
+ break;
+ }
+ if (!jv_equal(jv_copy(expected), jv_copy(actual))) {
+ printf("*** Expected %s", expect_error ? "error " : "");
+ jv_dump(jv_copy(expected), 0);
+ printf(", but got %s", expect_error ? "error " : "");
+ jv_dump(jv_copy(actual), 0);
+ printf(" for test at line number %u: %s\n", lineno, prog);
+ pass = 0;
+ }
+ } else if (!jv_is_valid(actual)) {
  jv_free(actual);
  printf("*** Insufficient results for test at line number %u: %s\n", lineno, prog);
  pass = 0;
  break;
+ } else if (expect_error) {
+ printf("*** Expected error ");
+ jv_dump(expected, 0);
+ printf(", but got non-error ");
+ jv_dump(actual, 0);
+ printf(" for test at line number %u: %s\n", lineno, prog);
+ pass = 0;
+ break;
  } else if (!jv_equal(jv_copy(expected), jv_copy(actual))) {
- printf("*** Expected ");
- jv_dump(jv_copy(expected), 0);
- printf(", but got ");
- jv_dump(jv_copy(actual), 0);
+ printf("*** Expected %s", expect_error ? "error " : "");
+ jv_dump(expected, 0);
+ printf(", but got %s", expect_error ? "error " : "");
+ jv_dump(actual, 0);
  printf(" for test at line number %u: %s\n", lineno, prog);
  pass = 0;
+ break;
  }
  jv as_string = jv_dump_string(jv_copy(expected), rand() & ~(JV_PRINT_COLOR|JV_PRINT_REFCOUNT));
  jv reparsed = jv_parse_sized(jv_string_value(as_string), jv_string_length_bytes(jv_copy(as_string)));
@@ -209,13 +250,18 @@ static void run_jq_tests(jv lib_dirs, int verbose, FILE *testdata, int skip, int
  jv_free(expected);
  jv_free(actual);
  }
- if (pass) {
+ if (compiled && pass) {
  jv extra = jq_next(jq);
  if (jv_is_valid(extra)) {
  printf("*** Superfluous result: ");
  jv_dump(extra, 0);
  printf(" for test at line number %u, %s\n", lineno, prog);
  pass = 0;
+ } else if (jv_invalid_has_msg(jv_copy(extra))) {
+ printf("*** Superfluous error result: ");
+ jv_dump(jv_invalid_get_msg(extra), 0);
+ printf(" for test at line number %u, %s\n", lineno, prog);
+ pass = 0;
  } else {
  jv_free(extra);
  }

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)