fix(runtime): js_dynamic_mod preserves sign of zero (fmod semantics)#5999
Merged
Conversation
js_dynamic_mod computed the remainder with the closed form a - (a / b).trunc() * b. That drops the sign of a zero result: -1 % -1 evaluates to -1.0 - (1.0 * -1.0) == +0.0, but JS % is C fmod, whose result takes the sign of the dividend, so -1 % -1 is -0. The same closed form also returned NaN for x % Infinity (should be x). This surfaced as a regression once a compound assignment on a separately-declared int-valued local (var x; x = -1; x %= -1) started routing through js_dynamic_mod: language/expressions/compound-assignment/mod-whitespace.js which asserts x %= -1 is -0. Rust's f64 % f64 is fmod, so replace the closed form with a % b. It matches the spec on the sign of zero, on x % Infinity == x, and on Infinity % y / x % 0 == NaN. Verified on an internal Linux sweep host: mod-whitespace passes; the compound-assignment and modulus slices are 100% (0 runtime-fails).
|
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)
📝 WalkthroughWalkthroughThe float modulo computation in ChangesFloat Modulo Fix
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 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 |
This was referenced Jul 5, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Jul 5, 2026
…essions A test262 sweep of main at the #6008 tip regressed vs the #5979 tip (parity 96.5% -> 95.8%, Temporal self-validated 99.1% -> 96.7%, 322 tests newly failing). git bisect (good 28948ea .. bad 76cc2b9, witness = built-ins/Boolean + Number + Temporal.Duration parity) lands on 849f297 "Improve Zod compatibility support" (#5874) as the first bad commit: 98.3% witness parity at its parent-2 (4f5bc6a) drops to 93.9% at #5874 (Boolean 89%->67%, Temporal 100%->95%, self-validated 100%->95.1%). #5874 was a 156-file monolithic squash that swept in unreviewed codegen/runtime WIP + two 5.1MB binaries (constv, direct) + ~30 stray tests/*.sh alongside the intended Zod work. Two swept changes dominate: - codegen/lower_call/property_get/number_string.rs gated the universal .toString() interception on is_numeric_expr || is_bigint_expr, so boxed built-in receivers (new String()/Number()/Date()) with a user-assigned built-in method bypassed the brand-check and stopped throwing TypeError (Boolean S15.6.4.2_A2_T1..T3) -> the "method is not a function" + SameValue clusters. - the runtime prototype-identity / class-registry / instanceof rewrite regressed Temporal subclass prototype resolution (Object.getPrototypeOf(result) === construct.prototype) -> the Temporal 159 + "(no output)" crash clusters. This is the inverse patch of the #5874 payload applied to CURRENT main (not a snapshot restore), so the later fleet merges are preserved: - #5991 (class X extends Promise): its promise_parent_runtime scaffold, emit_promise_subclass_init wiring, and new_helpers additions are kept (subclass executor resolves and .then sees the value). - #5999 (js_dynamic_mod fmod sign-of-zero): kept; only #5874s dynamic_number_operand double-coercion was dropped from the modulo path. - #5983 (external-http-client-pump dropped from stdlib full): untouched and links green. new.rs / new_helpers.rs: reverting new.rs to its pre-#5874 form put it at 2000 lines; applying #5991s +9 net lines on top pushed it to 2009, over the 2000-line file-size gate. Moved three self-contained ctor predicate helpers (effective_constructor_param_count, local_constructor_symbol_exists, ctor_chain_uses_new_target) into the new_helpers sibling to restore the split (new.rs -> 1939 lines). Verified on an internal Linux sweep host: witness slice back to 98.3% / self-validated 100% / Temporal.Duration 100%; fmt + file-size + gc-store-site + addr-class audits clean. #5874 remains in history for its author to re-land in reviewable pieces.
proggeramlug
pushed a commit
that referenced
this pull request
Jul 5, 2026
…main (preserves 5 later fixes) The #5983 squash unknowingly re-landed force-rewound PR #5874 (Improve Zod compatibility, 849f297, 161 files incl the constv/direct binaries and a class-lowering change that regresses temporal_subclass_capture_writeback_inner_class: 'class X extends <param>' flips from static New to ClassExprFresh/NewDynamic and the capture writeback is lost). Inverse patch of the #5874 payload applied to CURRENT main via 3-way, so the fleet merges that landed on top are preserved: - #5991 (extends Promise): new.rs/new_helpers.rs taken from the prior working revert (unchanged by the 11 intervening merges); its promise_parent_runtime + emit_promise_subclass_init kept, #5874's map_set_default_super_kind scaffold dropped. - #5999 (js_dynamic_mod fmod sign-of-zero): #5874's redundant dynamic_number_operand insertion removed, #5999's 'a % b' kept. - #6004/#6005/#6007 (exotic-receiver dispatch, proxy get, seal on functions): 3-way removed ONLY #5874's hunks from native_call_method.rs/proxy.rs; the later fixes' additions stay. Verified: temporal 6/6 (was 5/6 on main), 806 unchanged, stdlib links, proxy e2e green, and behavioral probes for #5999 (mod -0), #5991 (promise subclass = 42), #6004/#6007 (exotic dispatch + fn seal) all match node. Both stray binaries deleted. #5874 stays in history at 4500b5d for its author to re-land through review after fixing the extends regression.
proggeramlug
added a commit
that referenced
this pull request
Jul 5, 2026
…essions (#6015) A test262 sweep of main at the #6008 tip regressed vs the #5979 tip (parity 96.5% -> 95.8%, Temporal self-validated 99.1% -> 96.7%, 322 tests newly failing). git bisect (good 28948ea .. bad 76cc2b9, witness = built-ins/Boolean + Number + Temporal.Duration parity) lands on 849f297 "Improve Zod compatibility support" (#5874) as the first bad commit: 98.3% witness parity at its parent-2 (4f5bc6a) drops to 93.9% at #5874 (Boolean 89%->67%, Temporal 100%->95%, self-validated 100%->95.1%). #5874 was a 156-file monolithic squash that swept in unreviewed codegen/runtime WIP + two 5.1MB binaries (constv, direct) + ~30 stray tests/*.sh alongside the intended Zod work. Two swept changes dominate: - codegen/lower_call/property_get/number_string.rs gated the universal .toString() interception on is_numeric_expr || is_bigint_expr, so boxed built-in receivers (new String()/Number()/Date()) with a user-assigned built-in method bypassed the brand-check and stopped throwing TypeError (Boolean S15.6.4.2_A2_T1..T3) -> the "method is not a function" + SameValue clusters. - the runtime prototype-identity / class-registry / instanceof rewrite regressed Temporal subclass prototype resolution (Object.getPrototypeOf(result) === construct.prototype) -> the Temporal 159 + "(no output)" crash clusters. This is the inverse patch of the #5874 payload applied to CURRENT main (not a snapshot restore), so the later fleet merges are preserved: - #5991 (class X extends Promise): its promise_parent_runtime scaffold, emit_promise_subclass_init wiring, and new_helpers additions are kept (subclass executor resolves and .then sees the value). - #5999 (js_dynamic_mod fmod sign-of-zero): kept; only #5874s dynamic_number_operand double-coercion was dropped from the modulo path. - #5983 (external-http-client-pump dropped from stdlib full): untouched and links green. new.rs / new_helpers.rs: reverting new.rs to its pre-#5874 form put it at 2000 lines; applying #5991s +9 net lines on top pushed it to 2009, over the 2000-line file-size gate. Moved three self-contained ctor predicate helpers (effective_constructor_param_count, local_constructor_symbol_exists, ctor_chain_uses_new_target) into the new_helpers sibling to restore the split (new.rs -> 1939 lines). Verified on an internal Linux sweep host: witness slice back to 98.3% / self-validated 100% / Temporal.Duration 100%; fmt + file-size + gc-store-site + addr-class audits clean. #5874 remains in history for its author to re-land in reviewable pieces. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
pushed a commit
that referenced
this pull request
Jul 5, 2026
…main (preserves 5 later fixes) The #5983 squash unknowingly re-landed force-rewound PR #5874 (Improve Zod compatibility, 849f297, 161 files incl the constv/direct binaries and a class-lowering change that regresses temporal_subclass_capture_writeback_inner_class: 'class X extends <param>' flips from static New to ClassExprFresh/NewDynamic and the capture writeback is lost). Inverse patch of the #5874 payload applied to CURRENT main via 3-way, so the fleet merges that landed on top are preserved: - #5991 (extends Promise): new.rs/new_helpers.rs taken from the prior working revert (unchanged by the 11 intervening merges); its promise_parent_runtime + emit_promise_subclass_init kept, #5874's map_set_default_super_kind scaffold dropped. - #5999 (js_dynamic_mod fmod sign-of-zero): #5874's redundant dynamic_number_operand insertion removed, #5999's 'a % b' kept. - #6004/#6005/#6007 (exotic-receiver dispatch, proxy get, seal on functions): 3-way removed ONLY #5874's hunks from native_call_method.rs/proxy.rs; the later fixes' additions stay. Verified: temporal 6/6 (was 5/6 on main), 806 unchanged, stdlib links, proxy e2e green, and behavioral probes for #5999 (mod -0), #5991 (promise subclass = 42), #6004/#6007 (exotic dispatch + fn seal) all match node. Both stray binaries deleted. #5874 stays in history at 4500b5d for its author to re-land through review after fixing the extends regression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
language/expressions/compound-assignment/mod-whitespace.jsregressed:x %= -1(withx == -1) yielded0where the spec wants-0.js_dynamic_modcomputed the remainder with the closed forma - (a / b).trunc() * b. That drops the sign of a zero result:-1 % -1evaluates to-1.0 - (1.0 * -1.0) == +0.0. But JS%is Cfmod, whose result takes the sign of the dividend, so-1 % -1is-0. The same closed form also returnedNaNforx % Infinity(should bex).The bug surfaced once a compound assignment on a separately-declared int-valued local (
var x; x = -1; x %= -1) started routing throughjs_dynamic_mod(the codegen fast path that carries the-0correction isn't taken for this receiver shape).Fix
Rust's
f64 % f64isfmod, so replace the closed form witha % b. It matches the spec exactly on:-1 % -1 == -0,-0 % 5 == -0)x % ±Infinity == x±Infinity % y == NaN,x % 0 == NaNVerification
Built on an internal Linux sweep host:
mod-whitespace.jspasses (all whitespace variants,-0expected).language/expressions/compound-assignment+language/expressions/modulusslices are 100% (487 cases, 0 runtime-fails).Code-only (no version/changelog).
Summary by CodeRabbit