try/catch catches more than it should #1859

velicanu · 2019-03-07T18:45:12Z

Describe the bug
Error is not properly raised when checking an assigned variable from the output of a previous try-catch block.

To Reproduce
This should raise a "normal error", instead it returns null.
echo '{}' | jq '(try ["a","b"] catch null) as $selected | if ($selected|length<2) then $selected[0] else ("normal error " | error) end'
Exact same code except with halt_error instead of error raises halt_error.
echo '{}' | jq '(try ["a","b"] catch null) as $selected | if ($selected|length<2) then $selected[0] else ("halt error " | halt_error) end'

Expected behavior
The first line should raise a "normal error" not a null.

Environment
Tested in MacOS 10.14.3 using jq 1.6
Ubuntu 18.04.2 LTS using jq 1.5
jq play

Additional context
Full list of the variations I tried are in a shell-script here: https://gist.github.com/velicanu/f4f2bd8a5e67a3f0c4ad7bff89f8dab7

The text was updated successfully, but these errors were encountered:

nicowilliams · 2019-03-07T20:19:05Z

This is fascinating.

nicowilliams · 2019-03-07T22:20:04Z

The diagnosis is that try/catch catches more than it should.

The difference between the two examples is that halt_error cannot be caught because it terminates the VM immediately.

The problem is that the FORK_OPT machinery for catching errors can't distinguish between errors from the EXP in try EXP catch ... versus errors from expressions to the right. We'll need something like SUBEXP_BEGIN/SUBEXP_END but for this.

Utterly fascinating. Obviously, this is my mistake.

nicowilliams · 2019-03-07T22:20:43Z

@velicanu thanks for the report!

velicanu · 2019-03-07T22:22:18Z

my pleasure! thanks for looking into this!

nicowilliams · 2019-03-08T05:40:20Z

I think the fix is to note in the current stack frame the offset of the opcode that raised an error (e.g., a CALL_JQ), then as we backtrack to raise the error, if we happen upon a FORK_OPT, check if that offset is less than the FORK_OPT's branch, and if so, we can catch it, if not, not.

I do think it's kinda useful to be able to catch errors from expressions to the right of a |, so perhaps we should have syntax for that (and a variant of FORK_OPT for that). See the commentary on #1843 for example.

ravron · 2019-04-01T23:00:07Z

I think this is the same issue, please let me know if I'm off base. I've noticed something that also seems mighty weird:

$ jq -n '1 | .foo' ; echo $?
jq: error (at <unknown>): Cannot index number with string "foo"
5
$ jq -n '1? | .foo' ; echo $?
0

As in the discussion above, it appears that the ? (which the manual says is short for try EXP catch empty) is catching too much. I added to the conversation only because I thought the shorter repro might be helpful.

nicowilliams · 2019-04-01T23:20:11Z

@ravron indeed, that is the same problem!

nicowilliams · 2019-04-02T02:29:36Z

I'm currently playing with the following fix, which looks promising. The idea is to keep track of the instruction address at which an error was "produced", and if that's past the end of the try body, then don't jump to the catch side of the FORK_OPT. Keeping track of that address is tricky, and this patch works for some simple tests, but may not be complete.

$ ./jq -n '
      def inner: ("inner here"|debug|empty),.;
      def outer: ("outer here"|debug|empty),.;
      def e: .[.];
      "hi"|(try ((try inner catch ("inner catch"|debug|empty)),outer) catch (debug|"outer catch"|error))|e'
["DEBUG:","inner here"]
["DEBUG:","Cannot index string with string \"hi\""]
jq: error (at <unknown>): outer catch
$

Compare to jq-1.6:

        $ jq -n '
            def inner: ("inner here"|debug|empty),.;
            def outer: ("outer here"|debug|empty),.;
            def e: .[.];
            "hi"|(try ((try inner catch ("inner catch"|debug|empty)),outer) catch (debug|"outer catch"|error))|e'
        ["DEBUG:","inner here"]
