Skip to content

fix(runtime,codegen): bitnot ToNumber coercion + modulus zero/sign-of-zero #5822

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.

Sign up for GitHub

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

Merged
proggeramlug merged 2 commits into from
Jun 30, 2026
Merged

fix(runtime,codegen): bitnot ToNumber coercion + modulus zero/sign-of-zero #5822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a4524e
fix(runtime,codegen): bitnot ToNumber coercion + modulus zero/sign-of…
Jun 30, 2026
3b7f300
style: rustfmt
Jun 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions crates/perry-codegen/src/expr/binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,19 +216,33 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
// fptosi, producing the wrong result. `is_integer_valued_expr`
// only returns true when we can prove the value is a whole
// number (integer literals, integer loop counters, or nested
// integer arithmetic). For everything else we fall through
// to the `frem` path.
// integer arithmetic). A zero RHS falls through to `frem`
// because srem(x,0) is UB in LLVM (on ARM the CPU silently
// gives 0, but JS requires NaN for any x % 0). For everything
// else we fall through to the `frem` path.
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));
Comment on lines +223 to +245

@coderabbitai coderabbitai Bot Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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/src

Repository: 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.

}

let (l_raw, l_fallback_coerced) = lower_arithmetic_operand(ctx, left)?;
Expand Down
12 changes: 11 additions & 1 deletion crates/perry-runtime/src/value/dynamic_arith.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,17 @@ pub unsafe extern "C" fn js_dynamic_bitnot(a: f64) -> f64 {
);
return js_nanbox_bigint(result as i64);
}
(!(a as i64 as i32)) as f64
// Apply ToNumber first so that ~"3", ~true, ~new Boolean(true), etc.
// coerce correctly before the ToInt32 truncation.
let a_num = crate::builtins::js_number_coerce(a);
// 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
Comment on lines +464 to +471

@coderabbitai coderabbitai Bot Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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:

🏁 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/**' || true

Repository: 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/**' || true

Repository: 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.rs

Repository: 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.

}

/// Dynamic right shift: BigInt >> if either operand is BigInt, else i32 >> for numbers.
Expand Down
Loading