Fix/i32 min rem overflow (minor) - #5501
Conversation
Test262 conformance changes
Tested main commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5501 +/- ##
===========================================
+ Coverage 47.24% 62.81% +15.57%
===========================================
Files 476 536 +60
Lines 46892 60288 +13396
===========================================
+ Hits 22154 37871 +15717
+ Misses 24738 22417 -2321 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e6e00dd to
8b233c4
Compare
|
Feel free to mark this as ready for review whenever you're ready for a maintainer to take a look |
|
@nekevss Marked this Pr as ready for review. Let me know if there's anything you'd like me to change. |
There was a problem hiding this comment.
🟡 Changes recommended
The new rem_i32_min_by_neg_one test uses assert_eq which treats +0 and -0 as equal, so it doesn’t actually validate the negative-zero behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes an i32 signed-overflow panic in the % fast paths by switching to checked remainder and falling back to f64 arithmetic on overflow, aligning behavior with JavaScript (e.g., producing -0 instead of panicking).
Changes:
- Replace
x % ywithx.checked_rem(y)inrem()andrem_fast(), withf64fallback on overflow. - Add regression tests covering
i32::MIN % -1behavior for both the interpreter path andrem_fast().
File summaries
| File | Description |
|---|---|
| core/engine/src/value/operations.rs | Prevents % overflow panic for i32::MIN % -1 by using checked_rem with f64 fallback and correct -0 sign handling. |
| core/engine/src/value/tests.rs | Adds regression tests for i32::MIN % -1, including a rem_fast() sign-of-zero assertion. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Need to address Copilot's review first; either reject with a reasoning or include the review |
…ev#5481) The fast-path integer remainder (`rem` and `rem_fast`) used bare `x % y` which panics when `x = i32::MIN` and `y = -1` due to signed integer overflow. Replaced with `checked_rem`, falling back to `f64` arithmetic on overflow — the same pattern already used by `add`, `sub`, `mul`, and `div` in the same file. Closes boa-dev#5481
8b233c4 to
b9ff062
Compare
|
@jedel1043 I have addressed the Copilot review and now switched Ready for another look. |
This Pull Request fixes/closes #5481.
It changes the following:
Fixes a panic when computing
(-2147483648 | 0) % (-1 | 0). The fast-path integer remainder operations (rem and rem_fast) used bare x % y oni32values, which panics on signed integer overflow when x = i32::MIN and y = -1.Changes
core/engine/src/value/operations.rs: Replaced bare x % y with x.checked_rem(y) in both rem() and rem_fast(). On overflow (None), falls back to f64 arithmetic — the same pattern already used by add, sub, mul, and div in the same file.core/engine/src/value/tests.rs: Added regression test rem_i32_min_by_neg_one asserting the result is -0.0.Verification
(-2147483648 | 0) % (-1 | 0)now returns-0instead of panicking (matches Node.js behaviour)