fix(node:util): correct numericSeparator for negative fractions and exponent-form numbers - #36501
fix(node:util): correct numericSeparator for negative fractions and exponent-form numbers#36501robobun wants to merge 3 commits into
Conversation
…xponent-form numbers
util.inspect(n, { numericSeparator: true }) mangled negative fractional
numbers and numbers that stringify in exponent form:
-0.12 -> "0..12" (sign dropped, doubled decimal point)
1e-7 -> "0.-7"
1.23456e-7 -> "0.234_56e_-7"
-0 -> "0" (sign dropped)
formatNumber derived the fractional slice from String(Math.trunc(n)).length.
For -0.12, Math.trunc is -0 and String(-0) is "0", so the sign was lost and
the slice offset was wrong. For 1e-7, trunc is 0 so "0" bore no relation to
the actual "." position in "1e-7".
Port Node's current formatNumber: hoist the -0 check above the separator
branch, split the number's own string on indexOf("."), and pass exponent-form
strings through unchanged.
Closes #23098
Adopted from #32460.
Co-authored-by: 0xfandom <kashyapshivank01@gmail.com>
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesNumeric separator formatting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward port of Node's current formatNumber to fix numericSeparator output for negative fractions, -0, and exponent-form numbers.
What was reviewed:
- Traced the new control flow for each input class (
-0, integers, NaN/±Infinity, exponent-form integers and fractions, negative/positive decimals) — all reach the correct branch anddecimalIndexis never -1 in the final path. - Confirmed
StringPrototypeIndexOfand the other primordials used are already declared in this file. - Checked
addNumericSeparatoralready handles a leading-in the integer part, so-12_345.678_9groups correctly. - Tests cover the #23098 repro plus every branch touched; assertions are exact
toBestrings verified against Node.
Extended reasoning...
Overview
This PR fixes util.inspect(n, { numericSeparator: true }) for three input classes that were previously mangled: negative fractional numbers (-0.12 → 0..12), exponent-form numbers (1e-7 → 0.-7), and -0 under the numericSeparator branch (-0 → 0). The root cause was deriving the fractional slice offset from String(Math.trunc(n)).length, which is wrong when Math.trunc returns -0 (stringifies without the sign) or when the number's own string representation is in exponent form and unrelated to the trunc result. The fix hoists the -0 check above all branches, splits on the actual . position via indexOf, and passes any string containing e through unchanged — matching Node's current lib/internal/util/inspect.js.
Two files touched: ~25 lines rewritten in formatNumber (src/js/internal/util/inspect.js) and a new 47-line describe block in test/js/node/util/util.test.js.
Security risks
None. Pure string-formatting logic on a primitive number in a debug-inspection code path. No I/O, no allocation beyond short strings, no user-controllable resource sizing, and the primordial-captured helpers (StringPrototypeIndexOf, StringPrototypeSlice, ObjectIs, etc.) are already declared at the top of the file, so there's no new prototype-pollution surface.
Level of scrutiny
Low. This is a small, self-contained Node-compat bugfix in built-in JS that ports upstream Node's implementation verbatim. I walked through each branch: -0 returns early; !numericSeparator returns the raw string; integer-valued numbers either pass through (non-finite / e-form) or go to addNumericSeparator; NaN and fractional e-form pass through; and the remaining finite fractional case is guaranteed to contain . (any finite non-integer stringifies with either a . or an e, and e was already handled), so decimalIndex can't be -1. addNumericSeparator already special-cases a leading -, so the negative integer part groups correctly.
Other factors
Test coverage is thorough and follows repo conventions: exact toBe assertions (no snapshots needed for these one-liners), the original issue's repro array, per-value negative-fraction cases including a grouped one, -0/0, positive/negative large-integer grouping, both integer and fractional exponent-form inputs, and NaN/±Infinity. The PR description shows USE_SYSTEM_BUN=1 fails 4/6 and the debug build passes 6/6, satisfying the fails-for-the-right-reason requirement. No prior human review comments to address.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/js/internal/util/inspect.js`:
- Line 2217: Update the numeric conversion in the inspection formatting path
around numberString to avoid the mutable global String constructor; use
primitive interpolation consistent with the existing non-separator branch,
preserving the current output while remaining safe if globalThis.String is
replaced.
In `@test/js/node/util/util.test.js`:
- Line 469: Replace the explanatory regression-test comments around the affected
tests with only the relevant issue URL, and remove the standalone comment at
line 501. Keep the existing test names and test behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d48a4ee6-20a3-4c61-a2ff-c1c920141a03
📒 Files selected for processing (2)
src/js/internal/util/inspect.jstest/js/node/util/util.test.js
There was a problem hiding this comment.
LGTM — straightforward port of Node's formatNumber that fixes the String(Math.trunc(n)).length slice bug. The inline nit about test comments is optional cleanup.
What was reviewed:
- Traced every branch of the new
formatNumber(−0, non-separator, integer, ±Infinity, NaN, exponent-form both integer and fractional, negative fractions) —decimalIndexis always ≥ 0 when reached, andaddNumericSeparatoralready handles the leading-. - Confirmed
StringPrototypeIndexOfis already defined in the primordials block. - The comment-cop bot flag on line 2209 is a false positive — that two-line comment explains a real JS quirk (
String(-0) === "0"), not a workaround.
Extended reasoning...
Overview
Two-file change: src/js/internal/util/inspect.js rewrites formatNumber (~25 lines) to match Node's current implementation, and test/js/node/util/util.test.js adds a 47-line describe block covering the fixed cases. The bug was that the fractional part was sliced at String(Math.trunc(n)).length + 1, which is wrong when Math.trunc yields -0 (stringifies as "0", dropping the sign and shifting the offset) or when the number stringifies in exponent form (trunc's string bears no relation to the . position).
Security risks
None. Pure display formatting of already-typed number values in util.inspect; no parsing of untrusted input, no allocation sizing, no native code.
Level of scrutiny
Low. This is a Node-compat fix in a built-in JS module where the reference implementation (Node's lib/internal/util/inspect.js) is the spec per REVIEW.md, and the PR ports it verbatim. I traced each code path against the test inputs: -0 short-circuits before String(); NaN is caught before the indexOf(".") split (which would otherwise return −1); exponent-form fractionals like 1e-7 (where trunc(n) !== n) are now guarded by the new includes("e") check on the fractional branch; and addNumericSeparator at inspect.js:2189 already handles a leading - in the integer part, so -12_345.678_9 groups correctly.
Other factors
- Tests are exact-value assertions verified against Node, added to the existing test file (correct location per CLAUDE.md), and the PR body shows they fail under
USE_SYSTEM_BUN=1. - The only bug-hunter finding is a style nit about bug-history comments in the test — valid per REVIEW.md but not merge-blocking.
- The github-actions comment-cop inline comment on inspect.js:2209 misfires: the flagged comment is two short lines documenting why
ObjectIs(number, -0)must precedeString()conversion (a durable non-obvious invariant, not a workaround justification), and the first of those lines already existed before this PR.
There was a problem hiding this comment.
LGTM — thanks for dropping the bug-history comments in 8100899.
What was reviewed:
- Traced
formatNumberthrough-0, negative fractions, exponent-form (bothinteger === numberand fractional branches),NaN/±Infinity, and large-integer-as-exponent — all reach the correct return. - Confirmed
addNumericSeparatoralready handles the leading-in the newintegerPartslice (start = integerString[0] === "-" ? 1 : 0). - Confirmed
StringPrototypeIndexOfis already defined in this file (line 110). - On the open CodeRabbit note re
String(number): the file already callsString(...)~32 times and the replaced code did too — not a regression introduced here, and hardening that is a separate file-wide change.
Extended reasoning...
Overview
This PR rewrites formatNumber in src/js/internal/util/inspect.js to fix util.inspect(n, { numericSeparator: true }) for negative fractional numbers (e.g. -0.12 previously rendered as "0..12"), exponent-form numbers (1e-7 → "0.-7"), and -0 under the separator option. The fix is a near-verbatim port of Node's current formatNumber: hoist the Object.is(-0) check above the separator branch, stringify the number once and split on indexOf(".") instead of String(Math.trunc(n)).length, and pass through any string containing e. A new describe("inspect numericSeparator") block in test/js/node/util/util.test.js adds 6 tests covering the #23098 repro array, individual negative fractions, ±0, integer/fraction grouping, exponent-form inputs, and non-finite values.
Security risks
None. This is pure output-formatting logic over primitive numbers with no I/O, no allocation, and no user-controlled parsing. The one primordial-safety observation from CodeRabbit (String(number) reads the global String) is a pre-existing pattern used ~32 times throughout this file — the diff actually reduces the call count relative to the old code (String(integer) + String(number) → one String(number)), so it is not a regression introduced by this PR.
Level of scrutiny
Low-to-moderate. This is a small (~20-line) Node-compat bug fix in a well-isolated helper, ported directly from the upstream reference implementation. I traced every branch of the new formatNumber against representative inputs and confirmed addNumericSeparator already handles a leading - in the integer-part slice via its start offset, so -12_345.678_9 groups correctly. StringPrototypeIndexOf (the only newly-referenced primordial) is already defined at line 110.
Other factors
- My earlier nit about bug-history comments in the test file was addressed in commit 8100899 — the current diff carries no explanatory comments, and the test names reference #23098 directly.
- The PR body demonstrates the tests fail on
USE_SYSTEM_BUN=1(4 fail) and pass on the debug build (6 pass), satisfying the fails-for-the-right-reason requirement. - Test coverage spans the full input matrix (negative/positive, integer/fractional, exponent-form via both the
integer === numberand fractional paths,NaN,±Infinity,±0), with exact-value.toBeassertions. - The remaining unresolved CodeRabbit inline comment is not actionable within this PR's scope for the reason above.
|
Updated 11:29 PM PT - Jul 30th, 2026
✅ @robobun, your commit 810089971de5e35d19e02eacf413885c780c6a20 passed in 🧪 To try this PR locally: bunx bun-pr 36501That installs a local version of the PR into your bun-36501 --bun |
1 participant
What does this PR do?
util.inspect(n, { numericSeparator: true })mangled negative fractional numbers and numbers that stringify in exponent form. Fixes #23098. Adopted from #32460 by @0xfandom.-0.120..12-0.12-0.12-0.12340..12_34-0.123_4-0.123_41e-70.-71e-71e-7-1e-70.e-7-1e-7-1e-71.23456e-70.234_56e_-71.23456e-71.23456e-7-00-0-0formatNumberderived the fractional slice fromString(Math.trunc(n)).length + 1. For-0.12,Math.truncis-0andString(-0)is"0", so the sign was lost and the slice offset into"-0.12"was off by one. For1e-7, trunc is0so"0"bears no relation to the.position in"1e-7"andaddNumericSeparatorEndtreated"-7"as a digit run.The fix ports Node's current
formatNumberverbatim:-0check above thenumericSeparatorbranch (sinceString(-0) === "0")indexOf(".")rather than onString(Math.trunc(n)).lengthe) through unchangedNode v26.3.0 only had the
indexOfchange; the-0hoist and exponent-form guard landed upstream after that, so the "Node main" column above is what Node nightly produces.How did you verify your code works?
New
describe("inspect numericSeparator")block intest/js/node/util/util.test.jscovers the #23098 repro array, each negative fraction individually (including grouped-12_345.678_9),-0/0, large integer and fraction grouping, exponent-form inputs (1e-7,-1e-7,2e-7,1.23456e-7,1.2345...e+21), andNaN/Infinity/-Infinity.Also green: full
util.test.js(214),node-inspect-tests/parallel/util-format.test.js,node-inspect-tests/internal-inspect.test.js,test/js/bun/util/inspect.test.js. Theutil-inspect.test.js"no assertion failures 2" debug-build timeout reproduces identically on unmodified main (see #36147).Co-authored-by: 0xfandom kashyapshivank01@gmail.com
[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file