fix(runtime,codegen): bitnot ToNumber coercion + modulus zero/sign-of-zero#5822
Conversation
…-zero (#5815) Two spec bugs caught by the test262 radar: **`~x` (bitwise NOT) — `js_dynamic_bitnot`** - Missing `ToNumber` coercion before `ToInt32`: `~new Boolean(true)` gave `-1` (treating the NaN-boxed pointer as `0`) instead of `~1 = -2`. Fixed by calling `js_number_coerce` before the i64 truncation, which dispatches through `valueOf`/`toPrimitive` for wrapper objects and strings. - Wrong `ToInt32` for `±Infinity`/`NaN`: Rust's saturating `f64 as i64` gives `i64::MAX` for `+Infinity` → `i64::MAX as i32 = -1` → `~(-1) = 0`, but the spec says `ToInt32(±Infinity) = 0` → `~0 = -1`. Fixed with an explicit `if is_nan || !is_finite { 0i32 }` guard. Fixes `S11.4.8_A2.2_T1`, `S11.4.8_A3_T1`, `S11.4.8_A3_T2`, `S11.4.8_A3_T3` (100% parity on `language/expressions/bitwise-not`). **`x % y` (modulus) — integer fast path in codegen** - Zero-divisor UB: `srem(x, 0)` is undefined behaviour in LLVM (ARM gives `0` silently; JS requires `NaN`). Guard added: skip the `fptosi/srem` fast path when the RHS is a literal `0` or `0.0` and fall through to `frem` (which gives `NaN` per IEEE 754). - Sign-of-zero: `srem` returns an `i64` zero and `sitofp` always produces `+0.0`, but JS requires `-0.0` when the dividend was negative (e.g. `-1 % -1 === -0`). Fixed by emitting: `if m == 0 && l < 0.0 → fneg(0.0)`. Fixes `S11.5.3_A4_T2`, `S11.5.3_A4_T4`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTwo JavaScript spec compliance fixes: the ChangesJS Arithmetic Correctness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/perry-codegen/src/expr/binary.rs`:
- Around line 223-245: The integer modulo fast path in binary expression
lowering still relies on right_is_known_zero, which only excludes literal zero
but not integer-valued RHS expressions that can evaluate to zero at runtime. In
lower_expr / the BinaryOp::Mod branch, add a runtime check after computing ri in
the integer fast path and avoid calling srem when ri is 0; if the divisor is
zero at runtime, fall back to the floating-point remainder path instead. Use the
existing is_integer_valued_expr, lower_expr, and srem handling in Expr::Mod as
the place to make this guard.
In `@crates/perry-runtime/src/value/dynamic_arith.rs`:
- Around line 464-467: The bitwise-not path in dynamic_arith.rs is using `a_num
as i64 as i32`, which can saturate on large finite numbers and produce the wrong
ToInt32 result. Update the `!` handling to follow the existing JS ToInt32 helper
flow used elsewhere in `dynamic_arith.rs`/`perry-runtime`, applying `trunc()`
with `rem_euclid(2^32)` before casting to `i32`, while keeping the
NaN/±0/±Infinity case mapping to 0.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 561275ea-5fa6-46c5-a2a4-f6dc22cbfa48
📒 Files selected for processing (2)
crates/perry-codegen/src/expr/binary.rscrates/perry-runtime/src/value/dynamic_arith.rs
| let right_is_known_zero = matches!(**right, Expr::Integer(0)) | ||
| || matches!(**right, Expr::Number(v) if v == 0.0); | ||
| if matches!(op, BinaryOp::Mod) | ||
| && crate::type_analysis::is_integer_valued_expr(ctx, left) | ||
| && crate::type_analysis::is_integer_valued_expr(ctx, right) | ||
| && !right_is_known_zero | ||
| { | ||
| let l_raw = lower_expr(ctx, left)?; | ||
| let r_raw = lower_expr(ctx, right)?; | ||
| let blk = ctx.block(); | ||
| let li = blk.fptosi(DOUBLE, &l_raw, I64); | ||
| let ri = blk.fptosi(DOUBLE, &r_raw, I64); | ||
| let m = blk.srem(I64, &li, &ri); | ||
| return Ok(blk.sitofp(I64, &m, DOUBLE)); | ||
| // IEEE 754: when the integer remainder is 0 and the | ||
| // dividend was negative, the result must be -0.0. | ||
| // srem gives 0i64 → sitofp always produces +0.0, | ||
| // so correct: if m==0 && l<0 → fneg(0.0) = -0.0. | ||
| let result_f = blk.sitofp(I64, &m, DOUBLE); | ||
| let m_is_zero = blk.icmp_eq(I64, &m, "0"); | ||
| let l_neg = blk.fcmp("olt", &l_raw, "0.0"); | ||
| let need_neg = blk.and(I1, &m_is_zero, &l_neg); | ||
| let neg_result = blk.fneg(&result_f); | ||
| return Ok(blk.select(I1, &need_neg, DOUBLE, &neg_result, &result_f)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm is_integer_valued_expr can match non-literal (variable / loop-counter) expressions,
# making a runtime-zero RHS reachable on the srem fast path.
fd -e rs | xargs rg -nP 'fn\s+is_integer_valued_expr'
ast-grep run --pattern 'fn is_integer_valued_expr($$$) { $$$ }' --lang rust $(fd type_analysis.rs)Repository: PerryTS/perry
Length of output: 279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline crates/perry-codegen/src/type_analysis/numeric.rs --view expanded || true
ast-grep outline crates/perry-codegen/src/expr/binary.rs --view expanded || true
# Read the helper and the modulo fast-path in manageable slices.
sed -n '240,380p' crates/perry-codegen/src/type_analysis/numeric.rs
printf '\n--- binary.rs ---\n'
sed -n '200,280p' crates/perry-codegen/src/expr/binary.rs
# Find other uses of the helper to understand intended scope.
rg -n "is_integer_valued_expr\(" crates/perry-codegen/srcRepository: PerryTS/perry
Length of output: 9006
Guard the integer modulo fast path against runtime zero divisors
right_is_known_zero only filters literal 0/0.0, but is_integer_valued_expr also accepts integer locals, updates, and nested integer arithmetic, so a non-literal RHS can still be 0 at runtime and reach srem(I64, ..., 0). Add a runtime ri == 0 check and fall back to frem in that case.
🤖 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 `@crates/perry-codegen/src/expr/binary.rs` around lines 223 - 245, The integer
modulo fast path in binary expression lowering still relies on
right_is_known_zero, which only excludes literal zero but not integer-valued RHS
expressions that can evaluate to zero at runtime. In lower_expr / the
BinaryOp::Mod branch, add a runtime check after computing ri in the integer fast
path and avoid calling srem when ri is 0; if the divisor is zero at runtime,
fall back to the floating-point remainder path instead. Use the existing
is_integer_valued_expr, lower_expr, and srem handling in Expr::Mod as the place
to make this guard.
| // ES ToInt32: NaN, ±0, ±Infinity all map to 0; finite values use | ||
| // C-style i64 truncation (equivalent to modulo-2^32 + sign-extend). | ||
| let a_i32 = if a_num.is_nan() || !a_num.is_finite() { 0i32 } else { a_num as i64 as i32 }; | ||
| (!a_i32) as f64 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Rust f64 as i64 cast saturating behavior out of range
💡 Result:
As of Rust 1.45.0, casting a floating-point number (f64 or f32) to an integer type (i64, i32, etc.) using the as keyword is defined to perform a saturating cast [1][2][3]. This behavior is stable, safe, and explicitly defined, replacing what was previously considered undefined behavior (UB) [2][3][4]. The rules for as casting from f64 to i64 are as follows: 1. Rounding: The floating-point value is rounded towards zero (truncated) [5][2][4]. 2. Out-of-Range Values: - If the value is greater than i64::MAX, it saturates (clamps) to i64::MAX [5][2][6]. - If the value is less than i64::MIN, it saturates (clamps) to i64::MIN [5][2][6]. - Infinities (f64::INFINITY and f64::NEG_INFINITY) follow these same clamping rules (saturating to i64::MAX and i64::MIN, respectively) [2][6]. 3. NaN (Not a Number): NaN values are converted to 0 [2][3][6]. If you require performance-critical code where you can guarantee the floating-point value is within a valid range, you can use the to_int_unchecked method provided on floating-point primitives [7][3][4]. This method is unsafe because it bypasses the saturation checks; using it with values outside the representable range or with NaN/infinity results in undefined behavior [7][2][4].
Citations:
- 1: https://users.rust-lang.org/t/convert-an-f64-to-an-i32/50991
- 2: Define UB in float-to-int casts to saturate rust-lang/rust#71269
- 3: https://stackoverflow.com/questions/61144957/what-happens-when-casting-a-big-float-to-a-int
- 4: https://doc.rust-lang.org/rust-by-example/types/cast.html
- 5: https://rust-for-c-programmers.com/ch16/16_2_primitive_casting_with_as.html
- 6: https://rust.googlesource.com/rust/+/0ce5963923c8809036302d927ee6545cdd2071f9/tests/ui/numbers-arithmetic/saturating-float-casts-impl.rs
- 7: https://stackoverflow.com/questions/71430186/how-to-safely-convert-float-to-int-in-rust
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the referenced lines.
sed -n '420,500p' crates/perry-runtime/src/value/dynamic_arith.rs
echo
echo '--- SEARCH ToInt32 helpers ---'
rg -n "ToInt32|to_int32|Int32" crates/perry-runtime/src -g '!**/target/**' || trueRepository: PerryTS/perry
Length of output: 14514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '420,500p' crates/perry-runtime/src/value/dynamic_arith.rs
echo
echo '--- SEARCH ToInt32 helpers ---'
rg -n "ToInt32|to_int32|Int32" crates/perry-runtime/src -g '!**/target/**' || trueRepository: PerryTS/perry
Length of output: 14514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- math.rs ToInt32 helper ---'
sed -n '1,120p' crates/perry-runtime/src/math.rs
echo
echo '--- builtin_thunks.rs ToInt32 helper ---'
sed -n '70,110p' crates/perry-runtime/src/object/global_this/builtin_thunks.rs
echo
echo '--- parse_int helper ---'
sed -n '80,120p' crates/perry-runtime/src/builtins/numbers.rsRepository: PerryTS/perry
Length of output: 5596
Use the JS ToInt32 path here instead of as i64 as i32. f64 as i64 saturates for large finite inputs, so values like 1e300 produce the wrong bitwise-not result. Reuse the existing trunc().rem_euclid(2^32) helper logic before converting to i32.
🤖 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 `@crates/perry-runtime/src/value/dynamic_arith.rs` around lines 464 - 467, The
bitwise-not path in dynamic_arith.rs is using `a_num as i64 as i32`, which can
saturate on large finite numbers and produce the wrong ToInt32 result. Update
the `!` handling to follow the existing JS ToInt32 helper flow used elsewhere in
`dynamic_arith.rs`/`perry-runtime`, applying `trunc()` with `rem_euclid(2^32)`
before casting to `i32`, while keeping the NaN/±0/±Infinity case mapping to 0.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Fixes two spec compliance bugs found during the test262 grind (#5815):
~x—js_dynamic_bitnot(runtime)~new Boolean(true),~new String("3"),~{valueOf(){return 2}}were treating the NaN-boxed pointer as0(yielding-1) instead of coercing throughjs_number_coercefirst. Addedjs_number_coerce(a)before theToInt32truncation.f64 as i64givesi64::MAXfor+Infinity, theni64::MAX as i32 = -1, so~Infinityreturned0instead of-1. The ES spec saysToInt32(±Infinity) = 0, so~Infinity = ~0 = -1. Fixed with an explicitif is_nan || !is_finite { 0i32 }guard.Fixes
S11.4.8_A2.2_T1,S11.4.8_A3_T1,S11.4.8_A3_T2,S11.4.8_A3_T3→ 100% parity onlanguage/expressions/bitwise-not(15/15).x % y— integer modulus fast path (codegen)srem(x, 0)is LLVM undefined behaviour (ARM silently gives0; JS requiresNaNforx % 0). Added aright_is_known_zeroguard that skips thefptosi/sremfast path when the RHS is a literal0or0.0, falling through tofremwhich is IEEE 754 correct.sremalways produces an integer0, andsitofpconverts that to+0.0. JS requires-0.0when the dividend is negative (e.g.-1 % -1 === -0, checked via1 / (-1 % -1) === -Infinity). Fixed by emitting:if m == 0 && l < 0.0 { fneg(0.0) } else { sitofp(m) }.Fixes
S11.5.3_A4_T2,S11.5.3_A4_T4.Test plan
bitnot: -2 1 -2 -4(was-1 -1 -1 ...for wrapper objects)mod: -0 -Infinity NaN -Infinity(was0 Infinity 0 Infinity)language/expressions/bitwise-nottest262 subset: 15/15 pass (was 11/15)language/expressions/modulustest262 subset: 36/37 pass (remaining 1 is pre-existingObject(1n) % 1BigInt-wrapper TypeError, unrelated to this PR)lint,cargo-test,api-docs-drift,security-audit)🤖 Generated with Claude Code
Summary by CodeRabbit
%) behavior to better match JavaScript, including correct handling of-0results and cases involving zero divisors.~) for non-BigIntvalues so inputs are coerced and truncated according to JavaScript rules, with non-finite values treated as0.