fix(jq): admit a null/bool navigated-bind marker at an equal-valued sibling - #3152
Conversation
CoverageTotal: 94.04% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 174 tolerated region(s)
Patch coveragePatch: 100% (8/8 new lines covered)
|
CoverageTotal: 93.95% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 175 tolerated region(s)
Patch coveragePatch: 100% (8/8 new lines covered)
|
…, and restructure both conditions to match register_identical's idiom Review findings on PR #3152: - resolves_to_register's own TrackedVar arm (used by resolve_as_pattern's first-step identity test) had the identical node-identity-first gap resolve_node_eager just closed -- a null/bool marker bound from a navigated position, reached through a pattern source (`$y as $z | ...`), refused at an equal-valued sibling where jq answers. Same fix: OR in null_bool_identical. - Restructured both conditions to factor on value-equality first (`A && B && (C || D)` instead of `A && (B && C || D)`), matching register_identical's own idiom for the identical carve-out -- logically equivalent (null_bool_identical's own body already requires the values equal), just readable as the same rule at a glance instead of needing the boolean algebra worked out by hand. - Updated two doc comments (Origin::Untracked's own doc in expr.rs, and two limitations.md passages) that flatly claimed an Untracked marker "always refuses" / a sibling "still refuses" without the null/bool exception this fix (and #3136 itself) carves out.
|
Ran `/code-review` (standard effort). Two independent review passes found:
Extended the regression test with the pattern-context case (`$y as $z | ...`) covering the sibling fix, plus its own non-null/bool control. Full local suite green: tests, clippy (both feature sets), fmt, 100% patch coverage, and `scripts/jq-bind-origin-oracle-sweep.sh` re-run clean against the release binary (`agree=302 fabricate=0 mismatch=0 refuse-only-NEW=0`). |
…ibling jq's own jv_identical treats null/true/false as identical by value regardless of node (register_identical's own null_bool_identical carve-out already models this for the register itself). The resolver's Expr::TrackedVar arm in resolve_node_eager consulted the marker's origin/node-identity check first, so a null/bool marker bound from a navigated position refused at an equal-valued sibling where jq answers -- .a as $y | .c | $y |= 5 exited 5 where jq writes 5, and the path() read had the same gap. Fix ORs in null_bool_identical(&marker.value, value) alongside the existing node-identity check, gated the same way by trackable. A non-null/bool equal-valued sibling is unaffected and still refuses, since jq's own jv_identical requires actual pointer identity there. Flipped the pre-existing sweep row (navigated-bind-bool-sibling) from refuse-only to agree, and the CLI test that pinned the old refusal.
Review findings on PR #3152: - resolves_to_register's own TrackedVar arm (used by resolve_as_pattern's first-step identity test) had the identical node-identity-first gap resolve_node_eager just closed -- a null/bool marker bound from a navigated position, reached through a pattern source (`$y as $z | ...`), refused at an equal-valued sibling where jq answers. Same fix: OR in null_bool_identical. - Restructured both conditions to factor on value-equality first (`A && B && (C || D)` instead of `A && (B && C || D)`), matching register_identical's own idiom for the identical carve-out -- logically equivalent (null_bool_identical's own body already requires the values equal), just readable as the same rule at a glance instead of needing the boolean algebra worked out by hand. - Updated two doc comments (Origin::Untracked's own doc in expr.rs, and two limitations.md passages) that flatly claimed an Untracked marker "always refuses" / a sibling "still refuses" without the null/bool exception this fix (and #3136 itself) carves out.
Second review round on PR #3152: - Extracted the near-identical node-identity-or-null-bool formula both new TrackedVar arms hand-inlined into a shared marker_identical helper, matching register_identical's own precedent and its documented reason for existing as one function. - Fixed three doc comments left stale by the null/bool carve-out: demote_rebuilt_markers and a limitations.md passage both claimed an Untracked marker "never certifies"/nothing touches the certification rule, without the value-identity exception; a third quoted the old pre-helper formula verbatim. - Fixed two wrong example outputs introduced by this PR's own earlier doc edits: path(.a as $y | .c | $y) on {"a":true,"c":true} answers ["c"], not []; verified live against /usr/bin/jq 1.7.1.
b24a20e to
9557236
Compare
|
Second `/code-review` round found and fixed:
Full local suite green: tests, clippy (both feature sets), fmt, 100% patch coverage, sweep script re-run clean (`agree=302 fabricate=0 mismatch=0 refuse-only-NEW=0`), and the CLI binary directly re-verified against the pinned oracle post-rebase. |
Summary
jq's own
jv_identicaltreatsnull/true/falseas identical by value regardless ofnode (
register_identical's ownnull_bool_identicalcarve-out already models this for theregister itself). The resolver's
Expr::TrackedVararm inresolve_node_eagerconsulted themarker's origin/node-identity check first, so a
null/boolmarker bound from a navigatedposition refused at an equal-valued sibling where jq answers:
Fix ORs in
null_bool_identical(&marker.value, value)alongside the existing node-identitycheck, gated the same way by
trackable. A non-null/bool equal-valued sibling is unaffectedand still refuses, since jq's own
jv_identicalrequires actual pointer identity for those(verified live:
.a as $y | .c | path($y)on{"a":{"b":1},"c":{"b":1}}still refuses onboth).
Flipped the pre-existing oracle sweep row (
navigated-bind-bool-sibling,scripts/jq-bind-origin-oracle-sweep.sh) fromrefuse-onlytoagree, and the CLI test thatpinned the old refusal (
test_navigated_bind_residuals_refuse_cleanly_3037).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_navigated_bind_null_bool_sibling_answers_3136) covers all three ofnull/true/false, both the read (path()) and write (|=,del) directions, plusa control confirming a non-null/bool equal-valued sibling is unaffected — every row
confirmed live against
/usr/bin/jq1.7.1scripts/jq-bind-origin-oracle-sweep.shagainst the release binary:agree=302 fabricate=0 mismatch=0 refuse-only=35 refuse-only-NEW=0— no regressions across thewhole
$var-node-identity oracle matrixcargo llvm-cov+omni-dev coverage diff: 100% patch coverage (3/3 new lines)Fixes #3136.