diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index ef3da3bdd..30f10de1a 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -1273,6 +1273,37 @@ is the revert that established what the other one costs. jq), so the handler stays untracked unless the payload is `null`/`bool`; and `(if true then $x else . end) as $y` on an untracked stage, which binds `Untracked` because the condition is not evaluated where jq evaluates it and binds the marker. + + **`resolve_as_pattern`'s own first-step identity test recognizes every + `is_identity_passthrough` spelling now** — + [#3119](https://github.com/rust-works/succinctly/issues/3119). It used to recognize only a + bare `.` head; every other 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 — `del((. // 1) as {a:$v} ?// $v \| $v)` on + `{"a":"s","c":"s"}` wrote `null`, deleting the whole document, where jq deletes one key. + Closed by `resolves_to_register`, which re-derives the bare-`TrackedVar` arm's own + `marker.value == *reg && frame.certifies(...)` check at every level it recurses through + (not `is_identity_passthrough`'s weaker, deferred-certification guarantee — see that + function's own doc comment) and requires the register be truthy for an `Alternative` + *and* `is_raise_free_identity_passthrough(left)`, closing an `If`-on-`left`-with-an- + empty-condition hole two review rounds found live (`del(.a \| ((if empty then . else . + end) // {p:100,q:200}) as {p:$x} \| $x)` on `{"a":{"p":1,"q":2},"c":"keep"}` wrote + `{"a":{"q":2},"c":"keep"}` without that extra guard). + + **Two residual, refuse-only gaps remain**, both pinned in + `test_alternative_identity_passthrough_pattern_head_is_recognized_3119`: + - a marker frozen off the register (#3120's carried register, not `trackable`) whose + value happens to be truthy and value-equal to the register's own real node — jq answers + (`path(. as $x \| 5 \| ($x // 1) as {a:$q} ?// $z \| $q)` on `{"a":1}` is `["a"]`), but a + value-only check off-register cannot tell that real coincidence from an unrelated one + (the exact #3129 class of bug), so this stays refuse-only; + - an `if` whose branches don't *both* statically recognize, even when its condition is a + constant that always takes the recognized one — `path(.a \| (if true then . else 5 end) + as {b:$v} \| $v)` on `{"a":{"b":1}}` is jq's `["a","b"]`; the static rule cannot tell + it from a truly arbitrary condition without evaluating it, the same refuse-only cost + `test_identity_bind_position_traps_keep_refusing_2978`'s row 9 already pays for + `identity_bind_position`'s sibling mechanism. 3. **jq's pointer-identity artifacts on `*`/`+` with an empty operand** — `path(. as $x \| reduce (1) as $i (0; $x + {}))` on `{"a":1}` is `[]` in jq; succinctly refuses (likewise `$x * {}` and `$x + null`). This is not a rule jq implements but an diff --git a/src/jq/eval.rs b/src/jq/eval.rs index ff95ff888..1f9298c23 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -33496,6 +33496,80 @@ fn null_bool_identical(a: &OwnedValue, b: &OwnedValue) -> bool { matches!(a, OwnedValue::Null | OwnedValue::Bool(_)) && a == b } +/// 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 +/// is a *stronger* property than `is_identity_passthrough`/ +/// `is_raise_free_identity_passthrough` give (#3119 review, after two +/// rounds of live-verified false positives against those helpers reused +/// naively here): those two only certify a shape as a *candidate* whose +/// value gets re-checked later, at a separate use site +/// (`substitute_bound_var_at`'s `Origin::Snapshot`/`SnapshotAt` contract, +/// #844/#2978) -- there is no later check here, so a wrong "yes" is +/// immediately load-bearing, not deferred. +/// +/// Concretely, both helpers accept a `TrackedVar` purely by its `origin` +/// kind (`Snapshot`/`SnapshotAt`), never by comparing its *frozen* value +/// to the *current* register -- so a marker frozen at one position, +/// referenced again after the register has moved on and wrapped in +/// `if`/`try`/`//`, would wrongly inherit "identical to the current +/// register" from `trackable` alone if this function just recursed +/// through those two helpers' verdicts. Confirmed live: `. as $x | .a | +/// (if true then $x else $x end) as {x:$q} | $q` on `{"a":{"x":1,"y":2}}` +/// -- jq refuses (`$x` is the *root*, not `.a`); a naive +/// `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. +/// +/// Mirrors `is_identity_passthrough`'s grammar otherwise, with one +/// difference: `Alternative` is gated on the *runtime* register being +/// truthy (`A // B` only ever equals `.` exactly when `A` actually +/// produced the value) rather than on `is_raise_free_identity_passthrough +/// (left)` alone, which says nothing about which side's value reached +/// `bound` -- the same #3129 lesson applied to an immediate, non-deferred +/// certification instead of a later one. +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), + Expr::If { + then_branch, + else_branch, + .. + } => { + resolves_to_register(then_branch, trackable, reg, frame) + && resolves_to_register(else_branch, trackable, reg, frame) + } + Expr::Try { expr, .. } if is_raise_free_identity_passthrough(expr) => { + resolves_to_register(expr, trackable, reg, frame) + } + // #3119 third review round: `trackable && reg.is_truthy()` alone + // only proves `left`, *if it produces an output*, is truthy -- + // never filtered by `//` -- not that it produces one at all. An + // `If` reached directly as `head` is self-correcting (a `bound` + // this loop sees is always a real fork output, so an empty + // condition just means that output never existed); nested under + // `//` it is not, because `//` silently substitutes an unrelated + // `B` whenever `left` yields nothing, not only when `left` + // raises. `is_raise_free_identity_passthrough` is what excludes + // `If` (and anything containing one) from `left` here, the same + // guard `Try`'s arm above already needs and for the identical + // reason (confirmed live: `del(.a | ((if empty then . else . end) + // // {"p":100,"q":200}) as {p:$x} | $x)` on + // `{"a":{"p":1,"q":2}}` wrote `{"a":{"q":2}}` without this guard, + // where jq refuses). + Expr::Alternative(left, _) + if trackable && reg.is_truthy() && is_raise_free_identity_passthrough(left) => + { + resolves_to_register(left, trackable, reg, frame) + } + _ => false, + } +} + /// The register `path()`-tracking compares each `reduce`/`foreach` fold /// iteration's own navigation against — jq's `(path, value_at_path)` pair, /// derived empirically against jq 1.7.1 (#1440; no fold-specific machinery @@ -34617,19 +34691,32 @@ fn resolve_as_pattern<'a, S: EvalSemantics>( // jq's `value_at_path` entering this stage: `value` itself while // trackable, otherwise whatever the enclosing pipe carried in (#3120). let register = if trackable { Some(value) } else { register }; + // jq's `path_intact` at the pattern's first step: is the source value + // the register's own node? `resolves_to_register` (#3119) answers this + // for `head`/`trackable`/`register`/`frame`, which are all fixed before + // this loop starts, so it is computed once rather than per `bound`; a + // bare `.`/`TrackedVar` head is exactly what it recognizes at its own + // leaves, so this delegates fully rather than re-implementing them here + // (its own doc explains why `is_identity_passthrough` is unsound to + // reuse instead). Unrecognized shapes get no fallback here -- unlike + // the loop's own per-`bound` null/bool check below -- matching this + // arm's pre-#3119 behavior for a bare `.`/`TrackedVar` head exactly. + let head_resolves_to_register = + register.is_some_and(|reg| resolves_to_register(head, trackable, reg, frame)); for bound in &sources { - // jq's `path_intact` at the pattern's first step: is the source - // value the register's own node? `.` is, while `trackable`; a marker - // the frame certifies is (the `TrackedVar` arm's own rule; on an - // untracked stage `frame` sits at the carried register's own - // position, which is exactly what an `Origin::At` is certified - // against); anything else only by jq's null/bool value identity - // against the register. No register in hand: never. - let identical = register.is_some_and(|reg| match head { - Expr::Identity => trackable, - Expr::TrackedVar(marker) => marker.value == *reg && frame.certifies(&marker.origin), - _ => null_bool_identical(bound, reg), - }); + // A shape `resolves_to_register` refuses (including one it was + // never asked about, for `Expr::Identity`/`TrackedVar`) still gets + // jq's own null/bool value-identity fallback per `bound`, since a + // `B`/handler-derived value that happens to match the register + // that way coincides with jq's own `jv_identical` verdict + // regardless of shape. + let identical = match head { + Expr::Identity | Expr::TrackedVar(_) => head_resolves_to_register, + _ => { + head_resolves_to_register + || register.is_some_and(|reg| null_bool_identical(bound, reg)) + } + }; // Whether a first-step refusal is jq's own verdict rather than this // arm's guess: the source is not even value-equal to the register, // so it cannot be the register's node whatever jq's pointer says. @@ -99157,6 +99244,232 @@ mod tests { } } + /// `resolve_as_pattern`'s first-step identity test (jq's `path_intact`) + /// 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 (row 3: succinctly + /// wrote `null`, deleting the whole document, where jq deletes one key). + /// Rows 1-6 are the issue's own repro table; rows 7-8 are the `if` + /// twin of rows 1/3, not in the issue but the same gap (`if`'s own + /// `Expr` variant, not just `try`/`Alternative`); row 9 is a confirmed + /// refuse-only divergence from `resolves_to_register`'s own extra + /// runtime gate (see its doc comment, and + /// [docs/compliance/jq/limitations.md](../../../docs/compliance/jq/limitations.md)). + /// The next block is a second review round's two findings against the + /// *first* fix -- see `resolves_to_register`'s own doc comment for why + /// a naive `is_identity_passthrough`-reuse for `If`/`Try` was unsound. + /// The next two rows are a *third* review round's finding: the + /// `Alternative` arm's `reg.is_truthy()` gate alone doesn't prove + /// `left` produces a value at all, only that it isn't filtered if it + /// does -- an `If` on `left` with an empty-yielding condition slipped + /// through and got `B`'s unrelated literal certified as the register. + /// The final two rows are coverage for `resolves_to_register`'s own + /// design: the null/bool fallback is load-bearing (not vestigial), and + /// the `If` arm's `&&` (not `||`) is what a mismatched-branch case + /// refuses on, both confirmed live. Every row is confirmed live + /// against `/usr/bin/jq` 1.7.1. + #[test] + // Several rows' filter strings are jq object-construction literals + // (`{a:1}`), not formatting strings; clippy cannot tell the two apart + // from the brace shape alone. + #[allow(clippy::literal_string_with_formatting_args)] + fn test_alternative_identity_passthrough_pattern_head_is_recognized_3119() { + type Row = ( + &'static str, + &'static str, + Result<&'static [&'static str], String>, + ); + let rows: Vec = vec![ + ( + r#"{"a":"s","c":"s"}"#, + r"path((try . catch 1) as {a:$v} | $v)", + Ok(&[r#"["a"]"#]), + ), + ( + r#"{"a":"s","c":"s"}"#, + r"path((. // 1) as {a:$v} ?// $v | $v)", + Ok(&[r#"["a"]"#]), + ), + ( + r#"{"a":"s","c":"s"}"#, + r"del((. // 1) as {a:$v} ?// $v | $v)", + Ok(&[r#"{"c":"s"}"#]), + ), + ( + r#"{"a":"s","c":"s"}"#, + r"[path((try . catch 1) as {a:$v} ?// $v | ($v | .[]?))]", + Ok(&["[]"]), + ), + // Controls: the bare-`.` head was already fine. + ( + r#"{"a":"s","c":"s"}"#, + r"[path(. as {a:$v} ?// $v | ($v | .[]?))]", + Ok(&["[]"]), + ), + ( + r#"{"a":"s","c":"s"}"#, + r"del(. as {a:$v} ?// $v | $v)", + Ok(&[r#"{"c":"s"}"#]), + ), + // The `if` twin (not in the issue): both arms recognize, so the + // condition's own runtime answer doesn't matter. + ( + r#"{"a":"s","c":"s"}"#, + r"path((if true then . else . end) as {a:$v} | $v)", + Ok(&[r#"["a"]"#]), + ), + ( + r#"{"a":"s","c":"s"}"#, + r"del((if true then . else . end) as {a:$v} ?// $v | $v)", + Ok(&[r#"{"c":"s"}"#]), + ), + // #3119 review: `A // B`'s extra runtime gate (the register + // must also be truthy, not just `trackable`) is stricter than + // the issue's own suggested `bound == value` -- off-register + // (#3120's carried register, not this stage's own `value`), a + // truthy, non-null/bool coincidence is refused here even + // though it happens to be jq's own real node in this + // construction (jq answers `["a"]`; a value-only check can't + // tell a carried register's real node from a same-valued + // unrelated one, the exact #3129 class of bug, so this stays + // refuse-only rather than risk it). + ( + r#"{"a":1}"#, + r"path(. as $x | 5 | ($x // 1) as {a:$q} ?// $z | $q)", + Err( + r#"Invalid path expression near attempt to access element "a" of {"a":1}"# + .to_string(), + ), + ), + // #3119 second review round: two independently-found soundness + // gaps in the first fix, both now closed by + // `resolves_to_register`'s recursive TrackedVar re-check. + // + // 1. A recognized branch can itself contain a nested + // `Alternative` -- `is_identity_passthrough` accepts it as a + // candidate (its own "deliberate, safe under-approximation" + // is safe only because a LATER value-equality check defers + // the real verdict elsewhere, #844/#2978); this call site + // has no later check, so trusting `trackable` alone for a + // recognized `If`/`Try` wrongly certified whichever branch's + // value actually ran, even when that value was `B`'s (not + // `.` at all). + ( + "null", + r"path((if true then (. // {a:1}) else . end) as {a:$v} | $v)", + Err(r#"Invalid path expression near attempt to access element "a" of {"a":1}"# + .to_string()), + ), + // 2-4. A recognized branch can bottom out in a `TrackedVar` + // frozen at a DIFFERENT position than the current register + // -- the bare-`TrackedVar` arm re-checks `marker.value == + // *reg` itself, but the old `If`/`Try`/`Alternative` arms + // granted `identical` from `trackable` alone without ever + // reaching that check for a marker nested inside them. + ( + r#"{"a":{"x":1,"y":2}}"#, + r"del(. as $x | .a | (if true then $x else $x end) as {x:$q} | $q)", + Err( + r#"Invalid path expression near attempt to access element "x" of {"a":{"x":1,"y":2}}"# + .to_string(), + ), + ), + ( + r#"{"a":{"other":1},"b":"X"}"#, + r"path(. as $x | .a | (try $x catch 1) as {b:$v} | $v)", + Err( + r#"Invalid path expression near attempt to access element "b" of {"a":{"other":1},"b":"X"}"# + .to_string(), + ), + ), + ( + r#"{"a":{"other":1},"b":"X"}"#, + r"path(. as $x | .a | ($x // 1) as {b:$v} | $v)", + Err( + r#"Invalid path expression near attempt to access element "b" of {"a":{"other":1},"b":"X"}"# + .to_string(), + ), + ), + // #3119 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. `left` + // containing an `If` 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. + ( + r#"{"a":{"p":1,"q":2}}"#, + r"path(.a | ((if empty then . else . end) // {p:100,q:200}) as {p:$x} | $x)", + Err( + r#"Invalid path expression near attempt to access element "p" of {"p":100,"q":200}"# + .to_string(), + ), + ), + ( + r#"{"a":{"p":1,"q":2},"c":"keep"}"#, + r"del(.a | ((if empty then . else . end) // {p:100,q:200}) as {p:$x} | $x)", + Err( + r#"Invalid path expression near attempt to access element "p" of {"p":100,"q":200}"# + .to_string(), + ), + ), + // Coverage: a case where `resolves_to_register` itself refuses + // (an unrecognized `If` head, one branch a plain literal) but + // the null/bool value-identity fallback still admits it, + // because the register genuinely is `null` and the literal + // branch taken is also `null` -- proves the fallback in + // `resolve_as_pattern`'s own dispatch is load-bearing, not + // vestigial. + ( + r#"{"a":null}"#, + r"path(.a | (if true then null else . end) as $v ?// $z | $v)", + Ok(&[r#"["a"]"#]), + ), + // Coverage: a case where `resolves_to_register`'s `If` arm's + // `&&` (both branches must independently resolve) is what + // refuses, proving it isn't accidentally `||` -- the condition + // is always true here, so jq answers (it only evaluates the + // taken branch); the static check can't know that without + // evaluating the condition, so this is refuse-only, the safe + // direction, matching `test_identity_bind_position_traps_keep_refusing_2978`'s + // row 9 for the same shape in the sibling mechanism. + ( + r#"{"a":{"b":1}}"#, + r"path(.a | (if true then . else 5 end) as {b:$v} | $v)", + Err( + r#"Invalid path expression near attempt to access element "b" of {"b":1}"# + .to_string(), + ), + ), + ]; + + for (doc, filter, want) in &rows { + let json = doc.as_bytes(); + let index = JsonIndex::build(json); + let expr = parse(filter).unwrap_or_else(|e| panic!("{filter}: {e:?}")); + let got = eval::, JqSemantics>(&expr, index.root(json)); + match want { + Ok(want) => { + let got: Vec = got + .collect_owned::() + .iter() + .map(OwnedValue::to_json) + .collect(); + assert_eq!(got, *want, "{doc} | {filter}"); + } + Err(want) => match got { + QueryResult::Error(e) => assert_eq!(e.message, *want, "{doc} | {filter}"), + other => panic!("{doc} | {filter}: expected a refusal, got {other:?}"), // omni-dev: coverage tolerate-line reason="unreachable in a passing suite by design -- this is the panic message for the assertion above, only formatted if the match doesn't hit the Error arm (#3119)" + }, + } + } + } + /// #3133: a pipe nested under `try`/`?` in a pattern body used to carry /// no register, so a `$w` marker there could not re-establish, its /// navigation raised the resolver's own refusal, and `try` caught it --