BAD --->["DEBUG:","inner catch"]
BAD --->["DEBUG:","outer here"]
        ["DEBUG:","Cannot index string with string \"hi\""]
        jq: error (at <unknown>): outer catch
        $

This works at least for .[expr] as a source of errors (shown above), as well as for error as a source of errors:

$ ./jq -n '
      def inner: ("inner here"|debug|empty),.;
      def outer: ("outer here"|debug|empty),.;
      def e: error;
      "hi"|(try ((try inner catch ("inner catch"|debug|empty)),outer) catch (debug|"outer catch"|e))|e'
["DEBUG:","inner here"]
["DEBUG:","hi"]
jq: error (at <unknown>): outer catch
$

vs jq-1.6:

        $ jq -n '
              def inner: ("inner here"|debug|empty),.;
              def outer: ("outer here"|debug|empty),.;
              def e: error;
              "hi"|(try ((try inner catch ("inner catch"|debug|empty)),outer) catch (debug|"outer catch"|e))|e'
        ["DEBUG:","inner here"]
BAD --->["DEBUG:","inner catch"]
BAD --->["DEBUG:","outer here"]
        ["DEBUG:","hi"]
        jq: error (at <unknown>): outer catch
        $

Here's the patch, which it needs to be made pretty if at all possible.

diff --git a/src/execute.c b/src/execute.c
index 958c8c2..fa9539a 100644
--- a/src/execute.c
+++ b/src/execute.c
@@ -253,6 +253,7 @@ struct forkpoint {
   int path_len, subexp_nest;
   jv value_at_path;
   uint16_t* return_address;
+  uint16_t* call_address;     /* for tr/catch */
 };

 struct stack_pos {
@@ -274,6 +275,7 @@ void stack_save(jq_state *jq, uint16_t* retaddr, struct stack_pos sp){
   fork->value_at_path = jv_copy(jq->value_at_path);
   fork->subexp_nest = jq->subexp_nest;
   fork->return_address = retaddr;
+  fork->call_address = 0;
   jq->stk_top = sp.saved_data_stack;
   jq->curr_frame = sp.saved_curr_frame;
 }
@@ -339,6 +341,7 @@ uint16_t* stack_restore(jq_state *jq){
   }

   struct forkpoint* fork = stack_block(&jq->stk, jq->fork_top);
+  uint16_t* call_address = fork->call_address;
   uint16_t* retaddr = fork->return_address;
   jq->stk_top = fork->saved_data_stack;
   jq->curr_frame = fork->saved_curr_frame;
@@ -353,6 +356,11 @@ uint16_t* stack_restore(jq_state *jq){
   jq->value_at_path = fork->value_at_path;
   jq->subexp_nest = fork->subexp_nest;
   jq->fork_top = stack_pop_block(&jq->stk, jq->fork_top, sizeof(struct forkpoint));
+  if (jq->fork_top) {
+    fork = stack_block(&jq->stk, jq->fork_top);
+    if (!fork->call_address || fork->call_address < call_address)
+      fork->call_address = call_address;
+  }
   return retaddr;
 }

@@ -438,6 +446,7 @@ jv jq_next(jq_state *jq) {
   }
   assert(pc);

+  uint16_t *error_source = 0;
   int raising;
   int backtracking = !jq->initial_execution;
   jq->initial_execution = 0;
@@ -896,6 +905,12 @@ jv jq_next(jq_state *jq) {
         jq->finished = 1;
         return jv_invalid();
       }
+      if (!jv_is_valid(jq->error) && jq->fork_top && jq->curr_frame > jq->fork_top) {
+        struct forkpoint* frk = stack_block(&jq->stk, jq->fork_top);
+        error_source = frk->call_address;
+      } else {
+        error_source = pc;
+      }
       backtracking = 1;
       break;
     }
@@ -915,17 +930,22 @@ jv jq_next(jq_state *jq) {
         jv_free(stack_pop(jq));
         goto do_backtrack;
       }
