From dd0cc77eae703715d43337b6df0c2d488036b37c Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 09:33:00 +1000 Subject: [PATCH 1/3] fix(jq): keep a computed float bare across the reindex bridge (#2902) `to_json_for_reindex` spelled a bare `Float` with a display formatter (`format_float_yq` / `jq_bare_float_display`), and the reparse could not tell that text from a document literal, so a computed float came back as a `NumberLiteral` and every literal-preserving rule echoed the bridge's spelling: `[0.5+0.5] | .[0] | tostring` was `1.0` in yq mode (yq `1`), and `[2*1e16] | .[0] | tostring` was `"2E+16"` in jq mode (jq `"2e+16"`). The bridge now writes a finite bare `Float` as a token with a doubled exponent marker (`1e0e0`), the same design as the NaN/Infinity sentinels: unparseable as an ordinary number so no literal can collide, digit-leading so the semi-index scans it as a number. `from_number_bytes` and `JsonNumber::as_f64` decode it back to a bare `Float`; `number_literal()` and `as_i64` already refuse it. `to_json_yq` switches to yq's threshold-aware `format_float_yq`, which the old literal accident had been masking, and `reindex_bridge_is_identity` admits a finite `Float` now that the round trip is an identity on it. Also closes #1134 (every intervening stage before `tostring`) and #1144 (`join`/`@csv`/interpolation on a constructed array), and removes the recorded `[(0.5+0.5)] | .[0] == 1` divergence. --- docs/compliance/yq/limitations.md | 10 +- src/jq/eval.rs | 100 +++++-------- src/jq/eval_generic.rs | 120 +++++++++------- src/jq/value.rs | 224 +++++++++++++++++++----------- src/json/light.rs | 40 +++++- src/json/validate.rs | 172 +++++++++++++++++++++++ tests/jq_cli_tests.rs | 49 ++++++- tests/yq_cli_tests.rs | 145 +++++++++++++++++-- 8 files changed, 633 insertions(+), 227 deletions(-) diff --git a/docs/compliance/yq/limitations.md b/docs/compliance/yq/limitations.md index 093312148..ca979958d 100644 --- a/docs/compliance/yq/limitations.md +++ b/docs/compliance/yq/limitations.md @@ -3896,11 +3896,11 @@ its text (`numeric_display_string`, so `==` and `tostring` agree about a compute `True`/`yes`), `"~" == null` is `true` here (yq `false`), and a leading-zero, hex or underscored integer compares by its resolved decimal text (`1 == 01` is `true` here, `false` in yq). Every one of these already shows in `tostring`. -- **The reindex bridge re-spells a computed float**: `[(0.5+0.5)] | .[0] == 1` is `true` - in yq and `false` here, because the bridge serializes the computed `Float(1.0)` as - `1.0` and the text rule then sees `1.0` -- the same pre-existing artefact behind - `[(0.5+0.5)] | .[0] | tostring` printing `"1.0"` (yq `"1"`). `(0.5+0.5) == 1` itself, - which never crosses the bridge, is `true` in both. + +A third entry used to sit here -- the reindex bridge re-spelling a computed float, so that +`[(0.5+0.5)] | .[0] == 1` was `false` (yq `true`) and `[(0.5+0.5)] | .[0] | tostring` +printed `1.0` (yq `1`). Fixed by [#2902](https://github.com/rust-works/succinctly/issues/2902): +the bridge now writes a bare `Float` as a token the reparse hands back as a bare `Float`. Pinned by the `yq_text_equality_2785` module (`tests/yq_cli_tests.rs`), the `scalar_text_equality_2785`/`scalar_wildcard_equality_2785` goldens, and the diff --git a/src/jq/eval.rs b/src/jq/eval.rs index b64a219a1..587aeb4db 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -12409,23 +12409,17 @@ fn yq_join_nonfinite_part(value: &OwnedValue) -> Option { /// for that case, and `Int` formatting (`n.to_string()`, matching /// `to_json_yq()`'s `format!("{n}")` byte-for-byte) is likewise unaffected. /// -/// This still doesn't fully close #1124's own repro (`(2.0/2) | [.] | -/// join(",")` still gives `"1.0"`, not real yq's `"1"`): `builtin_join`'s -/// array branch only ever sees a cursor-backed element, and a *constructed* -/// array reaches that cursor by round-tripping through `to_json_for_reindex` -/// first, which bakes the decimal point into synthesized `NumberLiteral` -/// source text indistinguishable from a genuine document literal -- this -/// function never actually receives a bare `OwnedValue::Float` for that -/// case. Tracked separately as #1144 (broadened to cover `@csv`/`@tsv`/ -/// string interpolation too, which share the identical root cause), since -/// fixing it needs either tagging a reindexed `NumberLiteral` as -/// non-document-sourced or a cursor-free path for constructed arrays, not a -/// change here. This function still has real, confirmed effect on -/// [`yq_join_separator`]'s case (`sep_expr` evaluates directly to an owned -/// value, with no cursor round-trip in between) and on any element that -/// genuinely reaches `join` as a bare computed `Float` some other way (e.g. -/// a document-sourced `.nan`/`.inf` scalar, which `from_number_bytes` -/// likewise degrades to a bare `Float`, not a `NumberLiteral`). +/// Until #2902 this did not close #1124's own repro (`(2.0/2) | [.] | +/// join(",")` gave `"1.0"`, not real yq's `"1"`, tracked as #1144): +/// `builtin_join`'s array branch only ever sees a cursor-backed element, and +/// a *constructed* array reaches that cursor by round-tripping through +/// `to_json_for_reindex` first, which used to bake the decimal point into +/// synthesized `NumberLiteral` source text indistinguishable from a genuine +/// document literal, so this function never received a bare +/// `OwnedValue::Float` for that case. The bridge now hands a computed float +/// back as a bare `Float`, so the element reaches the `Float` arm here like +/// [`yq_join_separator`]'s case always did (`sep_expr` evaluates directly +/// to an owned value, with no cursor round-trip in between). fn yq_join_numeric_part(value: &OwnedValue) -> Option { match value { OwnedValue::Int(_) | OwnedValue::Float(_) | OwnedValue::NumberLiteral(..) => { @@ -37723,23 +37717,19 @@ fn eval_reduce<'a, W: Clone + AsRef<[u64]>, S: EvalSemantics>( /// O(*d*)-sized subtree at each of its *d* nodes — O(*d*²) work to resolve /// what is, for these shapes, an O(1) lookup (#491). /// -/// `Builtin::ToString` (`tostring`) is also fast-pathed here, but for -/// correctness, not just speed (#1054): this is the JSON-input/`-n`-mode -/// sibling of `eval_generic.rs`'s `eval_on_owned`, which has the same -/// bypass for the same reason -- without it, `EXPR | tostring` on a -/// genuinely computed value (e.g. `(1e10*2)`) serializes through -/// `to_json_for_reindex`'s decimal-only float spelling first, reparses as -/// a document-sourced-*looking* `NumberLiteral`, and echoes that baked -/// text verbatim per #1008's literal-preservation rule -- permanently -/// losing the scientific-notation spelling real yq applies to a computed -/// float before that round trip ever has a chance to run. `owned_to_string` -/// is the same function `builtin_tostring` calls directly, so this is -/// unobservable except in speed for every other value shape. -/// -/// Narrow on purpose, not exhaustive -- see `eval_on_owned`'s own doc -/// comment (`eval_generic.rs`) for the exact limitation (only the -/// immediately-next bare `tostring` is covered) and #1134, which tracks -/// the general fix neither sibling attempts. +/// `Builtin::ToString` (`tostring`) is also fast-pathed here -- the +/// JSON-input/`-n`-mode sibling of `eval_generic.rs`'s `eval_on_owned` +/// bypass. It was added for correctness (#1054): `EXPR | tostring` on a +/// genuinely computed value (e.g. `(1e10*2)`) used to serialize through +/// `to_json_for_reindex`'s decimal-only float spelling first, reparse as a +/// document-sourced-*looking* `NumberLiteral`, and echo that baked text +/// verbatim per #1008's literal-preservation rule. Since #2902 the bridge +/// hands a computed float back as a bare `Float` whatever the surrounding +/// expression shape (so the shapes #1134 listed -- `EXPR | . | tostring`, +/// `(tostring)`, `[EXPR] | .[0] | tostring`, `map(tostring)` -- are right +/// through the round trip too), and this arm is speed-only. +/// `owned_to_string` is the same function `builtin_tostring` calls +/// directly, so it is unobservable except in speed for every value shape. /// Converts `expr` to an `OwnedValue` directly, without the general /// evaluator, succeeding only when every leaf `expr` reaches is already an /// `Expr::Literal` -- the shape `substitute_var`'s `Expr::Var` arm (via @@ -38270,13 +38260,14 @@ fn produces_fresh_value(expr: &Expr) -> bool { /// `Builtin::ToString` and the literal-RHS `Arithmetic` accumulator shape /// are deliberately **not** here, though #2397's own plan proposed folding /// them in from [`eval_owned_fast_path`]. Doing so makes them reachable as -/// operands and as non-final pipe stages, which they never were: it changes -/// `succinctly yq -n '[0.5+0.5] | .[0] | tostring'` from `1.0` to `1` (a -/// latent yq-fidelity *fix*, tracked as #2902, but a behaviour change all -/// the same), and `arith_combine` returns its input verbatim for `. + null` -/// / `. + 0` / `. + ""`, so an `Arithmetic` arm reachable in `Fresh` -/// position lets a navigated subvalue escape — precisely what the -/// representation gate exists to prevent. Both found in review of #2897. +/// operands and as non-final pipe stages, which they never were, and +/// `arith_combine` returns its input verbatim for `. + null` / `. + 0` / +/// `. + ""`, so an `Arithmetic` arm reachable in `Fresh` position lets a +/// navigated subvalue escape — precisely what the representation gate +/// exists to prevent. Found in review of #2897, which also surfaced that +/// the fold changed `succinctly yq -n '[0.5+0.5] | .[0] | tostring'` from +/// `1.0` to `1`; that was the bridge re-spelling a computed float, fixed at +/// its source by #2902, so the two paths now agree there either way. fn eval_owned_pure( expr: &Expr, input: &OwnedValue, @@ -60956,27 +60947,6 @@ mod tests { format!("{:?}", normalize(r)) } - /// Whether the bridge's serialize-and-reparse changes the *text* of a - /// number in `value`: a bare, integral, finite `Float` (`Float(1.0)`) - /// goes through `to_json_for_reindex` as `1.0` and comes back as - /// `NumberLiteral(Float(1.0), "1.0")`, where the fast path still holds - /// the `Float` whose yq text is `1`. That is the bridge's pre-existing - /// float re-spelling (`[(0.5+0.5)] | .[0] | tostring` is `"1.0"` here - /// and `"1"` in real yq, on `main` before #2785 too), and since #2785 - /// yq-mode `==`/`!=` compare scalars by text, it now reaches the - /// comparison arms as well: `Float(1.0) == 1` is `true` on the fast - /// path (correct -- `(0.5+0.5) == 1` is `true` in yq) and `false` - /// through the bridge. The yq-mode half of the agreement check skips - /// these values rather than pin the bridge's wrong spelling. - fn bridge_respells_a_float(value: &OwnedValue) -> bool { - match value { - OwnedValue::Float(f) => f.is_finite() && f.fract() == 0.0, - OwnedValue::Array(items) => items.iter().any(bridge_respells_a_float), - OwnedValue::Object(map) => map.values().any(bridge_respells_a_float), - _ => false, - } - } - #[test] fn eval_owned_pure_agrees_with_the_reindex_bridge() { let values = pure_value_matrix(); @@ -60998,9 +60968,6 @@ mod tests { "jq mode: {src:?} on {value:?} disagrees with the reindex bridge" ); - if bridge_respells_a_float(value) { - continue; - } let fast = debug_normalize(eval_owned_input::, YqSemantics>( &expr, value, false, )); @@ -61075,9 +61042,6 @@ mod tests { fast, bridge, "jq mode: {src:?} with $y := {bound:?} on {value:?} disagrees with the bridge" ); - if bridge_respells_a_float(value) { - continue; - } let fast = debug_normalize(eval_owned_input::, YqSemantics>( &expr, value, false, )); diff --git a/src/jq/eval_generic.rs b/src/jq/eval_generic.rs index 099413794..afcb9de91 100644 --- a/src/jq/eval_generic.rs +++ b/src/jq/eval_generic.rs @@ -2555,28 +2555,19 @@ fn eval_on_owned( return format_result::(format_type, &owned, optional); } - // `tostring` needs the same bypass, for correctness here, not just - // speed (#1054): without it, `EXPR | tostring` on a genuinely computed - // value (e.g. `(1e10 * 2)`) serializes through `to_json_for_reindex`'s - // decimal-only float spelling below first, reparses as a - // document-sourced-*looking* number, and echoes that baked text - // verbatim per #1008's literal-preservation rule once `Builtin:: - // ToString`'s own arm (this file's `eval_builtin`, which now calls - // `owned_to_string` directly too) finally runs -- permanently losing - // the scientific-notation spelling real yq applies to a computed float - // before the round trip ever has a chance to run. - // - // Narrow on purpose, not exhaustive: this only matches `tostring` as - // the *immediately next* stage. Any intervening stage (even a no-op - // `.`), a parenthesized `(tostring)`, `tostring?`, or `map(...| - // tostring)` all still fall through to the round-trip below and - // reproduce the original bug, since each intervening stage re-enters - // this function with its own, different `expr` and bakes the float - // into a `NumberLiteral` before `tostring` ever sees it in its - // original form. Fixing that needs either threading "was this ever - // reindexed" through every intermediate call here, or the same - // origin-tracking mechanism #1128 already identifies as the real fix - // for `@json`'s sibling gap -- tracked as #1134, not attempted here. + // `tostring` takes the same bypass. It was added for correctness, not + // speed (#1054): `EXPR | tostring` on a genuinely computed value (e.g. + // `(1e10 * 2)`) used to serialize through `to_json_for_reindex`'s + // decimal-only float spelling below first, reparse as a + // document-sourced-*looking* number, and echo that baked text verbatim + // per #1008's literal-preservation rule once `Builtin::ToString`'s own + // arm finally ran -- and because this matches only the *immediately + // next* stage, any intervening stage (`EXPR | . | tostring`, + // `(tostring)`, `[EXPR] | .[0] | tostring`, `map(... | tostring)`) + // still reproduced it (#1134). #2902 fixed that at the source: the + // round trip below now writes a computed float as a token the reparse + // hands back as a bare `Float`, so every one of those shapes is right + // through the bridge too, and this arm is speed-only. if let Expr::Builtin(Builtin::ToString) = expr { return GenericResult::Owned(OwnedValue::String(owned_to_string::(&owned))); } @@ -2820,15 +2811,22 @@ const REINDEX_LITERAL_LEN_CAP: usize = 256; /// exceptions are all numeric, because `to_json_for_reindex` is a *formatter* /// as much as a serializer: /// -/// - A **bare `Float`** is re-spelled by that formatter's mode-forked rule -/// (yq keeps a whole number's decimal point at any magnitude, jq keeps the -/// bare `Display` spelling, #953) and comes back as a `NumberLiteral` -/// carrying that new text. This is the case the bridge is genuinely -/// load-bearing for: without the guard, `.outer.big | parent` on -/// `10000000000000000000.0` prints `1e+19` in yq mode. /// - A **NaN** `NumberLiteral` is replaced by `NAN_SENTINEL`. /// - A `NumberLiteral` whose source text exceeds /// [`REINDEX_LITERAL_LEN_CAP`] is discarded (#1211). +/// - A non-finite bare `Float` goes through a sentinel/overflow literal; +/// it comes back as the same value, but this predicate stays out of that +/// corner rather than reason about it. +/// +/// A bare finite **`Float`** used to head this list: the formatter re-spelled +/// it by a mode-forked rule (#953) and it came back as a `NumberLiteral` +/// carrying that new text, which was the case the bridge was genuinely +/// load-bearing for (without the guard, `.outer.big | parent` on +/// `10000000000000000000.0` printed `1e+19` in yq mode). Since #2902 the +/// bridge writes a bare finite `Float` as a token +/// (`crate::json::validate::computed_float_token`) that reparses to the same +/// bare `Float`, so the round trip is an identity on it too and the bypass +/// applies. /// /// Everything else -- `null`, booleans, strings (escaped and unescaped /// symmetrically), object keys, and the overwhelmingly common @@ -2838,14 +2836,11 @@ const REINDEX_LITERAL_LEN_CAP: usize = 256; /// /// A bare **`Int`** is the one node that is *normalized* rather than /// preserved and is still allowed through: it comes back as -/// `NumberLiteral(Int(n), "n")`. That is sound where a bare `Float` isn't, -/// because `to_json_for_reindex` writes an `Int` as exactly `format!("{n}")` -/// in both modes -- the only spelling an `i64` has -- so the literal the -/// bridge bakes in is the same text the bare `Int` renders as anyway, and -/// #1008's literal preservation has nothing new to echo. A `Float`'s -/// spelling, by contrast, is mode-forked and genuinely differs from what the -/// bare value would produce, which is exactly the `1e+19` breakage the guard -/// exists to prevent. +/// `NumberLiteral(Int(n), "n")`. That is sound because `to_json_for_reindex` +/// writes an `Int` as exactly `format!("{n}")` in both modes -- the only +/// spelling an `i64` has -- so the literal the bridge bakes in is the same +/// text the bare `Int` renders as anyway, and #1008's literal preservation +/// has nothing new to echo. /// /// Excluding `Int` is not merely conservative here, it is the difference /// between this fix applying to `succinctly yq` and not (code review): @@ -2867,7 +2862,7 @@ const REINDEX_LITERAL_LEN_CAP: usize = 256; /// alone can safely skip the bridge. pub(crate) fn reindex_bridge_is_identity(value: &OwnedValue) -> bool { match value { - OwnedValue::Float(_) => false, + OwnedValue::Float(f) => f.is_finite(), OwnedValue::Int(_) => true, OwnedValue::NumberLiteral(NumberRepr::Float(f), _) if f.is_nan() => false, OwnedValue::NumberLiteral(_, literal) => literal.len() <= REINDEX_LITERAL_LEN_CAP, @@ -2919,8 +2914,10 @@ pub(crate) fn reindex_bridge_is_identity(value: &OwnedValue) -> bool { /// is that a document-sourced float now arrives already carrying its /// spelling, so the round trip preserves it as a literal instead of having /// to synthesize one for everything. `[1e10 * 2]` keeps scientific notation -/// as a result; the ordinary-magnitude `join` gap (#1124/#1144, `[2.0/2]`) -/// is below the threshold and is untouched. +/// as a result. #2902 then closed the ordinary-magnitude `join` gap +/// (#1124/#1144, `[2.0/2]`) from the other side: the bridge now writes a +/// bare `Float` as a token it hands back as a bare `Float`, so this round +/// trip no longer bakes a computed float into a literal at any magnitude. /// /// Round-tripping just `values` (not the whole input document, unlike the /// old wildcard fallback these two arms replace) keeps `Expr::Array`'s and @@ -25007,20 +25004,37 @@ mod tests { ); } - // The shape the guard exists for: a bare `Float`, which #953's - // mode-forked re-spelling rewrites. Asserted in both directions -- - // the predicate refuses it, *and* the bridge really does change it, - // so this case can't quietly stop being a real one. - let respelled = OwnedValue::Float(1e19); - assert!(!super::reindex_bridge_is_identity(&respelled)); - assert!( - !round_trips_unchanged::(&respelled), - "sanity: the yq-mode bridge really does rewrite {respelled:?}" - ); - assert!( - !round_trips_unchanged::(&respelled), - "sanity: the jq-mode bridge really does rewrite {respelled:?}" - ); + // The shape the guard used to exist for: a bare `Float`, which #953's + // mode-forked re-spelling rewrote into a `NumberLiteral`. Since #2902 + // the bridge writes it as a token that reparses to the same bare + // `Float`, so it is an identity in both modes and the predicate admits + // it -- asserted in both directions so a formatter regression that + // starts re-spelling it again fails here, not in a user's `tostring`. + for computed in [ + OwnedValue::Float(1e19), + OwnedValue::Float(1.0), + OwnedValue::Float(-0.0), + OwnedValue::Float(5e-6), + ] { + assert!( + super::reindex_bridge_is_identity(&computed), + "the bypass must fire for a computed float: {computed:?}" + ); + assert!( + round_trips_unchanged::(&computed), + "sanity: the yq-mode bridge hands back {computed:?} unchanged" + ); + assert!( + round_trips_unchanged::(&computed), + "sanity: the jq-mode bridge hands back {computed:?} unchanged" + ); + } + assert!(!super::reindex_bridge_is_identity(&OwnedValue::Float( + f64::NAN + ))); + assert!(!super::reindex_bridge_is_identity(&OwnedValue::Float( + f64::INFINITY + ))); } /// The one thing [`reindex_bridge_is_identity`]'s `Int` arm rests on, and diff --git a/src/jq/value.rs b/src/jq/value.rs index 43957a9fa..25b345d05 100644 --- a/src/jq/value.rs +++ b/src/jq/value.rs @@ -1841,6 +1841,13 @@ impl OwnedValue { f64::INFINITY }); } + // The reindex bridge's computed-float token (#2902): a bare `Float` + // going in must be a bare `Float` coming out, never a + // `NumberLiteral` -- checked with the sentinels, before any of the + // literal-preserving arms below can see the text. + if let Some(f) = crate::json::validate::parse_computed_float_token(bytes) { + return Self::Float(f); + } if crate::json::validate::is_valid_number(bytes) { return core::str::from_utf8(bytes).map_or(Self::Null, Self::from_number_literal); } @@ -2207,20 +2214,28 @@ impl OwnedValue { /// this same literal-preserving JSON text, not [`to_json`](Self::to_json)'s /// jq-normalized one. /// - /// Also keeps a whole-number `Float`'s decimal point (`format_float_with_fraction`, - /// #953) -- unlike jq, which happily prints `1.0` as `1` in JSON (matching - /// real jq's own `tojson`), yq must not: `1.0` and `1` are different YAML - /// types, and dropping the point on a round trip changes it (#169's own - /// reasoning, reused here for the same class of value reached from a - /// different path -- an i64-overflow decimal integer scalar, which - /// `resolve_plain` also classifies as `!!float`, confirmed live against - /// the pinned oracle: real yq's `-o json` gives `100...0.0`, not - /// `100...0`). + /// A plain `Float` takes yq's computed-float spelling (`format_float_yq`): + /// a whole number keeps its decimal point (#953 -- unlike jq, which + /// happily prints `1.0` as `1` in JSON, matching real jq's own `tojson`, + /// yq must not: `1.0` and `1` are different YAML types, and dropping the + /// point on a round trip changes it, #169's own reasoning), and a + /// magnitude past yq's threshold goes scientific (`(1e10*2) | tojson` + /// is `2e+10` in real yq). The threshold used to be unreachable from + /// here -- `tojson` reached this function only after a reindex round + /// trip that had already baked the computed value into a `NumberLiteral` + /// spelled `2e+10`, which the literal arm then echoed -- and became + /// load-bearing once #2902 made that round trip hand back a bare `Float`. + /// The i64-overflow decimal integer scalar this used to spell out in + /// full (`resolve_plain` classifies it `!!float`; real yq's `-o json` + /// gives `100...0.0`, not `1e+20`, confirmed live) still does, because + /// #2438 records it as a `NumberLiteral` at the document boundary + /// ([`from_document_float`](Self::from_document_float)) and the literal + /// arm echoes that. pub(crate) fn to_json_yq(&self) -> String { self.to_json_at_depth( 0, crate::jq::stream::real_output_finite_literal, - crate::yaml::format_float_with_fraction, + crate::yaml::format_float_yq, yq_infinite_float_json_text, ) } @@ -2320,30 +2335,25 @@ impl OwnedValue { /// the bridge re-parses this text and hands the cursor to the full /// evaluator (#561, #472). /// - /// `S: EvalSemantics` picks the plain (non-`NumberLiteral`) `Float` - /// fallback's spelling (#953, #2438): yq applies its own magnitude - /// threshold (`format_float_yq` -- decimal-with-point for everyday - /// magnitudes, `e+NN`/`e-NN` past it), which is what a *computed* float - /// gets from real yq wherever it re-serializes one. This used to be an - /// unconditional `format_float_with_fraction` so that a **document** - /// float with no preserved literal (an i64-overflow YAML scalar reaching - /// this bridge via `[...]`/`map_values`/`with_entries`) would keep the - /// full decimal spelling real yq gives *it* at the same magnitude -- the - /// two answers genuinely differ for the identical `f64`, so the - /// provenance is now recorded upstream instead, at - /// [`from_document_float`](Self::from_document_float), leaving this - /// fallback free to spell the computed case correctly. jq keeps the - /// pre-existing bare `Display` (no forced - /// point): real jq's own convention drops a computed value's literal - /// formatting entirely (`1.0 + 4.0` prints `5`, not `5.0`), and a bare - /// `Float` reaching this fallback is by construction one that already - /// lost its `NumberLiteral` text — confirmed live this must stay - /// mode-gated, not unconditional: an earlier draft hardcoded yq's - /// formatter here unconditionally, which silently flipped - /// `reduce (1,2) as $i (1.0 + 4.0; [.])` in **jq** mode from the - /// correct `[[[5]]]` to `[[[5.0]]]` (caught in code review) since - /// `format_number_jq_compat` does not strip an explicit `.0` back off a - /// literal it's handed after the reparse. + /// A plain (non-`NumberLiteral`) finite `Float` is written as the + /// bridge-only token `crate::json::validate::computed_float_token` + /// (#2902), which [`from_number_bytes`](Self::from_number_bytes) and + /// `JsonNumber::as_f64` decode back to a bare `Float`. It used to be + /// written with a display formatter -- `format_float_yq` in yq mode + /// (#953, #2438), `jq_bare_float_display` in jq mode -- and since the + /// reparse cannot tell `1.0` written by this bridge from `1.0` written + /// in a document, the value came back as a `NumberLiteral` and every + /// literal-preserving rule downstream (#1008, #1054, #2456) echoed the + /// bridge's spelling: `[0.5+0.5] | .[0] | tostring` answered `1.0` + /// (real yq `1`) and `[2*1e16] | .[0] | tostring` answered `2E+16` + /// (real jq `2e+16`). The token carries no spelling at all, so each + /// mode re-derives its own from the `f64` after the round trip, and the + /// mode fork this fallback used to need is gone. A **document** float + /// with no preserved literal (an i64-overflow YAML scalar) still keeps + /// the full decimal spelling real yq gives *it* because its provenance + /// is recorded upstream, at + /// [`from_document_float`](Self::from_document_float), as a + /// `NumberLiteral` that echoes verbatim below. pub fn to_json_for_reindex(&self) -> String { self.to_json_for_reindex_at_depth::(0) } @@ -2459,24 +2469,21 @@ impl OwnedValue { .collect(); format!("{{{}}}", entries.join(",")) } - // yq spells a computed float by its own magnitude threshold - // (#953, #2438); jq keeps the original bare `Display` (see - // this function's own doc comment for why the fork is required, - // not optional). - // `infinite_fmt` is unreachable from here either way -- every - // NaN/infinite case is already handled by the arms above, before - // this fallback -- so which one is passed only matters for - // reading, not behavior; picked per-mode for consistency. - other if S::TAG == EvalTag::Yq => other.to_json_at_depth( - depth, - format_number_jq_compat, - crate::yaml::format_float_yq, - yq_infinite_float_json_text, - ), + // A bare finite `Float` is a *computed* value (or a tag-forced + // document float that never had a decimal spelling, #1176/#2438) + // and must come back out of the reparse as a bare `Float` again, + // so it is written as the bridge-only token + // `computed_float_token` decodes (#2902), not as either mode's + // display spelling -- see that function's doc for why the token + // is unparseable as an ordinary number. Same in both modes; the + // spelling is re-derived from the `f64` downstream, per mode. + // `infinite_fmt` is unreachable from here -- every NaN/infinite + // case is already handled by the arms above, before this + // fallback -- so which one is passed only matters for reading. other => other.to_json_at_depth( depth, format_number_jq_compat, - jq_bare_float_display, + crate::json::validate::computed_float_token, infinite_float_preview_text, ), } @@ -3579,6 +3586,31 @@ mod tests { assert!(NAN_SENTINEL.parse::().is_err()); } + /// #2902: the bridge's computed-float token materializes as a bare + /// `Float`, never as a `NumberLiteral` carrying the token's text -- + /// through the same public entry point real document numbers go + /// through, exactly like the overflow sentinels beside it. + #[test] + fn test_from_number_bytes_decodes_the_computed_float_token_2902() { + for f in [1.0, -0.0, 0.5, 2e16, 5e-6] { + let token = crate::json::validate::computed_float_token(f); + let got = OwnedValue::from_number_bytes(token.as_bytes()); + assert!( + matches!(got, OwnedValue::Float(back) if back.to_bits() == f.to_bits()), + "{token} materialized as {got:?}" + ); + } + // A genuine literal that merely ends in `e0` is still a literal. + assert_eq!( + OwnedValue::from_number_bytes(b"1e0"), + OwnedValue::from_number_literal("1e0") + ); + assert!(matches!( + OwnedValue::from_number_bytes(b"1e0"), + OwnedValue::NumberLiteral(_, _) + )); + } + /// #2438: the document boundary bakes yq's own decimal spelling into a /// float that has no source literal left, but only past yq's /// scientific-notation threshold -- inside the everyday range the value @@ -3604,38 +3636,70 @@ mod tests { } } - /// #2438: the bridge's yq fallback spells a *computed* float by yq's own - /// threshold, rather than forcing a decimal point at every magnitude. - /// #2456: jq mode's own fallback applies its own (different) threshold - /// too, rather than never reformatting a computed float at all -- this - /// used to pin the opposite (a full 21-digit decimal expansion) as - /// deliberately untouched by #2438, which #2456 fixes. + /// #2902: the bridge writes a bare finite `Float` as the computed-float + /// token in *both* modes -- no display spelling at all, so the reparse + /// cannot mistake it for a document literal. This used to pin each + /// mode's own display threshold here (#2438/#2456), which is exactly + /// what let `[0.5+0.5] | .[0] | tostring` come back as the literal `1.0`. #[test] - fn test_to_json_for_reindex_float_uses_each_modes_own_threshold_2438_2456() { - assert_eq!( - OwnedValue::Float(1e20).to_json_for_reindex::(), - "1e+20" - ); - assert_eq!( - OwnedValue::Float(2.0).to_json_for_reindex::(), - "2.0" - ); - assert_eq!( - OwnedValue::Float(1e20).to_json_for_reindex::(), - "1e+20" - ); - // jq's own threshold sits at a different magnitude than yq's fixed - // `>= 1e6`/`<= 1e-4` (#953): 1e10 already clears yq's, but jq's own - // digit-count rule (`decpt = 11 <= ndigits(1) + 15 = 16`) keeps it - // decimal there. - assert_eq!( - OwnedValue::Float(1e10).to_json_for_reindex::(), - "10000000000" - ); - assert_eq!( - OwnedValue::Float(1e10).to_json_for_reindex::(), - "1e+10" - ); + fn test_to_json_for_reindex_writes_a_computed_float_as_the_token_2902() { + for (f, token) in [ + (1e20, "1e20e0"), + (2.0, "2e0e0"), + (1e10, "1e10e0"), + (-0.5, "-5e-1e0"), + ] { + assert_eq!( + OwnedValue::Float(f).to_json_for_reindex::(), + token + ); + assert_eq!( + OwnedValue::Float(f).to_json_for_reindex::(), + token + ); + } + } + + /// #2902: the invariant the fix rests on -- serialize-and-reparse hands + /// back the same *representation* it was given, in both modes: a bare + /// `Float` stays bare (never a `NumberLiteral` carrying the bridge's + /// spelling), a `NumberLiteral` keeps its text verbatim, and an `Int` + /// takes its one spelling. + #[test] + fn test_reindex_round_trip_keeps_float_and_literal_apart_2902() { + use crate::json::JsonIndex; + fn round_trip(value: &OwnedValue) -> OwnedValue { + let json = value.to_json_for_reindex::(); + let bytes = json.as_bytes(); + let index = JsonIndex::build(bytes); + let root = index.root(bytes); + let crate::json::light::StandardJson::Array(items) = root.value() else { + panic!("expected the round-tripped array, got {json}"); + }; + OwnedValue::Array( + items + .map(|item| match item { + crate::json::light::StandardJson::Number(n) => { + OwnedValue::from_number_bytes(n.raw_bytes()) + } + other => panic!("expected a number, got {other:?}"), + }) + .collect(), + ) + } + let value = OwnedValue::Array(vec![ + OwnedValue::Float(1.0), + OwnedValue::from_number_literal("1.0"), + OwnedValue::Int(1), + OwnedValue::Float(2e16), + OwnedValue::from_number_literal("2e16"), + OwnedValue::Float(-0.0), + ]); + let want = "Array([Float(1.0), NumberLiteral(Float(1.0), \"1.0\"), \ + NumberLiteral(Int(1), \"1\"), Float(2e16), \ + NumberLiteral(Float(2e16), \"2e16\"), Float(-0.0)])"; + assert_eq!(format!("{:?}", round_trip::(&value)), want); + assert_eq!(format!("{:?}", round_trip::(&value)), want); } /// #2456: `jq_bare_float_display`'s digit-count threshold diff --git a/src/json/light.rs b/src/json/light.rs index 23492e29f..57ee510c2 100644 --- a/src/json/light.rs +++ b/src/json/light.rs @@ -2138,10 +2138,21 @@ impl<'a> JsonNumber<'a> { } /// Parse as f64. + /// + /// Also decodes the reindex bridge's computed-float token + /// (`crate::json::validate::computed_float_token`, #2902), which is + /// deliberately unparseable as an ordinary number: every cursor-level + /// reader of a bridged document (`length`, the math builtins, dates, + /// `isnan`, ...) reaches its value through this one accessor, so the + /// decode lives here rather than being repeated at each of them. Only + /// consulted once the ordinary parse has already failed, so a genuine + /// number pays nothing for it. pub fn as_f64(&self) -> Result { let bytes = self.raw_bytes(); let s = core::str::from_utf8(bytes).map_err(|_| JsonError::InvalidUtf8)?; - s.parse().map_err(|_| JsonError::InvalidNumber) + s.parse().or_else(|_| { + crate::json::validate::parse_computed_float_token(bytes).ok_or(JsonError::InvalidNumber) + }) } fn find_end(&self) -> usize { @@ -6232,6 +6243,33 @@ mod tests { assert_eq!(nested_number_span(b"1E", 0), 2); } + /// #2902: a reindexed computed float arrives here as the bridge's token, + /// which the scanner captures whole and `as_f64` decodes, while + /// `as_i64` and `number_literal()` both refuse it -- so a materializer + /// that tries the literal first (`to_owned_at_depth`) still ends on a + /// bare `Float`, never a `NumberLiteral` carrying the token's text. + #[test] + fn json_number_decodes_the_computed_float_token_2902() { + for (f, token) in [(1.0, "1e0e0"), (2e16, "2e16e0"), (-0.5, "-5e-1e0")] { + let json = format!("[{token}]"); + let bytes = json.as_bytes(); + assert_eq!(nested_number_span(bytes, 1), 1 + token.len()); + let index = JsonIndex::build(bytes); + let root = index.root(bytes); + let StandardJson::Array(mut items) = root.value() else { + panic!("expected an array"); + }; + let StandardJson::Number(n) = items.next().expect("one element") else { + panic!("expected a number"); + }; + assert_eq!(n.raw_bytes(), token.as_bytes()); + assert_eq!(n.as_f64().map(f64::to_bits), Ok(f64::to_bits(f)), "{token}"); + assert_eq!(n.as_i64(), Err(JsonError::InvalidNumber), "{token}"); + let number: StandardJson<'_, Vec> = StandardJson::Number(n); + assert_eq!(number.number_literal(), None, "{token}"); + } + } + /// #2072 step 1: the `'static` handle round-trips on *every* node of a /// nested document -- the root object, nested arrays and objects, and /// each scalar leaf -- and re-basing from an unrelated cursor of the diff --git a/src/json/validate.rs b/src/json/validate.rs index a3f278ca5..22eb80329 100644 --- a/src/json/validate.rs +++ b/src/json/validate.rs @@ -1049,10 +1049,182 @@ pub fn has_leading_dot(bytes: &[u8]) -> bool { is_valid_number(&fixed) } +/// The suffix [`computed_float_token`] appends to Rust's `{:e}` rendering, +/// giving the token its second exponent marker. +const COMPUTED_FLOAT_TOKEN_SUFFIX: &str = "e0"; + +/// The reindex bridge's spelling of a *computed* finite float (#2902) -- a +/// bare `OwnedValue::Float`, one with no source literal left -- built so the +/// reparse can tell it apart from every document literal and hand back a +/// bare `Float` again instead of a `NumberLiteral` carrying this text. +/// +/// Rust's shortest-round-trip exponential rendering (`1e0`, `1.5e0`, `2e16`, +/// `5e-6`, `-0e0`) followed by a second `e0`: `1e0e0`, `2e16e0`. That +/// doubled exponent marker is the whole design, borrowed from +/// `NAN_SENTINEL`/`INFINITY_SENTINEL` (`src/jq/value.rs`, #472/#1083): +/// `str::parse::()`/`::()` both reject it, so no valid JSON +/// literal -- and none of the jq-lenient spellings this crate preserves as +/// literals either (a leading `.`, a redundant leading zero, a trailing `.` +/// before the exponent) -- can ever spell the same bytes; it starts with a +/// digit or `-digit`, so the semi-index scanner classifies it as a number +/// (a `+`-prefixed token does not survive `JsonIndex::build` at all, +/// probed); and it is drawn from `[0-9.eE+-]`, so `nested_number_span` +/// (`src/json/light.rs`) captures it whole. +/// +/// This is *not* a display formatter: the value's spelling is re-derived +/// from the `f64` by whichever mode's computed-float rule applies once it +/// is back in an `OwnedValue` (`jq_bare_float_display`, `format_float_yq`, +/// `numeric_display_string`). Before #2902 the bridge spelled a computed +/// float with those display formatters directly, and the reparse -- which +/// cannot tell `1.0` written by the bridge from `1.0` written in a document +/// -- turned it into a literal, so `[0.5+0.5] | .[0] | tostring` echoed +/// `1.0` where real yq answers `1` and `[2*1e16] | .[0] | tostring` echoed +/// jq's literal-reformatting `2E+16` where real jq answers `2e+16`. +/// +/// Same accepted trade-off as the sentinels: an *invalid* document span +/// that happens to spell this shape (`[1e0e0]`, rejected by every strict +/// reader including jq's own) now materializes through the lenient +/// semi-index path as the float instead of `null`, exactly as `[9e999e999]` +/// already materializes NaN. +/// +/// `f` must be finite: NaN/±Infinity have their own sentinels, which every +/// caller emits first. +#[must_use] +pub(crate) fn computed_float_token(f: f64) -> String { + debug_assert!( + f.is_finite(), + "computed_float_token requires a finite value; NaN/Infinity use their own sentinels" + ); + let mut token = alloc::format!("{f:e}"); + token.push_str(COMPUTED_FLOAT_TOKEN_SUFFIX); + token +} + +/// Decodes a [`computed_float_token`], `None` for anything else -- the one +/// definition every reader of a `to_json_for_reindex` number token consults +/// (`OwnedValue::from_number_bytes`, `JsonNumber::as_f64`), so the check +/// cannot diverge between call sites the way three copies of one predicate +/// did in #106. +/// +/// Strips the suffix, then requires the remainder to start the way `{:e}` +/// does (a digit or `-digit`), to still carry an exponent marker, and to +/// parse as a finite `f64`. The exponent requirement is what keeps a genuine +/// literal out: `1e0` ends in `e0` too, but its remainder `1` has no `e`, +/// while every `{:e}` rendering has exactly one. +#[must_use] +pub(crate) fn parse_computed_float_token(bytes: &[u8]) -> Option { + let inner = bytes.strip_suffix(COMPUTED_FLOAT_TOKEN_SUFFIX.as_bytes())?; + let digits = inner.strip_prefix(b"-").unwrap_or(inner); + if !digits.first().is_some_and(u8::is_ascii_digit) || !inner.contains(&b'e') { + return None; + } + let f: f64 = core::str::from_utf8(inner).ok()?.parse().ok()?; + f.is_finite().then_some(f) +} + #[cfg(test)] mod tests { use super::*; + // ======================================================================== + // computed-float token tests (#2902) + // ======================================================================== + + /// Load-bearing for #2902, the same way + /// `test_nan_sentinel_is_unparseable_as_a_real_number` is for #472: the + /// token is only safe to reserve because no legitimately formatted + /// number can ever spell it -- both parses must fail, and it must still + /// be a number span to the semi-index scanner (digit-leading, drawn from + /// `[0-9.eE+-]`). + #[test] + fn computed_float_token_is_unparseable_as_a_real_number_2902() { + for f in [ + 0.0, + -0.0, + 1.0, + 0.1 + 0.2, + 5e-6, + 2e16, + 1e300, + f64::MAX, + 5e-324, + ] { + let token = computed_float_token(f); + assert!(token.parse::().is_err(), "{token}"); + assert!(token.parse::().is_err(), "{token}"); + assert!(!is_valid_number(token.as_bytes()), "{token}"); + assert!( + token + .bytes() + .next() + .is_some_and(|b| b.is_ascii_digit() || b == b'-'), + "{token}" + ); + assert!( + token + .bytes() + .all(|b| b.is_ascii_digit() || matches!(b, b'.' | b'e' | b'E' | b'+' | b'-')), + "{token}" + ); + } + assert_eq!(computed_float_token(1.0), "1e0e0"); + assert_eq!(computed_float_token(2e16), "2e16e0"); + assert_eq!(computed_float_token(-0.5), "-5e-1e0"); + } + + /// The decode is bit-exact, including the sign of zero and both + /// subnormal and maximal magnitudes -- the reparse must hand back the + /// very `f64` the bridge was given, not a nearby one. + #[test] + fn computed_float_token_round_trips_bit_exactly_2902() { + for f in [ + 0.0, + -0.0, + 1.0, + -1.0, + 0.1 + 0.2, + 5e-6, + 2e16, + 1e300, + f64::MIN_POSITIVE, + 5e-324, + f64::MAX, + -f64::MAX, + ] { + let token = computed_float_token(f); + let back = parse_computed_float_token(token.as_bytes()) + .unwrap_or_else(|| panic!("{token} must decode")); + assert_eq!(back.to_bits(), f.to_bits(), "{token}"); + } + } + + /// Nothing but a token decodes: a genuine literal that happens to end in + /// `e0` has no exponent left once the suffix is gone, the overflow + /// sentinels keep their own decoders, and a token with a non-finite or + /// malformed remainder is not one either. + #[test] + fn parse_computed_float_token_rejects_everything_else_2902() { + for text in [ + "1e0", + "10e0", + "1", + "1.0", + "1e0e00", + "e0", + "1ee0", + "9e999e999", + "8e999e999", + "-8e999e999", + "1e999e0", + "1e0e0e0", + "", + "e0e0", + "+1e0e0", + ] { + assert_eq!(parse_computed_float_token(text.as_bytes()), None, "{text}"); + } + } + // ======================================================================== // validate_jq_lenient tests (#2052) // ======================================================================== diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 0ac7561ce..ca9b926b4 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -32829,15 +32829,54 @@ fn test_tonumber_rejects_doubled_sign_1090() { } } +/// #2902: a computed float read back out of a container must not regain a +/// literal spelling. jq mode already answered `"1"` for the issue's own row +/// (its bridge spelled `Float(1.0)` as `1`), but one threshold up the same +/// mechanism bit: `[2*1e16] | .[0] | tostring` crossed the reindex bridge, +/// which spelled the computed value `2e+16`, reparsed it as a `NumberLiteral`, +/// and `tostring` then applied jq's *literal*-reformatting convention +/// (uppercase `E`, `2E+16`) instead of its computed-value one. The bridge now +/// writes a bare `Float` as a token it hands back as a bare `Float`. Every +/// expectation captured live from jq 1.7.1 (`-c`, `null` on stdin), the +/// already-right rows included so a genuine literal keeps echoing verbatim. +#[test] +fn test_computed_float_through_container_keeps_computed_spelling_2902() -> Result<()> { + for (filter, want) in [ + ("[2*1e16] | .[0] | tostring", r#""2e+16""#), + ("[2*1e16] | . | .[0] | tostring", r#""2e+16""#), + ("(2*1e16) as $x | [$x] | .[0] | tostring", r#""2e+16""#), + ("[2*1e17] | .[0] | tojson", r#""2e+17""#), + ("[1e-5/2] | .[0] | tojson", r#""5e-06""#), + ("[1e10] | map(. * 2 | tostring)", r#"["20000000000"]"#), + ("[0.5+0.5] | .[0] | tostring", r#""1""#), + ("[1.0] | .[0] | tostring", r#""1.0""#), + ("[1e2] | .[0] | tostring", r#""1E+2""#), + ("[1.50e10] | .[0]", "1.50E+10"), + ] { + let (stdout, code) = run_jq_stdin(filter, "null\n", &["-c"])?; + assert_eq!(code, 0, "`{filter}` exited {code}: {stdout:?}"); + assert_eq!(stdout.trim_end(), want, "`{filter}`"); + } + Ok(()) +} + /// #1090 follow-on: preserving `tonumber`'s literal must not start /// accepting text real jq rejects. The internal overflow sentinels -/// (`9e999e999` -> NaN, `8e999e999` -> Infinity) are ordinary user input -/// here, and routing this builtin through `OwnedValue::from_number_bytes` -/// -- which decodes them -- would silently turn jq's documented error into -/// a NaN. Error wording confirmed against jq 1.7.1. +/// (`9e999e999` -> NaN, `8e999e999` -> Infinity) and, since #2902, the +/// reindex bridge's computed-float token (`1e0e0` -> `1`) are ordinary +/// user input here, and routing this builtin through +/// `OwnedValue::from_number_bytes` -- which decodes them -- would silently +/// turn jq's documented error into a number. Error wording confirmed +/// against jq 1.7.1. #[test] fn test_tonumber_rejects_internal_overflow_sentinels_1090() { - for input in [r#""9e999e999""#, r#""8e999e999""#, r#""-8e999e999""#] { + for input in [ + r#""9e999e999""#, + r#""8e999e999""#, + r#""-8e999e999""#, + r#""1e0e0""#, + r#""2e16e0""#, + ] { let (stdout, stderr, code) = run_jq_full(&["tonumber"], Some(input)) .unwrap_or_else(|e| panic!("`{input} | tonumber` failed to run: {e}")); assert_ne!(code, 0, "`{input}` should error\nstdout: {stdout}"); diff --git a/tests/yq_cli_tests.rs b/tests/yq_cli_tests.rs index a4a665287..597813d21 100644 --- a/tests/yq_cli_tests.rs +++ b/tests/yq_cli_tests.rs @@ -22239,23 +22239,27 @@ fn test_yq_join_separator_computed_int_unaffected_1124() -> Result<()> { Ok(()) } -/// #1124 (partial fix, tracked further as #1144): `yq_join_element_part`'s -/// equivalent catch-all was fixed the same way as `yq_join_separator`'s -/// above, but a *constructed* array's computed-float element still doesn't -/// reach real yq's `"1"` answer -- `builtin_join`'s array branch only ever -/// receives a cursor-backed element, and reaching that cursor for a -/// constructed array requires a `to_json_for_reindex` round-trip that bakes +/// #1124/#1144, closed by #2902: a *constructed* array's computed-float +/// element now reaches real yq's `"1"` answer. `builtin_join`'s array branch +/// only ever receives a cursor-backed element, so a constructed array gets +/// there through a `to_json_for_reindex` round-trip -- which used to bake /// the decimal point into synthesized `NumberLiteral` source text -/// indistinguishable from a genuine document literal, so this fix's -/// `numeric_display_string` call never actually sees a bare `Float` here. -/// Pinned as a known, separately-tracked gap rather than silently -/// unasserted -- if #1144 closes this, this assertion should change to -/// `"1"` and the doc comment above should be updated to match. +/// indistinguishable from a genuine document literal (this test pinned +/// `"1.0"` as the known gap). The bridge now hands the element back as the +/// bare `Float` it was, so `yq_join_element_part`'s `numeric_display_string` +/// call finally sees it. A genuine document literal still echoes verbatim. +/// Both captured from yq v4.53.3. #[test] -fn test_yq_join_element_computed_float_known_gap_1124() -> Result<()> { - let (stdout, code) = run_yq_stdin(r#"(2.0 / 2) | [.] | join(",")"#, "null\n", &["-r"])?; - assert_eq!(code, 0); - assert_eq!(stdout.trim_end(), "1.0"); +fn test_yq_join_element_computed_float_matches_yq_1144() -> Result<()> { + for (filter, want) in [ + (r#"(2.0 / 2) | [.] | join(",")"#, "1"), + (r#"[2.0/2] | map(.) | join(",")"#, "1"), + (r#"[1.500] | join(",")"#, "1.500"), + ] { + let (stdout, code) = run_yq_stdin(filter, "null\n", &["-r"])?; + assert_eq!(code, 0, "{filter}"); + assert_eq!(stdout.trim_end(), want, "{filter}"); + } Ok(()) } @@ -45796,6 +45800,112 @@ fn test_yq_getpath_path_context_pulls_argument_generator_lazily_2259() -> Result Ok(()) } +/// #2902: a computed float read back out of a container must not regain a +/// literal spelling. `[0.5+0.5] | .[0] | tostring` crossed the reindex bridge +/// (`to_json_for_reindex` + `JsonIndex::build`), which spelled the computed +/// `Float(1.0)` as `1.0` and reparsed it as a `NumberLiteral` carrying that +/// text, so `tostring` echoed `1.0` where real yq answers `1`. The bridge now +/// writes a bare `Float` as a token it hands back as a bare `Float`. Every +/// expectation below was captured live from yq v4.53.3 (no flags, `null` on +/// stdin) -- including the rows that were already right, which pin that a +/// genuine literal still echoes verbatim and that the above-threshold +/// spellings the old literal accident happened to get right still hold. +mod computed_float_through_container_2902 { + use super::run_yq_stdin; + use anyhow::Result; + + fn check(rows: &[(&str, &str)]) -> Result<()> { + for (filter, want) in rows { + let (out, code) = run_yq_stdin(filter, "null\n", &[])?; + assert_eq!(code, 0, "`{filter}` exited {code}: {out:?}"); + assert_eq!(out, format!("{want}\n"), "`{filter}`"); + } + Ok(()) + } + + /// The issue's own table, plus the same value through deeper and + /// differently-shaped containers. + #[test] + fn tostring_drops_the_bridge_spelling_2902() -> Result<()> { + check(&[ + ("[0.5+0.5] | .[0] | tostring", "1"), + ("(0.5+0.5) | tostring", "1"), + ("1.0 | tostring", "1.0"), + ("[1.0] | .[0] | tostring", "1.0"), + ("[0.0*1] | .[0] | tostring", "0"), + ("[2.5+2.5] | .[] | tostring", "5"), + ("[[0.5+0.5]] | .[0][0] | tostring", "1"), + ("[0.5+0.5] | map(.) | .[0] | tostring", "1"), + ("[1e2] | .[0] | tostring", "1e2"), + ]) + } + + /// Not only `tostring`: plain output and the nested `!!float` tag follow + /// the computed value's own rule again. + #[test] + fn output_follows_the_computed_rule_2902() -> Result<()> { + check(&[ + ("[0.5+0.5] | .[0]", "1"), + ("[0.5+0.5]", "- !!float 1"), + ("[0.5+0.5] | .[0] | type", "!!float"), + ]) + } + + /// yq's JSON encoder keeps a whole computed float's `.0` and switches to + /// scientific notation past its magnitude threshold. The latter rows used + /// to be right only because the bridge's literal spelled them that way; + /// `to_json_yq` now applies the threshold itself. Compared trimmed, like + /// every other `tojson` test here: real yq's encoder ends the string + /// with a newline of its own, which succinctly does not reproduce. + #[test] + fn tojson_keeps_yqs_own_threshold_2902() -> Result<()> { + for (filter, want) in [ + ("[0.5+0.5] | .[0] | tojson", "1.0"), + ("[1e10*2] | .[0] | tojson", "2e+10"), + ("(1e10*2) | tojson", "2e+10"), + ("[1e-5/2] | .[0] | tojson", "5e-06"), + ] { + let (out, code) = run_yq_stdin(filter, "null\n", &[])?; + assert_eq!(code, 0, "`{filter}` exited {code}: {out:?}"); + assert_eq!(out.trim_end(), want, "`{filter}`"); + } + Ok(()) + } + + /// #1134's shapes: every intervening stage that used to re-bake the + /// value before `tostring` could see it. + #[test] + fn any_intervening_stage_is_fine_now_1134() -> Result<()> { + check(&[ + ("[1e10*2] | .[0] | tostring", "2e+10"), + ("[1e6*2] | .[0] | tostring", "2e+06"), + ("(1e10 * 2) | . | tostring", "2e+10"), + ("(1e10 * 2) | (tostring)", "2e+10"), + ("(1e10*2) as $x | $x | tostring", "2e+10"), + ("[1e10] | map(. * 2 | tostring)", "- \"2e+10\""), + ]) + } + + /// A tag-forced document float (`!!float 2`) is the one *document* shape + /// that is also a bare `Float`; it keeps answering the way it did, and + /// its text equality through a container (which used to see the + /// bridge's `2.0`) now agrees with yq too. + #[test] + fn tag_forced_document_float_is_unchanged_2902() -> Result<()> { + for (filter, want) in [ + ("[.a] | .[0] | tostring", "2"), + ("[.a] | .[0]", "2"), + ("[.a] | .[0] | tojson", "2.0"), + ("[.a] | .[0] == 2", "true"), + ] { + let (out, code) = run_yq_stdin(filter, "a: !!float 2\n", &[])?; + assert_eq!(code, 0, "`{filter}` exited {code}: {out:?}"); + assert_eq!(out.trim_end(), want, "`{filter}`"); + } + Ok(()) + } +} + /// #2785: real yq's `==`/`!=` between two scalars compares their *text*, with /// its wildcard matcher applied to the right-hand operand -- not jq's typed /// equality. See `eval::yq_scalar_text_eq`. Every expectation below was @@ -45840,6 +45950,11 @@ mod yq_text_equality_2785 { ("(1 + 1) == \"2\"", "true"), ("(0.5 + 0.5) == 1", "true"), ("(0.5 + 0.5) == \"1\"", "true"), + // #2902: the same computed float read back out of a container + // crosses the reindex bridge, which used to re-spell it `1.0`. + ("[0.5 + 0.5] | .[0] == 1", "true"), + ("[0.5 + 0.5] | .[0] == \"1\"", "true"), + ("[0.5 + 0.5] | .[0] == 1.0", "false"), (".a == (.b | tonumber)", "true"), ]) } From 3497352e3837144f1d45d81599294963435f89bc Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 10:20:41 +1000 Subject: [PATCH 2/3] fix(jq): address /code-review findings on #2902 Three regressions the token exposed, each on a route that had been relying on the bridge re-spelling a computed float: - `@yaml`/`@props` spelled a bare `Float` with plain `Display` under a comment claiming the bridge never yields one (`[1e17*1] | .[0] | @yaml` gave `100000000000000000`); both arms now take yq's computed-float rule. - A path-context pipe over an owned container (`reduce 1 as $x ([1e10*2]; .[0] | [tostring, key])`) hands the bridge's document to the *generic* materializer, whose literal-first chain fell through to `from_document_float` and re-baked the value as `"20000000000.0"`. A `DocumentValue::bridge_computed_float` hook, consulted before `number_literal()`, keeps it bare there and in `tonumber`. - yq's `--input-format json` DOM route printed a whole float as `a: 100` only because the jq-mode bridge collapsed `Float(100.0)` to an integer literal; real yq's JSON decoder types it `!!int`, so #978's canonicalizer (`from_number_literal_plain`) now does the same, with Go's saturating `int64` round-trip as the boundary. Fixes the pre-existing `--eval-all` divergence on the same route and moves `--argjson`'s rendering with it. Also: `reindex_bridge_is_identity` admits every bare `Float` (the sentinels make NaN/infinite ones identities too); `yq_float_fidelity_fixup` removed, now a dead round trip; tests build tokens with the encoder instead of hard-coding spellings; the container `tostring`/`tojson` rendering and the `tojson` trailing-newline divergences the verification surfaced are recorded in the yq limitations doc. --- docs/compliance/yq/limitations.md | 29 +++- src/bin/succinctly/yq_runner.rs | 35 ++-- src/jq/document.rs | 16 ++ src/jq/eval.rs | 35 ++-- src/jq/eval_generic.rs | 255 +++++++++--------------------- src/jq/value.rs | 49 +++++- src/json/light.rs | 26 ++- src/json/validate.rs | 21 +-- tests/jq_cli_tests.rs | 7 + tests/yq_cli_tests.rs | 151 ++++++++++++++---- 10 files changed, 348 insertions(+), 276 deletions(-) diff --git a/docs/compliance/yq/limitations.md b/docs/compliance/yq/limitations.md index ca979958d..f8dd76e66 100644 --- a/docs/compliance/yq/limitations.md +++ b/docs/compliance/yq/limitations.md @@ -4239,6 +4239,26 @@ output is unaffected, since neither appears in JSON for the feature-level gaps (position builtins after DOM conversion; `file_index`/`key`/ `document_index` inside object literals or `any`/`all`). +## `tostring`/`tojson` render a container compactly ([#2902](https://github.com/rust-works/succinctly/issues/2902), found while verifying) + +Real yq's `tostring` on a container is its YAML encoder (block style, `!!float` tags +where the spelling needs one), and its `tojson` is its indented JSON encoder, which ends +every string it produces -- scalar or container -- with a newline of its own. succinctly +renders both compactly and without that trailing newline. Captured live (v4.53.3, `null` +on stdin): + +| filter | real yq | succinctly yq | +|--------------------------|------------------------|---------------| +| `[1.0] \| tostring` | `- 1.0` | `[1.0]` | +| `[0.5+0.5] \| tostring` | `- !!float 1` | `[1.0]` | +| `[1] \| tojson` | `[\n 1\n]\n` + `\n` | `[1]` | +| `"x" \| tojson` | `"x"\n` + `\n` | `"x"` | +| `(0.5+0.5) \| tojson` | `1.0\n` + `\n` | `1.0` | + +Scalar `tostring` agrees in both (the #2902 fix above); the container spellings and the +trailing newline are pre-existing and not attempted there. The `tojson` tests in +`tests/yq_cli_tests.rs` compare trimmed output for this reason. + ## Evaluator resource caps apply in yq mode too, and are uncatchable (#2132) The five caps `succinctly jq` documents -- `MAX_RANGE`, `WHILE_UNTIL_MAX_STEPS`, @@ -4365,9 +4385,12 @@ Until #2052 the flag validated through `serde_json::Value` and materialized thro (see the jq-mode limitations doc for why) and moved yq's materialization onto the `JsonIndex` + `to_owned_canonicalizing_numbers_at_depth` pair its `--input-format json` path already uses. #978's convention is intact -- `--argjson` still discards a literal's source -spelling (`1.500` is `1.5`, `1.0` is `1.0`, `0099999999999999999999999` is `1e+23`) and -still does not preserve it the way `succinctly jq`'s own `--argjson` does (#1058 was -deliberately jq-mode-only). +spelling (`1.500` is `1.5`, `0099999999999999999999999` is `1e+23`) and still does not +preserve it the way `succinctly jq`'s own `--argjson` does (#1058 was deliberately +jq-mode-only). Since [#2902](https://github.com/rust-works/succinctly/issues/2902) that same +canonicalizer types a whole-valued float as an int, as real yq's own JSON decoder does +(`1.0` is `!!int` under `-p json`), so `--argjson x 1.0` and `1.e5` render as `1` and +`100000` where they rendered `1.0` and `100000.0` before. Two renderings do move, both because `serde_json` is no longer the one producing them (review of #2880 -- the PR text originally claimed nothing moved): diff --git a/src/bin/succinctly/yq_runner.rs b/src/bin/succinctly/yq_runner.rs index 57645fd63..14838fe25 100644 --- a/src/bin/succinctly/yq_runner.rs +++ b/src/bin/succinctly/yq_runner.rs @@ -1686,21 +1686,26 @@ fn stream_yaml_sort_keys_alias_fallback( /// RFC-8259 "null" substitution, wrong for this purely-internal round /// trip), matching `eval_owned_input`'s identical reindex bridge for /// `reduce`/`foreach` (#561, #472). -/// - Only the *fallback* arm — a plain, already-literal-less `Float` — is -/// `S`-gated, and `YqSemantics` there is actively wrong for this call -/// site specifically: `parse_input`'s `--input-format json` path already -/// collapses every number straight to a plain `Float`/`Int` via -/// `to_owned_canonicalizing_numbers` (#978, matching real yq's "a -/// JSON-sourced number never keeps its own spelling" convention) *before* -/// the value ever reaches here — forcing `YqSemantics`'s decimal point back onto -/// that already-canonicalized value reintroduced exactly the bug #978 -/// fixed (`--slurp --input-format json '.'` on `{"a":1e2}` regressed from -/// `[{"a":100}]` to `[{"a":100.0}]`, caught by CI). `JqSemantics`'s bare -/// fallback (no forced point) is correct for both the JSON-canonicalized -/// case and the untouched-overflow-scalar case (the latter is already -/// lossy through this whole-document round trip regardless of the point — -/// confirmed live, real yq keeps an untouched i64-overflow scalar -/// byte-for-byte via `-i`, e.g. `99999999999999999999` verbatim, which +/// - The *fallback* arm -- a plain, already-literal-less `Float` -- used to +/// be `S`-gated, and `YqSemantics` there was actively wrong for this call +/// site: `parse_input`'s `--input-format json` path collapses every +/// number to a plain `Int`/`Float` via `to_owned_canonicalizing_numbers` +/// (#978, matching real yq's "a JSON-sourced number never keeps its own +/// spelling" convention) *before* the value reaches here, and forcing +/// yq's decimal point back onto it reintroduced the bug #978 fixed +/// (`--slurp --input-format json '.'` on `{"a":1e2}` regressed from +/// `[{"a":100}]` to `[{"a":100.0}]`, caught by CI). Since #2902 the +/// bridge writes a bare `Float` as a mode-independent token that +/// reparses to the same bare `Float`, so `S` no longer changes the +/// round trip at all; `JqSemantics` stays only because nothing here +/// evaluates under it. That same change removed the accident this route +/// used to rely on -- jq mode spelled a whole `Float(100.0)` as `100`, +/// so it came back an *integer* literal and printed `a: 100` like real +/// yq -- which is why `from_number_literal_plain` (the #978 canonicalizer) +/// now types a whole-valued JSON float as `Int` itself, as yq's own +/// decoder does. An untouched i64-overflow scalar is already lossy +/// through this whole-document round trip regardless (confirmed live: +/// real yq keeps `99999999999999999999` byte-for-byte via `-i`, which /// this reindex-through-`f64` architecture cannot match either way). fn evaluate_input( input: &OwnedValue, diff --git a/src/jq/document.rs b/src/jq/document.rs index be50878e8..c3dea65d1 100644 --- a/src/jq/document.rs +++ b/src/jq/document.rs @@ -970,6 +970,22 @@ pub trait DocumentValue: Sized + Clone { None } + /// The value this number token carries if it is the reindex bridge's + /// computed-float token (`crate::json::validate::computed_float_token`, + /// #2902), `None` for every other value. + /// + /// A materializer must consult this *before* [`number_literal`](Self::number_literal) + /// / [`as_i64`](Self::as_i64) / [`as_f64`](Self::as_f64): the token + /// deliberately fails the first two and decodes through the third, so a + /// chain that falls through to `as_f64` and then records document + /// provenance (`OwnedValue::from_document_float`) would re-bake the + /// computed value into a decimal literal past yq's threshold -- the very + /// re-spelling the token exists to prevent. Only a JSON document can + /// hold one (the bridge re-indexes as JSON), so the default is `None`. + fn bridge_computed_float(&self) -> Option { + None + } + /// Try to get as a string. fn as_str(&self) -> Option>; diff --git a/src/jq/eval.rs b/src/jq/eval.rs index 587aeb4db..e0ba09143 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -15397,17 +15397,14 @@ fn props_value_to_string(value: &OwnedValue) -> String { OwnedValue::Float(f) if f.is_nan() || f.is_infinite() => { nonfinite_display_string::(*f).to_string() } - // Required for match exhaustiveness over the `Float` variant, but - // not reachable through any real filter (#1064, verified via - // `cargo llvm-cov` and a direct entry-point probe, not assumed): - // `@props` only ever sees a `Float` here via `eval_format`'s - // `to_owned_lossy(&value)`, which always round-trips a *finite* computed - // number through `from_number_bytes` -- and that always - // reconstructs a `NumberLiteral`, never a bare `Float`, for any - // valid RFC-8259 number text. Only a NaN/Infinity sentinel - // (guarded by the arm above, not this one) skips that - // reconstruction and arrives here as a bare `Float`. - OwnedValue::Float(f) => format!("{f}"), + // A computed float, spelled by yq's own computed-float rule + // (shortest form, scientific past yq's threshold): `[1e-5/2] | .[0] + // | @props` is `5e-06` in real yq. Reachable since #2902 -- the + // reindex bridge now hands a computed float back as a bare `Float` + // where it used to bake one into a `NumberLiteral` (which is why an + // earlier version of this arm documented itself as unreachable and + // spelled the value with plain `Display`, #1064). + OwnedValue::Float(f) => crate::yaml::format_float_yq_yaml(*f), OwnedValue::NumberLiteral(NumberRepr::Float(f), _) if f.is_nan() || f.is_infinite() => { nonfinite_display_string::(*f).to_string() } @@ -15446,17 +15443,11 @@ fn owned_to_yaml_at_depth(value: &OwnedValue, depth: usize) -> String { OwnedValue::Float(f) if f.is_nan() || f.is_infinite() => { nonfinite_display_string::(*f).to_string() } - // Required for match exhaustiveness over the `Float` variant, but - // not reachable through any real filter (#1064, verified via - // `cargo llvm-cov` and a direct entry-point probe, not assumed): - // `@yaml` only ever sees a `Float` here via `eval_format`'s - // `to_owned_lossy(&value)`, which always round-trips a *finite* computed - // number through `from_number_bytes` -- and that always - // reconstructs a `NumberLiteral`, never a bare `Float`, for any - // valid RFC-8259 number text. Only a NaN/Infinity sentinel - // (guarded by the arm above, not this one) skips that - // reconstruction and arrives here as a bare `Float`. - OwnedValue::Float(f) => format!("{f}"), + // A computed float, spelled by yq's own computed-float rule + // (shortest form, scientific past yq's threshold): `[1e17*1] | .[0] + // | @yaml` is `1e+17` in real yq. Reachable since #2902 -- see the + // identical arm in `props_value_to_string` above. + OwnedValue::Float(f) => crate::yaml::format_float_yq_yaml(*f), OwnedValue::NumberLiteral(NumberRepr::Float(f), _) if f.is_nan() || f.is_infinite() => { nonfinite_display_string::(*f).to_string() } diff --git a/src/jq/eval_generic.rs b/src/jq/eval_generic.rs index afcb9de91..7965b717c 100644 --- a/src/jq/eval_generic.rs +++ b/src/jq/eval_generic.rs @@ -281,6 +281,13 @@ fn to_owned_checked_at_depth( Ok(OwnedValue::Null) } else if let Some(b) = value.as_bool() { Ok(OwnedValue::Bool(b)) + } else if let Some(f) = value.bridge_computed_float() { + // #2902: a computed float re-indexed by the bridge (a path-context + // pipe over an owned container reaches this materializer with the + // bridge's own document) -- bare, never a literal, and never + // `from_document_float` below, which would re-bake it past yq's + // threshold; see `DocumentValue::bridge_computed_float`. + Ok(OwnedValue::Float(f)) } else if let Some(literal) = value.number_literal() { Ok(OwnedValue::from_number_literal(&literal)) } else if let Some(i) = value.as_i64() { @@ -289,8 +296,8 @@ fn to_owned_checked_at_depth( // #2438: a document scalar that got this far has no preservable // literal left (`number_literal()` answered `None` just above), so // this is the boundary where its provenance is still known -- see - // `OwnedValue::from_document_float`. Unreachable for JSON input, - // whose `number_literal()` override is unconditional. + // `OwnedValue::from_document_float`. For JSON input this is reached + // only by a lenient span `number_literal()` declines (#966). Ok(OwnedValue::from_document_float(f)) } else if let Some(s) = value.as_str() { Ok(OwnedValue::String(s.into_owned())) @@ -517,6 +524,13 @@ fn to_owned_at_depth( Ok(OwnedValue::Null) } else if let Some(b) = value.as_bool() { Ok(OwnedValue::Bool(b)) + } else if let Some(f) = value.bridge_computed_float() { + // #2902: a computed float re-indexed by the bridge (a path-context + // pipe over an owned container reaches this materializer with the + // bridge's own document) -- bare, never a literal, and never + // `from_document_float` below, which would re-bake it past yq's + // threshold; see `DocumentValue::bridge_computed_float`. + Ok(OwnedValue::Float(f)) } else if let Some(literal) = value.number_literal() { Ok(OwnedValue::from_number_literal(&literal)) } else if let Some(i) = value.as_i64() { @@ -525,8 +539,8 @@ fn to_owned_at_depth( // #2438: a document scalar that got this far has no preservable // literal left (`number_literal()` answered `None` just above), so // this is the boundary where its provenance is still known -- see - // `OwnedValue::from_document_float`. Unreachable for JSON input, - // whose `number_literal()` override is unconditional. + // `OwnedValue::from_document_float`. For JSON input this is reached + // only by a lenient span `number_literal()` declines (#966). Ok(OwnedValue::from_document_float(f)) } else if let Some(s) = value.as_str() { Ok(OwnedValue::String(s.into_owned())) @@ -2814,19 +2828,17 @@ const REINDEX_LITERAL_LEN_CAP: usize = 256; /// - A **NaN** `NumberLiteral` is replaced by `NAN_SENTINEL`. /// - A `NumberLiteral` whose source text exceeds /// [`REINDEX_LITERAL_LEN_CAP`] is discarded (#1211). -/// - A non-finite bare `Float` goes through a sentinel/overflow literal; -/// it comes back as the same value, but this predicate stays out of that -/// corner rather than reason about it. -/// -/// A bare finite **`Float`** used to head this list: the formatter re-spelled -/// it by a mode-forked rule (#953) and it came back as a `NumberLiteral` -/// carrying that new text, which was the case the bridge was genuinely -/// load-bearing for (without the guard, `.outer.big | parent` on +/// +/// A bare **`Float`** used to head this list: the formatter re-spelled a +/// finite one by a mode-forked rule (#953) and it came back as a +/// `NumberLiteral` carrying that new text, which was the case the bridge was +/// genuinely load-bearing for (without the guard, `.outer.big | parent` on /// `10000000000000000000.0` printed `1e+19` in yq mode). Since #2902 the -/// bridge writes a bare finite `Float` as a token +/// bridge writes a finite `Float` as a token /// (`crate::json::validate::computed_float_token`) that reparses to the same -/// bare `Float`, so the round trip is an identity on it too and the bypass -/// applies. +/// bare `Float`, and a NaN/infinite one was always written as a sentinel +/// `from_number_bytes` decodes back to the same bare `Float`, so the round +/// trip is an identity on every bare `Float` and the bypass applies. /// /// Everything else -- `null`, booleans, strings (escaped and unescaped /// symmetrically), object keys, and the overwhelmingly common @@ -2852,8 +2864,9 @@ const REINDEX_LITERAL_LEN_CAP: usize = 256; /// produces plain `Int`/`Float`. /// /// Deliberately an **input-side** predicate rather than an output-side fixup. -/// A first version of this fix re-applied `yq_float_fidelity_fixup` to the -/// *result* instead; code review showed that isn't the same transformation at +/// A first version of this fix re-applied `yq_float_fidelity_fixup` (the +/// output-side re-spelling pass #2902 has since removed) to the *result* +/// instead; code review showed that isn't the same transformation at /// all, and got it wrong in both directions -- re-spell-then-evaluate is not /// evaluate-then-re-spell once the pipe does any computing. /// `.outer.big|parent|.big|tostring` lost the document spelling @@ -2862,8 +2875,7 @@ const REINDEX_LITERAL_LEN_CAP: usize = 256; /// alone can safely skip the bridge. pub(crate) fn reindex_bridge_is_identity(value: &OwnedValue) -> bool { match value { - OwnedValue::Float(f) => f.is_finite(), - OwnedValue::Int(_) => true, + OwnedValue::Float(_) | OwnedValue::Int(_) => true, OwnedValue::NumberLiteral(NumberRepr::Float(f), _) if f.is_nan() => false, OwnedValue::NumberLiteral(_, literal) => literal.len() <= REINDEX_LITERAL_LEN_CAP, OwnedValue::Array(items) => items.iter().all(reindex_bridge_is_identity), @@ -2872,111 +2884,6 @@ pub(crate) fn reindex_bridge_is_identity(value: &OwnedValue) -> bool { } } -/// Re-derives every `Float`'s spelling in `values` through -/// [`eval_on_owned`]'s reindex bridge (`to_json_for_reindex`'s `S`-gated -/// formatter, #953), without touching anything else in the input document. -/// -/// `Expr::Array`/`Expr::Comma`'s own native `eval_single` arms (#1168) build -/// `values` straight from `to_owned`/`to_owned_cursor` -- or, for a builtin -/// with its own native construction (`to_entries`, ...), from whatever *that* -/// builtin's arm produced, which uses the identical `to_owned`/`to_owned_cursor` -/// conversion internally. Neither has any notion of "this bare `Float` came -/// from a document-sourced literal that overflowed `i64`, keep its decimal -/// point regardless of magnitude" -- only `to_json_for_reindex`'s own -/// `S`-gated fallback applies that rule (see its doc comment, -/// `src/jq/value.rs`), and *only* because, before `Expr::Array`/`Expr::Comma` -/// had native arms, `[...]`/`,` had no choice but to fall through to this -/// same bridge for lack of one. Adding native arms without also keeping this -/// fix regressed #953 for a direct cursor result (`[.a]`, caught by its own -/// regression test) *and*, less obviously, for a value one layer removed -/// through a builtin's own construction (`[to_entries]` on an overflow -/// field, caught in code review -- `Builtin::ToEntries` also reads the field -/// via `to_owned_cursor`, just wrapped in an entry object before `Expr::Array` -/// ever sees it, so scoping the fixup to only direct `GenericResult:: -/// OneCursor`/`ManyCursor` results missed this case entirely). -/// -/// This is why the fixup applies unconditionally to the *whole* constructed -/// result rather than trying to track, per value, whether it's document- -/// sourced or genuinely computed (e.g. `1e10 * 2`) -- that distinction isn't -/// recoverable from a `GenericResult` variant once a value has passed through -/// even one further construction step (`to_entries`'s object wrapping looks -/// identical, from here, to freshly computed arithmetic). -/// -/// #1168 accepted a gap as the price of that: a genuinely computed float -/// wrapped directly in `[...]`/`,` (`[1e10 * 2]`) also got its decimal point -/// forced, where real yq keeps scientific notation. #2438 closed it by -/// tagging the provenance at `OwnedValue`'s own construction site after all -/// -- [`OwnedValue::from_document_float`], applied at the document boundary -/// (`to_owned_at_depth`, `ResolvedScalar::to_owned_value`) rather than -/// reconstructed here -- which let `to_json_for_reindex`'s yq fallback -/// switch to yq's own magnitude threshold (`format_float_yq`). This function -/// is unchanged by that and still round-trips the whole result; what changed -/// is that a document-sourced float now arrives already carrying its -/// spelling, so the round trip preserves it as a literal instead of having -/// to synthesize one for everything. `[1e10 * 2]` keeps scientific notation -/// as a result. #2902 then closed the ordinary-magnitude `join` gap -/// (#1124/#1144, `[2.0/2]`) from the other side: the bridge now writes a -/// bare `Float` as a token it hands back as a bare `Float`, so this round -/// trip no longer bakes a computed float into a literal at any magnitude. -/// -/// Round-tripping just `values` (not the whole input document, unlike the -/// old wildcard fallback these two arms replace) keeps `Expr::Array`'s and -/// `Expr::Comma`'s actual fix — duplicate mapping keys survive a builtin's -/// own cursor-native conversion (`to_entries`, etc.) intact. Safe with -/// respect to that fix: nothing in `values` still has a duplicate *mapping -/// key* left to lose by round-tripping again — any genuine YAML duplicate -/// was already collapsed the moment `to_owned`/`to_owned_cursor` first -/// converted its mapping to an `IndexMap`-backed `Object`, before this ever -/// runs; a builtin with its own dedup-preserving fix has already turned its -/// duplicates into distinct array elements by this point instead, which -/// round-trip through JSON text with no collision to lose. -/// -/// A no-op in jq mode (`to_json_for_reindex`'s own `S`-gate already makes -/// the round trip itself a no-op there — jq drops a computed float's -/// literal formatting unconditionally), and a no-op whenever `values` has no -/// `Float`/`NumberLiteral(Float, _)` anywhere in its tree (`contains_float`) -/// — skipped outright rather than paying for a round trip that has nothing -/// to fix, which is the common case (`[.a, .b, .c]` over strings/objects/ -/// plain ints). `Ok` carries the fixed-up values back for the caller to -/// package (`Expr::Array` always wraps them in one `OwnedValue::Array`; -/// `Expr::Comma` collapses via [`owned_vec_to_generic_result`]); `Err` -/// carries an already-terminal `GenericResult` (an `Error`, in practice — -/// see [`eval_on_owned`]'s own doc comment for why this is defense-in-depth -/// rather than a reachable path for internally-constructed input) for the -/// caller to return as-is. -fn yq_float_fidelity_fixup( - values: Vec, -) -> Result, GenericResult> { - if S::TAG != EvalTag::Yq || values.is_empty() || !values.iter().any(contains_float) { - return Ok(values); - } - match eval_on_owned::(&Expr::Identity, OwnedValue::Array(values), false) { - GenericResult::Owned(OwnedValue::Array(fixed)) => Ok(fixed), - GenericResult::Error(e) => Err(GenericResult::Error(e)), - other => Err(other), - } -} - -/// Whether `value`'s tree contains a `Float`/`NumberLiteral(Float, _)` -/// anywhere -- the only shapes [`yq_float_fidelity_fixup`]'s round trip can -/// possibly change (see `to_json_for_reindex`'s own `S`-gated fallback, -/// which is scoped identically). A cheap pre-check so a document with no -/// float anywhere in the wrapped result skips the round trip entirely, -/// rather than paying for one that has nothing to do. -fn contains_float(value: &OwnedValue) -> bool { - match value { - OwnedValue::Float(_) => true, - OwnedValue::NumberLiteral(NumberRepr::Float(_), _) => true, - OwnedValue::Array(items) => items.iter().any(contains_float), - OwnedValue::Object(fields) => fields.values().any(contains_float), - OwnedValue::Null - | OwnedValue::Bool(_) - | OwnedValue::Int(_) - | OwnedValue::NumberLiteral(NumberRepr::Int(_), _) - | OwnedValue::String(_) => false, - } -} - /// Normalize a prefix and its terminator into a `GenericResult` (#400, #494). /// Mirrors [`super::eval::partial`] (the `QueryResult` equivalent) — an empty /// prefix collapses to the bare `Error`/`Break` variant. @@ -6566,17 +6473,14 @@ fn each_lazy_array_iterate_sink( /// #2543: when `remaining` (the stages still to apply to an already-computed /// `o`) is exactly one `Expr::Format`/`Builtin::ToString`, routes through /// `eval_on_owned` directly instead of the caller's own `Expr::Pipe`-wrapped -/// `eval_each_owned` fallback. `eval_on_owned` (this file) already -/// special-cases exactly these two shapes immediately after a computed value -/// to avoid rebaking its scientific-notation spelling as a -/// document-sourced-looking literal on the JSON round-trip below it (#1054); -/// the wrapped route missed both -- `eval_owned_fast_path` (`eval.rs`) has no -/// `Expr::Format` arm at all, and (before also being fixed there) didn't -/// recognize a `Builtin::ToString` wrapped in a trivial `Expr::Pipe` as the -/// same shape its own bare-`Builtin` arm matches. Confirmed live: -/// `succinctly jq -n '(2 * 1e16) | @json'` gave `"2E+16"` (jq's -/// literal-reformat convention) where real jq and every non-streaming -/// succinctly path give `"2e+16"` (the computed-value convention). +/// `eval_each_owned` fallback, reaching `eval_on_owned`'s own bypass for +/// exactly these two shapes. That bypass was load-bearing for correctness +/// when it was added (#1054/#2543: the JSON round trip below it re-baked a +/// computed float's spelling as a document-sourced-looking literal, so +/// `succinctly jq -n '(2 * 1e16) | @json'` gave `"2E+16"` for real jq's +/// `"2e+16"`); since #2902 the round trip hands a computed float back as a +/// bare `Float`, so for every value [`reindex_bridge_is_identity`] admits +/// the two routes agree and this is a speed-only short cut. /// /// Shared by [`fold_pipe_stages_sink`] and `continue_pipe_element_generic` /// so the allowed-shapes list and its short-circuit-safety rationale live in @@ -7929,16 +7833,16 @@ fn eval_single( unreachable!("materialize_lazy() already normalized every lazy variant") } }; - // Fixed up as one whole unit, not per-source (#953/#1168, see - // `yq_float_fidelity_fixup`'s own doc comment for why -- in - // short, a builtin's own construction around a document value - // (`to_entries`, ...) is indistinguishable here from a genuinely - // computed one, so the fixup can't be scoped any narrower than - // this without missing that case). - match yq_float_fidelity_fixup::(items) { - Ok(fixed) => GenericResult::Owned(OwnedValue::Array(fixed)), - Err(result) => result, - } + // No float re-spelling pass over `items` any more: #1168's + // `yq_float_fidelity_fixup` pushed every constructed result back + // through the reindex bridge so its yq-mode formatter would give + // a document-sourced overflow float its decimal spelling (#953), + // and #2438 then recorded that provenance at the document + // boundary instead (`OwnedValue::from_document_float`). Once + // #2902 made the bridge an identity on a bare `Float`, the round + // trip could no longer change anything it was meant to, and was + // removed. + GenericResult::Owned(OwnedValue::Array(items)) } // Comma: evaluate each operand in source order against the ambient @@ -7954,11 +7858,6 @@ fn eval_single( // `eval::eval_comma` defers to `eval::eval_try`. Handled natively so // a cursor-native builtin's fix doesn't lose it to the wildcard // fallback just for being joined with `,` either (#1168). - // - // `yq_float_fidelity_fixup` runs once over the whole collected `out` - // after the loop, not per-sibling -- same reasoning as `Expr::Array` - // above, and cheaper (one round trip for `.a, .b, .c` instead of up - // to three). // #2416 phase 3: `if` through the sink evaluator's own arm // (`each_if_generic`), which evaluates the condition and the taken // branch with the cursor, instead of the eager bridge -- so @@ -7986,13 +7885,10 @@ fn eval_single( &mut acc, &mut objects, ); - // Deliberately *not* passed through `yq_float_fidelity_fixup`: the - // bridge this arm replaces returned the eager evaluator's objects - // as built, leaving a computed float bare for the YAML emitter's - // nested-float rule (`{"a": (.a * 1e100)}` prints `a: 1e+100`, - // as real yq does). The fixup's reindex round trip spells that - // same float out in full, which is the `Expr::Array` arm's own - // pre-existing divergence (`[.a * 1e100]`), not one to inherit. + // Objects are returned as built, leaving a computed float bare + // for the YAML emitter's nested-float rule (`{"a": (.a * 1e100)}` + // prints `a: 1e+100`, as real yq does) -- since #2902 the + // `Expr::Array`/`Expr::Comma` arms above do the same. match built { Ok(()) => owned_vec_to_generic_result(objects), Err(ObjectEscapeGeneric::Suppressed) => GenericResult::None, @@ -8057,16 +7953,10 @@ fn eval_single( for expr in exprs { let result = eval_single::(expr, value.clone(), optional, cursor); if let Some(control) = push_generic_owned_values(result, &mut out) { - return match yq_float_fidelity_fixup::(out) { - Ok(fixed) => partial_generic(fixed, control), - Err(result) => result, - }; + return partial_generic(out, control); } } - match yq_float_fidelity_fixup::(out) { - Ok(fixed) => owned_vec_to_generic_result(fixed), - Err(result) => result, - } + owned_vec_to_generic_result(out) } // Fall back to the full evaluator for complex expressions @@ -14761,10 +14651,10 @@ fn eval_has_one_key( /// /// The generic twin of `eval::eval_array_construction`'s use inside /// `builtin_min_by`/`sort_by`/`group_by`/`unique_by`. It deliberately does -/// *not* route through this file's own `Expr::Array` arm, which additionally -/// applies `yq_float_fidelity_fixup` -- that fixup exists to make a computed -/// float *print* the way real yq prints it, and running it here would let an -/// output-formatting rule change which element sorts first. +/// *not* route through this file's own `Expr::Array` arm: until #2902 that +/// arm additionally re-spelled every computed float through the reindex +/// bridge (`yq_float_fidelity_fixup`, an output-formatting rule), and +/// running it here would have let it change which element sorts first. /// /// Atomic, matching `eval_array_construction`: any control from `f` -- error, /// break, halt, or the trailing control of a `Partial` -- discards the prefix @@ -21020,8 +20910,12 @@ fn eval_builtin( Builtin::ToNumber => { // Already a number: a passthrough, not a computation, so (like - // `.`) it keeps the source literal. - if let Some(literal) = value.number_literal() { + // `.`) it keeps the source literal -- or, for the reindex + // bridge's computed-float token, stays the bare `Float` it was + // (#2902, same reasoning as `to_owned_at_depth`'s own arm). + if let Some(f) = value.bridge_computed_float() { + GenericResult::Owned(OwnedValue::Float(f)) + } else if let Some(literal) = value.number_literal() { GenericResult::Owned(OwnedValue::from_number_literal(&literal)) } else if let Some(i) = value.as_i64() { GenericResult::Owned(OwnedValue::Int(i)) @@ -25006,15 +24900,20 @@ mod tests { // The shape the guard used to exist for: a bare `Float`, which #953's // mode-forked re-spelling rewrote into a `NumberLiteral`. Since #2902 - // the bridge writes it as a token that reparses to the same bare - // `Float`, so it is an identity in both modes and the predicate admits - // it -- asserted in both directions so a formatter regression that - // starts re-spelling it again fails here, not in a user's `tostring`. + // the bridge writes a finite one as a token that reparses to the same + // bare `Float` (a non-finite one always went through a sentinel that + // does the same), so it is an identity in both modes and the + // predicate admits it -- asserted in both directions so a formatter + // regression that starts re-spelling it again fails here, not in a + // user's `tostring`. for computed in [ OwnedValue::Float(1e19), OwnedValue::Float(1.0), OwnedValue::Float(-0.0), OwnedValue::Float(5e-6), + OwnedValue::Float(f64::NAN), + OwnedValue::Float(f64::INFINITY), + OwnedValue::Float(f64::NEG_INFINITY), ] { assert!( super::reindex_bridge_is_identity(&computed), @@ -25029,12 +24928,6 @@ mod tests { "sanity: the jq-mode bridge hands back {computed:?} unchanged" ); } - assert!(!super::reindex_bridge_is_identity(&OwnedValue::Float( - f64::NAN - ))); - assert!(!super::reindex_bridge_is_identity(&OwnedValue::Float( - f64::INFINITY - ))); } /// The one thing [`reindex_bridge_is_identity`]'s `Int` arm rests on, and diff --git a/src/jq/value.rs b/src/jq/value.rs index 25b345d05..6227925fd 100644 --- a/src/jq/value.rs +++ b/src/jq/value.rs @@ -1781,8 +1781,25 @@ impl OwnedValue { /// never wants the source spelling in the first place shouldn't have /// to pay, and the reason this exists as its own function rather than /// that two-call sequence. + /// + /// A whole-valued float becomes an `Int` (#2902): real yq's JSON decoder + /// types `1.0`, `1e2`, `-0.0` and `20000000000.0` as `!!int` and prints + /// them `1`, `100`, `0`, `20000000000` -- as does a decimal too big for + /// `i64`'s exact range but whose saturated conversion still round-trips + /// (`9223372036854775808.0` is `9223372036854775807`, `!!int`), while + /// `1e19`, `1e300` and `2.5` stay `!!float`; all captured live from yq + /// v4.53.3 with `-p json`. The rule is Go's `f == float64(int64(f))` + /// with a saturating cast, which Rust's `as` also is. Until #2902 the + /// `--slurp`/`--inplace` DOM route got the same rendering by accident: + /// the reindex bridge spelled a bare whole `Float` as `100` in jq mode + /// and reparsed it as an integer literal. pub fn from_number_literal_plain(literal: &str) -> Self { - Self::plain_number_from_repr(parse_i64_or_f64(literal)) + match parse_i64_or_f64(literal) { + Some(NumberRepr::Float(f)) if f.is_finite() && (f as i64) as f64 == f => { + Self::Int(f as i64) + } + repr => Self::plain_number_from_repr(repr), + } } /// The shared "parsed repr -> plain scalar" mapping @@ -3586,6 +3603,28 @@ mod tests { assert!(NAN_SENTINEL.parse::().is_err()); } + /// #2902: yq's JSON decoder types a whole-valued float as an int, with + /// Go's saturating `int64` round-trip deciding the boundary -- every + /// row captured from yq v4.53.3 with `-p json` (`.x | type` / `.x`). + #[test] + fn test_from_number_literal_plain_types_a_whole_json_float_as_int_2902() { + for (literal, want) in [ + ("1.0", OwnedValue::Int(1)), + ("1e2", OwnedValue::Int(100)), + ("-0.0", OwnedValue::Int(0)), + ("20000000000.0", OwnedValue::Int(20_000_000_000)), + ("9223372036854775808.0", OwnedValue::Int(i64::MAX)), + ("100000000000000000000", OwnedValue::Float(1e20)), + ("1e19", OwnedValue::Float(1e19)), + ("1e300", OwnedValue::Float(1e300)), + ("2.5", OwnedValue::Float(2.5)), + ("7", OwnedValue::Int(7)), + ] { + let got = OwnedValue::from_number_literal_plain(literal); + assert_eq!(format!("{got:?}"), format!("{want:?}"), "{literal}"); + } + } + /// #2902: the bridge's computed-float token materializes as a bare /// `Float`, never as a `NumberLiteral` carrying the token's text -- /// through the same public entry point real document numbers go @@ -3643,12 +3682,8 @@ mod tests { /// what let `[0.5+0.5] | .[0] | tostring` come back as the literal `1.0`. #[test] fn test_to_json_for_reindex_writes_a_computed_float_as_the_token_2902() { - for (f, token) in [ - (1e20, "1e20e0"), - (2.0, "2e0e0"), - (1e10, "1e10e0"), - (-0.5, "-5e-1e0"), - ] { + for f in [1e20, 2.0, 1e10, -0.5] { + let token = crate::json::validate::computed_float_token(f); assert_eq!( OwnedValue::Float(f).to_json_for_reindex::(), token diff --git a/src/json/light.rs b/src/json/light.rs index 57ee510c2..5735fe35b 100644 --- a/src/json/light.rs +++ b/src/json/light.rs @@ -2712,6 +2712,15 @@ impl<'a, W: AsRef<[u64]> + Clone> DocumentValue for StandardJson<'a, W> { } } + fn bridge_computed_float(&self) -> Option { + match self { + StandardJson::Number(n) => { + crate::json::validate::parse_computed_float_token(n.raw_bytes()) + } + _ => None, + } + } + fn number_literal(&self) -> Option> { match self { StandardJson::Number(n) => { @@ -6247,10 +6256,23 @@ mod tests { /// which the scanner captures whole and `as_f64` decodes, while /// `as_i64` and `number_literal()` both refuse it -- so a materializer /// that tries the literal first (`to_owned_at_depth`) still ends on a - /// bare `Float`, never a `NumberLiteral` carrying the token's text. + /// bare `Float`, never a `NumberLiteral` carrying the token's text. The + /// tokens come from the encoder itself: this pins the plumbing, not the + /// spelling (`validate.rs`'s own tests pin that). #[test] fn json_number_decodes_the_computed_float_token_2902() { - for (f, token) in [(1.0, "1e0e0"), (2e16, "2e16e0"), (-0.5, "-5e-1e0")] { + for f in [ + 1.0, + 2e16, + -0.5, + 0.0, + -0.0, + 5e-324, + f64::MAX, + 1e300, + 0.1 + 0.2, + ] { + let token = crate::json::validate::computed_float_token(f); let json = format!("[{token}]"); let bytes = json.as_bytes(); assert_eq!(nested_number_span(bytes, 1), 1 + token.len()); diff --git a/src/json/validate.rs b/src/json/validate.rs index 22eb80329..0e60b5ccb 100644 --- a/src/json/validate.rs +++ b/src/json/validate.rs @@ -1095,9 +1095,7 @@ pub(crate) fn computed_float_token(f: f64) -> String { f.is_finite(), "computed_float_token requires a finite value; NaN/Infinity use their own sentinels" ); - let mut token = alloc::format!("{f:e}"); - token.push_str(COMPUTED_FLOAT_TOKEN_SUFFIX); - token + alloc::format!("{f:e}{COMPUTED_FLOAT_TOKEN_SUFFIX}") } /// Decodes a [`computed_float_token`], `None` for anything else -- the one @@ -1153,20 +1151,11 @@ mod tests { assert!(token.parse::().is_err(), "{token}"); assert!(token.parse::().is_err(), "{token}"); assert!(!is_valid_number(token.as_bytes()), "{token}"); - assert!( - token - .bytes() - .next() - .is_some_and(|b| b.is_ascii_digit() || b == b'-'), - "{token}" - ); - assert!( - token - .bytes() - .all(|b| b.is_ascii_digit() || matches!(b, b'.' | b'e' | b'E' | b'+' | b'-')), - "{token}" - ); } + // That the semi-index scanner captures the token whole is pinned + // against the scanner itself, in `json/light.rs` + // (`json_number_decodes_the_computed_float_token_2902`), rather than + // by re-spelling its character class here. assert_eq!(computed_float_token(1.0), "1e0e0"); assert_eq!(computed_float_token(2e16), "2e16e0"); assert_eq!(computed_float_token(-0.5), "-5e-1e0"); diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index ca9b926b4..7eb6e2714 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -32848,6 +32848,13 @@ fn test_computed_float_through_container_keeps_computed_spelling_2902() -> Resul ("[2*1e17] | .[0] | tojson", r#""2e+17""#), ("[1e-5/2] | .[0] | tojson", r#""5e-06""#), ("[1e10] | map(. * 2 | tostring)", r#"["20000000000"]"#), + // The path-context route (`path(.)` needs it) hands the bridge's + // document to the generic materializer, which must keep the token + // bare too (found in review). + ( + "reduce 1 as $x ([2*1e16]; .[0] | [tostring, path(.)])", + r#"["2e+16",[]]"#, + ), ("[0.5+0.5] | .[0] | tostring", r#""1""#), ("[1.0] | .[0] | tostring", r#""1.0""#), ("[1e2] | .[0] | tostring", r#""1E+2""#), diff --git a/tests/yq_cli_tests.rs b/tests/yq_cli_tests.rs index 597813d21..106308de2 100644 --- a/tests/yq_cli_tests.rs +++ b/tests/yq_cli_tests.rs @@ -10006,13 +10006,17 @@ fn test_argjson_materialization_is_unchanged_by_2052() -> Result<()> { // chain and now from the validator's own accept-set. ("007", "7"), (".5", "0.5"), - ("1.e5", "100000.0"), + ("1.e5", "100000"), ("[007,.5]", "[7,0.5]"), - // #978's spelling-discarding convention, unchanged (`-o=json` - // renders a float with its own `.0` where YAML output does not, so - // these are the JSON spellings, captured from the pre-#2052 binary). + // #978's spelling-discarding convention (`-o=json` renders a float + // with its own `.0` where YAML output does not, so these are the + // JSON spellings, captured from the pre-#2052 binary) -- with one + // later change: since #2902 the same canonicalizer types a + // whole-valued float as an int, the way real yq's own JSON decoder + // does (`1.0` is `!!int` under `-p json`), so `1.e5` and `1.0` + // render without the point. ("1.500", "1.5"), - ("1.0", "1.0"), + ("1.0", "1"), ("1e100", "1e+100"), ("99999999999999999", "99999999999999999"), ("0099999999999999999999999", "1e+23"), @@ -10231,8 +10235,10 @@ fn test_argjson_shares_jq_dot_leniency_2240() -> Result<()> { (".05", r#"{"n":0.05}"#), (".007", r#"{"n":0.007}"#), ("-.05", r#"{"n":-0.05}"#), - ("1.e5", r#"{"n":100000.0}"#), - ("007.e5", r#"{"n":700000.0}"#), + // Whole-valued, so typed as an int by #978's canonicalizer since + // #2902 (real yq's JSON decoder does the same). + ("1.e5", r#"{"n":100000}"#), + ("007.e5", r#"{"n":700000}"#), ] { let (output, code) = run_yq_stdin( ".n = $n", @@ -45814,15 +45820,23 @@ mod computed_float_through_container_2902 { use super::run_yq_stdin; use anyhow::Result; - fn check(rows: &[(&str, &str)]) -> Result<()> { + /// Compared trimmed, like every other `tojson` test in this file: real + /// yq's JSON encoder ends its string with a newline of its own, which + /// succinctly does not reproduce (see `docs/compliance/yq/limitations.md`, + /// "`tostring`/`tojson` render a container compactly"). + fn check_on(input: &str, rows: &[(&str, &str)]) -> Result<()> { for (filter, want) in rows { - let (out, code) = run_yq_stdin(filter, "null\n", &[])?; + let (out, code) = run_yq_stdin(filter, input, &[])?; assert_eq!(code, 0, "`{filter}` exited {code}: {out:?}"); - assert_eq!(out, format!("{want}\n"), "`{filter}`"); + assert_eq!(out.trim_end(), *want, "`{filter}`"); } Ok(()) } + fn check(rows: &[(&str, &str)]) -> Result<()> { + check_on("null\n", rows) + } + /// The issue's own table, plus the same value through deeper and /// differently-shaped containers. #[test] @@ -45854,22 +45868,15 @@ mod computed_float_through_container_2902 { /// yq's JSON encoder keeps a whole computed float's `.0` and switches to /// scientific notation past its magnitude threshold. The latter rows used /// to be right only because the bridge's literal spelled them that way; - /// `to_json_yq` now applies the threshold itself. Compared trimmed, like - /// every other `tojson` test here: real yq's encoder ends the string - /// with a newline of its own, which succinctly does not reproduce. + /// `to_json_yq` now applies the threshold itself. #[test] fn tojson_keeps_yqs_own_threshold_2902() -> Result<()> { - for (filter, want) in [ + check(&[ ("[0.5+0.5] | .[0] | tojson", "1.0"), ("[1e10*2] | .[0] | tojson", "2e+10"), ("(1e10*2) | tojson", "2e+10"), ("[1e-5/2] | .[0] | tojson", "5e-06"), - ] { - let (out, code) = run_yq_stdin(filter, "null\n", &[])?; - assert_eq!(code, 0, "`{filter}` exited {code}: {out:?}"); - assert_eq!(out.trim_end(), want, "`{filter}`"); - } - Ok(()) + ]) } /// #1134's shapes: every intervening stage that used to re-bake the @@ -45886,24 +45893,108 @@ mod computed_float_through_container_2902 { ]) } - /// A tag-forced document float (`!!float 2`) is the one *document* shape - /// that is also a bare `Float`; it keeps answering the way it did, and - /// its text equality through a container (which used to see the - /// bridge's `2.0`) now agrees with yq too. + /// The path-context route: a `reduce`/`foreach` body that needs `key` + /// re-indexes its owned container and hands the bridge's document to + /// the *generic* materializer, whose literal-first chain would otherwise + /// fall through to `from_document_float` and re-bake the value past yq's + /// threshold (`"20000000000.0"`, found in review). #[test] - fn tag_forced_document_float_is_unchanged_2902() -> Result<()> { + fn generic_materializer_keeps_the_token_bare_2902() -> Result<()> { + check(&[ + ( + "reduce 1 as $x ([1e10*2]; .[0] | [tostring, key])", + "- \"2e+10\"\n- 0", + ), + ( + "reduce 1 as $x ([1e10*2]; .[0] | [tonumber, key])", + "- 2e+10\n- 0", + ), + ( + "reduce 1 as $x ([1e-5/2]; .[0] | [tostring, key])", + "- \"5e-06\"\n- 0", + ), + ( + "reduce 1 as $x ([0.5+0.5]; .[0] | [tostring, key])", + "- \"1\"\n- 0", + ), + ]) + } + + /// `@yaml`/`@props` spelled a bare `Float` with plain `Display` under a + /// comment claiming the bridge never yields one; with the token it does, + /// so both now take yq's computed-float rule (found in review). + #[test] + fn yaml_and_props_formats_use_the_computed_rule_2902() -> Result<()> { + check(&[ + ("[1e17*1] | .[0] | @yaml", "1e+17"), + ("[1e-5/2] | .[0] | @props", "5e-06"), + ("[0.5+0.5] | .[0] | @yaml", "1"), + ("[0.5+0.5] | .[0] | @props", "1"), + ("[1.0] | .[0] | @yaml", "1.0"), + ]) + } + + /// The `--input-format json` DOM route used to print a whole JSON float + /// as `100` only because the jq-mode bridge spelled `Float(100.0)` as + /// `100` and reparsed it as an integer literal; real yq's JSON decoder + /// types it `!!int` outright, which `from_number_literal_plain` now + /// does. Every row captured from yq v4.53.3 with `-p json` (`ea` for the + /// multi-document reader that shares the route). + #[test] + fn json_input_whole_float_is_an_int_2902() -> Result<()> { + const DOC: &str = "{\"a\":1e2,\"c\":1.0,\"d\":20000000000.0,\"e\":2.5,\"z\":-0.0,\ + \"i\":9223372036854775808.0,\"j\":1e19,\ + \"n\":100000000000000000000,\"big\":1e300}"; for (filter, want) in [ - ("[.a] | .[0] | tostring", "2"), - ("[.a] | .[0]", "2"), - ("[.a] | .[0] | tojson", "2.0"), - ("[.a] | .[0] == 2", "true"), + (".c | type", "!!int"), + (".a | type", "!!int"), + (".j | type", "!!float"), + (".e | type", "!!float"), + (".i", "9223372036854775807"), + (".j", "1e+19"), + (".z", "0"), + (".n", "1e+20"), + (".big", "1e+300"), + (".a | tojson", "100"), + (".d | tojson", "20000000000"), + ("[.c]", "- 1"), + ("[.d] | .[0]", "20000000000"), + ( + ".", + "a: 100\nc: 1\nd: 20000000000\ne: 2.5\nz: 0\ni: 9223372036854775807\n\ + j: 1e+19\nn: 1e+20\nbig: 1e+300", + ), ] { - let (out, code) = run_yq_stdin(filter, "a: !!float 2\n", &[])?; + let (out, code) = run_yq_stdin(filter, DOC, &["--input-format", "json", "--eval-all"])?; assert_eq!(code, 0, "`{filter}` exited {code}: {out:?}"); assert_eq!(out.trim_end(), want, "`{filter}`"); } + let (out, code) = run_yq_stdin(".", DOC, &["--input-format", "json", "--slurp"])?; + assert_eq!(code, 0, "slurp exited {code}: {out:?}"); + assert_eq!( + out.trim_end(), + "- a: 100\n c: 1\n d: 20000000000\n e: 2.5\n z: 0\n i: 9223372036854775807\n \ + j: 1e+19\n n: 1e+20\n big: 1e+300" + ); Ok(()) } + + /// A tag-forced document float (`!!float 2`) is the one *document* shape + /// that is also a bare `Float`; it keeps answering the way it did, and + /// its text equality through a container (which used to see the + /// bridge's `2.0`) now agrees with yq too. + #[test] + fn tag_forced_document_float_is_unchanged_2902() -> Result<()> { + check_on( + "a: !!float 2\n", + &[ + ("[.a] | .[0] | tostring", "2"), + ("[.a] | .[0]", "2"), + ("[.a] | .[0] | tojson", "2.0"), + ("[.a] | .[0] == 2", "true"), + ], + ) + } } /// #2785: real yq's `==`/`!=` between two scalars compares their *text*, with From b2d2be6824d840dc854aa83b6c7af2d01e923996 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 11:46:34 +1000 Subject: [PATCH 3/3] fix(jq): address remaining /code-review findings on #2902 Stale docs: eval_path_context_pipe_owned/eval_path_context_pipe_detached's doc comments, two of their own tests, and the jq limitations record still claimed a bare Float takes the cheap detached route reindex_bridge_is_identity now admits as an identity -- only NaN and an over-cap NumberLiteral still do. pure_value_matrix's comment repeated the same stale claim. Reuse: extracted sign_len(), collapsing a sign-stripping micro-pattern that had been hand-rolled four independent times in validate.rs (is_valid_number, strip_redundant_leading_zeros, has_leading_dot, parse_computed_float_token). Trimmed to_json_for_reindex's doc comment, which repeated computed_float_token's own bug-history narrative verbatim, to a pointer instead. Efficiency: from_number_bytes now tries is_valid_number before parse_computed_float_token, not after -- semantically identical (the token's doubled exponent marker makes is_valid_number reject every token unconditionally) but skips the token check's contains(&b'e') scan entirely for the common case, including a genuine literal that happens to end in the token's own suffix (5e0, 120e0). Hardening: computed_float_token's finiteness precondition is now a real assert!, not a debug_assert! -- every current caller already guarantees it, but a future one that didn't would otherwise splice unparseable text into reindexed JSON and corrupt the document silently in release builds. Simplification: test_reindex_round_trip_keeps_float_and_literal_apart_2902 no longer hand-rolls a JsonIndex/StandardJson::Array walk; a per-value loop through to_json_for_reindex + from_number_bytes covers the identical property with far less scaffolding, matching the two sibling tests beside it. Coverage: added a negative computed float through a container for both modes (only the sign-stripping unit tests exercised the negative case before; nothing walked the full arithmetic -> array -> reindex -> cursor pipeline end to end for one). --- docs/compliance/jq/limitations.md | 12 ++-- src/jq/eval.rs | 49 ++++++++------ src/jq/eval_generic.rs | 28 ++++---- src/jq/value.rs | 104 ++++++++++++++---------------- src/json/validate.rs | 47 +++++++++----- tests/jq_cli_tests.rs | 8 +++ tests/yq_cli_tests.rs | 9 +++ 7 files changed, 149 insertions(+), 108 deletions(-) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 264deb6d0..15b01b707 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -6005,10 +6005,14 @@ fix or a closed decision. `jq::eval_owned_with_file_index` is the one public entry that hands the path-context machinery an `OwnedValue` the caller built rather than one read -from a document, so it is the only way a bare `Float`, a NaN, or a -`NumberLiteral` longer than `REINDEX_LITERAL_LEN_CAP` (256 chars) can reach -`eval::eval_path_context_pipe_owned`. That door refuses the reindex bridge for -exactly those values (`reindex_bridge_is_identity`) and runs the pipe through +from a document, so it is the only way a NaN or a `NumberLiteral` longer than +`REINDEX_LITERAL_LEN_CAP` (256 chars) can reach +`eval::eval_path_context_pipe_owned`. (A bare `Float` used to be a third such +class; #2902 gave `to_json_for_reindex` a token spelling that survives the +reindex round trip intact, so a finite computed float is bridge-identity now +and takes the ordinary bridge instead of this door.) That door refuses the +reindex bridge for exactly those two remaining classes +(`reindex_bridge_is_identity`) and runs the pipe through `eval_generic::eval_path_context_pipe_detached` instead, which never serializes -- so `.[0] | parent`, `[.[0] | parent]` and `.[0] | parent | .[1]` all hand the literal back exactly as the caller spelled it diff --git a/src/jq/eval.rs b/src/jq/eval.rs index e0ba09143..f2a526d5e 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -41392,12 +41392,15 @@ fn builtin_isvalid<'a, W: Clone + AsRef<[u64]>, S: EvalSemantics>( /// /// One value class must not take the round trip: one the reindex would /// re-spell ([`crate::jq::eval_generic::reindex_bridge_is_identity`]) -- a -/// bare `Float`, a NaN, or a `NumberLiteral` past `REINDEX_LITERAL_LEN_CAP` -/// (#1211). `syq --eval-all '.[0] | .a | parent'` over a document holding -/// `.nan` reaches this door with exactly such a value, and reindexing it -/// prints `null` where the document said `.nan`. Until spine 2416's exit -/// that class was kept off the bridge by handing the pipe to the eager -/// evaluator; it now takes +/// NaN or a `NumberLiteral` past `REINDEX_LITERAL_LEN_CAP` (#1211). A bare +/// `Float` used to be a third such case, until #2902 gave +/// `to_json_for_reindex` a token spelling that survives the round trip +/// intact, so a finite computed float is bridge-identity now and no longer +/// reaches this door. `syq --eval-all '.[0] | .a | parent'` over a document +/// holding `.nan` reaches this door with exactly such a value, and +/// reindexing it prints `null` where the document said `.nan`. Until spine +/// 2416's exit that class was kept off the bridge by handing the pipe to +/// the eager evaluator; it now takes /// [`crate::jq::eval_generic::eval_path_context_pipe_detached`] -- the owned /// identity pipe rooted at a detached position, which never serializes and /// places the value exactly as the reindexed root cursor would have. A pipe @@ -60799,10 +60802,12 @@ mod tests { /// Each scalar also appears *wrapped* as `{"a": {"b": v}}` and `[v]`, so /// the arms that hand a navigated subvalue straight back (`Field`, /// `Index`, and the `Pipe` threading built on them) are diffed on every - /// numeric spelling too — the round trip those arms skip is a *formatter* - /// (`to_json_for_reindex` writes a bare `Float` decimally and the reparse - /// hands back a `NumberLiteral`), so an un-round-tripped `Float`/`Int` - /// reaching a caller is exactly where a divergence would hide. + /// numeric spelling too — the round trip those arms skip re-spells a + /// NaN or an over-cap `NumberLiteral` (`REINDEX_LITERAL_LEN_CAP`); a + /// bare finite `Float` used to be a third such case until #2902 made + /// that leg an identity (a token, not a formatter), so an + /// un-round-tripped `Float`/`Int` reaching a caller is exactly where a + /// divergence would hide. fn pure_value_matrix() -> Vec { let scalars = pure_scalar_matrix(); let mut out = scalars.clone(); @@ -82396,17 +82401,19 @@ mod tests { /// [`eval_path_context_pipe_owned`] gives a cursor-less owned value a /// position by serializing it into a throwaway document and walking that /// document's root cursor. `to_json_for_reindex`'s mode-forked formatter - /// re-spells a bare `Float`, a NaN and a `NumberLiteral` past - /// `REINDEX_LITERAL_LEN_CAP`, so `reindex_bridge_is_identity` keeps those - /// off the bridge; before the exit they went to the eager evaluator, and - /// they take `eval_generic::eval_path_context_pipe_detached` -- the owned - /// identity pipe at a detached root -- now. + /// re-spells a NaN and a `NumberLiteral` past `REINDEX_LITERAL_LEN_CAP`, + /// so `reindex_bridge_is_identity` keeps those off the bridge; before + /// the exit they went to the eager evaluator, and they take + /// `eval_generic::eval_path_context_pipe_detached` -- the owned identity + /// pipe at a detached root -- now. A bare `Float` used to be a third + /// such class, until #2902 gave `to_json_for_reindex` a token spelling + /// that survives the round trip intact: a finite computed float is + /// bridge-identity now, so it takes the ordinary bridge route instead of + /// this guard, and has no row here for the same reason the "not a row" + /// paragraph below explains for other bridge-identity values. /// /// Measured, not assumed: with the guard removed and the bridge taken, /// a 300-digit literal came back as `"1e+299"` and the NaN as `Null`. - /// (The third class, a bare `Float`, is re-spelled from `Float(1e19)` - /// into `NumberLiteral(1e19, "1e+19")` -- the same *text* either way, so - /// it is covered by the guard but has no row here that could fail.) /// /// One shape the detached route does *not* keep, and the reason it is /// not a row: a stage it hands to the ordinary owned evaluator @@ -82424,9 +82431,9 @@ mod tests { use alloc::string::ToString as _; let long = "1".repeat(300); // Only the over-cap literal has a spelling `to_json` can show: a NaN - // renders as `null` in jq mode whatever the route preserved, and a - // bare `Float` renders identically before and after the round trip - // (see the note above). All three take the same route. + // renders as `null` in jq mode whatever the route preserved. Both + // still take this guard's route; a bare `Float` no longer does + // (see the note above). let rows: &[(OwnedValue, &str)] = &[( OwnedValue::NumberLiteral(NumberRepr::Float(1e299), long.clone().into()), &long, diff --git a/src/jq/eval_generic.rs b/src/jq/eval_generic.rs index 7965b717c..4a1359b7b 100644 --- a/src/jq/eval_generic.rs +++ b/src/jq/eval_generic.rs @@ -23537,14 +23537,17 @@ fn eval_map_family_positioned_result( /// `eval::eval_path_context_pipe_owned` normally gives a cursor-less owned /// value a position by serializing it into a throwaway document and taking /// that document's root cursor. That round trip is a semantic identity for -/// almost every value ([`reindex_bridge_is_identity`]), but not for a bare -/// `Float`, a NaN, or a numeric literal past [`REINDEX_LITERAL_LEN_CAP`]: -/// `to_json_for_reindex`'s mode-forked formatter re-spells those, so -/// `syq --eval-all '.[0] | .a | parent'` over a document holding `.nan` -/// would print the ancestor with `null` in place of the NaN and `1e+19` in -/// place of `10000000000000000000.0`. Those values used to be kept off the -/// bridge by handing the pipe to the eager evaluator; with that evaluator -/// deleted, they take this route instead. +/// almost every value ([`reindex_bridge_is_identity`]), but not for a NaN +/// or a numeric literal past [`REINDEX_LITERAL_LEN_CAP`] (a bare finite +/// `Float` used to be a third such case, until #2902 gave +/// `to_json_for_reindex` a token spelling that survives the round trip +/// intact): `to_json_for_reindex`'s mode-forked formatter re-spells those, +/// so `syq --eval-all '.[0] | .a | parent'` over a document holding `.nan` +/// would print the ancestor with `null` in place of the NaN, and the same +/// pipe over an over-cap literal would print `1e+19` in place of +/// `10000000000000000000.0`. Those values used to be kept off the bridge by +/// handing the pipe to the eager evaluator; with that evaluator deleted, +/// they take this route instead. /// /// The owned identity pipe is the exact replacement: it never serializes, /// and a detached root is precisely the position the reindexed root cursor @@ -24773,10 +24776,11 @@ mod tests { /// "duplicated predicates diverge silently"). /// /// The corpus straddles every boundary the predicate draws: a bare - /// `Float` (re-spelled), a bare `Int` (normalized to a `NumberLiteral` - /// carrying the same text it already rendered as — the one sanctioned - /// exception, spelled out in `round_trips_unchanged`), a NaN literal - /// (replaced by `NAN_SENTINEL`), and a `NumberLiteral` either side of + /// finite `Float` (identity since #2902's token spelling, no longer + /// re-spelled), a bare `Int` (normalized to a `NumberLiteral` carrying + /// the same text it already rendered as — the one sanctioned exception, + /// spelled out in `round_trips_unchanged`), a NaN literal (replaced by + /// `NAN_SENTINEL`), and a `NumberLiteral` either side of /// `REINDEX_LITERAL_LEN_CAP`, which is duplicated from a private `const` /// inside `to_json_for_reindex`'s body and cannot be shared. /// diff --git a/src/jq/value.rs b/src/jq/value.rs index 6227925fd..41076e486 100644 --- a/src/jq/value.rs +++ b/src/jq/value.rs @@ -1858,16 +1858,23 @@ impl OwnedValue { f64::INFINITY }); } + if crate::json::validate::is_valid_number(bytes) { + return core::str::from_utf8(bytes).map_or(Self::Null, Self::from_number_literal); + } // The reindex bridge's computed-float token (#2902): a bare `Float` // going in must be a bare `Float` coming out, never a - // `NumberLiteral` -- checked with the sentinels, before any of the - // literal-preserving arms below can see the text. + // `NumberLiteral`. Checked only after `is_valid_number` fails -- + // never before it, and never merged into the same `if`/`else` + // ordering as an unconditional first check: the token's doubled + // exponent marker (its whole design, see `computed_float_token`) + // makes `is_valid_number` reject every token unconditionally, so + // this is never reached for an ordinary number, including one that + // happens to end in this token's own suffix (`5e0`, `120e0`) -- + // those take the literal-preserving arm above instead, without + // paying this check's `contains(&b'e')` scan at all. if let Some(f) = crate::json::validate::parse_computed_float_token(bytes) { return Self::Float(f); } - if crate::json::validate::is_valid_number(bytes) { - return core::str::from_utf8(bytes).map_or(Self::Null, Self::from_number_literal); - } // Real jq's own number reader also accepts a leading `.` (with or // without a preceding `-`) when at least one digit follows (`.5` // -> `0.5`, `-.5` -> `-0.5`) -- not valid per strict RFC 8259 @@ -2354,23 +2361,14 @@ impl OwnedValue { /// /// A plain (non-`NumberLiteral`) finite `Float` is written as the /// bridge-only token `crate::json::validate::computed_float_token` - /// (#2902), which [`from_number_bytes`](Self::from_number_bytes) and - /// `JsonNumber::as_f64` decode back to a bare `Float`. It used to be - /// written with a display formatter -- `format_float_yq` in yq mode - /// (#953, #2438), `jq_bare_float_display` in jq mode -- and since the - /// reparse cannot tell `1.0` written by this bridge from `1.0` written - /// in a document, the value came back as a `NumberLiteral` and every - /// literal-preserving rule downstream (#1008, #1054, #2456) echoed the - /// bridge's spelling: `[0.5+0.5] | .[0] | tostring` answered `1.0` - /// (real yq `1`) and `[2*1e16] | .[0] | tostring` answered `2E+16` - /// (real jq `2e+16`). The token carries no spelling at all, so each - /// mode re-derives its own from the `f64` after the round trip, and the - /// mode fork this fallback used to need is gone. A **document** float - /// with no preserved literal (an i64-overflow YAML scalar) still keeps - /// the full decimal spelling real yq gives *it* because its provenance - /// is recorded upstream, at - /// [`from_document_float`](Self::from_document_float), as a - /// `NumberLiteral` that echoes verbatim below. + /// (#2902) -- see that function's own doc comment for why a token + /// rather than a display formatter, and the pre-#2902 bug it replaces. + /// [`from_number_bytes`](Self::from_number_bytes) and `JsonNumber::as_f64` + /// decode it back to a bare `Float`. A **document** float with no + /// preserved literal (an i64-overflow YAML scalar) still keeps the full + /// decimal spelling real yq gives *it* because its provenance is + /// recorded upstream, at [`from_document_float`](Self::from_document_float), + /// as a `NumberLiteral` that echoes verbatim below. pub fn to_json_for_reindex(&self) -> String { self.to_json_for_reindex_at_depth::(0) } @@ -3702,39 +3700,37 @@ mod tests { /// takes its one spelling. #[test] fn test_reindex_round_trip_keeps_float_and_literal_apart_2902() { - use crate::json::JsonIndex; - fn round_trip(value: &OwnedValue) -> OwnedValue { - let json = value.to_json_for_reindex::(); - let bytes = json.as_bytes(); - let index = JsonIndex::build(bytes); - let root = index.root(bytes); - let crate::json::light::StandardJson::Array(items) = root.value() else { - panic!("expected the round-tripped array, got {json}"); - }; - OwnedValue::Array( - items - .map(|item| match item { - crate::json::light::StandardJson::Number(n) => { - OwnedValue::from_number_bytes(n.raw_bytes()) - } - other => panic!("expected a number, got {other:?}"), - }) - .collect(), - ) + // Per-value, mirroring the two sibling tests above (token-writing, + // token-decoding) rather than re-deriving their composition through + // a hand-rolled `JsonIndex`/`StandardJson::Array` walk: + // `to_json_for_reindex_at_depth`'s `Array` arm serializes each + // element independently and `JsonIndex` parses each number span + // independently too, so wrapping these six values in an array and + // walking a real index would prove nothing this per-value round + // trip through `from_number_bytes` doesn't already cover. + let cases: &[(OwnedValue, &str)] = &[ + (OwnedValue::Float(1.0), "Float(1.0)"), + ( + OwnedValue::from_number_literal("1.0"), + "NumberLiteral(Float(1.0), \"1.0\")", + ), + (OwnedValue::Int(1), "NumberLiteral(Int(1), \"1\")"), + (OwnedValue::Float(2e16), "Float(2e16)"), + ( + OwnedValue::from_number_literal("2e16"), + "NumberLiteral(Float(2e16), \"2e16\")", + ), + (OwnedValue::Float(-0.0), "Float(-0.0)"), + ]; + for (value, want) in cases { + for json in [ + value.to_json_for_reindex::(), + value.to_json_for_reindex::(), + ] { + let round_tripped = OwnedValue::from_number_bytes(json.as_bytes()); + assert_eq!(format!("{round_tripped:?}"), *want, "{value:?} -> {json}"); + } } - let value = OwnedValue::Array(vec![ - OwnedValue::Float(1.0), - OwnedValue::from_number_literal("1.0"), - OwnedValue::Int(1), - OwnedValue::Float(2e16), - OwnedValue::from_number_literal("2e16"), - OwnedValue::Float(-0.0), - ]); - let want = "Array([Float(1.0), NumberLiteral(Float(1.0), \"1.0\"), \ - NumberLiteral(Int(1), \"1\"), Float(2e16), \ - NumberLiteral(Float(2e16), \"2e16\"), Float(-0.0)])"; - assert_eq!(format!("{:?}", round_trip::(&value)), want); - assert_eq!(format!("{:?}", round_trip::(&value)), want); } /// #2456: `jq_bare_float_display`'s digit-count threshold diff --git a/src/json/validate.rs b/src/json/validate.rs index 0e60b5ccb..f915eed8e 100644 --- a/src/json/validate.rs +++ b/src/json/validate.rs @@ -850,6 +850,20 @@ pub fn validate_jq_lenient(input: &[u8]) -> Result<(), ValidationError> { Validator::new(input).jq_lenient().validate() } +/// Length (0 or 1) of an optional leading `-` sign at the start of `bytes`. +/// +/// Shared building block for every "does this number-shaped span start with +/// an optional sign, and what comes right after it" check in this file +/// ([`is_valid_number`], [`strip_redundant_leading_zeros`], +/// [`has_leading_dot`], `parse_computed_float_token`) -- previously each +/// hand-rolled its own sign-stripping in a different style (index bump, +/// slice split via `match` on `first()`, `strip_prefix`), a fourth +/// independent copy of the same one-byte check. +#[must_use] +fn sign_len(bytes: &[u8]) -> usize { + usize::from(bytes.first() == Some(&b'-')) +} + /// True if `bytes` is *exactly* one RFC 8259 JSON number token, with /// nothing before or after it. /// @@ -877,10 +891,7 @@ pub fn validate_jq_lenient(input: &[u8]) -> Result<(), ValidationError> { /// ``` #[must_use] pub fn is_valid_number(bytes: &[u8]) -> bool { - let mut i = 0; - if bytes.first() == Some(&b'-') { - i += 1; - } + let mut i = sign_len(bytes); match bytes.get(i) { Some(b'0') => i += 1, Some(b'1'..=b'9') => { @@ -946,10 +957,7 @@ pub fn is_valid_number(bytes: &[u8]) -> bool { /// receive one isolated span, so neither ever needed that machinery. #[must_use] pub fn strip_redundant_leading_zeros(bytes: &[u8]) -> Option> { - let (sign, rest) = match bytes.first() { - Some(b'-') => (&bytes[..1], &bytes[1..]), - _ => (&bytes[..0], bytes), - }; + let (sign, rest) = bytes.split_at(sign_len(bytes)); if rest.first() != Some(&b'0') || !rest.get(1).is_some_and(u8::is_ascii_digit) { return None; } @@ -1034,14 +1042,10 @@ pub fn has_trailing_dot_before_exponent(bytes: &[u8]) -> bool { /// own inserted-`0` candidate. #[must_use] pub fn has_leading_dot(bytes: &[u8]) -> bool { - let dot_pos = match bytes.first() { - Some(b'.') => Some(0), - Some(b'-') if bytes.get(1) == Some(&b'.') => Some(1), - _ => None, - }; - let Some(prefix_len) = dot_pos else { + let prefix_len = sign_len(bytes); + if bytes.get(prefix_len) != Some(&b'.') { return false; - }; + } let mut fixed = Vec::with_capacity(bytes.len() + 1); fixed.extend_from_slice(&bytes[..prefix_len]); fixed.push(b'0'); @@ -1089,9 +1093,18 @@ const COMPUTED_FLOAT_TOKEN_SUFFIX: &str = "e0"; /// /// `f` must be finite: NaN/±Infinity have their own sentinels, which every /// caller emits first. +/// +/// Checked with `assert!`, not `debug_assert!`, even though every current +/// caller already guarantees it (`to_json_at_depth`'s `Self::Float(f)` arm +/// re-checks `is_nan()`/`is_infinite()` before ever calling `float_fmt`): +/// `format!("{:e}", f64::NAN)` is `"NaN"` and the infinities are +/// `"inf"`/`"-inf"`, none digit-leading, so a future caller that skipped +/// the pre-filter would otherwise splice unparseable text into reindexed +/// JSON and corrupt the surrounding document silently in a release build, +/// rather than fail loudly the way an invariant violation should. #[must_use] pub(crate) fn computed_float_token(f: f64) -> String { - debug_assert!( + assert!( f.is_finite(), "computed_float_token requires a finite value; NaN/Infinity use their own sentinels" ); @@ -1112,7 +1125,7 @@ pub(crate) fn computed_float_token(f: f64) -> String { #[must_use] pub(crate) fn parse_computed_float_token(bytes: &[u8]) -> Option { let inner = bytes.strip_suffix(COMPUTED_FLOAT_TOKEN_SUFFIX.as_bytes())?; - let digits = inner.strip_prefix(b"-").unwrap_or(inner); + let digits = &inner[sign_len(inner)..]; if !digits.first().is_some_and(u8::is_ascii_digit) || !inner.contains(&b'e') { return None; } diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 7eb6e2714..3a834ef80 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -32859,6 +32859,14 @@ fn test_computed_float_through_container_keeps_computed_spelling_2902() -> Resul ("[1.0] | .[0] | tostring", r#""1.0""#), ("[1e2] | .[0] | tostring", r#""1E+2""#), ("[1.50e10] | .[0]", "1.50E+10"), + // A negative computed float takes the same route (found in + // review): the token's sign-stripping in + // `parse_computed_float_token` is unit-tested directly, but this + // is the only place the full pipeline -- arithmetic, negation, + // array, reindex, cursor decode -- was exercised end-to-end for a + // negative value. + ("[0-(0.5+0.5)] | .[0] | tostring", r#""-1""#), + ("[0-1e16] | .[0] | tostring", r#""-1e+16""#), ] { let (stdout, code) = run_jq_stdin(filter, "null\n", &["-c"])?; assert_eq!(code, 0, "`{filter}` exited {code}: {stdout:?}"); diff --git a/tests/yq_cli_tests.rs b/tests/yq_cli_tests.rs index 106308de2..7475d50dc 100644 --- a/tests/yq_cli_tests.rs +++ b/tests/yq_cli_tests.rs @@ -45851,6 +45851,13 @@ mod computed_float_through_container_2902 { ("[[0.5+0.5]] | .[0][0] | tostring", "1"), ("[0.5+0.5] | map(.) | .[0] | tostring", "1"), ("[1e2] | .[0] | tostring", "1e2"), + // A negative computed float takes the same route (found in + // review): `parse_computed_float_token`'s sign-stripping is + // unit-tested directly, but this is the only place the full + // pipeline -- arithmetic, negation, array, reindex, cursor + // decode -- is exercised end-to-end for a negative value. + // Real yq has no unary `-`, so `0 - x` stands in for it. + ("[0-(0.5+0.5)] | .[0] | tostring", "-1"), ]) } @@ -45876,6 +45883,8 @@ mod computed_float_through_container_2902 { ("[1e10*2] | .[0] | tojson", "2e+10"), ("(1e10*2) | tojson", "2e+10"), ("[1e-5/2] | .[0] | tojson", "5e-06"), + // Negative, same reasoning as the sibling `tostring` row above. + ("[0 - (1e10*2)] | .[0] | tojson", "-2e+10"), ]) }