From 0673dee1b38db5d49ae3f8cda0ba53fa4021c3ba Mon Sep 17 00:00:00 2001 From: William Langford Date: Fri, 17 Aug 2018 22:47:13 -0400 Subject: Fix 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. --- src/compile.c | 28 ++++++--------- src/execute.c | 12 +++++-- src/opcode_list.h | 2 +- tests/jq.test | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 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 -- cgit v1.2.3