+
+      uint16_t offset = *pc++;
+
       // `try EXP ...` exception caught in EXP
       // DESTRUCTURE_ALT doesn't want the error message on the stack,
       // as we would just want to throw it away anyway.
       if (opcode != ON_BACKTRACK(DESTRUCTURE_ALT)) {
         jv_free(stack_pop(jq)); // free the input
+        if (error_source > (pc + offset))
+          goto do_backtrack;
         stack_push(jq, jv_invalid_get_msg(jq->error));  // push the error's message
       } else {
+        error_source = 0;
         jv_free(jq->error);
       }
       jq->error = jv_null();
-      uint16_t offset = *pc++;
       pc += offset;
       break;
     }
@@ -937,6 +957,10 @@ jv jq_next(jq_state *jq) {
     }

     case CALL_BUILTIN: {
+      if (jq->fork_top && jq->curr_frame > jq->fork_top) {
+        struct forkpoint* frk = stack_block(&jq->stk, jq->fork_top);
+        frk->call_address = pc;
+      }
       int nargs = *pc++;
       jv top = stack_pop(jq);
       jv* in = cfunc_input;
@@ -976,6 +1000,10 @@ jv jq_next(jq_state *jq) {

     case TAIL_CALL_JQ:
     case CALL_JQ: {
+      if (jq->fork_top && jq->curr_frame > jq->fork_top) {
+        struct forkpoint* frk = stack_block(&jq->stk, jq->fork_top);
+        frk->call_address = pc;
+      }
       /*
        * Bytecode layout here:
        *
@@ -1246,6 +1274,8 @@ jv jq_next(jq_state *jq) {
     case ON_BACKTRACK(CORET):
     case ON_BACKTRACK(RET): {
       // resumed after top-level return
+      if (!jv_is_valid(jq->error))
+        error_source = pc;
       goto do_backtrack;
     }
     }

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.

nicowilliams · 2019-12-28T05:14:26Z

I've a fix for this. It's much simpler than I'd expected. Basically, rename FORK_OPT to TRY_BEGIN, add a TRY_END, and as errors backtrack through a TRY_END wrap them so that the next TRY_BEGIN to see them on backtrack does not catch them but just unwraps and re-raises.

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.

nicowilliams · 2019-12-28T08:23:03Z

Commit ee36e9a in my dlopen branch fixes this.

The fix turned out to be rather simple.

FORK_OPT got renamed to TRY_BEGIN, and a TRY_END was added. The bytecode generated looks like this:

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

TRY_END catches, wraps, and re-raises errors on backtrack.

TRY_BEGIN catches errors on backtrack and unwraps and re-raises if they were wrapped, else it jumps to the handler (without a stack_save(), so on backtrack from executing the handler, the TRY_BEGIN will not be evaluated again).

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.

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

Alanscut · 2020-01-08T07:16:21Z

hi @nicowilliams
I tested your branch dlopen and now the result is correct, but but I personally think that (at <stdin>: 1) may be a bit redundant, because the second test data, the returned result does not contain (at <stdin> :1)

echo '{}' | jq '(try ["a","b"] catch null) as $selected | if ($selected|length<2) then $selected[0] else ("normal error " | error) end'

------test result------
jq-master :  null

dlopen branch :  jq: error (at <stdin>:1): normal bug error

--------test 2----
echo '{}' | jq '(try ["a","b"] catch null) as $selected | if ($selected|length<2) then $selected[0] else ("halt error " | halt_error) end'

----test result---
jq-master :  jq: error: halt error

dlopen branch :  jq: error: halt error

nicowilliams · 2020-01-09T17:43:08Z

@Alanscut the at <stdin>:1 thing is added by main() -- it doesn't know anything about the error other than it was produced while processing an input read from stdin at line 1, so it says as much.

nicowilliams · 2023-07-22T17:27:13Z

Good call @emanuele6. I'll try to port ee36e9a to master.

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

Close jqlang#1885, jqlang#2140, jqlang#2011, jqlang#2220, jqlang#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).

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

nicowilliams added the bug label Mar 7, 2019

nicowilliams changed the title ~~Unexpected behavior when raising errors after checking a variable~~ try/catch catches more than it should. Mar 7, 2019

nicowilliams changed the title ~~try/catch catches more than it should.~~ try/catch catches more than it should Mar 7, 2019

This was referenced Mar 8, 2019

? operator eagerly suppresses break errors #1858

Closed

dlopen(), random, I/O, eval, and co-expressions (co-routines) #1843

Open

nicowilliams added a commit to nicowilliams/jq that referenced this issue Apr 3, 2019

Fix try/catch (fix jqlang#1859)

5bd9985

nicowilliams mentioned this issue Apr 12, 2019

jq 1.6 different (incorrect?) behavior for tonumber? // tostring compared to jq 1.5 #1885

Closed

diafour mentioned this issue Mar 31, 2020

fromjson or tonumber error and next jq_compile fails #2091

Closed

itchyny mentioned this issue May 13, 2020

Converting unix timestamps in milliseconds occurs Assertion failed! #2123

Closed

itchyny mentioned this issue Jun 12, 2020

|=.? returns empty #2140

Closed

itchyny mentioned this issue Dec 10, 2020

Try/catch in update assignment #2220

Closed

itchyny mentioned this issue Jul 9, 2021

Try "?" in variable expression difference to jq itchyny/gojq#82

Closed

wader mentioned this issue Sep 14, 2022

Strange behaviour when using two piped try catch error #2485

Closed

D3vil0p3r mentioned this issue Jun 8, 2023

[Request] gojq chaotic-aur/packages#2543

Closed

itchyny mentioned this issue Jun 25, 2023

Error in any(generator;condition) #2073

Closed

emanuele6 added this to the 1.7 release milestone Jul 22, 2023

itchyny mentioned this issue Jul 23, 2023

Fix try/catch catches more than it should #1859 #2750

Merged

emanuele6 closed this as completed in #2750 Jul 24, 2023

nicowilliams mentioned this issue Jul 27, 2023

Replace NEWS with NEWS.md with more details and examples #2599

Merged

itchyny mentioned this issue Jan 25, 2024

try-catch + ascii_downcase fails in "catch" condition #3016

Closed

uncle-fed mentioned this issue Jan 25, 2024

Error suppression operator breaks function behaviour #3018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

try/catch catches more than it should #1859

try/catch catches more than it should #1859

velicanu commented Mar 7, 2019

nicowilliams commented Mar 7, 2019

nicowilliams commented Mar 7, 2019

nicowilliams commented Mar 7, 2019

velicanu commented Mar 7, 2019

nicowilliams commented Mar 8, 2019

ravron commented Apr 1, 2019

nicowilliams commented Apr 1, 2019

nicowilliams commented Apr 2, 2019 •

edited

Loading

nicowilliams commented Dec 28, 2019

nicowilliams commented Dec 28, 2019 •

edited

Loading

Alanscut commented Jan 8, 2020

nicowilliams commented Jan 9, 2020

nicowilliams commented Jul 22, 2023

try/catch catches more than it should #1859

try/catch catches more than it should #1859

Comments

velicanu commented Mar 7, 2019

nicowilliams commented Mar 7, 2019

nicowilliams commented Mar 7, 2019

nicowilliams commented Mar 7, 2019

velicanu commented Mar 7, 2019

nicowilliams commented Mar 8, 2019

ravron commented Apr 1, 2019

nicowilliams commented Apr 1, 2019

nicowilliams commented Apr 2, 2019 • edited Loading

nicowilliams commented Dec 28, 2019

nicowilliams commented Dec 28, 2019 • edited Loading

Alanscut commented Jan 8, 2020

nicowilliams commented Jan 9, 2020

nicowilliams commented Jul 22, 2023

nicowilliams commented Apr 2, 2019 •

edited

Loading

nicowilliams commented Dec 28, 2019 •

edited

Loading