Fix/i32 min rem overflow (minor) - #5501
Conversation
github-actions
Bot
added
the
Waiting On Review
Test262 conformance changes
Tested main commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5501 +/- ##
===========================================
+ Coverage 47.24% 62.43% +15.19%
===========================================
Files 476 534 +58
Lines 46892 59747 +12855
===========================================
+ Hits 22154 37304 +15150
+ Misses 24738 22443 -2295 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…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
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.
| #[test] | ||
| fn rem_i32_min_by_neg_one() { | ||
| run_test_actions([TestAction::assert_eq("(-2147483648 | 0) % (-1 | 0)", -0.0)]); | ||
| } |
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)