Fix destructuring alternationfix-destructuring-alternation

Attempting to use the existing FORK_OPT opcode resulted in difficulty
knowing when to pop an error message off the stack and when not to. This
commit makes DESTRUCTURE_ALT a real opcode that is identical to
FORK_OPT, except for never pushing the error message onto the stack when
continuing from an error backtrack.

Some small changes were necessary to the DUP/POP behavior surrounding
destructuring to accomodate this.

4 files changed, 122 insertions, 21 deletions

diff --git a/src/compile.c b/src/compile.c
index 33a8e72c..bd0cb6d9 100644
--- a/src/compile.c
+++ b/src/compile.c
@@ -759,7 +759,7 @@ static void block_get_unbound_vars(block b, jv *vars) {
 
 /* Build wrappers around destructuring matchers so that we can chain them
  * when we have errors.  The approach is as follows:
- * FORK_OPT NEXT_MATCHER (unless last matcher)
+ * DESTRUCTURE_ALT NEXT_MATCHER (unless last matcher)
  * existing_matcher_block
  * JUMP BODY
  */
@@ -767,20 +767,18 @@ static block bind_alternation_matchers(block matchers, block body) {
   block preamble = {0};
   block altmatchers = {0};
   block mb = {0};
+  block final_matcher = matchers;
 
   // Pass through the matchers to find all destructured names.
-  while (matchers.first && matchers.first->op == DESTRUCTURE_ALT) {
-    block_append(&altmatchers, inst_block(block_take(&matchers)));
+  while (final_matcher.first && final_matcher.first->op == DESTRUCTURE_ALT) {
+    block_append(&altmatchers, inst_block(block_take(&final_matcher)));
   }
 
   // We don't have any alternations here, so we can use the simplest case.
   if (altmatchers.first == NULL) {
-    return bind_matcher(matchers, body);
+    return bind_matcher(final_matcher, body);
   }
 
-  // The final matcher needs to strip the error from the previous FORK_OPT
-  block final_matcher = BLOCK(gen_op_simple(POP), gen_op_simple(DUP), matchers);
-
   // Collect var names
   jv all_vars = jv_object();
   block_get_unbound_vars(altmatchers, &all_vars);
@@ -800,16 +798,11 @@ static block bind_alternation_matchers(block matchers, block body) {
   for (inst *i = altmatchers.first; i; i = i->next) {
     block submatcher = i->subfn;
 
-    // Get rid of the error from the previous matcher
-    if (mb.first != NULL) {
-      submatcher = BLOCK(gen_op_simple(POP), gen_op_simple(DUP), submatcher);
-    }
-
     // If we're successful, jump to the end of the matchers
     submatcher = BLOCK(submatcher, gen_op_target(JUMP, final_matcher));
 
-    // FORK_OPT to the end of this submatcher so we can skip to the next one on error
-    mb = BLOCK(mb, gen_op_target(FORK_OPT, submatcher), submatcher);
+    // DESTRUCTURE_ALT to the end of this submatcher so we can skip to the next one on error
+    mb = BLOCK(mb, gen_op_target(DESTRUCTURE_ALT, submatcher), submatcher);
 
     // We're done with this inst and we don't want it anymore
     // But we can't let it free the submatcher block.
@@ -1003,12 +996,13 @@ block gen_destructure(block var, block matchers, block body) {
   if (body.first && body.first->op == TOP)
     top = inst_block(block_take(&body));
 
-  if (matchers.first && matchers.first->op == DESTRUCTURE_ALT && !block_is_noop(var)) {
+  if (matchers.first && matchers.first->op == DESTRUCTURE_ALT) {
     block_append(&var, gen_op_simple(DUP));
-    block_append(&matchers, gen_op_simple(POP));
+  } else {
+    top = BLOCK(top, gen_op_simple(DUP));
   }
 
-  return BLOCK(top, gen_op_simple(DUP), gen_subexp(var), gen_op_simple(POP), bind_alternation_matchers(matchers, body));
+  return BLOCK(top, gen_subexp(var), gen_op_simple(POP), bind_alternation_matchers(matchers, body));
 }
 
 // Like gen_var_binding(), but bind `break`'s wildcard unbound variable
diff --git a/src/execute.c b/src/execute.c
index d5672165..8eb41cc7 100644
--- a/src/execute.c
+++ b/src/execute.c
@@ -799,21 +799,27 @@ jv jq_next(jq_state *jq) {
     }
 
     case FORK_OPT:
+    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(FORK_OPT):
+    case ON_BACKTRACK(DESTRUCTURE_ALT): {
       if (jv_is_valid(jq->error)) {
         // `try EXP ...` backtracked here (no value, `empty`), so we backtrack more
         jv_free(stack_pop(jq));
         goto do_backtrack;
       }
       // `try EXP ...` exception caught in EXP
-      jv_free(stack_pop(jq)); // free the input
-      stack_push(jq, jv_invalid_get_msg(jq->error));  // push the error's message
+      // 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
+        stack_push(jq, jv_invalid_get_msg(jq->error));  // push the error's message
+      }
       jq->error = jv_null();
       uint16_t offset = *pc++;
       pc += offset;
diff --git a/src/opcode_list.h b/src/opcode_list.h
index ffb27d2f..886131d7 100644
--- a/src/opcode_list.h
+++ b/src/opcode_list.h
@@ -44,5 +44,5 @@ OP(DEPS, CONSTANT, 0, 0)
 OP(MODULEMETA, CONSTANT, 0, 0)
 OP(GENLABEL, NONE, 0, 1)
 
-OP(DESTRUCTURE_ALT, NONE, 0, 0)
+OP(DESTRUCTURE_ALT, BRANCH, 0, 0)
 OP(STOREVN, VARIABLE, 1, 0)
diff --git a/tests/jq.test b/tests/jq.test
index 8771ba65..7e2dd430 100644
--- a/tests/jq.test
+++ b/tests/jq.test
@@ -726,6 +726,107 @@ null
 [4, 5, null, null, 7, null]
 [null, null, null, null, null, "foo"]
 
+# Destructuring DUP/POP issues
+.[] | . as {a:$a} ?// {a:$a} ?// {a:$a} | $a
+[[3],[4],[5],6]
+# Runtime error: "jq: Cannot index array with string \"c\""
+
+.[] as {a:$a} ?// {a:$a} ?// {a:$a} | $a
+[[3],[4],[5],6]
+# Runtime error: "jq: Cannot index array with string \"c\""
+
+[[3],[4],[5],6][] | . as {a:$a} ?// {a:$a} ?// {a:$a} | $a
+null
+# Runtime error: "jq: Cannot index array with string \"c\""
+
+[[3],[4],[5],6] | .[] as {a:$a} ?// {a:$a} ?// {a:$a} | $a
+null
+# Runtime error: "jq: Cannot index array with string \"c\""
+
+.[] | . as {a:$a} ?// {a:$a} ?// $a | $a
+[[3],[4],[5],6]
+[3]
+[4]
+[5]
+6
+
+.[] as {a:$a} ?// {a:$a} ?// $a | $a
+[[3],[4],[5],6]
+[3]
+[4]
+[5]
+6
+
+[[3],[4],[5],6][] | . as {a:$a} ?// {a:$a} ?// $a | $a
+null
+[3]
+[4]
+[5]
+6
+
+[[3],[4],[5],6] | .[] as {a:$a} ?// {a:$a} ?// $a | $a
+null
+[3]
+[4]
+[5]
+6
+
+.[] | . as {a:$a} ?// $a ?// {a:$a} | $a
+[[3],[4],[5],6]
+[3]
+[4]
+[5]
+6
+
+.[] as {a:$a} ?// $a ?// {a:$a} | $a
+[[3],[4],[5],6]
+[3]
+[4]
+[5]
+6
+
+[[3],[4],[5],6][] | . as {a:$a} ?// $a ?// {a:$a} | $a
+null
+[3]
+[4]
+[5]
+6
+
+[[3],[4],[5],6] | .[] as {a:$a} ?// $a ?// {a:$a} | $a
+null
+[3]
+[4]
+[5]
+6
+
+.[] | . as $a ?// {a:$a} ?// {a:$a} | $a
+[[3],[4],[5],6]
+[3]
+[4]
+[5]
+6
+
+.[] as $a ?// {a:$a} ?// {a:$a} | $a
+[[3],[4],[5],6]
+[3]
+[4]
+[5]
+6
+
+[[3],[4],[5],6][] | . as $a ?// {a:$a} ?// {a:$a} | $a
+null
+[3]
+[4]
+[5]
+6
+
+[[3],[4],[5],6] | .[] as $a ?// {a:$a} ?// {a:$a} | $a
+null
+[3]
+[4]
+[5]
+6
+
 . as $dot|any($dot[];not)
 [1,2,3,4,true,false,1,2,3,4,5]
 true