fix(jq): give a ? over a fan-out group jq's single abort scope

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

src/jq/eval.rs

@@ -24575,6 +24575,19 @@ fn set_path<S: EvalSemantics>(
 /// 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: [`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
 /// treating it as non-atomic would send an otherwise-flat `.a | . | .b`
@@ -25082,6 +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<Expr> {
+    // #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<Expr> = if here {
         inner
             .iter()
@@ -26637,6 +26668,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 +26708,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 +26728,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 +26745,104 @@ 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.
+///
+/// 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 {
+    matches!(
+        expr,
+        Expr::Identity | Expr::Field(_) | Expr::Index { .. } | Expr::Slice { .. }
+    )
+}
+
+/// [`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)?
+///
+/// `(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.
+///
+/// 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 {
+    let (mut seen, mut safe) = (0usize, true);
+    walk_optional_group(inner, &mut seen, &mut safe);
+    seen < 2 || safe
+}
+
+/// Flatten an expression into the list of path components it denotes.
+///
+/// `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: &Expr) {
     match expr {
         Expr::Identity => {}
@@ -26708,8 +26852,8 @@ fn push_path_components(out: &mut Vec<Expr>, 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 +26863,13 @@ fn push_path_components(out: &mut Vec<Expr>, 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 +29857,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))]),