Conversation
CSS rem(A, B) must take the sign of A (truncated division), matching JavaScript's % operator. The Rust port was computing floored modulo (sign of B), so rem(7px, -3px) produced -2px instead of 1px. Applies uniformly to Length, Time, and Angle values.
|
Warning Review limit reached
More reviews will be available in 3 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 8:17 AM PT - Jun 15th, 2026
✅ @robobun, your commit 43b6f9d43684abe6ad6a6c6306b6a46b3f106512 passed in 🧪 To try this PR locally: bunx bun-pr 32286That installs a local version of the PR into your bun-32286 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line spec fix with full sign-matrix test coverage.
Extended reasoning...
Overview
This PR changes a single closure in src/css/values/calc.rs for the CSS rem() math function, replacing the floored-modulo formula a - b * (a / b).floor() with Rust's % operator. It adds 12 minify test cases to test/js/bun/css/css.test.ts covering all sign combinations of rem() (across px/s/deg) and mod() (as a guard).
Correctness
Per CSS Values 4 §11.5, rem(A, B) must take the sign of A (truncated-division remainder, like JS %), while mod(A, B) takes the sign of B (floored). Rust's % on f32 is IEEE truncated remainder (sign of dividend), so the new code is exactly spec-correct. I verified each of the 12 test expectations by hand against the spec definitions; all are right. The old code incorrectly applied floored semantics to rem(), so this is a genuine regression fix from the Zig→Rust port. mod() is untouched and its guard tests confirm it still produces sign-of-divisor results.
Security risks
None. This is pure f32 arithmetic inside the CSS minifier's constant-folding path — no I/O, auth, memory safety, or untrusted-input parsing concerns beyond what already exists.
Level of scrutiny
Low. The diff is effectively one expression plus a comment, in a self-contained value-computation closure with no side effects. The behavior is fully determined by a public spec and is trivially testable. The added tests cover the full sign matrix and span three unit types that flow through the same generic path, so the fix is well-pinned.
Other factors
No CODEOWNERS cover these files. No prior human or bot reviews to address. The bug-hunting system found no issues. The PR description documents that the new tests fail on the old code and pass on the new, and that the full css.test.ts suite passes.
There was a problem hiding this comment.
LGTM — one-line spec fix with full sign-matrix test coverage.
Extended reasoning...
Overview
This PR changes a single closure in src/css/values/calc.rs for CalcUnit::Rem, replacing the floored-modulo formula a - b * (a / b).floor() with Rust's a % b (truncated remainder, sign of dividend). It adds 12 minify test cases to test/js/bun/css/css.test.ts covering all sign combinations of rem() (across px/s/deg) and mod() (px) as a regression guard.
Security risks
None. This is pure numeric evaluation of CSS rem()/mod() constant folding inside the CSS parser/minifier — no I/O, auth, allocation, or user-data handling changes.
Level of scrutiny
Low. The change is a one-expression substitution whose semantics are directly verifiable: Rust's % on f32 is IEEE truncated remainder (sign follows the dividend), which is exactly what CSS Values 4 §11.5 specifies for rem(A, B). I hand-checked all 12 test expectations against both the truncated and floored definitions and they are correct. The mod() path is untouched and the guard tests confirm it still produces floored (sign-of-divisor) results.
Other factors
No CODEOWNERS entry covers these files. No outstanding human reviewer comments. The bug-hunting system found no issues. The PR description accurately describes the regression (floored vs truncated semantics swapped during the Zig→Rust port) and the fix. Edge cases like b == 0 produce NaN under both old and new formulas, so no behavior change there.
|
The diff is green locally (all 12 new CI failures across both runs are unrelated to this change:
No failing lane touches |
Replaces #32290. #39515 (folding `min()`, `max()` and `clamp()` over plain numbers) is stacked on this PR and adds the number case to the helper introduced here. ### Problem - The CSS minifier drops the lower bound of a `clamp()` whose center is not above its maximum. `width: clamp(10px, 5px, 20px)` prints `width:min(20px,5px)`, which is 5px instead of 10px. `clamp(10%, 5%, 20%)` prints `min(20%,5%)`, and `clamp(1em, 2px, 3px)` prints `min(3px,2px)`, dropping the `1em` bound that may be the larger one. Same on bun 1.4.0 and main. Reported in #32290. - When the center is above the maximum, the result is left as `max(10px,20px)` instead of `20px`, because the minimum is never compared. - Cause: the `clamp()` arm of `Calc::parse_with` in `src/css/values/calc.rs` compares the center with the maximum and, when the center is not above it, sets `min = None` (`calc.rs:370` on main) where it means `max = None`. The `(None, Some(max))` arm of the match below (`calc.rs:379`) then prints `min(max, center)`. lightningcss does not have this bug and also compares the center with the minimum afterwards, this came in with the port. ### Fix - The arm is restructured around the definition `clamp(min, center, max) = max(min, min(center, max))`. If the center and the maximum are comparable, the maximum is applied (the center becomes the maximum when it is above it), then the minimum: a comparable minimum folds the result to one value, an incomparable one is kept as `max(min, center)`. If the center and the maximum are not comparable, the `clamp()` is kept unchanged, as before and as upstream. - The two comparisons go through a new `Calc::partial_cmp_args`, which orders two `Calc::Value` arguments through the existing `PartialCmp` and returns `None` for anything else, so a `Calc::Number`, a sum or a nested function is never compared, exactly as before. The `Option` bookkeeping is gone, and with it the `min(max, center)` output, which only the bug produced. - Every asserted output is the one lightningcss 1.33.0 prints for the same input, including the eleven rows taken from its own calc tests. - Tests: `test/js/bun/css/css.test.ts`, new `clamp() simplification` block: lightningcss's rows (`<length>` through `border-width`, sums as arguments, unit conversion, an incomparable center, an incomparable minimum, a sum as a center or a bound), the center equal to either bound, the minimum winning over a smaller maximum with the center between the bounds and, in the two rows of the clamp() WPT, above both (these only pass when the minimum is compared with the center after the maximum has replaced it), #32290's incomparable lower bound with the center below and above the maximum, an incomparable maximum with a comparable minimum, and `%`, `deg`, `s` and a `clamp()` inside `calc()`. 18 of the 24 rows fail on the released binary, all 24 pass with the fix. - Also run: the rest of `css.test.ts` and `test/bundler/css/` (1348 pass), `cargo clippy -p bun_css` clean. ### Background - `Calc<V>` is the parsed form of a math expression for a value type `V` (`Length`, `Percentage`, `Angle`, ...). `Calc::Value` holds a `V` and two of them can be ordered at parse time when their units convert into each other (`px` and `pt`, not `px` and `em`). `Calc::Number` holds a bare number. Comparing numbers is #39515. - Unparsed fallback: when a property's value parser rejects a value, bun keeps the declaration as the tokens it was written as. `rotate: clamp(0deg, 45deg, 30deg)` took that path on main (the `max()` the bug produced is not a value an `<angle>` accepts), which is why it printed unchanged rather than wrong. `width` accepts any math function, which is why it printed the wrong `min()`. <details><summary>Notes</summary> - Related but not fixed here: `clamp(1px , 2px , 3px)` (a space before a comma) is not simplified at all, because the `clamp()` arm reads each argument with `parse_sum`, which stops at whitespace that is not followed by `+` or `-`, while `min()`/`max()` read theirs through `parse_comma_separated`. The declaration is kept as written, so the output is correct, only longer. lightningcss 1.33.0 behaves the same. It is a parsing change with its own tests and belongs in its own PR. - Eleven of the rows are lightningcss's own `test_calc` clamp rows. bun has none of lightningcss's `test_calc`, `test_math_fn` or `test_trig` tables in `css.test.ts`; porting them as one block would give the open calc.rs PRs (#32286, #38489, #38501, #38639, #38654, #39515) a shared oracle instead of a describe block each. That is a test-only change and is not part of this PR. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 18 failed, 67 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/css/css.test.ts bun test v1.4.0 (8326d1b) test/js/bun/css/css.test.ts: Output .flexrow { flex-direction: row; } .flexcol { flex-direction: column; } .hello { flex-wrap: wrap; } .world { flex-wrap: nowrap; } (pass) css tests > .flexrow { flex-direction: row; } .flexcol { flex-direction: column; } .hello { flex-wrap: wrap; } .world { flex-wrap: nowrap; } [3.18ms] Output .foo { content: "+"; } (pass) css tests > .foo { content: "\2b"; } [1.04ms] Output div { --foo: 1 1 1 #0101011a, 2 2 2 #02020233; --bar: 1 1 1 #01010116, 2 2 2 #02020233; } (pass) css tests > custom property cases > div { --foo: 1 1 1 rgba(1, 1, 1, 0.1), 2 2 2 rgba(2, 2, 2, 0.2); --bar: 1 1 1 #01010116, 2 2 2 #02020233; } [1.02ms] Output :root { --my-color: red; --my-bg: white; --my-font-size: 16px; } .element { color: var(--my-color); background-color: var(--my-bg); font-size: var(--my-font-size); } (pass) css tests > custom property cases > :root { --my-color: red; --my-bg: w ... (truncated) release without fix: 67 skipped bun test v1.4.0-canary.1 (9c30321) test/js/bun/css/css.test.ts: Output .flexrow { flex-direction: row; } .flexcol { flex-direction: column; } .hello { flex-wrap: wrap; } .world { flex-wrap: nowrap; } (pass) css tests > .flexrow { flex-direction: row; } .flexcol { flex-direction: column; } .hello { flex-wrap: wrap; } .world { flex-wrap: nowrap; } [0.11ms] Output .foo { content: "+"; } (pass) css tests > .foo { content: "\2b"; } [0.02ms] Output div { --foo: 1 1 1 #0101011a, 2 2 2 #02020233; --bar: 1 1 1 #01010116, 2 2 2 #02020233; } (pass) css tests > custom property cases > div { --foo: 1 1 1 rgba(1, 1, 1, 0.1), 2 2 2 rgba(2, 2, 2, 0.2); --bar: 1 1 1 #01010116, 2 2 2 #02020233; } [0.03ms] Output :root { --my-color: red; --my-bg: white; --my-font-size: 16px; } .element { color: var(--my-color); background-color: var(--my-bg); font-size: var(--my-font-size); } (pass) css tests > custom property cases > :root { --my-color: red; --my-bg: white; --my-font-size: 16px; } .element { color: var(--my-color); background-color: var(--my-bg); font-size: ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 67 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/css/css.test.ts bun test v1.4.0 (8326d1b) test/js/bun/css/css.test.ts: Output .flexrow { flex-direction: row; } .flexcol { flex-direction: column; } .hello { flex-wrap: wrap; } .world { flex-wrap: nowrap; } (pass) css tests > .flexrow { flex-direction: row; } .flexcol { flex-direction: column; } .hello { flex-wrap: wrap; } .world { flex-wrap: nowrap; } [4.46ms] Output .foo { content: "+"; } (pass) css tests > .foo { content: "\2b"; } [1.13ms] Output div { --foo: 1 1 1 #0101011a, 2 2 2 #02020233; --bar: 1 1 1 #01010116, 2 2 2 #02020233; } (pass) css tests > custom property cases > div { --foo: 1 1 1 rgba(1, 1, 1, 0.1), 2 2 2 rgba(2, 2, 2, 0.2); --bar: 1 1 1 #01010116, 2 2 2 #02020233; } [1.08ms] Output :root { --my-color: red; --my-bg: white; --my-font-size: 16px; } .element { color: var(--my-color); background-color: var(--my-bg); font-size: var(--my-font-size); } (pass) css tests > custom property cases > :root { --my-color: red; --my-bg: w ... (truncated) release with fix: 67 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 643ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_brotli v0.0.0 (/workspace/bun/src/brotli) �[1m�[92m Compiling�[0m bun_output v0.0.0 (/workspace/bun/src/output) �[1m�[92m Compiling�[0m bun_clap ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/css/values/calc.rs | 56 +++++++++++++++++++-------------------------- test/js/bun/css/css.test.ts | 35 ++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 33 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/css/values/calc.rs 12 10 0 test/js/bun/css/css.test.ts 3 7 0 ``` </details> <!-- robobun:evidence:end -->
|
Stale PR review: keep open. The bug still reproduces on current main ( |
This branch has not been deployed
1 participant
Repro
Cause
CalcUnit::Reminsrc/css/values/calc.rscomputeda - b * (a / b).floor()(floored modulo, sign of divisor). Per CSS Values 4 §11.5,rem(A, B)takes the sign of A (truncated division). The Zig reference used@mod(a, b), which falls through to LLVMfrem(truncated) for the negative-divisor case, so the original behavior was spec-correct and regressed in the Rust port. Affects Length, Time, and Angle uniformly since all three flow through the sameparse_math_fnclosure.mod(A, B)(floored, sign of B) was already correct and is unchanged.Fix
Use Rust's
%operator, which is truncated remainder onf32(sign of dividend).Verification
Added
calc rem() and mod()cases totest/js/bun/css/css.test.tscovering all four sign combinations forrem()overpx/s/degplus themod()sign matrix as a guard. Withsrc/reverted the 6 mixed-signrem()cases fail; with the fix all 12 pass. Fullcss.test.tspasses (1105 pass / 67 skip).