fix(jq): give a ? over a fan-out group jq's single abort scope - #2930
Conversation
CoverageTotal: 93.5% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 91 tolerated region(s)
Patch coveragePatch: 100% (27/27 new lines covered)
Indirect coverage changes🔴 0 lines lost coverage, 🟢 1 lines gained coverage on unchanged code. Indirect changes
|
CoverageTotal: 93.4% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 92 tolerated region(s)
Patch coveragePatch: 100% (27/27 new lines covered)
Indirect coverage changes🔴 0 lines lost coverage, 🟢 1 lines gained coverage on unchanged code. Indirect changes
|
|
Both findings addressed; the crash one was serious and the diagnosis exactly right. The crash was self-inflicted duplication. I wrote the scope-safety rule twice — the expression form skipped The better half of the fix was dropping the splice gate entirely. Gating that site is what makes it a fixed point, and it turns out to be unnecessary — Your point about the 756-combination sweep is the one I want to keep: it varied neither a bare The three documentation findings are fixed too — including |
6ac0daf to
14cacb6
Compare
`(A | B)?` is `try (A | B)`: a failure anywhere inside prunes the whole group's branch and stops the group's own generators. Two sites rewrote it into `A? | B?` instead -- `push_path_components` (#1311) for the resolver, `splice_optional_group` (#1294) for the write walkers -- turning one abort scope into N independent per-step ones, so a generator inside the group resumed past an error a later component raised. Both rewrites exist to stop the group's `?` leaking onto what *follows* the group, and both are correct for the pure-navigation groups they were written for. A single-valued chain has one branch, so pruning at step k and aborting the group are indistinguishable. Put a generator inside and they part company. #2909 filed this as a side-effect count. It is not: the output diverges, and silently. Captured live against jq 1.7.1: echo '{"a":{"b":1},"c":[1,2]}' | jq -c 'del((.[] | .[0])?)' jq: {"a":{"b":1},"c":[1,2]} was: {"a":{"b":1},"c":[2]} echo '[{"a":1},5,{"a":3}]' | jq -c '((.[] | .a)?) |= 99' jq: [{"a":99},5,{"a":3}] was: [{"a":99},5,{"a":99}] -- an element deleted that jq keeps, and a slot written that jq never reaches. The issue's own diagnosis needs two corrections as well: `..` is not required (any pipe does it), and the `?` never reaches a `?`-handling arm in the diverging shapes -- it is dissolved before resolution. One predicate, `optional_group_is_scope_safe`, gates both rewrites: distribute only when the group has fewer than two components or every one of them is single-valued. Conservative on purpose -- a group like `(.[] | .a?)` that happens not to diverge today is also called unsafe, which only changes which correct arm handles it. An unsafe group then has to *reach* that arm, so `needs_path_prepass` and `needs_fanout_pass` both answer `true` for it. Both are needed and for different reasons: the former is what routes `del`/`=`/`|=`/`path` through the resolver at all, and the latter peels `Optional` before testing, so `Optional(Pipe([Iterate, Field]))` would otherwise be mistaken for a single-valued static tail. With both set the group reaches `resolve_optional_sink` -- the arm that was already right, and the reason the issue's three "neighbouring spellings agree" rows agree. `wrap_optional_branch`'s `depth() != 1` arm was annotated "unreached today", tolerated for coverage, on reasoning that held only while a group was always distributed before reaching a resolver. It is live now -- verified by instrumenting it -- and needed no new logic: the old comment had already stated jq's answer for exactly this shape. Verified by a 2,400-combination differential sweep (30 group bodies, 14 of them randomised, x 10 outer spellings x 8 inputs) against jq 1.7.1: 8 divergences, all one pre-existing shape (`del((.[0:1] | .. | ..)?)`, which diverges identically on the merge-base and without any `?` at all -- filed separately). Zero regressions. A further 756-combination scan for an "invalid path component" leak from an un-distributed group reaching a native walker: none. Refs #2909
Three tests, every expectation captured from jq 1.7.1 rather than derived. The fix'\''s own rows, including the two silent wrong answers (`del` keeping an element it used to remove, `|=` skipping a slot it used to write) and the duplicated-path row -- plus the issue'\''s `stderr` repro and the `.`-piped form that shows `..` was never the trigger. The upper edge: the three neighbouring spellings that already matched, one of which (`path((...)?)` with no enclosing pipe) reaches `resolve_optional_sink` directly and is therefore the fix'\''s reference behaviour, not merely a control. The lower edge, which is the one that matters for the gate: #1311'\''s and #1294'\''s own pure-primitive shapes. Widen `optional_group_is_scope_safe` and these stop being distributed, which is exactly the "invalid path component" failure #1311 was filed for. The two error rows also pin that the group'\''s `?` still does not leak onto what follows it -- the failure #1294 fixed, and the thing an over-correction here would re-break. Refs #2909
The review found a **crash class** my own 756-combination sweep missed:
`del(.a | (. | .[])?)` overflowed the stack (exit 134) where jq and the
merge-base both answer `{"a":{},"c":[1,2]}`. Six shapes, jq and yq mode
alike.
Cause: I wrote the rule twice. `optional_group_is_scope_safe` skipped
`Identity` and recursed into `Pipe`/`Paren`; the slice form counted
`Identity` and treated a `Paren(Pipe(..))` element as multi-valued. So a
group could be *safe* to the routing gate -- staying on the native walker --
and *unsafe* to `splice_optional_group`, which then kept it opaque by
returning `[Optional(Pipe(inner))] ++ rest`: the very list the caller had
just decomposed. The walker re-derived the same `here` and re-entered.
That is this repo's own "duplicated predicates diverge silently" lesson
(#106), in a commit whose message claims "one predicate". There is one walk
now, with one entry point.
The second half of the fix is to stop gating `splice_optional_group` at
all. Gating it is what makes it a fixed point, and it turns out to be
unnecessary: `needs_path_prepass` already answers `true` for a group that
can fan out, so such a group is resolved through `resolve_optional_sink`
before any walker sees it. Verified by re-running the sweep with that site
ungated -- widened to 3,840 combinations, with the review's crash shapes and
`.`/paren-heavy randomised bodies added: zero output differences, zero
crashes. Six of those shapes are now CLI tests, which fail loudly rather
than subtly if either half regresses.
Three documentation findings, all mine:
* `is_atomic_path_component`'s comment asserted that an opaque group "never
reaches this walker anyway" as the safety argument for the whole change.
The crash proved it was an assertion, not an invariant. It now names the
routing gate as the actual mechanism and points at
`splice_optional_group`'s own comment for why that site stays ungated.
* `push_path_components` had lost its doc comment -- absorbed into the new
predicate's rustdoc, the same defect `b6abf5d76` fixed for
`resolve_leaf`. Restored.
* `path_component_is_single_valued`'s lead question was inverted relative to
its return value.
Refs #2909
…per arm Patch coverage flagged the `Optional`/`Paren` arm as never hit, and the call graph says why: `walk_optional_group`, its only caller, has already looked through both wrappers by the time it asks. The arm was dead the moment the two scope-safety predicates were merged into one walk. Removed rather than tolerated -- the function collapses to a flat `matches!` -- with the reason recorded so it is not re-added by symmetry with the walk above it. Refs #2909
99d35e5 to
46bf851
Compare
Summary
(A | B)?istry (A | B)— one abort scope over the whole group: afailure anywhere inside prunes the group's branch and stops the group's own
generators. Two sites rewrote it into
A? | B?instead —push_path_components(#1311) for the resolver,splice_optional_group(#1294) for the write walkers — turning one abort scope into N independent
per-step ones.
Both rewrites exist to stop the group's
?leaking onto what follows thegroup, and both are correct for the pure-navigation groups they were written
for: a single-valued chain has one branch, so pruning at step k and aborting
the group are indistinguishable. Put a generator inside the group and they
part company — it resumes past an error a later component raised.
The issue understated this
#2909 filed it as a side-effect count with output matching. The output
diverges, and silently. All captured live from
/usr/bin/jq1.7.1:Two further corrections to the issue's own diagnosis:
..is not required(
[path(. | (.[]|stderr|.+0)?)]diverges identically), and it is notresolve_optional_sinkvs another?-handling arm — in the diverging shapesthe
?never reaches a?-handling arm at all, because it is dissolved beforeresolution.
One row of the issue's own list does not diverge here:
((.[] | .a)?) = 99already matched, so it is in the must-not-change set rather than the fixed set.
How
optional_group_is_scope_safe— one predicate, both rewrites gated on it:distribute only when the group has fewer than two components, or every one of
them is single-valued (
Identity/Field/Index/Slice, throughParen/Optional). Conservative on purpose: a group like(.[] | .a?)thathappens not to diverge today is also called unsafe, which only changes which
correct arm handles it.
An unsafe group then has to reach that arm, so
needs_path_prepassandneeds_fanout_passboth answertruefor it. Both are needed, for differentreasons: the former is what routes
del/=/|=/paththrough the resolverat all; the latter peels
Optionalbefore testing, soOptional(Pipe([Iterate, Field]))would otherwise be mistaken for asingle-valued static tail. With both set, the group reaches
resolve_optional_sink— the arm that was already right, and the reason theissue's three "neighbouring spellings agree" rows agree.
wrap_optional_branch'sdepth() != 1arm was annotated "unreached today" andcoverage-tolerated, on reasoning that held only while a group was always
distributed before reaching a resolver. It is live now — verified by
instrumenting it — and needed no new logic: the old comment had already stated
jq's answer for exactly this shape.
Verification beyond the suite
14 of them randomised from a component alphabet rather than hand-picked, × 10
outer spellings × 8 inputs): 8 divergences, 0 regressions. All 8 are one
pre-existing shape —
del((.[0:1] | .. | ..)?)— which diverges identicallyon the merge-base and with no
?anywhere, so it is not this class. Filedas jq: del() through a slice piped into repeated .. deletes the wrong element #2929.
walker (the failure mode jq: a ? on a multi-component group ahead of more path still hits 'invalid path component' for = #1311 was filed for): none. That is the audit of
push_path_components' 11 callers the plan asked for, done empirically.Test plan
cargo build --features clicargo test --features cli,simd,regex,serde— 8554 passed, 0 failed(including jq: update_path's Expr::Pipe splice over-suppresses a ? guard past its own group #1294's and jq: a ? on a multi-component group ahead of more path still hits 'invalid path component' for = #1311's own tests, which are the gate's lower edge)
cargo clippy --all-targets --all-features -- -D warningscargo clippy --all-targets --features std,simd,serde,cli,regex,bench-runner,large-tests,mmap-tests -- -D warningscargo fmt --checkmove, and the scope-safe class that must keep being distributed
Fixes #2909