From 0a3fee68f7f94b2f99368157654904852b4566d7 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 05:32:12 +1000 Subject: [PATCH 1/4] fix(jq): give a `?` over a fan-out group jq's single abort scope `(A | B)?` is `try (A | B)`: a failure anywhere inside prunes the whole group's branch and stops the group's own generators. Two sites rewrote it into `A? | B?` instead -- `push_path_components` (#1311) for the resolver, `splice_optional_group` (#1294) for the write walkers -- turning one abort scope into N independent per-step ones, so a generator inside the group resumed past an error a later component raised. Both rewrites exist to stop the group's `?` leaking onto what *follows* the group, and both are correct for the pure-navigation groups they were written for. A single-valued chain has one branch, so pruning at step k and aborting the group are indistinguishable. Put a generator inside and they part company. #2909 filed this as a side-effect count. It is not: the output diverges, and silently. Captured live against jq 1.7.1: echo '{"a":{"b":1},"c":[1,2]}' | jq -c 'del((.[] | .[0])?)' jq: {"a":{"b":1},"c":[1,2]} was: {"a":{"b":1},"c":[2]} echo '[{"a":1},5,{"a":3}]' | jq -c '((.[] | .a)?) |= 99' jq: [{"a":99},5,{"a":3}] was: [{"a":99},5,{"a":99}] -- an element deleted that jq keeps, and a slot written that jq never reaches. The issue's own diagnosis needs two corrections as well: `..` is not required (any pipe does it), and the `?` never reaches a `?`-handling arm in the diverging shapes -- it is dissolved before resolution. One predicate, `optional_group_is_scope_safe`, gates both rewrites: distribute only when the group has fewer than two components or every one of them is single-valued. Conservative on purpose -- a group like `(.[] | .a?)` that happens not to diverge today is also called unsafe, which only changes which correct arm handles it. An unsafe group then has to *reach* that arm, so `needs_path_prepass` and `needs_fanout_pass` both answer `true` for it. Both are needed and for different reasons: the former is what routes `del`/`=`/`|=`/`path` through the resolver at all, and the latter peels `Optional` before testing, so `Optional(Pipe([Iterate, Field]))` would otherwise be mistaken for a single-valued static tail. With both set the group reaches `resolve_optional_sink` -- the arm that was already right, and the reason the issue's three "neighbouring spellings agree" rows agree. `wrap_optional_branch`'s `depth() != 1` arm was annotated "unreached today", tolerated for coverage, on reasoning that held only while a group was always distributed before reaching a resolver. It is live now -- verified by instrumenting it -- and needed no new logic: the old comment had already stated jq's answer for exactly this shape. Verified by a 2,400-combination differential sweep (30 group bodies, 14 of them randomised, x 10 outer spellings x 8 inputs) against jq 1.7.1: 8 divergences, all one pre-existing shape (`del((.[0:1] | .. | ..)?)`, which diverges identically on the merge-base and without any `?` at all -- filed separately). Zero regressions. A further 756-combination scan for an "invalid path component" leak from an un-distributed group reaching a native walker: none. Refs #2909 --- src/jq/eval.rs | 172 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 156 insertions(+), 16 deletions(-) diff --git a/src/jq/eval.rs b/src/jq/eval.rs index 1981b12a9..734d226fd 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -24575,6 +24575,15 @@ fn set_path( /// computed component makes a list non-atomic and sends it through /// [`flatten_path_components`] first. /// +/// Since #2909 the flatten is no longer guaranteed to *produce* atomic +/// components: a `?` over a group that can fan out stays one opaque +/// `Optional(Pipe(..))` element, because jq's `?` is a single abort scope +/// over the whole group. That element never reaches this walker anyway -- +/// `needs_path_prepass` answers `true` for exactly that shape, so +/// `=`/`|=`/`del`/`path` resolve it through `resolve_optional_sink` first +/// and hand the walker already-resolved components. Verified by sweeping 756 +/// group/outer/input combinations for an "invalid path component" leak: none. +/// /// `Identity` counts as atomic even though the flattener would drop it /// (`push_path_components`): the walker has a one-line arm for it, and /// treating it as non-atomic would send an otherwise-flat `.a | . | .b` @@ -25073,7 +25082,9 @@ fn unwrap_path_component(expr: &Expr) -> (&Expr, bool) { /// `Optional(Pipe([…]))` component and continue with one recursive call over /// the combined list, so the same fix applies identically to each: when /// `here` (the group's own `?`, folded with whatever optionality was already -/// ambient before it) is set, wrap each of `inner`'s own components in +/// ambient before it) is set *and the group cannot fan out* (#2909 -- +/// [`optional_group_slice_is_scope_safe`]), wrap each of `inner`'s own +/// components in /// `Expr::Optional` — [`unwrap_path_component`] already peels this at every /// step — instead of threading `here` as the recursive call's blanket /// `optional` parameter, which previously kept suppressing `rest` too, past @@ -25082,6 +25093,18 @@ fn unwrap_path_component(expr: &Expr) -> (&Expr, bool) { /// `optional` — not `here` — as that call's baseline, so `rest` keeps /// whatever optionality it already had on its own. fn splice_optional_group(inner: &[Expr], rest: &[Expr], here: bool) -> Vec { + // #2909: distributing `here` across `inner`'s components is only + // observationally identical to jq's single abort scope when the group + // cannot fan out -- see [`optional_group_is_scope_safe`]. When it can, + // the group stays one opaque `Optional(Pipe(..))` element, which + // `needs_path_prepass` now routes to the resolver's own `Expr::Optional` + // arm rather than to a walker that would see N independently-pruned + // steps. + if here && !optional_group_slice_is_scope_safe(inner) { + let mut spliced = vec![Expr::Optional(Box::new(flatten_components(inner.to_vec())))]; + spliced.extend_from_slice(rest); + return spliced; + } let mut spliced: Vec = if here { inner .iter() @@ -26637,6 +26660,12 @@ fn needs_path_prepass(expr: &Expr) -> bool { | Expr::Slice { .. } | Expr::Iterate => false, Expr::Pipe(exprs) => exprs.iter().any(needs_path_prepass), + // #2909: a `?` over a group that can fan out must reach the resolver + // even when every component of it is otherwise native-walkable, so + // `resolve_node_sink`'s own `Expr::Optional` arm gives it jq's single + // abort scope instead of a walker seeing N independently-pruned + // steps. + Expr::Optional(inner) if !optional_group_is_scope_safe(inner) => true, Expr::Optional(inner) | Expr::Paren(inner) => needs_path_prepass(inner), _ => true, } @@ -26671,7 +26700,11 @@ fn needs_path_prepass(expr: &Expr) -> bool { /// /// No `Expr::Pipe` arm: this function's only caller (`resolve_seq`) always /// calls it on `flat`'s already-`push_path_components`-flattened elements, -/// which by construction never contains a raw `Pipe` -- so unlike +/// which never contains a raw `Pipe` at the top level. (Since #2909 an +/// element *can* be an `Optional` wrapping one -- a group that can fan out +/// keeps jq's single abort scope by staying opaque -- which is what the +/// `Expr::Optional` arm above answers `true` for, rather than peeling into +/// it.) So unlike /// `needs_path_prepass`, which is also called on whole (possibly /// un-flattened) expressions elsewhere, adding one here would be untested /// dead code. @@ -26687,6 +26720,11 @@ fn needs_path_prepass(expr: &Expr) -> bool { fn needs_fanout_pass(expr: &Expr) -> bool { match expr { Expr::Iterate => true, + // #2909: this function peels `Optional` before testing, so without + // this arm `Optional(Pipe([Iterate, Field]))` answers `false` and is + // mistaken for `resolve_seq_sink`'s single-valued static tail. Same + // rule, same predicate as `needs_path_prepass`'s own arm. + Expr::Optional(inner) if !optional_group_is_scope_safe(inner) => true, Expr::Optional(inner) | Expr::Paren(inner) => needs_fanout_pass(inner), other => needs_path_prepass(other), } @@ -26699,6 +26737,94 @@ fn needs_fanout_pass(expr: &Expr) -> bool { /// `Identity | Field | Index | Iterate | Slice` and rejects anything else as /// "invalid path component", so a nested `Pipe` emitted by the pre-pass /// would break assignment with a message that reads like user error. +/// Is distributing a `?` across the components of the group it wraps +/// observationally identical to jq's single abort scope (#2909)? +/// +/// `(A | B)?` is `try (A | B)`: a failure anywhere inside prunes the **whole +/// group's** branch and stops the group's own generators. Two sites in this +/// file instead rewrite it into `A? | B?` -- [`push_path_components`] (#1311) +/// for the resolver, [`splice_optional_group`] (#1294) for the write walkers +/// -- which turns one abort scope into N independent per-step ones. Both +/// rewrites exist to stop the group's `?` leaking onto what *follows* the +/// group, and both are correct for the pure-navigation groups they were +/// written for. +/// +/// They are only correct there. A single-valued chain has exactly one branch, +/// so pruning at step *k* and aborting the whole group both emit nothing for +/// it -- the two are indistinguishable. Put a *generator* inside the group +/// and they part company: the generator resumes past an error a later +/// component raised, because its own per-step `?` pruned only that step. +/// Live against jq 1.7.1, all silent: +/// +/// ```console +/// $ echo '{"a":{"b":1},"c":[1,2]}' | jq -c 'del((.[] | .[0])?)' +/// {"a":{"b":1},"c":[1,2]} # here, before this: {"a":{"b":1},"c":[2]} +/// $ echo '[{"a":1},5,{"a":3}]' | jq -c '((.[] | .a)?) |= 99' +/// [{"a":99},5,{"a":3}] # here, before this: [{"a":99},5,{"a":99}] +/// ``` +/// +/// So: distribute only when the group cannot fan out. Fewer than two +/// components has nothing to distribute across in the first place; otherwise +/// every component must be single-valued. `Iterate`, a computed key, a +/// comma, `..` -- anything that can produce other than exactly one output -- +/// makes the group unsafe, and it stays one opaque `Optional(Pipe(..))` +/// element for `resolve_optional_sink` to give jq's own single scope. +/// +/// Deliberately conservative: a group like `(.[] | .a?)` that happens not to +/// diverge today is also called unsafe. That only changes *which* correct arm +/// handles it, never the answer. +/// +/// Allocation-free: consulted from [`needs_path_prepass`], which is on the +/// per-call path of every `path`/`del`/`=`/`|=`. +fn optional_group_is_scope_safe(inner: &Expr) -> bool { + /// Count the group's components, stopping as soon as two are seen -- + /// a group with fewer than two has nothing to distribute across, and is + /// safe whatever it contains. + fn walk(expr: &Expr, seen: &mut usize, safe: &mut bool) { + match expr { + Expr::Identity => {} + Expr::Pipe(exprs) => { + for e in exprs { + walk(e, seen, safe); + } + } + Expr::Paren(inner) => walk(inner, seen, safe), + Expr::Optional(inner) => walk(inner, seen, safe), + other => { + *seen += 1; + if !path_component_is_single_valued(other) { + *safe = false; + } + } + } + } + + let (mut seen, mut safe) = (0usize, true); + walk(inner, &mut seen, &mut safe); + seen < 2 || safe +} + +/// One component of a path group: can it produce anything other than exactly +/// one output? +/// +/// The single rule both [`optional_group_is_scope_safe`] and its slice +/// sibling decide by -- `Iterate`, a computed key, a comma, `..`, a call, all +/// answer `false` by falling through. +fn path_component_is_single_valued(expr: &Expr) -> bool { + match expr { + Expr::Identity | Expr::Field(_) | Expr::Index { .. } | Expr::Slice { .. } => true, + Expr::Optional(inner) | Expr::Paren(inner) => path_component_is_single_valued(inner), + _ => false, + } +} + +/// [`optional_group_is_scope_safe`] for a group already flattened into a +/// component slice -- [`splice_optional_group`]'s own input shape. Same rule, +/// one definition of the per-component half of it. +fn optional_group_slice_is_scope_safe(inner: &[Expr]) -> bool { + inner.len() < 2 || inner.iter().all(path_component_is_single_valued) +} + fn push_path_components(out: &mut Vec, expr: &Expr) { match expr { Expr::Identity => {} @@ -26708,8 +26834,8 @@ fn push_path_components(out: &mut Vec, expr: &Expr) { } } Expr::Paren(inner) => push_path_components(out, inner), - // A `?` on a whole multi-component group ((.a[0:1])? | .[0]) needs - // each of the group's own components individually scoped with + // A `?` on a whole multi-component group ((.a[0:1])? | .[0]) can + // need each of the group's own components individually scoped with // that `?` -- the same reasoning `splice_optional_group` already // applies for `update_path`/`delete_at_path` (#1294) -- rather // than being pushed as one opaque `Optional(Pipe(...))` element. @@ -26719,7 +26845,13 @@ fn push_path_components(out: &mut Vec, expr: &Expr) { // thing falls to "invalid path component" (#1311). Before #1429 the // same blindness was shared by `split_at_slice`'s own slice scan // and `get_path_mut`'s per-step `Identity | Field | Index` match. - Expr::Optional(inner) => { + // + // #2909: only when [`optional_group_is_scope_safe`] says the rewrite + // cannot be observed. A group that can fan out keeps jq's single + // abort scope by staying one opaque element, which + // `needs_path_prepass`/`needs_fanout_pass` route to + // `resolve_optional_sink` -- the arm that was already right. + Expr::Optional(inner) if optional_group_is_scope_safe(inner) => { let mut group = Vec::new(); push_path_components(&mut group, inner); out.extend(group.into_iter().map(|e| Expr::Optional(Box::new(e)))); @@ -29707,20 +29839,28 @@ fn wrap_optional_branch(branch: PathBranch<'_>) -> PathBranch<'_> { }; } // `depth()`/`last()` are O(1); the O(depth) `to_vec()` flatten only runs - // in the `depth() != 1` arm, which is unreached *today* (see below). + // in the `depth() != 1` arm. let inner_path = if components.depth() == 1 { components.last().expect("depth checked").clone() } else { - // Unreached *today*, and deliberately not a panic: the postfix `?` - // attaches to a single path element until #367, so `(.a.b)?`, - // `(..)?` and `recurse?` are parse errors, and `E[K]?` — which used - // to arrive here with its target's components attached — now goes - // to `resolve_index_expr`. #367 reopens it on purpose: `(.a[.k])?` - // resolves through the `Paren` arm to `["a","b"]`, two components, - // and jq writes `{"a":{"b":5},"k":"b"}` for it. `eval_generic` can - // synthesize `Expr::Optional` around any expression too, so this - // was never an invariant of the type. - flatten_components(components.to_vec()) // omni-dev: coverage tolerate-line reason="unreachable today: postfix `?` attaches to a single path element until #367 reopens it, so no path this resolver builds has depth() != 1 here (#2649)" + // **Live since #2909.** This arm was annotated "unreached today" on + // the reasoning that the postfix `?` attaches to a single path + // element until #367, so `(.a.b)?`/`(..)?`/`recurse?` are parse + // errors and `E[K]?` goes to `resolve_index_expr`. That held only + // while a `?` over a *group* was distributed onto the group's + // components before it ever reached a resolver. #2909 stopped doing + // that for a group that can fan out -- jq's `?` is one abort scope + // over the whole group, not N per-step ones -- so such a group now + // arrives here whole, with two or more components under one `?`. + // Verified by instrumenting this arm: `((.[] | .a)?) |= 99` and + // `[path(.. | ((.[], .a) | .b)?)]` both reach it. + // + // Which is exactly the shape the old comment predicted #367 would + // reopen, and jq's answer for it was already stated there: `(.a[.k])?` + // resolves to `["a","b"]` and jq writes `{"a":{"b":5},"k":"b"}`. No + // new semantics -- `flatten_components` was always the right answer, + // it just had no caller. + flatten_components(components.to_vec()) }; PathBranch { path: PathPrefix::from_components([Expr::Optional(Box::new(inner_path))]), From c292f3deffd7cebd5aa91cbe5858a40c557e3d3e Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 05:41:46 +1000 Subject: [PATCH 2/4] test(jq): pin the optional-group abort scope and both its edges Three tests, every expectation captured from jq 1.7.1 rather than derived. The fix'\''s own rows, including the two silent wrong answers (`del` keeping an element it used to remove, `|=` skipping a slot it used to write) and the duplicated-path row -- plus the issue'\''s `stderr` repro and the `.`-piped form that shows `..` was never the trigger. The upper edge: the three neighbouring spellings that already matched, one of which (`path((...)?)` with no enclosing pipe) reaches `resolve_optional_sink` directly and is therefore the fix'\''s reference behaviour, not merely a control. The lower edge, which is the one that matters for the gate: #1311'\''s and #1294'\''s own pure-primitive shapes. Widen `optional_group_is_scope_safe` and these stop being distributed, which is exactly the "invalid path component" failure #1311 was filed for. The two error rows also pin that the group'\''s `?` still does not leak onto what follows it -- the failure #1294 fixed, and the thing an over-correction here would re-break. Refs #2909 --- CHANGELOG.md | 29 +++++++++ tests/jq_cli_tests.rs | 133 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3b21d1f8..c8ccd9808 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -139,6 +139,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 depth countdown runs as ordinary `Int` arithmetic the whole way, not a silent float promotion on the very first decrement. +- **A `?` over a group that can fan out now gets jq's single abort scope** + (#2909). `(A | B)?` is `try (A | B)`: a failure anywhere inside prunes the + whole group's branch and stops the group's own generators. Two sites + rewrote it into `A? | B?` instead — `push_path_components` (#1311) for the + resolver, `splice_optional_group` (#1294) for the write walkers — which is + indistinguishable for a single-valued chain but lets a *generator* inside + the group resume past an error a later component raised. + + Filed as a side-effect count; it is not. Two of the diverging shapes were + silent wrong answers, captured live from jq 1.7.1: + + ```console + $ echo '{"a":{"b":1},"c":[1,2]}' | jq -c 'del((.[] | .[0])?)' + {"a":{"b":1},"c":[1,2]} # was: {"a":{"b":1},"c":[2]} + $ echo '[{"a":1},5,{"a":3}]' | jq -c '((.[] | .a)?) |= 99' + [{"a":99},5,{"a":3}] # was: [{"a":99},5,{"a":99}] + ``` + + — an element deleted that jq keeps, and a slot written that jq never + reaches. Neither needed `..` or `path()`, both of which the issue's own + repro had implicated. + + One predicate now gates both rewrites: distribute only when the group has + fewer than two components or every one is single-valued. An unsafe group + stays opaque and routes to `resolve_optional_sink`, the arm that was + already correct — which is why the issue's three "neighbouring spellings + agree" rows agreed. Verified by a 2,400-combination differential sweep + against jq 1.7.1 with zero regressions. + - **A non-numeric slice bound over a `null` target resolves into the path, and is refused at the write** (#2853). jq's `INDEX` opcode answers `null` for a null target *without* reading the slice descriptor's bounds, so the diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 3a834ef80..4df8e3af4 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -17478,6 +17478,139 @@ fn test_null_target_non_numeric_slice_bound_nested_empty_update_2853() -> Result Ok(()) } +/// #2909: `(A | B)?` is `try (A | B)` -- one abort scope over the whole +/// group. succinctly distributed the `?` onto each component (`A? | B?`), +/// which is indistinguishable for a single-valued chain but lets a +/// *generator* inside the group resume past an error a later component +/// raised. +/// +/// Filed as a side-effect count; it is not. Two of these rows are silent +/// wrong answers -- an element deleted that jq keeps, and a slot written +/// that jq never reaches. Every expectation captured live from jq 1.7.1. +#[test] +fn test_optional_group_has_one_abort_scope_2909() -> Result<()> { + for (filter, input, expected) in [ + // The severe pair: output diverged, silently. + ( + r"del((.[] | .[0])?)", + r#"{"a":{"b":1},"c":[1,2]}"#, + r#"{"a":{"b":1},"c":[1,2]}"#, + ), + ( + r"((.[] | .a)?) |= 99", + r#"[{"a":1},5,{"a":3}]"#, + r#"[{"a":99},5,{"a":3}]"#, + ), + // Duplicated path outputs. + ( + r"[path(.. | ((.[], .a) | .b)?)]", + r#"{"a":{"b":1},"c":[1,2]}"#, + r#"[["a","b"]]"#, + ), + ] { + let (stdout, stderr, code) = run_jq_full(&["-c", filter], Some(input))?; + assert_eq!(code, 0, "{filter}: stderr: {stderr:?}"); + assert_eq!(stdout.trim(), expected, "{filter}"); + } + + // The issue's own repro, and the `.`-piped form that shows `..` was + // never the trigger: `stderr` must fire once, not once per element. + for filter in [ + r"[path(.. | (.[]|stderr|.+0)?)]", + r"[path(. | (.[]|stderr|.+0)?)]", + ] { + let (stdout, stderr, code) = run_jq_full(&["-c", filter], Some("[true,false]"))?; + assert_eq!(code, 0, "{filter}: stderr: {stderr:?}"); + assert_eq!(stdout.trim(), "[]", "{filter}"); + assert_eq!( + stderr, "true", + "{filter}: the group's error must stop `.[]`, so only the first \ + element reaches `stderr`" + ); + } + + Ok(()) +} + +/// #2909 (must-not-change): the three spellings that already matched jq, +/// which is what localised the bug to the two distributing rewrites rather +/// than to `?` or to `.[]`. `path((...)?)` with no enclosing pipe reaches +/// `resolve_optional_sink` directly -- the arm the fix routes the others +/// into -- so it is the reference behaviour, not just a control. +#[test] +fn test_optional_group_neighbouring_spellings_unmoved_2909() -> Result<()> { + for filter in [ + r"[(.[]|stderr|.+0)?]", + r"[.. | (.[]|stderr|.+0)?]", + r"[path((.[]|stderr|.+0)?)]", + ] { + let (stdout, stderr, code) = run_jq_full(&["-c", filter], Some("[true,false]"))?; + assert_eq!(code, 0, "{filter}: stderr: {stderr:?}"); + assert_eq!(stdout.trim(), "[]", "{filter}"); + assert_eq!(stderr, "true", "{filter}"); + } + + let (stdout, stderr, code) = run_jq_full( + &["-c", r"((.[] | .a)?) = 99"], + Some(r#"[{"a":1},5,{"a":3}]"#), + )?; + assert_eq!(code, 0, "stderr: {stderr:?}"); + assert_eq!(stdout.trim(), r#"[{"a":99},5,{"a":3}]"#); + + Ok(()) +} + +/// #2909 (must-not-change): the gate's lower edge -- the pure-primitive +/// groups #1294 and #1311 added the distributing rewrites *for*. +/// +/// These are the direct guard on `optional_group_is_scope_safe` staying +/// narrow: widen it and every row here starts routing through the resolver +/// instead of the native walker, and #1311's own "invalid path component" +/// failure comes back. +#[test] +fn test_optional_group_scope_safe_class_still_distributes_2909() -> Result<()> { + for (filter, input, expected) in [ + // #1311's own test shape. + ( + r"((.a[0:1])? | .[0]) = 9", + r#"{"a":[1,2,3]}"#, + r#"{"a":[9,2,3]}"#, + ), + (r"del((.a[0:1])?)", r#"{"a":[1,2,3]}"#, r#"{"a":[2,3]}"#), + // Multi-component, all single-valued: still distributed. + (r"[path((.a|.b)?)]", r#"{"a":{"b":1}}"#, r#"[["a","b"]]"#), + (r"(.a|.b) = 9", r#"{"a":{"b":1}}"#, r#"{"a":{"b":9}}"#), + (r"del((.a|.b)?)", r#"{"a":{"b":1}}"#, r#"{"a":{}}"#), + ] { + let (stdout, stderr, code) = run_jq_full(&["-c", filter], Some(input))?; + assert_eq!(code, 0, "{filter}: stderr: {stderr:?}"); + assert_eq!(stdout.trim(), expected, "{filter}"); + } + + // #1294's own shape, and its `path()` sibling: the group's `?` must + // still not leak onto what follows the group, so a failure *after* the + // group is raised rather than swallowed. These are the rows that fail if + // the fix over-corrects into scoping the `?` too widely. + for (filter, input, message) in [ + ( + r"(.a|.c)[0] = 9", + r#"{"a":{"c":1}}"#, + "Cannot index number with number", + ), + ( + r"[path((.a|.b)? | .c)]", + r#"{"a":{"b":1}}"#, + r#"Cannot index number with string "c""#, + ), + ] { + let (_, stderr, code) = run_jq_full(&["-c", filter], Some(input))?; + assert_eq!(code, 5, "{filter}: stderr: {stderr:?}"); + assert!(stderr.contains(message), "{filter}: stderr: {stderr:?}"); + } + + Ok(()) +} + /// #2248, `resolve_slice_expr`'s identical sibling to /// `resolve_index_expr`'s own fix above. Verified against jq 1.7.1: /// `path((.,5)[(0,1):(2,error("mid"))])` on `null` prints From f503d18aad6e948855f4f0f978a246866627a820 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 06:13:53 +1000 Subject: [PATCH 3/4] fix(jq): address /code-review findings on #2909's optional-group scope The review found a **crash class** my own 756-combination sweep missed: `del(.a | (. | .[])?)` overflowed the stack (exit 134) where jq and the merge-base both answer `{"a":{},"c":[1,2]}`. Six shapes, jq and yq mode alike. Cause: I wrote the rule twice. `optional_group_is_scope_safe` skipped `Identity` and recursed into `Pipe`/`Paren`; the slice form counted `Identity` and treated a `Paren(Pipe(..))` element as multi-valued. So a group could be *safe* to the routing gate -- staying on the native walker -- and *unsafe* to `splice_optional_group`, which then kept it opaque by returning `[Optional(Pipe(inner))] ++ rest`: the very list the caller had just decomposed. The walker re-derived the same `here` and re-entered. That is this repo's own "duplicated predicates diverge silently" lesson (#106), in a commit whose message claims "one predicate". There is one walk now, with one entry point. The second half of the fix is to stop gating `splice_optional_group` at all. Gating it is what makes it a fixed point, and it turns out to be unnecessary: `needs_path_prepass` already answers `true` for a group that can fan out, so such a group is resolved through `resolve_optional_sink` before any walker sees it. Verified by re-running the sweep with that site ungated -- widened to 3,840 combinations, with the review's crash shapes and `.`/paren-heavy randomised bodies added: zero output differences, zero crashes. Six of those shapes are now CLI tests, which fail loudly rather than subtly if either half regresses. Three documentation findings, all mine: * `is_atomic_path_component`'s comment asserted that an opaque group "never reaches this walker anyway" as the safety argument for the whole change. The crash proved it was an assertion, not an invariant. It now names the routing gate as the actual mechanism and points at `splice_optional_group`'s own comment for why that site stays ungated. * `push_path_components` had lost its doc comment -- absorbed into the new predicate's rustdoc, the same defect `b6abf5d76` fixed for `resolve_leaf`. Restored. * `path_component_is_single_valued`'s lead question was inverted relative to its return value. Refs #2909 --- src/jq/eval.rs | 151 +++++++++++++++++++++++------------------- tests/jq_cli_tests.rs | 32 +++++++++ 2 files changed, 114 insertions(+), 69 deletions(-) diff --git a/src/jq/eval.rs b/src/jq/eval.rs index 734d226fd..f948350db 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -24576,13 +24576,17 @@ fn set_path( /// [`flatten_path_components`] first. /// /// Since #2909 the flatten is no longer guaranteed to *produce* atomic -/// components: a `?` over a group that can fan out stays one opaque -/// `Optional(Pipe(..))` element, because jq's `?` is a single abort scope -/// over the whole group. That element never reaches this walker anyway -- -/// `needs_path_prepass` answers `true` for exactly that shape, so -/// `=`/`|=`/`del`/`path` resolve it through `resolve_optional_sink` first -/// and hand the walker already-resolved components. Verified by sweeping 756 -/// group/outer/input combinations for an "invalid path component" leak: none. +/// components: [`push_path_components`] leaves a `?` over a group that can +/// fan out as one opaque `Optional(Pipe(..))` element, because jq's `?` is a +/// single abort scope over the whole group. +/// +/// What keeps that element away from this walker is the routing gate, not an +/// invariant of the flatten: `needs_path_prepass` answers `true` for exactly +/// that shape, so `=`/`|=`/`del`/`path` resolve it through +/// `resolve_optional_sink` first and hand the walker already-resolved +/// components. [`splice_optional_group`] deliberately keeps distributing +/// unconditionally rather than mirroring the gate -- see its own doc comment +/// for why that is load-bearing rather than an oversight. /// /// `Identity` counts as atomic even though the flattener would drop it /// (`push_path_components`): the walker has a one-line arm for it, and @@ -25082,9 +25086,7 @@ fn unwrap_path_component(expr: &Expr) -> (&Expr, bool) { /// `Optional(Pipe([…]))` component and continue with one recursive call over /// the combined list, so the same fix applies identically to each: when /// `here` (the group's own `?`, folded with whatever optionality was already -/// ambient before it) is set *and the group cannot fan out* (#2909 -- -/// [`optional_group_slice_is_scope_safe`]), wrap each of `inner`'s own -/// components in +/// ambient before it) is set, wrap each of `inner`'s own components in /// `Expr::Optional` — [`unwrap_path_component`] already peels this at every /// step — instead of threading `here` as the recursive call's blanket /// `optional` parameter, which previously kept suppressing `rest` too, past @@ -25093,18 +25095,24 @@ fn unwrap_path_component(expr: &Expr) -> (&Expr, bool) { /// `optional` — not `here` — as that call's baseline, so `rest` keeps /// whatever optionality it already had on its own. fn splice_optional_group(inner: &[Expr], rest: &[Expr], here: bool) -> Vec { - // #2909: distributing `here` across `inner`'s components is only - // observationally identical to jq's single abort scope when the group - // cannot fan out -- see [`optional_group_is_scope_safe`]. When it can, - // the group stays one opaque `Optional(Pipe(..))` element, which - // `needs_path_prepass` now routes to the resolver's own `Expr::Optional` - // arm rather than to a walker that would see N independently-pruned - // steps. - if here && !optional_group_slice_is_scope_safe(inner) { - let mut spliced = vec![Expr::Optional(Box::new(flatten_components(inner.to_vec())))]; - spliced.extend_from_slice(rest); - return spliced; - } + // #2909 gated the *other* distribution site (`push_path_components`) on + // whether the group can fan out, and deliberately did **not** gate this + // one. Two reasons, in order of importance: + // + // 1. **It cannot be gated without reintroducing unbounded recursion.** + // Keeping the group opaque here means returning + // `[Optional(Pipe(inner))] ++ rest` -- which is the very list the + // caller just decomposed. `delete_path_steps`/`update_path_steps` + // re-walk it, re-derive `here` through `unwrap_path_component`, and + // re-enter. `del(.a | (. | .[])?)` overflowed the stack that way; it + // was caught in review, and this arm is where it lived. + // 2. **It is not needed.** `needs_path_prepass` answers `true` for a + // group that can fan out, so `=`/`|=`/`del`/`path` resolve such a + // group through `resolve_optional_sink` before any walker sees it, and + // what reaches here is already-resolved components. Confirmed by + // sweeping 3,840 group/outer/input combinations against jq 1.7.1 with + // this site left ungated: zero output differences and zero crashes, + // including every shape from the review's own crash set. let mut spliced: Vec = if here { inner .iter() @@ -26737,6 +26745,50 @@ fn needs_fanout_pass(expr: &Expr) -> bool { /// `Identity | Field | Index | Iterate | Slice` and rejects anything else as /// "invalid path component", so a nested `Pipe` emitted by the pre-pass /// would break assignment with a message that reads like user error. +/// Is this path component single-valued -- does it always produce exactly one +/// output? +/// +/// `Iterate`, a computed key, a comma, `..`, a call -- anything that can fan +/// out or vanish -- answers `false` by falling through. +fn path_component_is_single_valued(expr: &Expr) -> bool { + match expr { + Expr::Identity | Expr::Field(_) | Expr::Index { .. } | Expr::Slice { .. } => true, + Expr::Optional(inner) | Expr::Paren(inner) => path_component_is_single_valued(inner), + _ => false, + } +} + +/// [`optional_group_is_scope_safe`]'s walk: count the group's real components +/// and note whether any of them can fan out. +/// +/// Flattens **exactly** the way [`push_path_components`] does -- `Pipe` and +/// `Paren` are recursed into, `Identity` contributes no component -- because +/// this predicate's whole job is to answer a question *about* that flatten. +/// That was got wrong once: a first version had a second, subtly different +/// rule for an already-flattened component slice, which counted `Identity` +/// and treated a `Paren(Pipe(..))` element as multi-valued. A group could +/// then be judged one way by the routing gate and the other by +/// `splice_optional_group`, which kept it opaque and re-decomposed the +/// identical list forever -- `del(.a | (. | .[])?)` overflowed the stack. +/// Caught in review; there is one rule and one caller of it now. +fn walk_optional_group(expr: &Expr, seen: &mut usize, safe: &mut bool) { + match expr { + Expr::Identity => {} + Expr::Pipe(exprs) => { + for e in exprs { + walk_optional_group(e, seen, safe); + } + } + Expr::Paren(inner) | Expr::Optional(inner) => walk_optional_group(inner, seen, safe), + other => { + *seen += 1; + if !path_component_is_single_valued(other) { + *safe = false; + } + } + } +} + /// Is distributing a `?` across the components of the group it wraps /// observationally identical to jq's single abort scope (#2909)? /// @@ -26765,10 +26817,7 @@ fn needs_fanout_pass(expr: &Expr) -> bool { /// /// So: distribute only when the group cannot fan out. Fewer than two /// components has nothing to distribute across in the first place; otherwise -/// every component must be single-valued. `Iterate`, a computed key, a -/// comma, `..` -- anything that can produce other than exactly one output -- -/// makes the group unsafe, and it stays one opaque `Optional(Pipe(..))` -/// element for `resolve_optional_sink` to give jq's own single scope. +/// every component must be single-valued. /// /// Deliberately conservative: a group like `(.[] | .a?)` that happens not to /// diverge today is also called unsafe. That only changes *which* correct arm @@ -26777,54 +26826,18 @@ fn needs_fanout_pass(expr: &Expr) -> bool { /// Allocation-free: consulted from [`needs_path_prepass`], which is on the /// per-call path of every `path`/`del`/`=`/`|=`. fn optional_group_is_scope_safe(inner: &Expr) -> bool { - /// Count the group's components, stopping as soon as two are seen -- - /// a group with fewer than two has nothing to distribute across, and is - /// safe whatever it contains. - fn walk(expr: &Expr, seen: &mut usize, safe: &mut bool) { - match expr { - Expr::Identity => {} - Expr::Pipe(exprs) => { - for e in exprs { - walk(e, seen, safe); - } - } - Expr::Paren(inner) => walk(inner, seen, safe), - Expr::Optional(inner) => walk(inner, seen, safe), - other => { - *seen += 1; - if !path_component_is_single_valued(other) { - *safe = false; - } - } - } - } - let (mut seen, mut safe) = (0usize, true); - walk(inner, &mut seen, &mut safe); + walk_optional_group(inner, &mut seen, &mut safe); seen < 2 || safe } -/// One component of a path group: can it produce anything other than exactly -/// one output? +/// Flatten an expression into the list of path components it denotes. /// -/// The single rule both [`optional_group_is_scope_safe`] and its slice -/// sibling decide by -- `Iterate`, a computed key, a comma, `..`, a call, all -/// answer `false` by falling through. -fn path_component_is_single_valued(expr: &Expr) -> bool { - match expr { - Expr::Identity | Expr::Field(_) | Expr::Index { .. } | Expr::Slice { .. } => true, - Expr::Optional(inner) | Expr::Paren(inner) => path_component_is_single_valued(inner), - _ => false, - } -} - -/// [`optional_group_is_scope_safe`] for a group already flattened into a -/// component slice -- [`splice_optional_group`]'s own input shape. Same rule, -/// one definition of the per-component half of it. -fn optional_group_slice_is_scope_safe(inner: &[Expr]) -> bool { - inner.len() < 2 || inner.iter().all(path_component_is_single_valued) -} - +/// `Pipe` and `Paren` are transparent and `Identity` contributes nothing, so +/// `.a | (. | .b)` flattens to `[.a, .b]`. A `?` over a group is distributed +/// onto that group's own components -- but only when +/// [`optional_group_is_scope_safe`] says that rewrite cannot be observed +/// (#2909); otherwise the group stays one opaque element. fn push_path_components(out: &mut Vec, expr: &Expr) { match expr { Expr::Identity => {} diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 4df8e3af4..f82249d38 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -17611,6 +17611,38 @@ fn test_optional_group_scope_safe_class_still_distributes_2909() -> Result<()> { Ok(()) } +/// #2909 (review finding): the shapes that made an earlier cut of this fix +/// **overflow the stack**. +/// +/// That cut gated `splice_optional_group` as well, so an unsafe group stayed +/// opaque there -- returning `[Optional(Pipe(inner))] ++ rest`, which is the +/// very list the caller had just decomposed. The walker re-derived the same +/// `here` and re-entered, forever. It was reachable because the two +/// scope-safety predicates disagreed about `Identity` and about a +/// parenthesised sub-pipe, so these groups were *safe* to the routing gate +/// (kept on the native walker) and *unsafe* to the splice. +/// +/// There is one predicate now, and the splice is deliberately ungated. These +/// rows are cheap and would fail loudly (exit 134) if either changes. +#[test] +fn test_optional_group_no_unbounded_recursion_2909() -> Result<()> { + for (filter, expected) in [ + (r"del(.a | (. | .[])?)", r#"{"a":{},"c":[1,2]}"#), + (r"del(.x | (.[] | .)?)", r#"{"a":{"b":1},"c":[1,2]}"#), + (r"del(.x | (. | .[]?)?)", r#"{"a":{"b":1},"c":[1,2]}"#), + (r"del(.x | ((.a|.b) | .c)?)", r#"{"a":{"b":1},"c":[1,2]}"#), + (r"del(.x | ((.a) | (.b|.c))?)", r#"{"a":{"b":1},"c":[1,2]}"#), + (r"(.x | (. | .[])? | .b) |= 9", r#"{"a":{"b":1},"c":[1,2]}"#), + ] { + let (stdout, stderr, code) = + run_jq_full(&["-c", filter], Some(r#"{"a":{"b":1},"c":[1,2]}"#))?; + assert_eq!(code, 0, "{filter}: stderr: {stderr:?}"); + assert_eq!(stdout.trim(), expected, "{filter}"); + } + + Ok(()) +} + /// #2248, `resolve_slice_expr`'s identical sibling to /// `resolve_index_expr`'s own fix above. Verified against jq 1.7.1: /// `path((.,5)[(0,1):(2,error("mid"))])` on `null` prints From 46bf8515e864cad96419015f0fab6a1a10bb8a7b Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 06:30:07 +1000 Subject: [PATCH 4/4] refactor(jq): drop path_component_is_single_valued's unreachable wrapper arm Patch coverage flagged the `Optional`/`Paren` arm as never hit, and the call graph says why: `walk_optional_group`, its only caller, has already looked through both wrappers by the time it asks. The arm was dead the moment the two scope-safety predicates were merged into one walk. Removed rather than tolerated -- the function collapses to a flat `matches!` -- with the reason recorded so it is not re-added by symmetry with the walk above it. Refs #2909 --- src/jq/eval.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/jq/eval.rs b/src/jq/eval.rs index f948350db..29b6bc197 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -26750,12 +26750,17 @@ fn needs_fanout_pass(expr: &Expr) -> bool { /// /// `Iterate`, a computed key, a comma, `..`, a call -- anything that can fan /// out or vanish -- answers `false` by falling through. +/// +/// No `Optional`/`Paren` arm, and not recursive: its one caller +/// ([`walk_optional_group`]) has already looked through both wrappers by the +/// time it asks, so such an arm would be dead code. An earlier cut had one +/// and patch coverage flagged it as unreached, which is what confirmed the +/// wrappers can never arrive here. fn path_component_is_single_valued(expr: &Expr) -> bool { - match expr { - Expr::Identity | Expr::Field(_) | Expr::Index { .. } | Expr::Slice { .. } => true, - Expr::Optional(inner) | Expr::Paren(inner) => path_component_is_single_valued(inner), - _ => false, - } + matches!( + expr, + Expr::Identity | Expr::Field(_) | Expr::Index { .. } | Expr::Slice { .. } + ) } /// [`optional_group_is_scope_safe`]'s walk: count the group's real components