fix(node:util): correct numericSeparator for negative fractional numbers - #32460
fix(node:util): correct numericSeparator for negative fractional numbers#324600xfandom wants to merge 3 commits into
Conversation
`util.inspect(n, { numericSeparator: true })` mangled negative
fractional numbers: `-0.12` rendered as `0..12` (missing sign, doubled
decimal point). `formatNumber` derived the integer part from
`String(Math.trunc(n))`, but `Math.trunc(-0.12)` is `-0` and
`String(-0)` is `"0"` — dropping the sign and throwing off the offset
used to slice out the fractional digits.
Split the number's own string representation on the decimal point
instead, matching Node's current implementation, and move the `-0`
check ahead of the separator branch so `-0` keeps its sign there too.
Closes oven-sh#23098
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesnumericSeparator formatting fix
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
|
Came across this while chasing a fuzz finding that has the same root cause: exponent-form numbers are corrupted by the old util.inspect(1.23456e-7, { numericSeparator: true })
// before: '0.234_56e_-7' (leading 1 replaced with 0)
util.inspect(1e-7, { numericSeparator: true })
// before: '0.-7'
util.inspect(-1.23456e-7, { numericSeparator: true })
// before: '0..23_456_e-7'The fix in this PR already handles these (via the expect(sep(1.23456e-7)).toBe("1.23456e-7");
expect(sep(-1.23456e-7)).toBe("-1.23456e-7");
expect(sep(1e-7)).toBe("1e-7");
expect(sep(1.5e21)).toBe("1.5e+21");Closed #32865 as a duplicate of this. |
|
Came at this from fuzzing Still reproduces on main today ( util.inspect(-1e-7, { numericSeparator: true }); // "0.e-7" sign gone
util.inspect(1e-7, { numericSeparator: true }); // "0.-7" two distinct
util.inspect(2e-7, { numericSeparator: true }); // "0.-7" numbers, one output
util.inspect(-0.5, { numericSeparator: true }); // "0..5"This diff, applied on top of current main, fixes all of them. It applies cleanly; I built it and ran On matching Node, since that's the obvious objection to the
The One divergence worth a line in the description: hoisting the Coverage for the exponent path, if you want to drop it into the it("does not insert separators into exponential notation", () => {
expect(sep(1e-7)).toBe("1e-7");
expect(sep(2e-7)).toBe("2e-7");
expect(sep(-1e-7)).toBe("-1e-7");
expect(sep(1.2345678e-7)).toBe("1.2345678e-7");
expect(sep(5e-324)).toBe("5e-324");
expect(sep(1e21)).toBe("1e+21");
});
it("is applied to typed array elements and format specifiers", () => {
expect(sep(new Float64Array([1e-7, -1e-7]))).toBe("Float64Array(2) [ 1e-7, -1e-7 ]");
expect(sep(new Float64Array([-0.5, 1234.5678]))).toBe("Float64Array(2) [ -0.5, 1_234.567_8 ]");
expect(util.formatWithOptions({ numericSeparator: true }, "%d", -1e-7)).toBe("-1e-7");
}); |
The old integer-length split corrupted these: 1e-7 became "0.-7", -1e-7 lost its sign as "0.e-7", and 1.23456e-7 became "0.234_56e_-7".
|
Thanks — added those as test cases in 0397027: expect(sep(1e-7)).toBe("1e-7"); // was "0.-7"
expect(sep(-1e-7)).toBe("-1e-7"); // was "0.e-7" (sign dropped)
expect(sep(2e-7)).toBe("2e-7"); // was "0.-7" (collided with 1e-7)
expect(sep(1.23456e-7)).toBe("1.23456e-7"); // was "0.234_56e_-7"
expect(sep(-0.5)).toBe("-0.5"); // was "0..5"
expect(sep(1234567891234567891234)).toBe("1.234567891234568e+21");Also added NaN/±Infinity cases. Note the exponent forms come back unseparated rather than with separators spliced into the mantissa — Node has no defined behavior here (its own
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/util/util.test.js`:
- Around line 462-463: Replace the explanatory comments on the affected
regression-test lines with exactly the issue URL comment specified in the
review, including the same URL and formatting; do not retain the existing
explanation.
🪄 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 Plus
Run ID: 081c2901-1ce6-4e27-a620-1398d07377e8
📒 Files selected for processing (1)
test/js/node/util/util.test.js
| // The old integer-length split corrupted these: 1e-7 became "0.-7", | ||
| // -1e-7 lost its sign as "0.e-7", and 1.23456e-7 became "0.234_56e_-7". |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the issue URL as the regression-test comment.
Replace the explanatory comments on Lines 462-463 and 469 with exactly:
- // The old integer-length split corrupted these: 1e-7 became "0.-7",
- // -1e-7 lost its sign as "0.e-7", and 1.23456e-7 became "0.234_56e_-7".
+ // https://github.com/oven-sh/bun/issues/23098
...
- // Integers large enough to stringify in exponent form take the same path.As per coding guidelines, regression tests must use exactly the issue URL comment.
Also applies to: 469-469
🤖 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 `@test/js/node/util/util.test.js` around lines 462 - 463, Replace the
explanatory comments on the affected regression-test lines with exactly the
issue URL comment specified in the review, including the same URL and
formatting; do not retain the existing explanation.
Source: Coding guidelines
|
Addressed in 869e1c4 — swapped the explanatory comments for the issue URL ( |
|
Triage note: #36501 was a second copy of this change (same |
3 participants
What does this PR do?
Fixes
util.inspect(value, { numericSeparator: true })for negative fractional numbers. Fixes #23098.Before:
After (matches Node):
// [ 0.123_4, -0.12, -0.123, -0.123_4, -1.234 ]formatNumberderived the integer part fromString(Math.trunc(n)). For-1 < n < 0,Math.trunc(-0.12)is-0andString(-0)is"0", so the sign was dropped and the offset used to slice the fractional digits was off by one — producing0..12. The fix splits the number's own string representation on the decimal point (matching Node's currentformatNumber) and moves the-0check ahead of the separator branch so-0keeps its sign there too.How did you verify your code works?
Added a
numericSeparatordescribe block totest/js/node/util/util.test.js:[ 0.123_4, -0.12, -0.123, -0.123_4, -1.234 ])-12345.6789→-12_345.678_9)-0keeps its sign with the separator enabledThe new tests pass on the debug build and fail against the release
bun(proving they exercise the change). Fullutil.test.js(196) and the Node inspect suite pass with no regressions.