Conversation
|
Updated 9:52 AM PT - Jul 6th, 2026
❌ @alii, your commit 46e30e8 has some failures in 🧪 To try this PR locally: bunx bun-pr 33332That installs a local version of the PR into your bun-33332 --bun |
WalkthroughThis PR replaces several CSS parser and serialization panic paths with debug assertions or fallback behavior, changes color fallback APIs from bitflags to enum values, refactors margin/padding extraction to return options, removes unused animation timeline variants, reshapes selector parsing/serialization, and updates syntax repetition handling and tests. ChangesPanic hardening and API refactors
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…base The variants were deleted earlier on this branch; #33322 (merged to main) added deep_clone with an unreachable arm for them. Rebase merged both without a textual conflict, leaving a reference to non-existent variants.
2bb0c82 to
f557d51
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/css/properties/animation.rs (1)
154-267: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the repeated name-disambiguation check.
direction,fill_mode, andplay_stateeach repeat the samecss::parse_utility::parse_string::<T>(dest.arena, n, T::parse).is_ok()pattern againstname_str. Per this repo's own review rule, a pattern repeated a third time in the same diff should be pulled into a shared helper.♻️ Proposed helper
+fn name_disambiguates<T: css::EnumProperty>(dest: &Printer, name_str: Option<&[u8]>) -> bool { + name_str.is_some_and(|n| { + css::parse_utility::parse_string::<T>(dest.arena, n, T::parse).is_ok() + }) +}Then call
name_disambiguates::<AnimationDirection>(dest, name_str), etc., at each site (keeping the extra!eql_case_insensitive_ascii(n, b"none", true)guard specific tofill_mode).As per coding guidelines, "The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site."
🤖 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 `@src/css/properties/animation.rs` around lines 154 - 267, The name-disambiguation logic in AnimationName::to_css is duplicated across direction, fill_mode, and play_state, so extract it into a shared helper and reuse it at each site. Add a small helper like name_disambiguates::<T>(dest, name_str) that wraps the css::parse_utility::parse_string::<T>(dest.arena, n, T::parse).is_ok() check, then call it for AnimationDirection, AnimationFillMode, and AnimationPlayState. Keep the fill_mode-specific !strings::eql_case_insensitive_ascii(n, b"none", true) guard alongside the helper call.Source: Coding guidelines
src/css/values/easing.rs (1)
131-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead-but-safe fallback arms are fine.
Lines 188-192 duplicate the outer match's
Linear/Ease/EaseIn/EaseOut/EaseInOutarms and are unreachable in practice (already excluded by the outer_branch), but writing explicit keyword strings instead ofunreachable!()is a safe, low-risk way to satisfy exhaustiveness per the PR's hardening goal. Optionally, this could be flattened into a single match to avoid the duplicated arms, but that's a pre-existing structural choice, not introduced by this diff.🤖 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 `@src/css/values/easing.rs` around lines 131 - 196, Simplify EasingFunction::to_css by flattening the nested match so the Linear/Ease/EaseIn/EaseOut/EaseInOut cases are handled once instead of being duplicated in the fallback arm. Keep the existing special-case handling for CubicBezier and Steps, but remove the redundant keyword-string arms in the inner match to make the control flow clearer and easier to maintain.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/css/properties/properties_generated.rs`:
- Around line 2806-2827: The allowlists in the property parsing branch for
animation properties incorrectly include VendorPrefix::MS, which lets
unsupported -ms-animation and -ms-animation-name through. Update the matching
logic in properties_generated.rs for the PropertyId::AnimationName and
PropertyId::Animation cases to remove VendorPrefix::MS from the allowed prefixes
so they only accept prefixes actually emitted by PrefixFeature::Animation and
PrefixFeature::AnimationName.
In `@src/css/rules/mod.rs`:
- Around line 852-861: The placeholder StyleRule created in the code path around
core::mem::replace should use Location::dummy() instead of Location::default()
so synthesized rules are marked consistently with the rest of the codebase.
Update the style::StyleRule initialization in this block to use the dummy source
location sentinel, keeping the existing selectors, vendor_prefix, declarations,
and rules unchanged.
In `@src/css/selectors/parser.rs`:
- Around line 3555-3565: The attribute selector parsing in parser logic is
dropping namespaces for lowercase namespaced attributes because the
value-selector path only falls back to AttributeOther when namespace.is_some()
and the local name is not ASCII lowercase. Update the selector handling around
AttrQName::Specific/Any and the attribute value branch so the optimized
no-namespace variant is used only when namespace.is_none(), and keep namespaced
selectors like svg|href routed through the namespaced path even for lowercase
local names.
---
Outside diff comments:
In `@src/css/properties/animation.rs`:
- Around line 154-267: The name-disambiguation logic in AnimationName::to_css is
duplicated across direction, fill_mode, and play_state, so extract it into a
shared helper and reuse it at each site. Add a small helper like
name_disambiguates::<T>(dest, name_str) that wraps the
css::parse_utility::parse_string::<T>(dest.arena, n, T::parse).is_ok() check,
then call it for AnimationDirection, AnimationFillMode, and AnimationPlayState.
Keep the fill_mode-specific !strings::eql_case_insensitive_ascii(n, b"none",
true) guard alongside the helper call.
In `@src/css/values/easing.rs`:
- Around line 131-196: Simplify EasingFunction::to_css by flattening the nested
match so the Linear/Ease/EaseIn/EaseOut/EaseInOut cases are handled once instead
of being duplicated in the fallback arm. Keep the existing special-case handling
for CubicBezier and Steps, but remove the redundant keyword-string arms in the
inner match to make the control flow clearer and easier to maintain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2de3420-ccc0-4dcc-be68-ec5b872ec581
📒 Files selected for processing (25)
src/css/css_parser.rssrc/css/lib.rssrc/css/media_query.rssrc/css/properties/animation.rssrc/css/properties/background.rssrc/css/properties/border.rssrc/css/properties/custom.rssrc/css/properties/margin_padding.rssrc/css/properties/properties_generated.rssrc/css/properties/size.rssrc/css/rules/container.rssrc/css/rules/mod.rssrc/css/selectors/parser.rssrc/css/selectors/selector.rssrc/css/small_list.rssrc/css/targets.rssrc/css/values/calc.rssrc/css/values/color.rssrc/css/values/easing.rssrc/css/values/gradient.rssrc/css/values/image.rssrc/css/values/length.rssrc/css/values/percentage.rssrc/css/values/syntax.rstest/js/bun/css/css.test.ts
💤 Files with no reviewable changes (1)
- src/css/lib.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/css/properties/animation.rs (1)
154-267: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the repeated name-disambiguation check.
direction,fill_mode, andplay_stateeach repeat the samecss::parse_utility::parse_string::<T>(dest.arena, n, T::parse).is_ok()pattern againstname_str. Per this repo's own review rule, a pattern repeated a third time in the same diff should be pulled into a shared helper.♻️ Proposed helper
+fn name_disambiguates<T: css::EnumProperty>(dest: &Printer, name_str: Option<&[u8]>) -> bool { + name_str.is_some_and(|n| { + css::parse_utility::parse_string::<T>(dest.arena, n, T::parse).is_ok() + }) +}Then call
name_disambiguates::<AnimationDirection>(dest, name_str), etc., at each site (keeping the extra!eql_case_insensitive_ascii(n, b"none", true)guard specific tofill_mode).As per coding guidelines, "The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site."
🤖 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 `@src/css/properties/animation.rs` around lines 154 - 267, The name-disambiguation logic in AnimationName::to_css is duplicated across direction, fill_mode, and play_state, so extract it into a shared helper and reuse it at each site. Add a small helper like name_disambiguates::<T>(dest, name_str) that wraps the css::parse_utility::parse_string::<T>(dest.arena, n, T::parse).is_ok() check, then call it for AnimationDirection, AnimationFillMode, and AnimationPlayState. Keep the fill_mode-specific !strings::eql_case_insensitive_ascii(n, b"none", true) guard alongside the helper call.Source: Coding guidelines
src/css/values/easing.rs (1)
131-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead-but-safe fallback arms are fine.
Lines 188-192 duplicate the outer match's
Linear/Ease/EaseIn/EaseOut/EaseInOutarms and are unreachable in practice (already excluded by the outer_branch), but writing explicit keyword strings instead ofunreachable!()is a safe, low-risk way to satisfy exhaustiveness per the PR's hardening goal. Optionally, this could be flattened into a single match to avoid the duplicated arms, but that's a pre-existing structural choice, not introduced by this diff.🤖 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 `@src/css/values/easing.rs` around lines 131 - 196, Simplify EasingFunction::to_css by flattening the nested match so the Linear/Ease/EaseIn/EaseOut/EaseInOut cases are handled once instead of being duplicated in the fallback arm. Keep the existing special-case handling for CubicBezier and Steps, but remove the redundant keyword-string arms in the inner match to make the control flow clearer and easier to maintain.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/css/properties/properties_generated.rs`:
- Around line 2806-2827: The allowlists in the property parsing branch for
animation properties incorrectly include VendorPrefix::MS, which lets
unsupported -ms-animation and -ms-animation-name through. Update the matching
logic in properties_generated.rs for the PropertyId::AnimationName and
PropertyId::Animation cases to remove VendorPrefix::MS from the allowed prefixes
so they only accept prefixes actually emitted by PrefixFeature::Animation and
PrefixFeature::AnimationName.
In `@src/css/rules/mod.rs`:
- Around line 852-861: The placeholder StyleRule created in the code path around
core::mem::replace should use Location::dummy() instead of Location::default()
so synthesized rules are marked consistently with the rest of the codebase.
Update the style::StyleRule initialization in this block to use the dummy source
location sentinel, keeping the existing selectors, vendor_prefix, declarations,
and rules unchanged.
In `@src/css/selectors/parser.rs`:
- Around line 3555-3565: The attribute selector parsing in parser logic is
dropping namespaces for lowercase namespaced attributes because the
value-selector path only falls back to AttributeOther when namespace.is_some()
and the local name is not ASCII lowercase. Update the selector handling around
AttrQName::Specific/Any and the attribute value branch so the optimized
no-namespace variant is used only when namespace.is_none(), and keep namespaced
selectors like svg|href routed through the namespaced path even for lowercase
local names.
---
Outside diff comments:
In `@src/css/properties/animation.rs`:
- Around line 154-267: The name-disambiguation logic in AnimationName::to_css is
duplicated across direction, fill_mode, and play_state, so extract it into a
shared helper and reuse it at each site. Add a small helper like
name_disambiguates::<T>(dest, name_str) that wraps the
css::parse_utility::parse_string::<T>(dest.arena, n, T::parse).is_ok() check,
then call it for AnimationDirection, AnimationFillMode, and AnimationPlayState.
Keep the fill_mode-specific !strings::eql_case_insensitive_ascii(n, b"none",
true) guard alongside the helper call.
In `@src/css/values/easing.rs`:
- Around line 131-196: Simplify EasingFunction::to_css by flattening the nested
match so the Linear/Ease/EaseIn/EaseOut/EaseInOut cases are handled once instead
of being duplicated in the fallback arm. Keep the existing special-case handling
for CubicBezier and Steps, but remove the redundant keyword-string arms in the
inner match to make the control flow clearer and easier to maintain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2de3420-ccc0-4dcc-be68-ec5b872ec581
📒 Files selected for processing (25)
src/css/css_parser.rssrc/css/lib.rssrc/css/media_query.rssrc/css/properties/animation.rssrc/css/properties/background.rssrc/css/properties/border.rssrc/css/properties/custom.rssrc/css/properties/margin_padding.rssrc/css/properties/properties_generated.rssrc/css/properties/size.rssrc/css/rules/container.rssrc/css/rules/mod.rssrc/css/selectors/parser.rssrc/css/selectors/selector.rssrc/css/small_list.rssrc/css/targets.rssrc/css/values/calc.rssrc/css/values/color.rssrc/css/values/easing.rssrc/css/values/gradient.rssrc/css/values/image.rssrc/css/values/length.rssrc/css/values/percentage.rssrc/css/values/syntax.rstest/js/bun/css/css.test.ts
💤 Files with no reviewable changes (1)
- src/css/lib.rs
🛑 Comments failed to post (3)
src/css/properties/properties_generated.rs (1)
2806-2827: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify PrefixFeature has Animation/AnimationName variants and correct browser mappings rg -n 'PrefixFeature' src/css/targets.rs | head -50 rg -n 'Animation' src/css/targets.rsRepository: oven-sh/bun
Length of output: 203
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- PrefixFeature enum and mappings ---' rg -n 'enum PrefixFeature|PrefixFeature::Animation|AnimationName|AnimationTimelineShorthand|prefix_feature' src/css -g'*.rs' -g'*.ts' echo '--- targets.rs around Feature mappings ---' sed -n '430,540p' src/css/targets.rs echo '--- properties_generated.rs around the cited lines ---' sed -n '2788,2845p' src/css/properties/properties_generated.rsRepository: oven-sh/bun
Length of output: 13439
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the PrefixFeature type and any Animation-related variants/usages. rg -n 'PrefixFeature' src/css rg -n 'AnimationName|AnimationTimelineShorthand|Animation\b' src/css/targets.rs src/css -g'*.rs'Repository: oven-sh/bun
Length of output: 13883
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,340p' src/css/prefixes.rsRepository: oven-sh/bun
Length of output: 9825
🌐 Web query:
caniuse CSS animations vendor prefixes -webkit -moz -o -ms animation animation-name💡 Result:
As of July 5, 2026, vendor prefixes for CSS animations—including -webkit-, -moz-, -o-, and -ms-—are not required for modern web development [1][2]. Standard, unprefixed CSS animation properties and the
@keyframesat-rule are widely supported across all modern browsers, including Chrome, Edge, Firefox, Safari, and Opera [1][3]. The necessity for these prefixes was relegated to legacy browser versions that have long since been deprecated or phased out [1][4][2]. Key points regarding vendor prefixes for animations: - Unprefixed support: Modern browsers (e.g., Chrome 43+, Firefox 16+, Safari 9+, Edge 12+) support the standard, unprefixed animation syntax by default [1][3]. - Legacy usage: Prefixes like -webkit- or -moz- were used in early browser implementations (e.g., Firefox 5-15, Chrome 4-42) to provide experimental support, but these are no longer needed for current projects [1]. - No -ms- support: The -ms- prefix was never officially used for CSS animations or keyframes in Internet Explorer [5]. - Best practice: Instead of using vendor prefixes, you should use the standard, unprefixed properties [2]. If you need to detect support for older environments or specific features, use the@supportsat-rule (e.g.,@supports(animation-name: test)) or feature detection in JavaScript [6][7]. Including vendor prefixes in your CSS today serves no functional purpose for modern users and can even lead to bloated, harder-to-maintain code [5][4]. If you are working on a project that requires supporting extremely old, legacy browsers, you may choose to use build-time tools (such as Autoprefixer) to automatically manage any remaining necessary prefixes based on your specified target browser list [2].Citations:
- 1: https://www.testmuai.com/learning-hub/css-animation-browser-support/
- 2: https://weser.io/blog/vendor-prefixes-in-2024
- 3: https://modern-css.com/browser-support/
- 4: https://blog.omgmog.net/post/why-vendor-prefixes-are-still-in-your-css/
- 5: salesforce/lwc#2620
- 6: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/At-rules/@media/-webkit-animation
- 7: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/At-rules/%40supports
🌐 Web query:
caniuse animation -ms prefix@-ms-keyframes``💡 Result:
There is no such thing as an
@-ms-keyframesat-rule in CSS [1][2]. Internet Explorer 10 and later, which introduced support for CSS animations, implemented the standard unprefixed@keyframessyntax [2][3]. The -ms- prefix was never used for keyframes in any stable version of Internet Explorer or Microsoft Edge [1][4][2]. Any references to@-ms-keyframesoften stem from misunderstandings or pre-release developer preview versions that are long obsolete [1][5]. Using an -ms- prefix for keyframes is not only unnecessary but can also cause the entire animation rule to be ignored by the browser [4]. To implement CSS animations, you should use only the standard, unprefixed@keyframessyntax [4][2][6].Citations:
- 1: https://stackoverflow.com/questions/28545533/how-should-you-prefix-transform-properties-in-css3-animations
- 2: https://web.archive.org/web/20130305072149/http:/msdn.microsoft.com/en-us/library/ie/hh772747.aspx
- 3: https://learn.microsoft.com/en-us/previous-versions/hh781508(v=vs.85)
- 4: cssinjs/jss#695
- 5: CSSLint/csslint#295
- 6: https://css-tricks.com/snippets/css/keyframe-animation-syntax/
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' src/css/prefixes.rs printf '\n--- property id prefix handling ---\n' sed -n '1260,1375p' src/css/properties/properties_generated.rs printf '\n--- feature/prefix application logic ---\n' sed -n '1,120p' src/css/properties/properties_generated.rsRepository: oven-sh/bun
Length of output: 12278
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find how VendorPrefix is converted/filtered during serialization. rg -n 'intersects\(pre\)|VendorPrefix::MS|prefixes_for\(|prefix_feature\(\)' src/css -g'*.rs'Repository: oven-sh/bun
Length of output: 29819
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect whether the parser intentionally allows prefixes that prefixes_for never emits. sed -n '1,140p' src/css/prefixes.rs printf '\n---\n' sed -n '1260,1375p' src/css/properties/properties_generated.rs printf '\n---\n' sed -n '5800,5865p' src/css/properties/properties_generated.rsRepository: oven-sh/bun
Length of output: 10260
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '320,390p' src/css/prefixes.rsRepository: oven-sh/bun
Length of output: 3223
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path text = Path('src/css/prefixes.rs').read_text().splitlines() for start in [330]: for i in range(start-1, min(len(text), start+80)): print(f"{i+1}:{text[i]}") PYRepository: oven-sh/bun
Length of output: 3985
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path text = Path('src/css/prefixes.rs').read_text().splitlines() for i in range(330-1, min(len(text), 410)): print(f"{i+1}:{text[i]}") PYRepository: oven-sh/bun
Length of output: 3985
Remove
VendorPrefix::MSfrom theanimationallowlists.PrefixFeature::AnimationandPrefixFeature::AnimationNamenever emit MS, so-ms-animation/-ms-animation-nameare accepted here even though the feature table doesn't support them.🤖 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 `@src/css/properties/properties_generated.rs` around lines 2806 - 2827, The allowlists in the property parsing branch for animation properties incorrectly include VendorPrefix::MS, which lets unsupported -ms-animation and -ms-animation-name through. Update the matching logic in properties_generated.rs for the PropertyId::AnimationName and PropertyId::Animation cases to remove VendorPrefix::MS from the allowed prefixes so they only accept prefixes actually emitted by PrefixFeature::Animation and PrefixFeature::AnimationName.src/css/rules/mod.rs (1)
852-861: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check whether Location derives/implements Default, and how dummy()/default() are used elsewhere rg -n -B3 -A3 'struct Location' src/css/lib.rs rg -n -B2 -A8 'impl Location' src/css/lib.rs rg -n 'Location::default\(\)|Location::dummy\(\)' src/cssRepository: oven-sh/bun
Length of output: 898
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the surrounding code paths for the placeholder rule and nearby location handling. sed -n '620,670p' src/css/rules/mod.rs printf '\n---\n' sed -n '840,870p' src/css/rules/mod.rs printf '\n---\n' sed -n '2548,2580p' src/css/css_parser.rsRepository: oven-sh/bun
Length of output: 4762
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the Location definition and all call sites to distinguish Default vs dummy semantics. sed -n '320,350p' src/css/lib.rs printf '\n---\n' rg -n -A2 -B2 'Location::default\(\)|Location::dummy\(\)' src/cssRepository: oven-sh/bun
Length of output: 1995
Use
Location::dummy()for the placeholder rule
Location::default()is fine here, butdummy()is the established sentinel for synthesized rules with no real source location, so this keeps placeholder handling consistent.🤖 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 `@src/css/rules/mod.rs` around lines 852 - 861, The placeholder StyleRule created in the code path around core::mem::replace should use Location::dummy() instead of Location::default() so synthesized rules are marked consistently with the rest of the codebase. Update the style::StyleRule initialization in this block to use the dummy source location sentinel, keeping the existing selectors, vendor_prefix, declarations, and rules unchanged.src/css/selectors/parser.rs (1)
3555-3565: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve namespaces for lowercase attribute selectors.
The
AttrQName::Specific/Anymapping createsSome(namespace), but the value-selector path below only usesAttributeOtherwhennamespace.is_some() && !local_name_is_ascii_lowercase. Lowercase namespaced attributes like[svg|href=x]can fall through toAttributeInNoNamespaceand lose the namespace. Gate the optimized no-namespace variant onnamespace.is_none()instead.Proposed fix
- if namespace.is_some() && !local_name_is_ascii_lowercase { + if namespace.is_some() || !local_name_is_ascii_lowercase {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if namespace.is_some() || !local_name_is_ascii_lowercase {🤖 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 `@src/css/selectors/parser.rs` around lines 3555 - 3565, The attribute selector parsing in parser logic is dropping namespaces for lowercase namespaced attributes because the value-selector path only falls back to AttributeOther when namespace.is_some() and the local name is not ASCII lowercase. Update the selector handling around AttrQName::Specific/Any and the attribute value branch so the optimized no-namespace variant is used only when namespace.is_none(), and keep namespaced selectors like svg|href routed through the namespaced path even for lowercase local names.
There was a problem hiding this comment.
Thanks for addressing the earlier nits — both look fixed. No bugs found, but this is a ~25-file structural refactor of the CSS crate (selector qname parsing split, minify_style_arm placeholder change, calc/length add arm reshaping, new ColorFallback/as_single semantics), so it warrants a pass from someone who owns src/css/.
Extended reasoning...
Overview
This PR removes all 66 unreachable!() sites from src/css/ across 25 files. The changes fall into several categories: (1) type restructuring — new ColorFallback enum split from the ColorFallbackKind bitflags, to_light_dark() returning a tuple, parse_qualified_name split into type-selector and attribute-selector variants with distinct return types, extract_* returning Option<&T>; (2) dead-code deletion — AnimationTimeline::{Scroll,View} and payload types, AtRulePrelude::FontFeatureValues, Token::raw, VendorPrefix::from_name_str, four deprecated to_css shims; (3) borrowck-workaround collapse — matches!-then-rematch patterns folded into direct or-pattern matches in calc.rs, length.rs, percentage.rs, selector.rs, custom.rs; (4) debug_assert! + safe fallback for cssparser delimiter contracts. Regression tests were added for the two user-input-reachable sites.
Security risks
None identified. This is purely internal to the CSS parser/printer/minifier; no auth, crypto, network, filesystem, or FFI surface is touched. The changes replace release-build panics with either compile-time exhaustiveness or debug-assert-plus-graceful-fallback, which strictly reduces DoS surface on adversarial CSS input.
Level of scrutiny
High. While each individual pattern is defensible, several changes need equivalence verification by someone familiar with the crate:
- The
parse_qualified_name→parse_qualified_name+parse_attr_qualified_namesplit duplicates ~60 lines of parser control flow with subtly different token handling (the*-after-star and*-as-local-name paths diverge between the two). minify_style_armnow takes&mut StyleRule<R>and leaves an emptyStyleRuleplaceholder viamem::replaceinstead ofCssRule::Ignored; whether any downstream code observes the placeholder needs confirmation.- The
length.rs/percentage.rsaddrestructuring widens the first two match arms (dropping thematches!(*ca, Calc::Value(_))guard) and handles the non-Valuecase inline; I traced this as equivalent to the old fallthrough viainto_calc(), but it deserves a second pair of eyes. ColorFallbackKind::as_single()returnsNonefor the OKLAB bit (and any multi-bit set), where the oldget_fallbackpath would have panicked — whetherlowest()can ever yield OKLAB determines if this is a silent behavior change.ColorFallback::supports_condition()gained anRgbarm that the old bitflags version panicked on; it appears unreachable from current callers but is new API surface.
Other factors
Both of my earlier cosmetic comments (gradient.rs -> spacing, stale Multiplier comment in syntax.rs) were addressed in follow-up commits and the threads are resolved. The bug-hunting system found nothing this run. Test coverage is stated as 1328 passing across the CSS/bundler suites. The ~200-variant exhaustive match in targets.rs is verbose but does exactly what the description says (compile-time coverage of compat::Feature). Given the breadth of parser/serializer control-flow reshaping, this is the kind of PR a src/css/ owner should sign off on rather than a bot.
…lector tests - targets.rs: from_compat returns Option<Features>; should_compile_same falls back to !is_compatible() for unmapped features instead of the always-true contains(empty()) path. - properties_generated.rs: debug_assert on __to_css_helper Custom arm. - css_parser.rs: debug_assert on the second parse_until_before contract site, matching its sibling. - css.test.ts: drop the 6 tests that exercised unchanged arms; add QNamePrefix selector matrix ([svg|attr], |foo, |*, svg|*, [*] error).
There was a problem hiding this comment.
Both earlier nits are addressed and I didn't find any bugs this pass, but this is a ~25-file refactor that reshapes several public-ish contracts (ColorFallback enum, split qualified-name parsing, minify_style_arm signature, length/percentage add rewrites) — worth a human sanity-check on the behavior-preservation claims before merge.
Extended reasoning...
Overview
This PR eliminates all 67 unreachable!() sites from src/css/ across 25 files. The changes fall into four buckets: (1) type restructuring so invalid states are unrepresentable — new ColorFallback enum split from the ColorFallbackKind bitflags, CssColor::to_light_dark() returning a tuple, parse_qualified_name split into type-selector and attribute-selector variants with distinct return types, SizeHandlerSpec::extract_* returning Option<&T>, new RepeatSeparator enum; (2) collapsing stale borrowck-workaround matches!-then-rematch patterns into direct or-pattern matches (calc.rs, length.rs, percentage.rs, selector.rs, custom.rs); (3) deleting dead variants/helpers (AnimationTimeline::{Scroll,View} + payload types, AtRulePrelude::FontFeatureValues, Token::raw, VendorPrefix::from_name_str, four deprecated to_css shims); (4) converting cssparser-contract invariants to debug_assert! + safe fallback.
Security risks
None identified. This is entirely internal to the CSS parser/minifier — no auth, crypto, filesystem, network, or user-input validation surfaces are touched. The changes convert release-build panics into either compile errors or debug-asserts with graceful fallbacks, which is strictly a robustness improvement (a reachable unreachable!() on user CSS input was previously a DoS vector).
Level of scrutiny
Medium-high. While the intent (remove panics) is straightforward and the individual patterns are sound, several changes require careful behavior-preservation verification that I can't fully validate without running the suite:
length.rs/percentage.rsaddrewrites restructure the match arms — the new nested-match shape must produce identicalCalc::Sumtrees for every (Calc-variant × non-Calc) combination as before.minify_style_armnow takes&mut StyleRuleand swaps in an emptyStyleRuleplaceholder instead ofCssRule::Ignored— the caller loop's subsequent handling of the swapped-out slot needs to be equivalent.parse_attr_qualified_nameis ~60 lines of freshly-written parsing logic split from the shared function; the added tests cover the matrix but a maintainer familiar with the Lightning CSS reference should confirm the error-kind choices match.ColorFallback::Rgb.supports_condition()is a new code path (previouslyunreachable!()) — it's never called in practice per the callers, but the value chosen (rgb(0, 0, 0)) is a design decision.- The 200-variant exhaustive arm in
Features::from_compatis mechanical but easy to typo.
Other factors
Both of my earlier cosmetic nits (gradient.rs -> spacing, syntax.rs stale comment) are resolved in the current diff. The bug-hunting system found nothing this run. Tests were added for the QNamePrefix split. The PR description is thorough and the commit log shows iterative review response. Given the scope (25 files, core CSS pipeline) and the number of independent refactors bundled together, a human maintainer sign-off is the right bar here — this is well beyond a mechanical change even though each individual pattern is defensible.
…ll_jsc, node-fallbacks, and misc crates (#39420) Net -4,402 lines (172 files, +509 / -4,911). Everything removed has zero references across `src/`, `scripts/`, `test/`, `packages/` and the regenerated `build/debug/codegen/` output, and the removal builds: removing a Rust or C++ definition that still had a caller fails to compile or link, so a green build is the reference check for those; JS, codegen and manifest removals were additionally grepped by name (including `bun:internal-for-testing` consumers under `test/`). Most of these were first identified by the sweeps that were closed yesterday for merge conflicts (#37272, #37062, #38439, #37089, #38703). This PR re-applies the subset that still applies on current main, minus anything an open PR already deletes and minus small hunks in files that change daily (see "Left out" below), plus a few new finds. ### react_compiler (-1,166) - `validate_no_derived_computations_in_effects_exp` and its ~22 exclusive helpers/types (~1.1k lines in `validation/validate_no_derived_computations_in_effects.rs`). The pipeline only ever calls the non-`_exp` validation; the `_exp` env-config flag was parsed from fixture pragmas and read by nothing. `react-compiler-fixtures.test.ts` now lists the two pragmas as ignored instead of handled; the fixture suite still passes. - `SymbolHost` (back-compat alias of `Host`; `DESIGN.md` updated), `HirBox`, `is_use_state_type`, `default_true`. ### C++ bindings (-1,350 across 64 files) - `JSDOMConvertBufferSource.h`: the IDL typed-array specializations and `toPossiblyShared*Array` helpers for every element type no binding converts (only the Uint8Array/ArrayBuffer views are used). - `JSDOMPromiseDeferred.h/.cpp`: `resolveWithJSValue`, `resolveWithNewlyCreated`, `resolveCallbackValueWithNewlyCreated` and friends. - `IDLTypes.h`: the `IDLUnsupportedType` family, the IDB / WebGL / `ScheduledAction` wrappers and stale forward declarations; `JSDOMConvertDate.h/.cpp` (the only `IDLDate` converter) deleted along with its two includes. The 8-line `IDLDate` struct itself stays for now (see "Left out"). `JSDOMConvertScheduledAction.h`, orphaned in the same way, is already deleted by #35775, so it is not touched here. - `NetworkLoadMetrics.h`: WebKit networking-stack fields/accessors bun never reads. - Deleted files: `JSWorkerOptions.h/.cpp` (`JSWorker.cpp` builds `WorkerOptions` by hand), `JSMIMEBindings.h/.cpp` (`createMIMEBinding` had no callers; include dropped from `ZigGlobalObject.cpp`), `JSDOMIterator.cpp` (`addValueIterableMethods`). - `ncrypto.h/.cpp`: `peekError`, `BignumPointer::isZero`, `EVPKeyCtxPointer::sign`, unused copy/move `operator=` overloads and an `AsymmetricKeyEncodingConfig` constructor. - Smaller: `JSDOMOperationReturningPromise.h` (`call*ReturningOwnPromise`), `JSDOMAttribute.h` (`setPassingPropertyName`, `setStatic`), `JSDOMGuardedObject` (`DoNotRegisterWithGlobalObjectTag`), `Event`/`EventTarget` (`resetBeforeDispatch`, default-handled flags, `isNode()`), `EventEmitter::{eventTypes,eventListeners}`, `HTTPHeaderIdentifiers::identifierFor`, `JSURLPatternResult` `convertDictionary` stubs, `PerformanceTiming::monotonicTimeToIntegerMilliseconds`, `ContextDestructionObserver::protectedScriptExecutionContext`, `expectedEnumerationValues<CryptoKeyUsage>`, `BufferSource::mutableData`/`toBufferSource`, `BunString__toInt32`, `BunString::utf8ByteLength`, the `Ref<StringImpl>` overload of `toCrossThreadShareable`, `normalWorld()`, `TextEncoding(const String&)`, `JSBuffer` `createBuffer`/`constructFromEncoding` overloads, the `JSX509Certificate` `m_infoAccess` lazy property and the non-legacy branch of `computeInfoAccess` (the prototype getter reads the view directly; x509 tests pass), `BakeAdditionsToGlobalObject::wrapComponent`, `jsFunction_lsanDoLeakCheck`, and the dead `BunObject+exports.h` macro entries (together with the Rust `BunObject_callback_nanoseconds` export the `nanoseconds` entry declared; `Bun.nanoseconds` is `functionBunNanoseconds` in `BunObject.cpp`). ### install_jsc / install_types / bun:internal-for-testing (-344) - `install_jsc/dependency_jsc.rs` and `update_request_jsc.rs` deleted: their only consumers were the `npa` / `npmTag` exports of `bun:internal-for-testing`, which no test imports. The `dispatch_js2native.rs` re-exports, the `generate-js2native.ts` file-map entry and the `lsanDoLeakCheck` export (tests use `isASANEnabled`) go with them. - `install_types/lib.rs`: the `ExternalString` / `SlicedString` / `SemverString` re-export modules; nothing names those paths (everything imports the types from `bun_semver`). New in this PR. ### node-fallbacks (-400) and codegen (-392) - `util.js`: a ~270 line commented-out `util.types` block. - `package.json` / `bun.lock` / `tsconfig.json`: dependencies and path mappings nothing imports (`esbuild`, `buffer`, `events`, `util`, `url`, `process`, `path-browserify`, `os-browserify`, `timers-browserify`, `tty-browserify`, `vm-browserify`, ...). `build-fallbacks.ts` marks every builtin name external, and the remaining sources only import the packages still listed; `bun install --frozen-lockfile` in the directory is a no-op and `bundler_browser.test.ts` passes. - `src/codegen/generate-unified-source-bundles.rb`: WebKit's Ruby generator, superseded by `scripts/build/unified.ts`; nothing invokes it. ### Rust crates (-1,200 across bun_jsc, bun_runtime, css, uws_sys and leaf crates) - bun_jsc: `BuiltinName::get` + `BUILTIN_NAME_MAP`, `MarkedArrayBuffer::to_js` (the `ArrayBuffer::alloc` Uint8Array arm and its `Bun__allocUint8ArrayForCopy` binding are kept, so `alloc` keeps mirroring `create`), the `__dangerouslySetPtr` wrapper `js_class_module!` emitted into every class, `ErrorBuilder::new`, `TopExceptionScope::new`, `Task::new`, `JSCell::to_js`, `JSGlobalObject::{to_js,ref_,ctx}`, `job::{on_js_thread,off_thread}`, `AbortReason` impl, `JSPromise` settle helpers, `UUID::ZERO`, `TagPayload::get`. - bun_runtime: the `target_os = "wasi"` directory-iterator backend in `dir_iterator.rs` (no shipped target is wasi and the `bun_sys::wasi` module it imports does not exist), the `Display` impls in `assert/myers_diff.rs`, the non-unix stub and not-macos escapes in `fs_events.rs` (the file is only compiled on macOS), `ArrayBufferSink::to_js`, unused re-exports in `node.rs` / `api/bun/spawn.rs` / `ffi/mod.rs` (with `abi_type` formatters narrowed to `pub(crate)`), `Error::UnableToDecode`, `MyersDiff::Error::OutOfMemory`. The first three are new in this PR. - css: the inherent `eql` / `to_css` / `parse` forwarders whose callers all go through the `CssEql` / `ToCss` / `Parse` trait impls (`values/calc.rs` and friends), `generics::{implement_eql,parse}`, `TokenList::parse_with_options`, `CssString::parse`. - uws_sys: the `uws_loop_defer`, `uws_res_clear_corked_socket`, `uws_ws_iterate_topics` and `uws_h3_req_get_parameter` C shims plus their Rust declarations and wrappers (`Loop::{uncork,wake,next_tick,run}`), `AnyResponse::init`, `SocketGroup::is_empty`, `socket.rs` `group()` accessors and the `SocketTcp`/`SocketTls` aliases, `Opcode::Close`, `WindowsLoop`. - leaf crates: `windows_sys` constants and their `bun_sys::windows` re-exports, `zlib`/`zlib_sys` declarations (`deflateInit_`, `inflateInit_`, the `gz*` file API, legacy type aliases), `sha_hmac` deprecated-API hashers (`SHA512` raw, `RIPEMD160`, `MD5_SHA1`, `Blake2` evp), `wyhash` `HashInt` impls for u16/u64, `libarchive` commented-out Zig-era callbacks, `bun_alloc` (`AllocError::name`, `usable_size`, `BSSList::init`), `clap::Error::WriteFailed`, `csrf` error variants, `pe::Error::{InputIsSigned,InsufficientSpace}`, `md` `Setextheader`, `opaque_mut_nn`, `cares_sys` `AddrInfo_hints::is_empty`, `boringssl_sys` constants, `errno` `Mode` re-exports, `bounded_array::get`, `string::write::Result`, `SplitIterator::rest`, `OutOfRangeValue` impls, `symbol::Map::init`, `sql_jsc` re-exports. ### src/js (-32) - Unused REPL primordials entries in `internal/repl/node-primordials.js`; with that gone `SafeWeakSet` had no importer, so `internal/primordials.js` stops exporting it (new in this PR). Unused export-object entries in `internal/fs/watch.ts` and `internal/readline/interface.js`. ### scripts (-9) - `glob-sources.ts` `src/*.c` pattern (matched nothing since `asan-config.c` was deleted), the write-only `BUN_DEP_*` defines in `depVersionsHeader.ts`, the unread `kqueue` config field. ### Verification - `bun bd` (full debug build) passes. - `bun run rust:check-all`: all 11 target triples ok (covers the windows/darwin-only removals in `windows_sys`, `sys/windows`, `zlib_sys/win32.rs`, `fs_events.rs`, `windows-shim`). - `cargo fmt --check`, `cargo clippy --workspace`, clang-format on every touched C++ file, prettier, and `bun run lint` are clean. - All of `test/internal/source-lints/` (including the new `dead-symbols-react-compiler-webcore-idl-misc.test.ts` that guards these symbols, and `dead-code-escapes` against the updated `dead-code-escape-limits.json`), `react-compiler-fixtures.test.ts`, `bundler_browser.test.ts`, css, cryptohasher/hash, node:assert, zlib, url, events, websocket, inspect and x509 tests pass. `serve.test.ts` has the same 4 failures as the unmodified release build in this container (IPv6 / root port range), nothing else. ### Left out on purpose (follow-up candidates) - The pre-engine inbound path in `h2_frame_parser.rs` (~2.2k lines, still dead, #37272's diff still applies cleanly): the file has had 15 commits in the last two weeks, so it is better landed on its own. - Deletions already owned by open PRs: simdutf wrappers (#38958), `getStackTraceForThrownValue` (#37450), `validateOneOf` (#38401), the redis error variants (#34829), `URL::from_js` (#33889 / #34577), `schema::api` re-exports (#37095), `FsPath` (#39327), `NodeJSFS` `Null` impl (#38065), the deprecated selector `to_css` (#33332). - Small hunks in high-churn files (`bindings.cpp`, `ZigGlobalObject.cpp`, `Blob.rs`, `streams.rs`, `BunProcess.cpp`, the `ManifestLoad::LoadFromMemory` parameter across `src/install`, the watcher `loader` parameter), and the `#[no_mangle]` statics (`Zig_ErrorCode*`, `Bun__versions_*`) nothing on the C++ side reads. - `VM::has_termination_request` and its `JSC__VM__hasTerminationRequest` shim in `bindings.cpp`: a dead pair, kept intact here because `bindings.cpp` is the most actively edited file in the tree; both halves go together in a follow-up. - `IDLDate` in `IDLTypes.h`: its only converter is deleted here, but the struct itself is left in place so this PR's file deletions stay independent of the header edits; removing the struct is a one-hunk follow-up once the converter files are gone. - Found but not removed here: the windows shim's `ReadWithoutLaunch` mode (~110 lines, overlaps #36200), `node_quic_binding.rs` constants JS never destructures (~35 lines), the native `NodeJSFS.unwatchFile` / `FSWatcher.hasRef` / `QuicSession.silentClose` / `QuicEndpoint.ref` bindings JS never calls, never-constructed `bun_install::Error` variants, and ~160 lines of `$`-declarations in `src/js/builtins.d.ts` with no users. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 1 · 172 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 10 failed, 320 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/react-compiler-fixtures.test.ts test/internal/source-lints/dead-symbols-react-compiler-webcore-idl-misc.test.ts bun test v1.4.0 (8326d1b) test/internal/source-lints/dead-symbols-react-compiler-webcore-idl-misc.test.ts: 88 | // Arena box alias with zero uses (HirVec is the one HIR actually uses). 89 | ["src/react_compiler/hir/mod.rs", /\bHirBox\b/], 90 | // Type predicate whose only callers were in the removed _exp validation. 91 | ["src/react_compiler/hir/mod.rs", /\bis_use_state_type\b/], 92 | ]; 93 | expect(resurrected(checks)).toEqual([]); ^ error: expect(received).toEqual(expected) - [] + [ + "src/react_compiler/validation/validate_no_derived_computations_in_effects.rs: validate_no_derived_computations_in_effects_exp", + "src/react_compiler/hir/environment_config.rs: validate_no_derived_computations_in_effects_exp", + "src/react_compiler/program.rs: validate_no_derived_computations_in_effects_exp", + "src/react_compiler/program.rs: \bSymbolHo ... (truncated) release without fix: 10 failed, 1146 skipped bun test v1.4.0-canary.1 (21a4206) test/internal/source-lints/dead-symbols-react-compiler-webcore-idl-misc.test.ts: 88 | // Arena box alias with zero uses (HirVec is the one HIR actually uses). 89 | ["src/react_compiler/hir/mod.rs", /\bHirBox\b/], 90 | // Type predicate whose only callers were in the removed _exp validation. 91 | ["src/react_compiler/hir/mod.rs", /\bis_use_state_type\b/], 92 | ]; 93 | expect(resurrected(checks)).toEqual([]); ^ error: expect(received).toEqual(expected) - [] + [ + "src/react_compiler/validation/validate_no_derived_computations_in_effects.rs: validate_no_derived_computations_in_effects_exp", + "src/react_compiler/hir/environment_config.rs: validate_no_derived_computations_in_effects_exp", + "src/react_compiler/program.rs: validate_no_derived_computations_in_effects_exp", + "src/react_compiler/program.rs: \bSymbolHost\b", + "src/react_compiler/lib.rs: \bSymbolHost\b", + "src/react_compiler/hir/mod.rs: \bHirBox\b", + "src/react_compiler/hir/mod.rs: \bis_use_state_type\b", + ] - Expected - 1 + Received + 9 at <anonymous> (/workspace/bun/test/internal/source ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 320 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/react-compiler-fixtures.test.ts test/internal/source-lints/dead-symbols-react-compiler-webcore-idl-misc.test.ts bun test v1.4.0 (8326d1b) test/internal/source-lints/dead-symbols-react-compiler-webcore-idl-misc.test.ts: (pass) dead react_compiler symbols do not reappear [21.23ms] (pass) dead exe_format symbols do not reappear [2.63ms] (pass) dead bun_core / bun_alloc / bun_ast / bun_ptr items do not reappear [18.66ms] (pass) dead bun_css items do not reappear [21.91ms] (pass) dead bun_jsc items do not reappear [20.67ms] (pass) dead FFI-crate items do not reappear [30.81ms] (pass) dead Rust symbols (install, webcore, jsc, leaf crates) do not reappear [18.03ms] (pass) unused re-export names do not reappear [12.35ms] (pass) stale build-script entries do not reappear [6.80ms] (pass) dead Rust FFI wrappers and trait methods do not reappear [22.18ms] (pass) dead C++ binding helpers do not reappear [99.82ms] (pass) dead WebCore / IDL binding code does not reappear [114.33ms] (pass) dead code in install_jsc, install_t ... (truncated) release with fix: 1146 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 641ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/130] install /workspace/bun/src/node-fallbacks bun install v1.4.0-canary.1 (21a4206) Checked 111 installs across 104 packages (no changes) [3.00ms] [2/130] gen node-fallbacks/react-refresh.js Bundled 1 module in 5ms react-refresh.js 4.81 KB (entry point) [3/130] gen JSBuffer.lut.h Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp [4/130] gen generated_host_exports.rs generated_host_exports.rs: 92 exports (host=3, lazy=10, generic=79, rust=0); 241 extern-C blocks audited [5/130] gen node-fallbacks/*.js [6/130] gen cpp.rs (cppbind) [7/130] gen JS modules (bundle-modules) Preprocess modules (7887ms) Bundle modules (42ms) Postprocesss modules (259ms) Bundle Functions (695ms) Generate Code (32ms) [8.94s] Bundled "src/js" for production 2630 kb 197 internal modules 13 native modules 91 internal functions across 17 files [7/129] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknow ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` scripts/build/config.ts | 4 - scripts/build/depVersionsHeader.ts | 3 - scripts/build/source.ts | 2 +- scripts/glob-sources.ts | 1 - src/ast/symbol.rs | 8 - src/boringssl_sys/boringssl.rs | 8 - src/bun_alloc/lib.rs | 29 +- src/bun_core/bounded_array.rs | 8 - src/bun_core/fmt.rs | 17 - src/bun_core/string/immutable.rs | 7 - src/bun_core/string/mod.rs | 11 - src/bun_core/string/write.rs | 3 - src/bun_core/windows_sys.rs | 2 +- src/cares_sys/c_ares.rs | 6 - src/clap/error.rs | 9 - src/clap/lib.rs | 5 - src/codegen/generate-js2native.ts | 1 - src/codegen/generate-unified-source-bundles.rb | 392 ------- src/csrf/lib.rs | 3 - src/css/css_parser.rs | 19 +- src/css/generics.rs | 10 - src/css/lib.rs | 2 +- src/css/properties/custom.rs | 8 +- src/css/rules/mod.rs | 7 +- src/css/values/angle.rs | 4 - src/css/values/calc.rs | 132 +-- src/css/values/css_string.rs | 6 - src/css/values/time.rs | 7 - src/css_derive/lib.rs | 4 +- src/css_jsc/css_internals.rs | 11 - src/errno/darwin_errno.rs | 1 - src/errno/freebsd_errno.rs | 1 - src/errno/linux_ ... (truncated) ``` </details> **gate history** · 4 passed · 1 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests scripts/build/config.ts 0 0 0 scripts/build/depVersionsHeader.ts 0 0 0 scripts/build/source.ts 0 0 0 scripts/glob-sources.ts 0 0 0 src/ast/symbol.rs 0 0 0 src/boringssl_sys/boringssl.rs 0 0 0 src/bun_alloc/lib.rs 0 0 0 src/bun_core/bounded_array.rs 0 0 0 src/bun_core/fmt.rs 0 0 0 src/bun_core/string/immutable.rs 0 0 0 src/bun_core/string/mod.rs 0 0 0 src/bun_core/string/write.rs 0 0 0 src/bun_core/windows_sys.rs 0 0 0 src/cares_sys/c_ares.rs 0 0 0 src/clap/error.rs 0 0 0 src/clap/lib.rs 0 0 0 (+ 156 more files) ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
This branch has not been deployed
2 participants
What does this PR do?
Removes all 67
unreachable!()sites fromsrc/css/by making the invalid states compile errors instead of runtime panics.Each site was traced to its invariant and cross-referenced against Lightning CSS. The fixes fall into a handful of patterns:
Type restructuring — the invalid state is no longer representable:
ColorFallbackKindbitflags was used both as a set and as a single discriminant → newColorFallbackenum with three variants;get_fallbackmatches exhaustively.CssColor::to_light_dark()returned aCssColorthat was always theLightDarkvariant, forcing every caller tolet-else unreachable!()→ now returns a(Box<CssColor>, Box<CssColor>)tuple.parse_qualified_name(in_attr_selector: bool)returned a 6-variantQNamePrefixwhere each of its two callers panicked on the other's variants → split into two return types.margin_paddingextract_*fns panicked on wrong variant → returnOption<&T>; callers fuse the tag guard.QueryCondition::extra_to_csshad a panicking default → made a required trait method.Features::from_compatreturnsOption<Features>(wasFeatureswith anempty()fallthrough);should_compile_samehandlesNoneby falling back to!is_compatible()— semantically correct instead of the always-truecontains(empty()).Stale borrowck workarounds —
matches!()guard followed by re-matching the same value with_ => unreachable!()→ collapsed to direct by-value match / or-patterns (calc.rs,selector.rs,custom.rs,css_parser.rs).Dead code deleted — variants and helpers with zero construction sites:
AnimationTimeline::Scroll/Viewand their four payload types (ScrollTimeline,ViewTimeline,Scroller,ScrollAxis),AtRulePrelude::FontFeatureValues,Token::raw(),VendorPrefix::from_name_str, four#[deprecated]to_csspoison-pill shims.Internal contracts — cssparser
parse_until_beforedelimiter guarantees and similar invariants that can't be encoded in types →debug_assert!with a safe fallback (return error / emit nothing) instead of a release-build panic.How did you verify your code works?
rg 'unreachable!' src/css/→ 0 (was 67 including one added by css: scope animation-name to its @keyframes hash in CSS modules (#18921) #33322 while this branch was open).cargo clippy -p bun_css --no-depsclean.bun bd test test/js/bun/css/css.test.ts test/bundler/css/ test/bundler/esbuild/css.test.ts— all pass.[svg|attr],|foo,|*,svg|*, and the[*]error path.