fix(jq): let a ?// retry in a slice bound supersede the slice's stash - #3434
Merged
Merged
Conversation
CoverageTotal: 94.32% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 280 tolerated region(s)
Patch coveragePatch: 80.39% (41/51 new lines covered)
Uncovered new lines (10)
|
newhoggy
added a commit
that referenced
this pull request
Sep 28, 2026
… the pull Review of #3434: a `first`/`limit`/`nth` around a retrying slice bound reports its own count stop as `Stopped` after the `?//` retry inside it re-invoked the sink, whose `begin()` had rightly cleared the stash -- a combination the first version declared unreachable, so `.[first([[0]] as [$a] ?// [[$a]] | $a):]` on `[10,20,30]` panicked (exit 101) on both evaluators where jq 1.7.1 answers `[10,20,30]` and main raised. A `Stopped` with no stash behind it is the bound's wrapper finishing, not an escape: the inner `end` drive continues and the outer pull completes with `out`. Also hoist `end`'s retry fallback out of the per-value closure and drop the comments that still described the removed `terminal`. Part of #3293.
newhoggy
added a commit
that referenced
this pull request
Sep 28, 2026
…a retry Second review of #3434 found the root of the panic the previous commit worked around: `each_limit`, `take_at_index` (`first`/`nth`) and their `eval_generic` twins set `outer_stopped` when the wrapping sink stopped and never cleared it, so after a `?//` retry inside them re-invoked the sink they reported a stale `Stopped` to whatever enclosed them -- the #3293 Class 2 shape in the consumers themselves. The slice collector's "a stashless Stopped is completion" workaround hid the panic but not the wrong answers: a comma after the wrapper was cut short (`[.[(first([[1]] as [$a] ?// [[$a]] | $a), 2):]]` gave `[[20,30]]` where jq 1.7.1 gives `[[20,30],[30]]`) and a filter dropping the retried value kept the stale error. Reset the flag at the top of each invocation, as #3411 did for the value-mode sinks; the final flag then reflects the last invocation, so an enclosing consumer that really stopped still makes jq's #1519 second answer (`[first(first(1 as $x ?// $y | 1))]` is still `[1,1]`). The slice collectors' `Stopped` arms go back to their strict form: an inner `Stopped` propagates, and the outer one is unreachable again. Part of #3293.
newhoggy
added a commit
that referenced
this pull request
Sep 28, 2026
Second review of #3434: add the shapes that exposed the stale consumer stop -- a comma after the wrapper in either slice bound, and a filter dropping the retried value -- to the slice table, and a `RETRY_ROWS_WRAPPER_STOP_3293` table pinning the reset outside slices, including the #1519 double-answer controls. Both run on both evaluators. Part of #3293.
CoverageTotal: 94.41% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 279 tolerated region(s)
Patch coveragePatch: 80.39% (41/51 new lines covered)
Uncovered new lines (10)
|
newhoggy
added a commit
that referenced
this pull request
Sep 28, 2026
…ry too Third review of #3434: `isempty`, `any`/`all` (with a generator), `IN` and generic `repeat` record the wrapping sink's stop exactly as `first`/`limit`/`nth` did, and never cleared it when a `?//` retry re-invoked their sink. Reached through a stop-then-continue shape (`input`), the stale `Stopped` hit the slice collector's restored `unreachable!` -- `[10,20,30] | [.[(isempty([[1]] as [$a] ?// [[$a]] | $a) | input):]]` panicked (exit 101) where jq 1.7.1 answers `[[20,30]]` -- and elsewhere promoted an error the retry had moved past (`[(isempty(G) | (input | if . == "a" then error("x") else . end)), 9]` raised on main too; jq answers `[1,9]`). All reset per invocation now, in both evaluators, with one pointer comment each to `each_limit`. The slice collectors no longer abort over a stash-less `Stopped`: any enclosing driver that still reports a stale stop finishes the slice with what it produced instead of crashing the process. Part of #3293.
Slice 3 of #3293. Both slice collectors (`eval_generic::eval_slice_expr` and `eval::eval_slice_expr`) parked a per-pair escape as a finished `Partial` result -- `terminal = partial(take(out), control)` -- and a `?//` retry inside either bound could never clear it: `[10,20] | .[([[0]] as [$a] ?// [[$a]] | $a):]` raised "slice indices must be integers" after the retry, where jq 1.7.1 answers `[10,20]`; a retry that produced nothing, raised, or failed to destructure kept the stale error too. The escape now lives in a `StashedEscape` apart from `out`: both bound sinks reset it per invocation, the `end` drive is settled per `start` value, and the final result folds `out` in as the `Partial` prefix only if the stash survived (`retry_superseded`). Moving the prefix out of the stash is what lets a superseded escape leave the outputs already produced in place. Part of #3293.
Add a `RETRY_ROWS_SLICE_BOUND_3293` table (every value captured from jq 1.7.1): a retry in either bound that answers, produces nothing, raises or fails to destructure, plus controls for the output prefix the fix moved out of the stash -- a prefix before a later bound's error, a generator in either bound, a slice error with no `?//`, and a `halt_error` that must not retry. Run it over a document input and, through the owned-route test, over a value built under `-n`. Part of #3293.
… the pull Review of #3434: a `first`/`limit`/`nth` around a retrying slice bound reports its own count stop as `Stopped` after the `?//` retry inside it re-invoked the sink, whose `begin()` had rightly cleared the stash -- a combination the first version declared unreachable, so `.[first([[0]] as [$a] ?// [[$a]] | $a):]` on `[10,20,30]` panicked (exit 101) on both evaluators where jq 1.7.1 answers `[10,20,30]` and main raised. A `Stopped` with no stash behind it is the bound's wrapper finishing, not an escape: the inner `end` drive continues and the outer pull completes with `out`. Also hoist `end`'s retry fallback out of the per-value closure and drop the comments that still described the removed `terminal`. Part of #3293.
…a retry Second review of #3434 found the root of the panic the previous commit worked around: `each_limit`, `take_at_index` (`first`/`nth`) and their `eval_generic` twins set `outer_stopped` when the wrapping sink stopped and never cleared it, so after a `?//` retry inside them re-invoked the sink they reported a stale `Stopped` to whatever enclosed them -- the #3293 Class 2 shape in the consumers themselves. The slice collector's "a stashless Stopped is completion" workaround hid the panic but not the wrong answers: a comma after the wrapper was cut short (`[.[(first([[1]] as [$a] ?// [[$a]] | $a), 2):]]` gave `[[20,30]]` where jq 1.7.1 gives `[[20,30],[30]]`) and a filter dropping the retried value kept the stale error. Reset the flag at the top of each invocation, as #3411 did for the value-mode sinks; the final flag then reflects the last invocation, so an enclosing consumer that really stopped still makes jq's #1519 second answer (`[first(first(1 as $x ?// $y | 1))]` is still `[1,1]`). The slice collectors' `Stopped` arms go back to their strict form: an inner `Stopped` propagates, and the outer one is unreachable again. Part of #3293.
Second review of #3434: add the shapes that exposed the stale consumer stop -- a comma after the wrapper in either slice bound, and a filter dropping the retried value -- to the slice table, and a `RETRY_ROWS_WRAPPER_STOP_3293` table pinning the reset outside slices, including the #1519 double-answer controls. Both run on both evaluators. Part of #3293.
…ry too Third review of #3434: `isempty`, `any`/`all` (with a generator), `IN` and generic `repeat` record the wrapping sink's stop exactly as `first`/`limit`/`nth` did, and never cleared it when a `?//` retry re-invoked their sink. Reached through a stop-then-continue shape (`input`), the stale `Stopped` hit the slice collector's restored `unreachable!` -- `[10,20,30] | [.[(isempty([[1]] as [$a] ?// [[$a]] | $a) | input):]]` panicked (exit 101) where jq 1.7.1 answers `[[20,30]]` -- and elsewhere promoted an error the retry had moved past (`[(isempty(G) | (input | if . == "a" then error("x") else . end)), 9]` raised on main too; jq answers `[1,9]`). All reset per invocation now, in both evaluators, with one pointer comment each to `each_limit`. The slice collectors no longer abort over a stash-less `Stopped`: any enclosing driver that still reports a stale stop finishes the slice with what it produced instead of crashing the process. Part of #3293.
Fourth review of #3434 (no regression found against main or the base across ~5,500 differential cases, both routes): - The slice collectors' stash-less `Stopped` arm degraded silently. It still finishes with `out` in a release build, but now panics under `debug_assertions`, so a driver that regresses into a stale stop fails the suite rather than truncating a slice unseen. Region-form coverage markers, which rustfmt cannot split off the lines they excuse. - The consumer test ran only with `-n` (the `eval.rs` route); add the cursor-route rows that reach the `eval_generic.rs` twins. - Revert the `repeat` reset: no `?//` retry reaches that closure, so it changed nothing, and `first(repeat(G))` diverges exactly as on main. - `each_limit`'s comment said every consumer resets its stop; `//` still does not (no probe found it diverging) -- say so. Part of #3293.
newhoggy
force-pushed
the
issue-3293-slice3-slice-bounds-collected-index
branch
from
September 28, 2026 20:59
b70eb3f to
e2f049b
Compare
The `stash.is_set()` guard returns every real stop before the inner `end_flow` match, so its `Stopped` arm is reached only by a stale enclosing driver -- the same by-design-unreachable state as the exit arm. Say so, and tolerate it in coverage the way the exit arm is. Part of #3293.
Contributor
Author
|
Coverage note: all 10 uncovered patch lines are the slice collectors' stash-less |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Slice 3 of #3293: a
?//retry inside a slice bound now supersedes the escape that the retried-past alternative left in the slice collector. Fixed in both evaluators:eval_generic::eval_slice_expr(cursor route) andeval::eval_slice_expr(owned route).[10,20])main.[([[0]] as [$a] ?// [[$a]] | $a):][10,20]slice indices must be integers, exit 5.[([[0]] as [$a] ?// $b | $a // empty):].[([[0]] as [$a] ?// $b | $a | if . == null then error("E2") else . end):]E2.[([[0]] as [$a] ?// {k: $b} | $a):]Cannot index array with string "k"Both collectors parked a per-pair escape as a finished result:
terminal = partial(take(out), control). Nothing could clear it after a retry, and it held the output prefix hostage. The escape now lives in aStashedEscape(#3411), kept apart fromout:enddrive is settled perstartvalue;outin as thePartialprefix only if the stash survived (retry_superseded).Moving the prefix out of the stash is what lets a superseded escape leave already-produced outputs in place.
The collected computed-index route (
[.[G]]), the other slice-3 item on #3293, already matches jq on both routes on currentmain. I checked the four retry endings under[.[G]],[.[G], 7]and[.[G]] | length.Review round (
/code-review high).[first([[0]] as [$a] ?// [[$a]] | $a):]panicked (exit 101) on both evaluators; jq gives[10,20,30].Stoppedas completion. The second review showed that only hid the real bug:each_limitandtake_at_index(first/nth), plus their generic twins, never clearedouter_stopped, so after a?//retry they reported a staleStoppedto whatever enclosed them. That's jq: audit other fan-out sinks for #2952's stale-escape-across-a-?//-retry shape #3293's Class 2 shape in the consumers themselves. It cut a following comma short ([.[(first(G), 2):]]gave[[20,30]]; jq gives[[20,30],[30]]) and kept the stale error when a filter dropped the retried value.?//retry supersede verdicts stashed by value-mode fan-out sinks #3411 did for the value-mode sinks. The jq:?//alternatives re-run once per alternative under a short-circuiting consumer's internal label/break #1519 double answers are unchanged ([first(first(1 as $x ?// $y | 1))]is still[1,1]), and the slice collectors'Stoppedarms are back to their strict form.RETRY_ROWS_WRAPPER_STOP_3293table, both run on both evaluators.isempty,any/all,INand genericrepeatkeep the same stale flag. Reached through a stop-then-continue shape (input), it panicked in the slice collector.isempty,any/allandINnow reset per invocation in both evaluators. The dedicated test hasinputrows on both routes: six owned-route rows under-nand four cursor-route rows that reach the generic twins.mainacross about 5,500 differential cases. A stash-lessStoppedin the slice collectors now panics underdebug_assertions, so a regressing driver fails the suite, and in a release build it finishes without. The arm carries region-form coverage markers, which rustfmt can't split off. Therepeatreset was reverted: no retry reaches it, andfirst(repeat(G))diverges exactly as onmain.//doesn't reset its stop flag, but no probe has found it diverging.terminalcomments;end's retry fallback is hoisted out of the per-value closure.resolve_slice_expr_sinkunderpath/del/|=) still lets the abandoned alternative's slice error win. It's the write-direction resolver, so it belongs with jq: audit other fan-out sinks for #2952's stale-escape-across-a-?//-retry shape #3293's path-context slice and its own review; I've listed it there.take+partialrather thanresume_from_escape, so a stashed nonretryable flag is not cleared at this reclaim. That's unchanged frommain'sterminalpath.Part of #3293.
Test plan
/usr/bin/jq1.7.1.RETRY_ROWS_SLICE_BOUND_3293table (38 rows, including 15 wrapper rows, generated from jq 1.7.1), run over a document input and, through the owned-route test, over a-n-built value.cargo test --features cli,simd,regex,serde: 9443 passed (after rebase), 0 failed.cargo test --no-default-featurespasses.cargo clippy -- -D warningsfor all three variants;cargo fmt --check;RUSTDOCFLAGS=-D warnings cargo doc.