-
-
Notifications
You must be signed in to change notification settings - Fork 781
fix(noPrecisionLoss): correctly handle large hex literals #8172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 96e6537 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughReplaces parsing-based overflow/precision checks for non-decimal numeric literals with a bitwise digit analysis in the correctness/noPrecisionLoss lint rule (computes bits-per-digit and checks highest/lowest set-bit positions for overflow or precision loss). Changes non-decimal number parsing in parse_js_number to a manual accumulator path that produces f64 for large hex/binary/octal literals. Adds and expands tests for noPrecisionLoss and noConstantMathMinMaxClamp covering large hex, binary, octal and decimal literals (including Infinity/overflow cases). Adds patch release notes. No public API signature changes. Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/biome_js_analyze/tests/specs/correctness/noPrecisionLoss/invalid.js (1)
36-42: Invalid repro literals exercise the new edge conditions wellThe added cases for 2^53+1, 2^100+1 and the 2^1024 overflow nicely mirror the new valid tests and directly cover the precision‑loss and overflow branches in the updated rule. Only minor nit:
0x20000000000001now appears twice in this file; not wrong, just a bit redundant if you ever want to trim the fixtures.crates/biome_js_analyze/src/lint/correctness/no_precision_loss.rs (1)
120-177: Bit‑span check for base 2/8/16 looks correct and matches the testsThe MSB/LSB tracking plus
span > 53andmsb >= 1024conditions give a precise characterisation of when an integer literal stops being exactly representable as an f64 or overflows, and the new tests cover those branches well. The only tiny nits (purely optional): the explicit'_'skip is redundant givensplit_into_radix_and_numberalready removes underscores, and you could early‑return oncespanexceeds 53 ormsbhits 1024 to avoid scanning the rest of very long literals—but that’s micro‑optimisation territory.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
crates/biome_js_analyze/tests/specs/correctness/noConstantMathMinMaxClamp/invalid_large_hex.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noPrecisionLoss/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noPrecisionLoss/valid.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
.changeset/odd-kings-obey.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_precision_loss.rs(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noConstantMathMinMaxClamp/invalid_large_hex.js(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noPrecisionLoss/invalid.js(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noPrecisionLoss/valid.js(1 hunks)crates/biome_js_syntax/src/numbers.rs(2 hunks)
🔇 Additional comments (5)
.changeset/odd-kings-obey.md (1)
1-7: Changeset entry looks good.The format is correct, references are accurate, and the description clearly conveys what was fixed and which rules are affected. No issues here.
crates/biome_js_syntax/src/numbers.rs (2)
40-46: Non‑decimal accumulator looks correct and avoids i64 limitsThe
value = value * base + digitpath is a clean way to parse large binary/octal/hex literals directly asf64, and it lines up with the new tests around 2^53+1 and overflow. No issues spotted here.
83-89: Good boundary coverage for hex literalsThe new tests around
0x20000000000001and the huge 2^1024 literal nicely pin down both precision‑loss and overflow behaviour for hex parsing. Comment + expected values match what JS actually does at runtime.crates/biome_js_analyze/tests/specs/correctness/noConstantMathMinMaxClamp/invalid_large_hex.js (1)
1-2: Nice regression test for large hex in Math.min/Math.maxThis captures the previous i64 overflow edge case well and should keep
noConstantMathMinMaxClamphonest for very large hex literals.crates/biome_js_analyze/tests/specs/correctness/noPrecisionLoss/valid.js (1)
79-85: New “valid” big literals match the exact‑representable setMarking 2^53, 2^100 and the 2^1023 hex literal as valid is consistent with the updated “exact f64” notion of precision loss and the linked issue. Tests look spot on.
crates/biome_js_analyze/src/lint/correctness/no_precision_loss.rs
Outdated
Show resolved
Hide resolved
...biome_js_analyze/tests/specs/correctness/noConstantMathMinMaxClamp/invalid_large_hex.js.snap
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/correctness/noPrecisionLoss/invalid.js
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/correctness/noPrecisionLoss/invalid.js
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/correctness/noPrecisionLoss/valid.js
Outdated
Show resolved
Hide resolved
CodSpeed Performance ReportMerging #8172 will improve performances by 6.65%Comparing Summary
Benchmarks breakdown
Footnotes
|
crates/biome_js_analyze/src/lint/correctness/no_precision_loss.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/correctness/no_precision_loss.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/correctness/no_precision_loss.rs
Outdated
Show resolved
Hide resolved
ematipico
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 💯
dyc3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff!
Disclaimer: large parts of this PR were written by Gemini 3, I reviewed each change manually and made some changes.
Summary
Fixes #8145, hex literals like 0x20_0000_0000_0000 or 0x30_0000_0000_0000 would previously be incorrectly flagged, instead of correctly checking if the provided literal is exactly representable as an f64 it would just check if it's in the safe integer range.
While working on this I also noticed a problem that affects another lint rule noConstantMathMinMaxClamp where large hex literals above 2^63 - 1 would never trigger the error because as_number() internally tried to parse into an i64 which is incorrect.
Test Plan
There are existing tests along with some newly added ones.