diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 04e5d98b0..fdd37ad77 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -1658,15 +1658,21 @@ answers `["b"]` — and classified the two residuals appended below): `path(.a[])`) always agreed — nothing in it is observable per element — and keeps #2061/ #2168's no-materialization cursor walk untouched. - **Two shapes still collect**, neither introduced by that work and both tracked as - [#2925](https://github.com/rust-works/succinctly/issues/2925). A document the reindex bridge - will not round-trip identically (any `Float`, or a number literal too large to survive the - trip) takes the bridge, which collects — so the *document*, not the filter, selects the - route: `[limit(1; path((.a|stderr),(.b|stderr)))]` writes `1` on `{"a":1,"b":2}` and `12` - once a 300-digit literal is added. And the stop reaches the branch producer but not a - generator in *index* position: `[limit(1; path(.[("a"|stderr),("b"|stderr)]))]` writes `ab` - where jq writes `a`, which is `resolve_index_expr`'s eager key evaluation — the same function - #2032 sits in. + **Two shapes still collected after that work, neither introduced by it, both tracked and + now closed as [#2925](https://github.com/rust-works/succinctly/issues/2925).** A generator + in *index* position closed with [#2267](https://github.com/rust-works/succinctly/issues/2267): + `E[K]`/`E[S:T]` are native `resolve_node_sink` arms now, so a bound reaches the key and + bound generators too (`[limit(1; path(.[("a"|stderr),("b"|stderr)]))]` writes `a`, matching + jq, where it used to write `ab`). And a document the reindex bridge will not round-trip + identically (a number literal past `REINDEX_LITERAL_LEN_CAP`, 256 characters, or a NaN + spelling — a bare `Float` has been bridge-identity since #2902 and no longer selects this + route) now forwards demand across the bridge the same way, instead of collecting every path + first: `[limit(1; path((.a|stderr),(.b|stderr)))]` writes `1` whether or not a 300-digit + literal sits elsewhere in the document. `to_json_for_reindex`'s own respelling of an + over-cap literal (`1E+300` for the literal above) is a separate, still-open value-fidelity + gap on the same bridge — [#3025](https://github.com/rust-works/succinctly/issues/3025) — + unaffected by this fix, since it changes only which entry point the bridge hands the + document to, not what the bridge does to the document itself. - **`recurse(f)`/`recurse(f; cond)` past its native stack budget finishes one node's own `f` before descending.** `resolve_recurse_sink` (#2235) streams each visited node to a bounded consumer as soon as diff --git a/src/jq/eval.rs b/src/jq/eval.rs index 744fba850..bf45ed406 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -45999,9 +45999,15 @@ fn builtin_path<'a, W: Clone + AsRef<[u64]>, S: EvalSemantics>( // consulted `optional` via `suppress_or_raise` -- this function was the // one place the pattern hadn't been carried over. Reachable both via the // public `succinctly::jq::eval::eval` library API and, via the CLI, - // whenever `eval_generic.rs`'s `Builtin::Path` fallback re-enters the - // full evaluator for a non-`reindex_bridge_is_identity` value (a `Float` - // anywhere in the document). + // whenever `eval_generic.rs`'s *eager* `eval_single` arm for + // `Builtin::Path` re-enters the full evaluator for a non- + // `reindex_bridge_is_identity` value (a number literal past + // `REINDEX_LITERAL_LEN_CAP`, or a NaN spelling -- a bare `Float` has + // been bridge-identity since #2902). The *lazy* `eval_each_generic` arm + // for the same builtin no longer reaches here on that route (#2925): it + // hands the same non-identity case to `eval_each_owned` instead, which + // re-enters through `eval_each`'s own lazy `Builtin::Path` arm + // (`each_path`), not this eager one. let owned = to_owned_or_suppress!(&value, optional); builtin_path_on_owned::(expr, &owned, optional) } diff --git a/src/jq/eval_generic.rs b/src/jq/eval_generic.rs index 23f29f5ca..2ebc99135 100644 --- a/src/jq/eval_generic.rs +++ b/src/jq/eval_generic.rs @@ -9146,8 +9146,8 @@ fn eval_each_generic( // navigation, nothing observable per output, and the #2061/#2168 // route that avoids materializing the document at all), and a // document the reindex bridge would not round-trip identically - // keeps the bridge -- and so keeps the collecting behaviour, a - // recorded residual (#2925). + // keeps the bridge -- which now forwards demand too (#2925), so + // every route here streams. Expr::Builtin(Builtin::Path(path_expr)) if !(cursor.is_some() && path_expr_is_cursor_navigable(path_expr)) => { @@ -9165,20 +9165,31 @@ fn eval_each_generic( Err(e) => return Flow::Escaped(Control::Error(e)), }; if !reindex_bridge_is_identity(&owned) { - // Hand the bridge the document already materialized above, - // the way the eager arm does -- re-entering `eval_single` - // would run `RootWitness::of` + `demote_for_reentry` + - // `to_owned_with_cursor` a second time over the whole - // document to rebuild exactly these values. - let owned_builtin_expr = Expr::Builtin(Builtin::Path(path_expr.clone())); - return drain_result_generic( - eval_on_owned::( - &owned_builtin_expr, - owned, - optional, - Reentry::Against(root), - ), - sink, + // #2925: demand-forwarding, not `eval_on_owned`'s eager + // collect -- a consumer wrapping this arm (`limit`, `first`, + // `label`/`break`) can now stop the walk early on a document + // the reindex bridge would not round-trip identically (an + // over-256-char number literal, a NaN spelling), matching + // jq's own generator order instead of over-running it. + // `eval_each_owned`, not `eval_on_owned` -- `eval.rs`'s + // `eval_each` has its own lazy `Builtin::Path` arm (#2908), + // the same demand-forwarding entry point + // [`bridge_to_each_owned_flow`] above uses (same contract: + // `Reentry::Against(root)` + a sink-wrapping closure). + // `expr` itself, not a rebuilt `owned_builtin_expr` -- it + // already *is* `Builtin::Path(path_expr)` (this arm's own + // match scrutinee), so passing it directly avoids cloning + // the subtree `reentry.reroot` is about to walk anyway. + // Passing the whole wrapper (not bare `path_expr`, as + // `demoted` above does) is what lets the precheck see the + // resolver-reaching node itself -- the reason `demoted` + // reroots `path_expr` directly instead, per its own comment. + return eval_each_owned::( + expr, + &owned, + optional, + Reentry::Against(root), + &mut |v| sink.push(GenericItem::Owned(v)), ); } each_path_on_owned::(&demoted, &owned, false, &mut |v| { diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index e85c2ee96..5b0afaade 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -49338,6 +49338,20 @@ fn path_mode_fold_resolves_init_by_demand_2903() -> Result<()> { fn path_results_stream_to_their_consumer_2908() -> Result<()> { let abc = r#"{"a":1,"b":2,"c":3}"#; let probes = "(.a|stderr), (.b|stderr), (.c|stderr)"; + // #2925: the reindex bridge's own lazy `Builtin::Path` arm, exercised on + // a document it will not round-trip identically (a number literal past + // `REINDEX_LITERAL_LEN_CAP`, #2902), so `path(f)` takes the bridge + // instead of the cursor walk above. Every probe below reads only a + // small named field (`.a`/`.b`/`.c`/`.x`), never the oversized `n` + // field itself -- printing *that* to stderr hits a separate, unrelated + // bug (the bridge respells an over-cap literal, #3025), which this row + // set deliberately does not exercise. + let abc_big = format!(r#"{{"a":1,"b":2,"c":3,"n":{}}}"#, "9".repeat(300)); + let label_big = format!(r#"{{"a":1,"c":1,"n":{}}}"#, "9".repeat(300)); + let prefix_big = format!(r#"{{"a":{{"b":1}},"c":1,"n":{}}}"#, "9".repeat(300)); + let altq_big = format!(r#"{{"x":[true,false],"n":{}}}"#, "9".repeat(300)); + let cap256 = format!(r#"{{"a":1,"b":2,"n":{}}}"#, "9".repeat(256)); + let cap257 = format!(r#"{{"a":1,"b":2,"n":{}}}"#, "9".repeat(257)); for (input, filter, want_out, want_err, want_code) in [ // A bound outside `path()` now stops the generator inside it. ( @@ -49487,6 +49501,101 @@ fn path_results_stream_to_their_consumer_2908() -> Result<()> { String::new(), 0, ), + // #2925: the reindex-bridge route (a document `reindex_bridge_is_identity` + // rejects) now forwards demand the same way the cursor-navigable route + // above always did, instead of collecting every path before the bound + // outside `path()` ever saw one. Every row below is captured whole from + // jq 1.7.1 -- stdout, stderr and exit code -- on a document holding a + // 300-character number literal (well past `REINDEX_LITERAL_LEN_CAP`, + // #2902), which is what selects this route; the identical filter on the + // cap-sized documents above stays on this same table, matching jq. + ( + abc_big.as_str(), + "[limit(1; path((.a|stderr), (.b|stderr), (.c|stderr)))]".to_string(), + "[[\"a\"]]\n".to_string(), + "1".to_string(), + 0, + ), + ( + abc_big.as_str(), + "[first(path((.a|stderr), (.b|stderr), (.c|stderr)))]".to_string(), + "[[\"a\"]]\n".to_string(), + "1".to_string(), + 0, + ), + ( + abc_big.as_str(), + "[limit(2; path((.a|stderr), (.b|stderr), (.c|stderr)))]".to_string(), + "[[\"a\"],[\"b\"]]\n".to_string(), + "12".to_string(), + 0, + ), + // A `label`/`break` bound reaches it too, same as the cap-sized row. + ( + label_big.as_str(), + "[label $o | path((.a|stderr),(.c|stderr)) | ., break $o]".to_string(), + "[[\"a\"]]\n".to_string(), + "1".to_string(), + 0, + ), + // #2680's prefix rule still holds on this route: a resolved sibling + // already emitted survives a later one erroring. + ( + prefix_big.as_str(), + "path(.a.b, .c.d)".to_string(), + "[\"a\",\"b\"]\n".to_string(), + "jq: error (at :0): Cannot index number with string \"d\"\n".to_string(), + 5, + ), + ( + prefix_big.as_str(), + "first(path(.a.b, .c.d))".to_string(), + "[\"a\",\"b\"]\n".to_string(), + String::new(), + 0, + ), + ( + prefix_big.as_str(), + "[path(.a.b, .c.d)?]".to_string(), + "[[\"a\",\"b\"]]\n".to_string(), + String::new(), + 0, + ), + // The `?//` retry row from the table above, over a document that + // takes the bridge -- `.x` narrows away from the oversized `n` + // field before the `foreach`/`stderr` runs, so this still probes + // #2925's routing without also probing #3025's unrelated respelling. + ( + altq_big.as_str(), + "[limit(1; path(.x | foreach (1 as $x ?// $y | (stderr|1)) as $v (.; .)))]" + .to_string(), + "[[\"x\"],[\"x\"]]\n".to_string(), + "[true,false][true,false]".to_string(), + 0, + ), + // End-to-end agreement at the cap boundary: 256 characters is + // `REINDEX_LITERAL_LEN_CAP` itself (stays on the identity/cursor + // route), 257 is one past it (takes the bridge route above) -- both + // now stream identically, so this CLI-level pair cannot by itself + // tell which route ran (both produce byte-identical output). The + // boundary itself is what `test_reindex_bridge_identity_predicate_agrees_1909` + // pins directly, against `reindex_bridge_is_identity`; these two + // rows are here for jq-agreement coverage on either side of it, not + // as that guard's substitute. + ( + cap256.as_str(), + "[limit(1; path((.a|stderr),(.b|stderr)))]".to_string(), + "[[\"a\"]]\n".to_string(), + "1".to_string(), + 0, + ), + ( + cap257.as_str(), + "[limit(1; path((.a|stderr),(.b|stderr)))]".to_string(), + "[[\"a\"]]\n".to_string(), + "1".to_string(), + 0, + ), ] { let (stdout, stderr, code) = run_jq_full(&["-c", &filter], Some(input))?; assert_eq!(code, want_code, "{filter}: stdout: {stdout:?} stderr: {stderr:?}"); @@ -49494,32 +49603,6 @@ fn path_results_stream_to_their_consumer_2908() -> Result<()> { assert_eq!(stderr, want_err, "{filter}"); } - // #2925's one remaining shape. The second one it recorded -- a generator - // in index position -- closed with #2267 and is in the table above now, - // which is why this is a single case rather than the loop it used to be. - // - // A document the reindex bridge will not round-trip identically takes - // the bridge, which collects -- so the *document* selects the route, not - // the filter. The same filter on `{"a":1,"b":2}` is in the table above, - // matching jq. jq 1.7.1 writes `1`; this writes `12`, and the assertions - // below carry both so that closing the gap trips this test rather than - // passing quietly. - let big = format!(r#"{{"a":1,"b":2,"n":{}}}"#, "9".repeat(300)); - let filter = "[limit(1; path((.a|stderr),(.b|stderr)))]"; - let (jq_err, our_err) = ("1", "12"); - let (stdout, stderr, code) = run_jq_full(&["-c", filter], Some(&big))?; - assert_eq!(code, 0, "{filter}: stdout: {stdout:?} stderr: {stderr:?}"); - assert_eq!(stdout, "[[\"a\"]]\n", "{filter}"); - assert_ne!( - our_err, jq_err, - "{filter}: this row exists because the two differ -- if they no longer do, \ - move it into the table above" - ); - assert_eq!( - stderr, our_err, - "{filter}: #2925's residual changed -- if it closed, move this case into the \ - table above with jq's own stderr ({jq_err:?})" - ); Ok(()) }