diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 04e6b1435..ff39af7bd 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -863,7 +863,11 @@ is the revert that established what the other one costs. equal-valued sibling `path(.a as $y \| .c \| $y)` still refuses — exactly the #1466 class the frame witness exists to keep closed, now for navigated bindings too. yq mode is unchanged: `substitute_bound_var`'s widening is jq-mode only - ([#2643](https://github.com/rust-works/succinctly/issues/2643)). + ([#2643](https://github.com/rust-works/succinctly/issues/2643)). The sibling refusal is + itself the general (non-`null`/`bool`) case: jq's own `jv_identical` admits + `null`/`true`/`false` by value regardless of node, so the same shape on `{"a":true,"c":true}` + answers `["c"]` on both + ([#3136](https://github.com/rust-works/succinctly/issues/3136)). Fourteen rows stay refuse-only, each pinned in `test_path_bind_origin_matrix_refuse_only_2042` (`src/jq/eval.rs`) and `scripts/jq-bind-origin-oracle-sweep.sh`'s own `REFUSE_ONLY` list: @@ -894,7 +898,10 @@ is the revert that established what the other one costs. `Snapshot` marker not proven to name that call's own document node to `Untracked` first, so `Frame::certifies`'s unconditional `Snapshot => true` never gets a chance to admit the rebuilt copy — refuse-only by construction, since the only transition is `Snapshot → - Untracked` and `Untracked` never certifies. A documented residual remains: a handful of + Untracked` and `Untracked` never certifies by node identity (a `null`/`bool` rebuilt copy + still certifies by jq's own value-identity rule, #3136 — sound, since jq's `jv_identical` + has no pointer identity for those three values at all, rebuilt or not). A documented + residual remains: a handful of constructions jq's own reference-counted `jv` passes an embedded node through *without copying it* (`{k:.} \| .k`, `. + {}`, `reduce empty as $i (.; .)`, and `path(.[0] | $x)` navigation *inside* a `path()` call over `[.]`) now refuse rather than silently accept a @@ -1036,11 +1043,14 @@ is the revert that established what the other one costs. is no cursor at that funnel to promote against), a navigated bind on an *owned-rooted* document (`input | .a as $y | .a | path($y)`, `jq -n '{a:{b:1}} | .a as $y | .a | ($y.b) = 9'`, a `tojson|fromjson`-rebuilt root: the marker's node is an `OwnedIdentity` position, and - `marker_is_root` reads only a document node against a live cursor — no `OwnedRoot` twin), - and a `null`/`bool` marker at an - equal-valued sibling (`.a as $y | .c | $y |= 5` on `{"a":true,"c":true}`: jq's - `jv_identical` admits those by value regardless of node, the resolver's `TrackedVar` arm - consults the origin first — pre-existing). The library entry point `succinctly::jq::eval` + `marker_is_root` reads only a document node against a live cursor — no `OwnedRoot` twin). + A `null`/`bool` marker at an equal-valued sibling (`.a as $y | .c | $y |= 5` on + `{"a":true,"c":true}`) used to be refuse-only the same way — jq's `jv_identical` admits + those by value regardless of node, but the resolver's `TrackedVar` arm consulted the + origin first — closed by + [#3136](https://github.com/rust-works/succinctly/issues/3136), which ORs in the same + null/bool value-identity carve-out `register_identical` already makes, gated the same way + by the existing node-identity check. The library entry point `succinctly::jq::eval` takes the eager evaluator for a program `needs_path_context` does not route (a `path(f)` with an argument, a write), so through it these rows keep refusing as before; the CLI's route is the generic evaluator, where they answer. diff --git a/scripts/jq-bind-origin-oracle-sweep.sh b/scripts/jq-bind-origin-oracle-sweep.sh index 11d1f55a9..3946df2fb 100755 --- a/scripts/jq-bind-origin-oracle-sweep.sh +++ b/scripts/jq-bind-origin-oracle-sweep.sh @@ -497,7 +497,6 @@ navigated-bind-positional-assign:#3037 residual -- same as navigated-bind-positi navigated-bind-embed:#2889 -- the owned-embed residual ({k:.a} | .k), unchanged by #3037 navigated-bind-reduce-update:#3037 residual -- the UPDATE of reduce re-enters the eager evaluator with an owned accumulator; its eval_as carries no node for a navigated bind (#2072 gave the generic evaluator that, not this one), and there is no cursor at the funnel to promote against navigated-bind-catch-handler:#3037 residual -- same as navigated-bind-reduce-update, through a catch handler -navigated-bind-bool-sibling:pre-existing -- the jv_identical rule of jq admits a null/bool by value regardless of node, but the TrackedVar arm of the resolver consults the origin first; ($y | .) = 5 already answers since the . stage re-establishes by value navigated-bind-owned-root:#3037 residual -- a navigated bind on an owned-rooted document; the marker node is an OwnedIdentity position and marker_is_root reads only a document node against a live cursor, no OwnedRoot twin navigated-bind-input-root:#3037 residual -- same as navigated-bind-owned-root, on the input-queue route identity-if-arms-differ:#2978 -- identity_bind_position is static: an if whose arms sit at different positions ($p at [], . at ["a"]) proves neither, so the bind stays a bare Snapshot and getpath has no position to compose from; jq evaluates the condition diff --git a/src/jq/eval.rs b/src/jq/eval.rs index 8201bb1cc..744fba850 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -29212,10 +29212,14 @@ fn marker_needs_demotion(marker: &Tracked, root: &RootWitness) -> bool { /// Rewrite every [`Expr::TrackedVar`] marker in `expr` whose /// [`marker_needs_demotion`] against `root` into an [`Origin::Untracked`] -/// copy (#2642) -- a demotion, never a promotion: `Frame::certifies` already -/// refuses `Untracked` unconditionally, so this is the only change needed to -/// stop a rebuilt copy from being admitted, without touching `Frame`, -/// `Origin`, or `resolve_node`'s certification rule itself. +/// copy (#2642) -- a demotion, never a promotion: `Frame::certifies` still +/// refuses `Untracked` by node identity unconditionally, so this is the +/// only change needed to stop a rebuilt copy from being admitted *that +/// way*. [`marker_identical`]'s null/bool value-identity carve-out (#3136) +/// applies uniformly to every `Origin` including a demoted `Untracked` +/// one -- correctly, since jq's own `jv_identical` has no pointer identity +/// for `null`/booleans at all, rebuilt or not -- so this demotion still +/// costs only node-identity-based acceptance, never the null/bool one. /// /// The walk itself is [`rewrite_markers`], shared with the one promotion /// this evaluator performs ([`reroot_markers`], #3037): `Cow::Borrowed` @@ -31628,9 +31632,19 @@ fn resolve_node_eager<'a, S: EvalSemantics>( // `Origin::Untracked` (#2072: a navigated binding wrapped only for // the cursor routes' sake, made outside any resolver invocation) // never certifies, so this refuses exactly as it did before #2072 - // gave every `as` binding a `TrackedVar` wrapper. + // gave every `as` binding a `TrackedVar` wrapper -- except the one + // case jq's own `jv_identical` admits regardless of node identity + // (#3136): `null`/`true`/`false` are identical by value alone (the + // same carve-out `null_bool_identical` makes for `register_identical` + // and the pattern-walk's own first-step check), so an + // `Origin::Untracked` marker at an equal-valued sibling still + // certifies when its value is one of those three, where the + // node-identity rule above never can. Expr::TrackedVar(marker) => { - if trackable && marker.value == *value && frame.certifies(&marker.origin) { + if trackable + && marker.value == *value + && (frame.certifies(&marker.origin) || null_bool_identical(&marker.value, value)) + { Ok(vec![PathBranch::new( PathPrefix::root(), Cow::Borrowed(value), @@ -33549,6 +33563,18 @@ fn null_bool_identical(a: &OwnedValue, b: &OwnedValue) -> bool { matches!(a, OwnedValue::Null | OwnedValue::Bool(_)) && a == b } +/// Whether `marker` certifies against `target` -- node identity +/// (`Frame::certifies`) or, failing that, jq's `jv_identical` null/bool +/// value-identity carve-out (#3136). Shared by `resolve_node_eager`'s and +/// `resolves_to_register`'s otherwise-identical `Expr::TrackedVar` arms so +/// a future refinement of this rule can't apply to one and silently miss +/// the other, the same duplication risk [`null_bool_identical`]'s own doc +/// comment names. +fn marker_identical(marker: &Tracked, target: &OwnedValue, frame: &Frame) -> bool { + marker.value == *target + && (frame.certifies(&marker.origin) || null_bool_identical(&marker.value, target)) +} + /// Whether `expr`, given it runs to completion without raising or /// yielding zero outputs, is *provably* `reg` -- `resolve_as_pattern`'s /// first-step identity test (jq's `path_intact`) needs exactly this, which @@ -33573,9 +33599,9 @@ fn null_bool_identical(a: &OwnedValue, b: &OwnedValue) -> bool { /// `is_identity_passthrough(then_branch) && is_identity_passthrough /// (else_branch) => trackable` arm answered `["a","x"]` and `del` wrote /// through the mismatch. This function instead re-runs the bare- -/// `TrackedVar` arm's own check (`marker.value == *reg && -/// frame.certifies(&marker.origin)`) at every level it recurses to, so a -/// stale marker refuses wherever it's reached, not just at the top. +/// `TrackedVar` arm's own check ([`marker_identical`]) at every level it +/// recurses to, so a stale marker refuses wherever it's reached, not just +/// at the top. /// /// Mirrors `is_identity_passthrough`'s grammar otherwise, with one /// difference: `Alternative` is gated on the *runtime* register being @@ -33587,7 +33613,10 @@ fn null_bool_identical(a: &OwnedValue, b: &OwnedValue) -> bool { fn resolves_to_register(expr: &Expr, trackable: bool, reg: &OwnedValue, frame: &Frame) -> bool { match unwrap_paren(expr) { Expr::Identity => trackable, - Expr::TrackedVar(marker) => marker.value == *reg && frame.certifies(&marker.origin), + // #3136: [`marker_identical`], shared with `resolve_node_eager`'s + // own `TrackedVar` arm, so a future refinement of this rule can't + // apply to one and silently miss the other. + Expr::TrackedVar(marker) => marker_identical(marker, reg, frame), Expr::If { then_branch, else_branch, diff --git a/src/jq/expr.rs b/src/jq/expr.rs index 094680753..f2f407603 100644 --- a/src/jq/expr.rs +++ b/src/jq/expr.rs @@ -161,10 +161,13 @@ pub struct Tracked { /// - [`Origin::Untracked`] -- the #2072 witness for a binding whose source /// navigated, made *outside* any resolver invocation (an ordinary `as` /// binding, not one syntactically inside `path()`'s argument): there is -/// no invocation to certify against, so this marker always refuses, -/// exactly as an un-wrapped literal did before #2072 gave every `as` -/// binding a `TrackedVar` wrapper so its `Tracked::node` could carry -/// cursor identity. `path(.a as $y | .c | $y)` still refuses, matching jq. +/// no invocation to certify against, so this marker refuses by node +/// identity, exactly as an un-wrapped literal did before #2072 gave every +/// `as` binding a `TrackedVar` wrapper so its `Tracked::node` could carry +/// cursor identity. `path(.a as $y | .c | $y)` on `{"a":{"b":1},"c":{"b":1}}` +/// still refuses, matching jq -- but jq's own `jv_identical` admits +/// `null`/`true`/`false` by value regardless of node (#3136), so the same +/// filter on `{"a":true,"c":true}` answers `["c"]` on both. #[derive(Debug, Clone, PartialEq)] pub enum Origin { /// Frozen from the ambient input itself; certified by value equality @@ -190,7 +193,8 @@ pub enum Origin { path: super::eval::BindPath, }, /// Frozen from a navigated position outside any resolver invocation - /// (#2072); never certified. + /// (#2072); never certified by node identity, only by the null/bool + /// value-identity carve-out (#3136) every other `Origin` also gets. Untracked, } diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 1cd8feb99..097fbefd0 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -60792,10 +60792,6 @@ fn test_navigated_bind_traps_still_refuse_3037() -> Result<()> { /// `tojson|fromjson`-rebuilt root): the marker's node is an /// `OwnedIdentity` position, and `marker_is_root` reads only a document /// node against a live cursor -- no `OwnedRoot` twin (review finding). -/// - A `null`/`bool` marker at an equal-valued sibling: jq's `jv_identical` -/// admits those by value regardless of node, but the resolver's -/// `TrackedVar` arm consults the origin first (pre-existing; `($y | .) -/// = 5` already answers, since the `.` stage re-establishes by value). #[test] // jq filter literals like `{b:1}`/`{k:.a}` are not formatting strings; // clippy cannot tell the two apart from the brace shape alone (as `*_2642`). @@ -60813,7 +60809,6 @@ fn test_navigated_bind_residuals_refuse_cleanly_3037() -> Result<()> { r#"{"a":{"b":1}}"#, r"try error(.) catch (.a as $y | .a | path($y))", ), - (r#"{"a":true,"c":true}"#, r".a as $y | .c | $y |= 5"), ( r#"{"a":{"b":1}}"#, r"(tojson|fromjson) | .a as $y | .a | path($y)", @@ -60833,6 +60828,68 @@ fn test_navigated_bind_residuals_refuse_cleanly_3037() -> Result<()> { Ok(()) } +/// #3136: jq's own `jv_identical` treats `null`/`true`/`false` as identical +/// by value regardless of node (the same carve-out `null_bool_identical` +/// already made for `register_identical` and the pattern-walk's own +/// first-step check) -- but `resolve_node_eager`'s `Expr::TrackedVar` arm +/// consulted node identity first, so a `null`/`bool` marker bound from a +/// navigated position refused at an equal-valued *sibling* where jq +/// answers. All three values, and both write forms (`|=`, `del`), not just +/// the `path()` read `test_navigated_bind_residuals_refuse_cleanly_3037` +/// used to pin as refuse-only. The last two rows are +/// `resolve_as_pattern`'s own sibling gap (review): a bare-var pattern +/// source (`$y as $z | ...`) reaches `resolves_to_register`'s identical +/// `TrackedVar` arm, which had the same gap independently. +#[test] +fn test_navigated_bind_null_bool_sibling_answers_3136() -> Result<()> { + for (input, filter, want) in [ + (r#"{"a":true,"c":true}"#, r".a as $y | .c | path($y)", "[]"), + (r#"{"a":true,"c":true}"#, r".a as $y | .c | $y |= 5", "5"), + (r#"{"a":null,"c":null}"#, r".a as $y | .c | path($y)", "[]"), + ( + r#"{"a":false,"c":false}"#, + r"del(.a as $y | .c | $y)", + r#"{"a":false}"#, + ), + ( + r#"{"a":true,"c":true}"#, + r".a as $y | .c | $y as $z | path($z)", + "[]", + ), + ( + r#"{"a":false,"c":false}"#, + r"del(.a as $y | .c | $y as $z | $z)", + r#"{"a":false}"#, + ), + ] { + let (stdout, stderr, code) = run_jq_full(&["-c", filter], Some(input))?; + assert_eq!(code, 0, "#3136: `{filter}`: stderr={stderr:?}"); + assert_eq!(stdout.trim_end(), want, "#3136: `{filter}`"); + } + // Controls: a non-null/bool equal-valued sibling still refuses -- jq's + // own jv_identical requires actual pointer identity for those, which + // this fix does not (and must not) widen. One direct bind, one through + // a pattern source, exercising both fixed call sites' own gates. + for (filter, input) in [ + (r".a as $y | .c | path($y)", r#"{"a":{"b":1},"c":{"b":1}}"#), + ( + r".a as $y | .c | $y as $z | path($z)", + r#"{"a":{"b":1},"c":{"b":1}}"#, + ), + ] { + let (stdout, stderr, code) = run_jq_full(&["-c", filter], Some(input))?; + assert_eq!( + code, 5, + "#3136 control: non-null/bool sibling must still refuse, `{filter}`: got stdout={stdout:?} stderr={stderr:?}" + ); + assert!( + stderr.contains("Invalid path expression"), + "#3136 control: `{filter}` -- stderr: {stderr:?}" + ); + } + Ok(()) +} + /// yq mode is untouched by #3037: real yq v4.53.3 treats `($y.b) = 9` /// through a variable as a no-op and prints the document unchanged /// (`b: 1`), at the marker's own node and at a sibling alike, where