fix(jq): recognize every identity-passthrough spelling in a pattern's first-step identity test - #3147
Conversation
CoverageTotal: 94.02% ⚪ 0.01 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 167 tolerated region(s)
Patch coveragePatch: 99.32% (146/147 new lines covered)
Uncovered new lines (1)
|
CoverageTotal: 93.92% ⚪ 0.01 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 168 tolerated region(s)
Patch coveragePatch: 99.32% (146/147 new lines covered)
Uncovered new lines (1)
|
… first-step identity test resolve_as_pattern's path_intact check only recognized a bare `.` head as the register's own node. Every other is_identity_passthrough spelling (try . catch 1, if true then . else . end, . // 1) fell to the null/bool catch-all, wrongly decided the step was not intact, and either refused (a single pattern) or retried a ?// chain onto the wrong alternative -- which del()/= then wrote through (silent wrong delete: succinctly wrote null where jq deletes one key). First fix attempt bolted three ad-hoc If/Try/Alternative match arms onto the identical computation, reusing is_identity_passthrough / is_raise_free_identity_passthrough directly. Review (four independent passes) found this unsound: those two predicates only certify a shape as a candidate whose value is re-checked LATER, at a separate use site (substitute_bound_var_at's Origin::Snapshot/SnapshotAt contract) -- there is no later check in resolve_as_pattern, so trusting `trackable` alone for a recognized shape wrongly certified whichever branch's value actually ran, including a nested `A // B`'s B-side, or a TrackedVar frozen at an earlier, different position than the current register. Fixed with a single recursive helper, resolves_to_register, that re-runs the bare-TrackedVar arm's own check (marker.value == *reg && frame.certifies(&marker.origin)) at every nesting level it recurses through, rather than trusting trackable as a stand-in. Its Alternative arm requires the register be truthy at each level (A // B only equals `.` exactly when A actually produced the value), matching the #3129 lesson applied to an immediate, non-deferred certification instead of a deferred one. Fixes #3119.
b737cde to
7e812aa
Compare
… hole, hoist and dedupe the call site Third review round: resolves_to_register's Alternative arm gated only on `trackable && reg.is_truthy()`, which proves left is never filtered by // if it produces a value, not that it produces one at all. An If on left whose condition can itself yield nothing (if empty then . else . end) slipped through: // falls through to B regardless of the register's truthiness whenever left yields empty, and B (a fresh literal) was wrongly certified as the register's own node -- the same #3129 class of bug reintroduced one level deeper. Fixed by additionally requiring is_raise_free_identity_passthrough(left), matching the guard already used for Try, and matching is_identity_passthrough's own (already-#3129-fixed) Alternative arm. Also, from the second review round's reuse/efficiency findings: - resolve_as_pattern's identical computation delegated to resolves_to_register for Identity/TrackedVar too, removing the duplicated inline checks, while preserving their exact pre-#3119 behavior (no null/bool fallback for those two leaf shapes). - Hoisted the now-single resolves_to_register call out of the per-`bound` loop, since head/trackable/register/frame are all fixed before the loop starts. - Removed the redundant If|Try|Alternative variant enumeration at the call site, since resolves_to_register's own catch-all already refuses every other shape. Recorded both confirmed refuse-only residuals (an off-register truthy-value coincidence, and a mismatched-branch If) in docs/compliance/jq/limitations.md per ADR-0018, and added test coverage for the null/bool fallback and the If arm's && (not ||) proving both are load-bearing, not vestigial.
|
Ran `/code-review` (standard effort) three times on this PR — the first pass caught a fundamental soundness gap in the initial fix (reusing `is_identity_passthrough`'s weaker, deferred-certification guarantee for an immediate verdict), the second caught a residual empty-condition hole one level deeper in the corrected `Alternative` arm, and the third (after fixing both) found no further correctness issues across two independent fresh passes. Fixed across all three rounds:
18 regression rows now cover the full history: the issue's original repro, the first fix's own gap, the second's, plus two rows proving `resolves_to_register`'s null/bool fallback and its `If` arm's `&&` (not `||`) are both load-bearing. Every row confirmed live against `/usr/bin/jq` 1.7.1. Full local suite green: tests, clippy (both feature sets), fmt, 99.32% patch coverage (the one uncovered line a pre-existing-pattern defensive panic arm, `tolerate`-annotated). |
Summary
resolve_as_pattern's first-step identity test (jq'spath_intact) only recognized abare
.head as "the source value is the register's own node." Every otheris_identity_passthroughspelling (try . catch 1,if true then . else . end,. // 1) fell to the null/bool catch-all, wrongly decided the step was not intact, andeither refused (a single pattern) or retried onto the next
?//alternative —binding a different alternative than jq does, which
del/=then write through. Theload-bearing case is a silent wrong delete:
del((. // 1) as {a:$v} ?// $v | $v)on{"a":"s","c":"s"}wrotenull(deleting the whole document) where jq deletes one key,leaving
{"c":"s"}.Fix: add dedicated
If/Try/Alternativematch arms to theidenticalcomputation,mirroring
is_identity_passthrough's own grammar exactly. TheAlternative(A // B)arm additionally requires the register be truthy —
A // Bonly equals.exactly whenAactually produced the value;Bruns, and is completely unconstrained, whenever theregister is null/false. This is stricter than the issue's own suggested
bound == valuegate (which alone could re-admit a #3129-style value-coincidence fabrication off-register,
per #3120's carried-register mechanism) and stays sound in that case, at the cost of one
confirmed refuse-only divergence from jq (documented and pinned in the new test).
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_identity_passthrough_pattern_head_is_recognized_3119) pins all 6 rows from the issue's own repro table against/usr/bin/jq1.7.1 (including both controls), plus aniftwin not in the issue but the same gap, plus a confirmed refuse-only off-register divergence from theAlternativearm's extra runtime gatecargo llvm-cov+omni-dev coverage diff: 98.65% patch coverage (73/74), the one remaining line a defensive panic arm annotatedomni-dev: coverage tolerate-line, matching the identical pre-existing pattern elsewhere in this filedel((. // 1) as {a:$v} ?// $v | $v)on{"a":"s","c":"s"}now matches jq's{"c":"s"}exactly, where it previously silently deleted the whole documentFixes #3119.