fix(runtime): large integers print V8 shortest round-trip, not exact (#6127) - #6176
Merged
Conversation
…6127) Whole-number `f64`s in `[2^53, i64::MAX)` were printed as the exact integer via an `as i64` cast, but at/above 2^53 several integers map to one double and the exact value carries more significant digits than V8's shortest round-trip decimal (`2**58` → `288230376151711740`, not `…744`; `2**60` → `…847000`, not `…846976`). The cast threshold `i64::MAX` (~9.2e18) is far above 2^53, so every duplicated number-to-string fast path across the runtime shared the bug. Root: below 2^53 every integer is exactly representable, so `as i64` equals the shortest decimal; at/above it the shortest form can have trailing zeros the exact integer does not. Introduce `INT_EXACT_FASTPATH_LIMIT = 2^53` and route values at/above it through the existing shortest-round-trip formatter (Rust's `{}` / `js_format_f64`, which is positional up to 1e21). The small-integer fast path (the overwhelming majority) is unchanged, so hot paths keep the cheap cast. Surfaces fixed (all validated byte-for-byte vs `node --experimental-strip-types`): - `console.log` / `console.dir` (plain + inspect array/object nesting) - `console.error` (stderr) and `console.table` - `JSON.stringify` (main path, api path, and the node:stream JSON path — the latter previously fell to `ryu`, which emits scientific notation; large integers now use positional `js_format_f64`) - `util.format('%i')` (truncating integer specifier) - `Number.prototype.toString` (radix-less), boxed `Number` `toString`/`valueOf` - `Number.prototype.toLocaleString` — its grouping path built digits from an `abs.trunc() as u64` cast (which also overflowed past ~1.8e19); now derived from the f64's always-positional shortest `{}` form (`2**60` → `1,152,921,504,606,847,000`). Out of scope (documented): the two "Invalid typed array length: <n>" RangeError message sites (`buffer/from.rs`, `typedarray/mod.rs`) format a value for an error string, not as a program value; left unchanged. Small integers, negatives, `-0`, non-integers, `1e21`/`1e-7` exponential thresholds, `toString(radix)`, and BigInt are unaffected; json/tofixed/ object-key-order/console regression tests stay green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNumber-formatting paths now use a 2^53 exact-integer fast-path limit instead of ChangesInteger fast-path limit unification
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/perry-runtime/src/builtins/formatting.rs (2)
1080-1084: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using
format_integral_f64to avoid duplicating the threshold logic.The inline
n.abs() < INT_EXACT_FASTPATH_LIMITgate duplicates the same logic already encapsulated informat_integral_f64. If the threshold or fallback ever changes, three sites must be updated in lockstep. Restructuring to call the helper keeps the decision in one place.♻️ Proposed refactor
} else if is_negative_zero(n) { "-0".to_string() - } else if n.fract() == 0.0 && n.abs() < INT_EXACT_FASTPATH_LIMIT { - (n as i64).to_string() + } else if n.fract() == 0.0 { + format_integral_f64(n) } else { format_finite_number_js(n) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/builtins/formatting.rs` around lines 1080 - 1084, The `format_finite_number_js` branch is duplicating the integral fast-path threshold check with `n.abs() < INT_EXACT_FASTPATH_LIMIT`, which should be centralized in `format_integral_f64`. Update this logic to route the integer-like `f64` case through `format_integral_f64` instead of inlining the gate, keeping the fast-path decision in one place and avoiding drift if the threshold or fallback behavior changes.
1741-1745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame duplication as
format_jsvalue— consider usingformat_integral_f64here too.This site mirrors the inline threshold check at line 1080. The same refactor applies for consistency and single-source-of-truth.
♻️ Proposed refactor
} else if is_negative_zero(n) { "-0".to_string() - } else if n.fract() == 0.0 && n.abs() < INT_EXACT_FASTPATH_LIMIT { - (n as i64).to_string() + } else if n.fract() == 0.0 { + format_integral_f64(n) } else { format_finite_number_js(n) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/builtins/formatting.rs` around lines 1741 - 1745, The numeric formatting branch in `formatting.rs` duplicates the same integral fast-path logic used by `format_jsvalue`; refactor this branch to reuse `format_integral_f64` instead of repeating the `fract`/`abs` threshold check. Update the `format_finite_number_js` path so the behavior stays identical while centralizing the integer formatting decision in the shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/builtins/console.rs`:
- Around line 100-103: `js_console_log_dynamic` is using Rust’s `f64` Display in
its fallback path, which makes the new integer-valued double range format
differently from the other console helpers. Update the fallback branch in
`js_console_log_dynamic` to use `format_finite_number_js(n)` just like
`js_console_error_dynamic` and `js_console_warn_dynamic`, and keep the fast
integer path only for values that are safely representable as `i64`.
---
Nitpick comments:
In `@crates/perry-runtime/src/builtins/formatting.rs`:
- Around line 1080-1084: The `format_finite_number_js` branch is duplicating the
integral fast-path threshold check with `n.abs() < INT_EXACT_FASTPATH_LIMIT`,
which should be centralized in `format_integral_f64`. Update this logic to route
the integer-like `f64` case through `format_integral_f64` instead of inlining
the gate, keeping the fast-path decision in one place and avoiding drift if the
threshold or fallback behavior changes.
- Around line 1741-1745: The numeric formatting branch in `formatting.rs`
duplicates the same integral fast-path logic used by `format_jsvalue`; refactor
this branch to reuse `format_integral_f64` instead of repeating the
`fract`/`abs` threshold check. Update the `format_finite_number_js` path so the
behavior stays identical while centralizing the integer formatting decision in
the shared helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9f01e42-44a5-4ba8-bda6-86067f0a6805
📒 Files selected for processing (11)
crates/perry-runtime/src/builtins/console.rscrates/perry-runtime/src/builtins/formatting.rscrates/perry-runtime/src/builtins/formatting/util_format.rscrates/perry-runtime/src/builtins/mod.rscrates/perry-runtime/src/builtins/table.rscrates/perry-runtime/src/date.rscrates/perry-runtime/src/json/stringify.rscrates/perry-runtime/src/json/stringify_api.rscrates/perry-runtime/src/node_stream_json.rscrates/perry-runtime/src/object/native_call_method/common_methods.rscrates/perry-runtime/src/object/native_call_method/primitive_methods.rs
…er formatter (#6127) Address CodeRabbit review: `js_console_log_dynamic`'s non-integer / large-integer fallback printed the number via Rust's `f64` Display (`println!("{}{}", p, n)`) while the sibling `js_console_error_dynamic` / `js_console_warn_dynamic` paths use `format_finite_number_js`. Rust's Display never emits scientific notation, so a single-argument `console.log(1e21)` printed `1000000000000000000000` instead of Node's `1e+21` (and likewise for `1e-7`-scale magnitudes) — a pre-existing gap this PR's lowered fast-path threshold routed more values through. Route the fallback through `format_finite_number_js`, which applies the shortest round-trip digits AND the 1e21/1e-6 exponential thresholds, matching the other console dynamic printers and `String(n)`. The `[2^53, i64::MAX)` integers this PR targets were already correct (positional either way); this fixes the ≥1e21 case and removes the inconsistency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Whole-number
f64s in[2^53, i64::MAX)printed the exact integer instead of V8's shortest round-trip decimal.Fixes #6127.
Root cause
Every duplicated number-to-string fast path used
if n.fract() == 0.0 && n.abs() < (i64::MAX as f64) { n as i64 }. Thei64::MAXthreshold (~9.2e18) is far above 2^53, so integer-valued doubles between 2^53 and i64::MAX printed the exact integer. Below 2^53 every integer is exactly representable, so the cast equals the shortest decimal; at/above it the shortest form can carry trailing zeros the exact integer does not..toString()/String()/template literals were already correct because they usejs_format_f64(Rust's shortest{}).Fix
Add
INT_EXACT_FASTPATH_LIMIT = 2^53and route values at/above it through the existing shortest-round-trip formatter (positional up to 1e21). The small-integer fast path — the overwhelming majority — is unchanged, so hot paths keep the cheap cast and only rare large integers change (wrong → right). Surfaces:console.log/dir(plain + inspect array/object),console.error,console.tableJSON.stringify(main + api + node:stream path — the stream path fell toryu, which emits scientific notation, so large integers now use positionaljs_format_f64)util.format('%i')Number.prototype.toString(radix-less), boxedNumbertoString/valueOfNumber.prototype.toLocaleString— grouping built digits fromabs.trunc() as u64(which also overflowed past ~1.8e19); now from the f64's always-positional shortest{}formOut of scope (documented): the two
"Invalid typed array length: <n>"RangeErrormessage sites (buffer/from.rs,typedarray/mod.rs) format a value for an error string, not as a program value — left unchanged.Validation
Byte-for-byte vs
node --experimental-strip-types:%i,toLocaleStringgrouping all match.-0, non-integers,1e21/1e-7exponential thresholds,toString(radix), BigInt.cargo fmt+ GC store-site inventory clean.Code-only per contributor convention (no version/CHANGELOG bump).
Summary by CodeRabbit
f64values across console logging, tables, JSON stringify, locale string rendering, andtoString/toLocaleString, especially around the exact-integer precision boundary.