Repository navigation
css: return RGBA from parse_hash_color instead of a tuple - #39173
Conversation
parse_hash_color returned (u8, u8, u8, f32) and both callers unpacked it as (r, g, b, a) only to build an RGBA from it, converting the byte channels to floats and back on the way. Build the RGBA in the parser and hand it out directly. The alpha byte was already exact, so the resulting RGBA values are unchanged. Drops the matching tuple_wants_struct entry for css_parser.rs from the mordant baseline.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: ready for review. Refactor only, no behavior change intended.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical refactor returning RGBA directly instead of a tuple, with no behavior change.
What was reviewed:
- Confirmed
parse_hash_color/parse_hash_color_implhave exactly the two callers updated here;OPAQUEis module-private and only used insideparse_hash_color_impl. - Verified the removed byte→f32→byte round-trip through
clamp_unit_f32((x/255.0 * 255.0).round()) is identity for all u8, andOPAQUE = 1.0mapped to255. - Confirmed the dropped
RGBAimport incustom.rshas no remaining uses;mordant-baseline.tomlchange removes only the one resolved entry.
Extended reasoning...
Overview
This PR changes css_parser::color::parse_hash_color{,_impl} to return the crate's RGBA struct (four u8 fields) instead of a (u8, u8, u8, f32) tuple, resolving a mordant tuple_wants_struct lint. Both callers — CssColor::parse in values/color.rs and the hash-token arm of TokenList::parse_into_impl in properties/custom.rs — previously destructured the tuple and immediately built an RGBA from it; now they take the RGBA directly. The module-private OPAQUE constant changes from f32 = 1.0 to u8 = 255. The corresponding baseline entry is removed from mordant-baseline.toml.
Security risks
None. This is pure internal refactoring of hex-color parsing with no change to input handling, bounds, or external surface.
Level of scrutiny
Low. The change is small, mechanical, and self-contained. I verified via grep that (1) parse_hash_color and parse_hash_color_impl are called only from the two sites updated in this PR, (2) OPAQUE is used only inside parse_hash_color_impl, so its type change has no other consumers, and (3) the dropped RGBA import in custom.rs has no remaining references. The old alpha path was byte / 255.0 fed through clamp_unit_f32 ((v * 255.0).round().clamp(0,255) as u8), which is the identity for every u8; the custom.rs caller additionally round-tripped R/G/B through from_floats the same way. So constructing RGBA { red, green, blue, alpha } directly is byte-for-byte equivalent.
Other factors
The PR description reports the full CSS test suites pass (css.test.ts 1142, color.test.ts 993, bundler CSS 169) covering all four hash lengths through both callers, plus clean cargo check/clippy/dylint. The baseline-file edit is a single-line deletion matching the resolved lint. No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — mechanical refactor that returns RGBA directly instead of a tuple both callers immediately rebuilt into one.
What was reviewed:
- Verified
parse_hash_color/parse_hash_color_implandOPAQUEhave no other callers insrc/; both call sites now just wrap the result inCssColor::Rgba. - Checked the removed byte→f32→byte round-trip through
clamp_unit_f32((x/255.0 * 255.0).round()) is the identity for allu8, andOPAQUE: 1.0mapped to 255 — no observable behavior change. - The now-unused
RGBAimport incustom.rsis dropped; the mordant baseline entry is decremented per the regen procedure.
Extended reasoning...
Overview
This PR changes css_parser::color::parse_hash_color / parse_hash_color_impl to return the crate's RGBA struct directly instead of (u8, u8, u8, f32), updates the two callers in src/css/values/color.rs and src/css/properties/custom.rs to consume it, retypes the module-private OPAQUE constant from f32 1.0 to u8 255, and removes the corresponding tuple_wants_struct entry from mordant-baseline.toml.
Security risks
None. This is pure value-type plumbing inside the CSS hex-color parser; no untrusted-length arithmetic changes, no allocation, no FFI, no user-facing surface.
Level of scrutiny
Low. The change is a type-level refactor with a straightforward equivalence argument: previously the tuple's u8 channels went through RGBA::new/RGBA::from_floats, which apply clamp_unit_f32(x) = (x * 255.0).round().clamp(0,255) as u8. For every u8 byte b, (b as f32 / 255.0 * 255.0).round() recovers b exactly (f32 error is far below 0.5), and clamp_unit_f32(1.0) == 255, so the new direct-byte construction is byte-for-byte identical to the old path for all 3/4/6/8-digit inputs. Grep confirms no other consumers of parse_hash_color* or OPAQUE exist.
Other factors
The refactor is guarded by existing coverage the author ran on the debug build (css.test.ts 1142 pass covering all hash lengths through both callers, color.test.ts 993 pass, test/bundler/css/ 169 pass), plus cargo check/clippy/mordant clean. REVIEW.md's "every behavioral change ships a test" doesn't apply here — there is no behavioral change to observe, and the mordant baseline is the check that flips. The dead RGBA import is removed in the same PR per the dead-code rule. The author already flagged the trivial baseline-file rebase against #39122.
There was a problem hiding this comment.
LGTM — mechanical refactor replacing the (u8, u8, u8, f32) return with RGBA, no behavior change.
What was reviewed:
- Confirmed
parse_hash_color/parse_hash_color_implhave exactly the two updated callers andOPAQUEis used nowhere else in the crate. - Verified the removed byte→f32→byte round trip via
clamp_unit_f32((n/255.0*255.0).round()) is the identity for all u8 inputs, andOPAQUE1.0 mapped to 255. - Checked the
RGBAimport drop incustom.rsleaves no dangling references; the mordant baseline diff removes only the one resolved entry.
Extended reasoning...
Overview
This PR changes css_parser::color::parse_hash_color / parse_hash_color_impl to return the crate's RGBA struct directly instead of a (u8, u8, u8, f32) tuple, and updates the two callers (CssColor::parse in values/color.rs and the hash-token arm of TokenList::parse_into_impl in properties/custom.rs) to wrap the result in CssColor::Rgba without re-packing. OPAQUE becomes u8 255 (its only uses are the two 3/6-digit arms in the same function). One line is removed from mordant-baseline.toml for the resolved tuple_wants_struct finding.
Security risks
None. This is a pure internal type-shape refactor of a hex-color parser with no I/O, allocation, or user-facing surface change.
Level of scrutiny
Low. The change is small, mechanical, and has a straightforward equivalence argument: the old paths went u8 → f32/255 → clamp_unit_f32 → u8 via RGBA::new / RGBA::from_floats; clamp_unit_f32(x) is (x*255.0).round().clamp(0,255) as u8, which is the identity for every n as f32 / 255.0 with n in 0..=255, and 1.0 maps to 255. So each of the four match arms produces the byte-identical RGBA it did before. Grep confirms there are no other callers of parse_hash_color, parse_hash_color_impl, or OPAQUE, and RGBA is no longer referenced in custom.rs after the import is dropped.
Other factors
The PR description documents that the existing suites (css.test.ts covering 3/4/6/8-digit hashes through both callers, color.test.ts, and test/bundler/css/) all pass on the debug build, and mordant is clean over the trimmed baseline. No new test is expected for a behavior-preserving refactor whose only observable flip is the lint ratchet. There are no prior reviewer comments to address.
2 participants
Problem
tuple_wants_structflagscolor::parse_hash_colorinsrc/css/css_parser.rs:5730: it returns(u8, u8, u8, f32), and both callers (CssColor::parseinsrc/css/values/color.rs:408,TokenListhash tokens insrc/css/properties/custom.rs:653) unpack it as(r, g, b, a). The channel names live only at the call sites, and since the first three fields are allu8, a swapped pair still compiles.RGBAout of the tuple, so the function already had a named type for its result;custom.rsalso converted each byte to a float andRGBA::from_floatsconverted it straight back.Fix
parse_hash_color/parse_hash_color_implbuild theRGBA(red/green/blue/alpha, allu8) themselves and return it; the callers wrap it inCssColor::Rgba.OPAQUEbecomes theu8alpha255it was being converted to.RGBA::new/RGBA::from_floats, which is the identity for all 256 values (checked with f32 arithmetic), andOPAQUE(1.0) mapped to255the same way.tuple_wants_struct:src/css/css_parser.rsis removed frommordant-baseline.toml. The[bun_css]section was regenerated withMORDANT_BASELINE_WRITE=1 cargo dylint --all -p bun_css --no-deps(the per-crate form ofbun run rust:mordant:baseline); that line is the only difference, thevalues/color.rs = 5entry is unchanged.RGBAproduced for every hash is byte for byte what it was), so there is no test that fails before it and passes after it; the check that does flip is the mordant run, which reports thecss_parser.rsfinding over the trimmed baseline without this change and is clean with it. The existing coverage below is what guards the refactor.bun bd:bun bd test test/js/bun/css/css.test.ts: 1142 pass. This covers both callers with every hash length: 8-digit hashes inside custom properties (--bar: ... #01010116round-trips throughcustom.rs),#0006/#ffffff80/ 3- and 6-digit hashes in regular properties throughcolor.rs.bun bd test test/js/bun/css/color.test.ts(Bun.color): 993 pass.bun bd test test/bundler/css/: 169 pass.cargo check -p bun_css,cargo clippy -p bun_css --no-depsandcargo dylint --all -p bun_css --no-deps(mordant) are clean; the latter writes notarget/mordant/over-baseline.txt.Background
css_parser::coloris the port of thecssparsercrate's color helpers;parse_hash_colorparses the text after#(3, 4, 6 or 8 hex digits) into channels.RGBA(src/css/values/color.rs) is the crate's byte-per-channel color and is whatCssColor::Rgbastores, which is why both callers were converting to it.mordant-baseline.tomlis the ratchet for the mordant lint pack: it records the per-(lint, file) finding counts that predate the job so CI only fails on new findings. Fixing a site means removing (or decrementing) its entry; mordant's write mode rewrites one section per compiled crate, so regenerating with onlybun_csscompiled touches only that section.