fix(jq): require A // B's left to be raise-and-empty-free, not merely raise-free - #3142
Conversation
CoverageTotal: 93.91% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 167 tolerated region(s)
Patch coveragePatch: 100% (14/14 new lines covered)
Indirect coverage changes🔴 2 lines lost coverage, 🟢 0 lines gained coverage on unchanged code. Indirect changes
|
CoverageTotal: 94% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 166 tolerated region(s)
Patch coveragePatch: 100% (14/14 new lines covered)
Indirect coverage changes🔴 2 lines lost coverage, 🟢 0 lines gained coverage on unchanged code. Indirect changes
|
…ntity_bind_position by is_identity_passthrough Review findings on PR #3142: - identity_bind_position's own position() closure had drifted wider than is_identity_passthrough's grammar (If/Alternative arms with no raise/empty gate), safe only via a comment-documented invariant at a call site three functions away. Gate it at its own single choke point instead, so a future edit can't silently reopen the #3129 hole through this untouched branch. - The new regression test duplicated the doc/message fixture and query! loop shape of two existing, table-driven tests instead of extending them. Folded its rows into test_raising_try_body_is_not_an_identity_passthrough_2978 (the main repro rows and the control) and test_identity_bind_position_traps_keep_refusing_2978 (the positional twin), and dropped the now-redundant standalone test function. - Trimmed a doc comment restating the same mechanism a third time on the match arm itself.
|
Ran `/code-review` (standard effort) on the initial commit. Fixed all 4 confirmed findings in a follow-up commit:
One review pass (a subagent finding that arrived after the main synthesis) additionally raised a plausible-sounding deeper concern: that `is_raise_free_identity_passthrough`'s own `Alternative` arm (`Expr::Alternative(left, _) => is_raise_free_identity_passthrough(left)`) ignores its right operand, so a `try (A // B) catch C`-shaped body could theoretically let an unconstrained `B`/`C` through as "raise-free" whenever `A` is falsy. I spent real effort trying to construct a live divergence from `/usr/bin/jq` 1.7.1 for this (several structural variants, including one needing a `$root`-rebind to route around jq's own body-continues-from-pre- All local checks green: full test suite, clippy (both feature sets), fmt, 100% patch coverage (`omni-dev coverage diff`), and the original repro re-verified end-to-end against the pinned oracle after the refactor. |
|
Filed #3143 for the deeper, unconfirmed |
… raise-free is_identity_passthrough's Alternative arm admitted any A that itself passed the wider identity-passthrough grammar, including an `if` -- but `//` runs B whenever A yields nothing, not only when A raises, and an `if`'s condition is arbitrary enough to yield nothing on its own (`if empty then . else . end`). B's value was then certified as Origin::Snapshot and written through wherever an equal-valued node existed, fabricating a path del()/= then wrote through where jq refuses. Fixes #3129.
…ntity_bind_position by is_identity_passthrough Review findings on PR #3142: - identity_bind_position's own position() closure had drifted wider than is_identity_passthrough's grammar (If/Alternative arms with no raise/empty gate), safe only via a comment-documented invariant at a call site three functions away. Gate it at its own single choke point instead, so a future edit can't silently reopen the #3129 hole through this untouched branch. - The new regression test duplicated the doc/message fixture and query! loop shape of two existing, table-driven tests instead of extending them. Folded its rows into test_raising_try_body_is_not_an_identity_passthrough_2978 (the main repro rows and the control) and test_identity_bind_position_traps_keep_refusing_2978 (the positional twin), and dropped the now-redundant standalone test function. - Trimmed a doc comment restating the same mechanism a third time on the match arm itself.
446aa61 to
f8fbf76
Compare
Summary
is_identity_passthrough'sExpr::Alternative(left, _)arm admitted anyAinA // Bthat itself passed the wider identity-passthrough grammar, including anif. But//runsBwheneverAyields nothing, not only whenAraises,and an
if's condition is arbitrary enough to yield nothing on its own(
if empty then . else . end).B's value was then certified asOrigin::Snapshotand written through wherever an equal-valued node existed —del()/=fabricated a path jq refuses to touch.Fix: require
Ato satisfyis_raise_free_identity_passthrough(the sameraise-and-empty-free grammar a
trybody already needs), not the wideris_identity_passthroughgrammar.identity_bind_position's ownAlternativearm needs no change — it's only ever consulted once
is_identity_passthroughalready holds for the bind source, so it's inert on shapes the narrower gate now
excludes.
Accepted, deliberate cost:
((if true then . else . end) // 1)becomesrefuse-only under the fix even though jq itself would answer it — the safe
direction per this codebase's divergence rule, matching how a
trybody with anifinside it already behaves (#2978).Test plan
cargo build --features clicargo test --features cli,simd,regex,serde(full suite, 0 failures)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 --checktest_alternative_left_empty_if_is_not_an_identity_passthrough_3129) pins all four repro forms from the issue (path/del/=, plus the positional twin) against/usr/bin/jq1.7.1's own refusal message, and a control ((. // 1) as $xon a truthy.) confirming the baseA // Brule is unchanged(if true then . else . end) // 1flipstrue→false), verified live against jq 1.7.1 that jq still answers there while succinctly is now refuse-only (accepted per the issue)cargo llvm-cov+omni-dev coverage diff: 100% patch coverage (24/24 new lines)del(.a | ((if empty then . else . end) // {"b":1}) as $x | .k | $x | .b)on{"a":{"k":{"b":1}}}now matches jq's refusal exactly, where it previously silently wrote{"a":{"k":{}}}Fixes #3129.