Fix BigInt shift_right/shift_left for large shift amounts#4710
Fix BigInt shift_right/shift_left for large shift amounts#4710jedel1043 merged 2 commits intoboa-dev:mainfrom
Conversation
I would suggest adding a comment to the specific section of the spec that mentions this, just for future reference. |
Test262 conformance changes
Fixed tests (1):Broken tests (8): |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4710 +/- ##
==========================================
+ Coverage 47.24% 57.12% +9.87%
==========================================
Files 476 552 +76
Lines 46892 60500 +13608
==========================================
+ Hits 22154 34559 +12405
- Misses 24738 25941 +1203 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Per the ECMAScript specification, the Signed Right Shift Operator (
For sufficiently large positive
Therefore, shifting by a very large positive amount must return |
|
@Ansh-699 That sound more like it's not part of the spec. I guess we can still apply this safeguard, but let's be very clear with a comment that this is just a best-effort safeguard, not a spec-related result. |
|
ok no worries |
Right-shifting a BigInt by a value that doesn't fit in i32 (e.g. 1000n >> 1000000000000000n) incorrectly threw RangeError. Per spec, arithmetic right shift by a large positive amount should return 0n for non-negative values and -1n for negative values. The same logic is applied symmetrically to shift_left when the shift amount is a large negative number.
Ansh-699
force-pushed
the
fix/bigint-shift-right-large-amount
branch
from
7eda4e5 to
f237dd4
Compare
|
Updated — thanks for the feedback! Changes in this push:
|
jedel1043
left a comment
jedel1043
left a comment
There was a problem hiding this comment.
Looks great! Just some small nitpicks about the style, but the implementation looks good
core/engine/src/bigint.rs
Outdated
| if y.inner.sign() == Sign::Minus { | ||
| // x >> (large negative) is equivalent to x << (large positive), which overflows. | ||
| Err(JsNativeError::range() | ||
| .with_message("Maximum BigInt size exceeded") | ||
| .into()) | ||
| } else { | ||
| // x >> (large positive): all bits are shifted out. | ||
| if x.inner.sign() == Sign::Minus { | ||
| Ok(Self::new(RawBigInt::from(-1))) | ||
| } else { | ||
| Ok(Self::zero()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Nitpick: maybe this would look a bit nicer with a match
match (x.inner.sign(), y.inner.sign()) {
(Sign::Minus, Sign::Plus) => -1,
(Sign::Plus, Sign::Plus) => 0,
(_, Sing::Minus) => Err,
}
core/engine/src/bigint.rs
Outdated
| None => { | ||
| if y.inner.sign() == Sign::Minus { | ||
| // x << (large negative) is equivalent to x >> (large positive): all bits shifted out. | ||
| if x.inner.sign() == Sign::Minus { | ||
| Ok(Self::new(RawBigInt::from(-1))) | ||
| } else { | ||
| Ok(Self::zero()) | ||
| } | ||
| } else { | ||
| // x << (large positive) overflows. | ||
| Err(JsNativeError::range() | ||
| .with_message("Maximum BigInt size exceeded") | ||
| .into()) | ||
| } |
There was a problem hiding this comment.
Same suggestion as above; a match could be easier to read overall.
jedel1043
added
bug
2 participants
1000n >> 1000000000000000nincorrectly throwsRangeError: Maximum BigInt size exceededinstead of returning0n. The shift amount does not fit ini32, soto_i32()returnsNoneand the code falls through to the error branch unconditionally.Per the ECMAScript spec (and confirmed against V8/SpiderMonkey), arithmetic right-shifting by a very large positive amount should return:
0nfor non-negative values-1nfor negative valuesThe same fix is applied symmetrically to
shift_leftwhen the shift amount is a large negative number (equivalent to a large right-shift).Changes:
JsBigInt::shift_right: check sign ofyin theNonearm — large positiveyreturns0n/-1n, large negativeykeeps theRangeErrorJsBigInt::shift_left: mirror logic — large negativeyreturns0n/-1n, large positiveykeeps theRangeErrorassert_native_errorwithassert_eqfor the right-shift case, added coverage for negative BigInt and zero