Conversation
In relative color syntax a channel keyword is a <number>. The Calc<Percentage> pass of RelativeComponentParser::parse_number_or_percentage materialized it as a Percentage, so `calc(r * 50%)` was a percentage times a percentage and the whole color failed to parse (and rgb()/hsl() with such an alpha emitted `rgb(255 0 0/calc(alpha*50%))`). The keyword is now a Calc::Number in that pass. That pass was also the only place min()/max() over channel keywords got folded, because reduce_args only reduced Calc::Value arguments. It now reduces Calc::Number arguments too, so min(r, g) keeps folding through the number pass and min(r, .5) folds as well. Percentage::from_calc returned NaN for anything that was not a plain value, which turned the type error in `calc(r + 50%)` or `opacity: calc(50% + 1)` into a NaN component. It now fails to parse like Angle and Time do, so such declarations are left as written.
A bare channel keyword is a number, so min(r, 50%), round(r, 30%) and calc(alpha - 20%) are type errors and stay as written (lightningcss leaves them unparsed too), while min(r * 100%, 50%) and round(r * 100%, 30%) fold.
hypot(A) is sqrt(A * A), so it is abs(A). parse_hypot returned the argument as parsed instead, which kept the sign of a negative value (hypot(-3px) became -3px) and, when the argument was not a plain value, dropped the function around it (hypot(1px + 10%) became the bare sum 1px + 10%, which is not valid CSS). The one-argument case now goes through apply_map with absf, the same step the abs() arm uses. When that cannot fold (a percentage, a sum, a function that did not evaluate) the caller keeps hypot(...) as written.
Collaborator
Author
|
Status: reproduced on bun 1.4.0 and main with |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — one-argument hypot() now folds via apply_map(.., absf), exactly mirroring the abs() arm, which is correct per CSS Values 4 (hypot(A) = sqrt(A²) = |A|).
What was reviewed:
- Confirmed
parse_hypothas a single caller and the&mut [Self]→&[Self]signature change is sound;argsstays owned by the caller for theMathFunction::Hypot(args)fallback whenapply_mapreturnsNone. - Traced
apply_map→try_mapfor eachCalc<V>instantiation: Length/Angle/Time/Number fold,Percentage::try_mapreturnsNonesohypot(-30%)stays wrapped — matches the pinned tests. - Checked the
Nonepath coversSum/Product/Functionargs, sohypot(1px + 10%)no longer emits a bare sum. - 32 new
minify_testcases cover every value type, nesting incalc(), and control cases (abs(), two-arg); PR verified 27 fail on 1.4.0.
Extended reasoning...
Overview
This PR fixes a minification correctness bug in Bun's CSS parser: single-argument hypot() was being folded to its argument unchanged instead of to the argument's absolute value. The change is confined to Calc::parse_hypot in src/css/values/calc.rs — the one-argument branch now returns Self::apply_map(&args[0], absf) instead of moving the raw argument out with mem::replace. Because the function no longer mutates args, its signature relaxes from &mut [Self] to &[Self]. A new 32-case describe block in test/js/bun/css/css.test.ts pins the behavior across every Calc<V> instantiation.
Security risks
None. This is a pure constant-folding change in the CSS minifier over values that have already been parsed. No untrusted-length arithmetic, no allocation sizing, no FFI, no JS heap interaction.
Level of scrutiny
Low-to-medium. The diff is four lines of Rust in a self-contained helper with one call site. The mathematical identity hypot(A) ≡ abs(A) is unambiguous in CSS Values 4, and the implementation reuses the exact same primitive (apply_map + absf) that the adjacent CalcUnit::Abs arm already uses — so every case where abs() is correct, hypot() of one argument is now correct too, and every case abs() leaves unevaluated, hypot() leaves unevaluated (wrapped as MathFunction::Hypot, preserving the input's function name for is_compatible fidelity).
I verified: (1) parse_hypot has exactly one caller, which still owns args for the fallback; (2) apply_map returns None for Sum/Product/Function variants and for Calc::Value whose try_map returns None — I checked Percentage::try_map in src/css/values/percentage.rs:96 and it always returns None, so hypot(-30%) correctly falls through to the wrapped form; (3) no memory-management concern — the old mem::replace moved the arg out and left a dummy, the new path borrows and constructs a fresh Calc via try_map, and the untouched args Vec drops normally in the caller.
Other factors
- The two- and three-plus-argument paths in
parse_hypotare unchanged; the 2-arg control test (hypot(-3px, 4px)→5px) confirms no regression there. - Test coverage is thorough and follows the existing
minify_testconvention in the same file: length, length-percentage, angle, angle-percentage (conic-gradient stop), time, number, bare percentage (opacity, rgb alpha), nested incalc(), plusabs()control cases pinned alongside to demonstrate the two now behave identically. The PR description reports 27 of 32 fail underUSE_SYSTEM_BUN=1, satisfying the fails-for-the-right-reason requirement. - The PR is stacked on #38513 (which changes
Percentage::from_calcto error rather than NaN on non-value operands); the diff here is relative to that base and does not include #38513's change, so this review covers only thehypotdelta. The last four bare-percentage tests depend on #38513's behavior, which is already visible in the preloadedfrom_calcimpl forPercentage. - No prior reviewer comments to address; no CODEOWNERS-restricted paths.
Collaborator
Author
robobun
force-pushed
the
farm/cf388f29/css-relative-color-calc-percentage
branch
from
August 14, 2026 19:35
c143e7d to
1d67098
Compare
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 19, 2026
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 -->
This branch has not been deployed
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
hypot()with a single argument minifies to that argument unchanged.margin-left: hypot(-3px)becomes-3px,rotate: hypot(-90deg)becomes-90deg,transition-delay: hypot(-2s)becomes-2s,line-height: hypot(-3)becomes-3. CSS Values 4 defineshypot(A)as the square root of the sum of the squares of its arguments, sohypot(A)isabs(A)and all of these should lose the sign. The wrong value also flows into surrounding arithmetic:calc(100% - hypot(-3px))becomescalc(100% + 3px).hypot(1px + 10%)becomes the bare1px + 10%andhypot(2 * min(1px, 1em))becomes2*min(1px,1em), neither of which is valid CSS outside a math function.hypot(min(-1px, -1em))becomesmin(-1px,-1em), which has the wrong sign.Calc::parse_hypot(src/css/values/calc.rs:921) special-casesargs.len() == 1by moving the argument out and returning it as parsed. Nothing applies the absolute value, and because a value is returned the caller never reaches itsMathFunction::Hypot(args)fallback. lightningcss has the same shortcut, so upstream output is not the reference here; the spec is.bun build --minify(table below).Fix
Calc::apply_map(&args[0], absf), which is the same step theabs()arm directly below thehypot()arm already uses.parse_hypotno longer needs&mut, so it takes&[Self].hypot(-3px)is3px,hypot(-1in)is1in,hypot(-90deg)is90deg,hypot(-2s)is2s,hypot(-3)is3,calc(100% - hypot(-3px))iscalc(100% - 3px)).apply_mapreturnsNone(a percentage, a sum, a product, a function that did not evaluate) the existing caller fallback keepshypot(...)in the output as written, sohypot(1px + 10%),hypot(2*min(1px,1em)),hypot(min(-1px,-1em))andhypot(-30%)now print exactly that. This is byte for byte whatabs()of the same arguments prints today, and the tests pin a few of theabs()forms next to thehypot()ones.hypot(A)andabs(A)are the same function of one argument, so giving them the same folding rule makeshypot()correct in every case whereabs()is, and leaves it unevaluated (rather than wrong) in exactly the cases whereabs()is left unevaluated. Keeping the function ashypot()rather than rewriting it toabs()means the output only ever uses functions the input used, which is what the rest of this file does for everything it cannot fold (and whatis_compatiblereports on).hypot(50%)in a bare<percentage>context is now aMathFunctioninstead of a value, and on mainPercentage::from_calcturns any non-value operand of a sum into NaN, which prints as0. Without css: parse calc(<channel> * <percentage>) in relative colors #38513 this change would therefore turn the currently correctopacity: calc(10% + hypot(50%))(.6) intoopacity:0, the same thingopacity: calc(10% + abs(50%))prints on 1.4.0 today. With css: parse calc(<channel> * <percentage>) in relative colors #38513 the declaration is kept as written, and the last block of tests pins that foropacityand anrgb()alpha. Not duplicating that one-line change here; the PR retargets to main once css: parse calc(<channel> * <percentage>) in relative colors #38513 merges.parse_hypot; the two changes touch adjacent lines and whichever lands second is a one-line rebase.bun bd test test/js/bun/css/css.test.ts(newhypot() with one argumentblock, 32 cases covering everyCalc<V>instantiation: length, length-percentage, angle, angle-percentage in aconic-gradient()stop, time, number, bare percentage inopacityand in anrgb()alpha, plushypot()nested incalc()and the unchanged two-argument form). 27 of the 32 fail on the released 1.4.0 (USE_SYSTEM_BUN=1); the other 5 are thehypot(3px), two-argument andabs()control cases.test/js/bun/css/css.test.tsandcolor.test.tsin full with the change (2253 pass, 0 fail).Background
Calc<V>is the parsed form of a math function over values of typeV(a length, an angle, a bare percentage, ...).Calc::Value/Calc::Numberare things that were fully evaluated at parse time;Calc::Sum/Calc::Productare partially evaluated trees;Calc::Functionis a math function kept for the output because it could not be evaluated.Calc::apply_map(arg, f)applies a unaryf32 -> f32to aCalc::Numberor, through the value type'stry_map, to aCalc::Value, and returnsNonefor anything else.Percentage::try_mapalways returnsNonebecause a percentage's sign is only known once it is resolved against something; this is whyabs(-30%)(and nowhypot(-30%)) is kept as written.Calc::Sumasa + bwith no wrapper, relying on whatever owns it at the top of a property value to be aCalc::Function(calc(...),min(...), ...). Returning aSumstraight out of a math function, as the old one-argument branch did, is how the bare1px + 10%got emitted.Percentage::from_calcis how acalc()sum in a bare<percentage>property (opacity, a color's alpha) converts a non-value operand back into aPercentage. On main it produces NaN for anything that is not a plain value; css: parse calc(<channel> * <percentage>) in relative colors #38513 makes it a parse error, which keeps the declaration verbatim.bun build --minify before (1.4.0 / main) and after this PR