Skip to content

css: pass SelectorFlags to SelectorBuilder::build instead of three bools - #39122

Merged
alii merged 2 commits into
mainfrom
farm/03c445e7/css-selector-build-flags
Aug 15, 2026
Merged

alii merged 2 commits into
mainfrom
farm/03c445e7/css-selector-build-flags

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • mordant's bare_bool_args flags SelectorBuilder::build in src/css/selectors/builder.rs:113: it takes parsed_pseudo: bool, parsed_slotted: bool, parsed_part: bool, and Selector::from_component_in calls it as build(false, false, false) (src/css/selectors/parser.rs:1886), where nothing says which flag is which.
  • The three bools exist only to be ORed into a SelectorFlags value inside build() (HAS_PSEUDO, HAS_SLOTTED, HAS_PART), so the function already had a named representation for its input; the bools were an extra layer in front of it.

Fix

  • build() takes the SelectorFlags directly. The OR chain moves unchanged into parse_selector, the one caller that computes the flags from the parsing state; from_component_in passes SelectorFlags::empty(). The flags stored on the resulting selector are bit for bit what they were before, so no behavior change and no new test.
  • The lint only considers functions with two or more bool parameters; build() now has none, and nothing else in builder.rs qualifies, so the bare_bool_args:src/css/selectors/builder.rs entry is removed from mordant-baseline.toml. That is the only entry the change clears, so the file was edited by hand rather than regenerated.
  • Verified with bun bd:
    • bun bd test test/js/bun/css/: 2287 pass, 68 skip. The 9 failures are all timeouts of stress tests in this slow debug+ASAN container: the fuzz tests (css-fuzz.test.ts, gated !isCI with a hardcoded 10s budget), and six expansion-bound tests that pass when rerun with --timeout 120000.
    • bun bd test test/bundler/css/: 169 pass.
    • bun bd test test/bundler/esbuild/css.test.ts: 56 pass (includes DeduplicateRules, which compares selectors including these flags).

Background

  • SelectorBuilder accumulates the simple selectors and combinators of one selector while it is being parsed; build() turns them into the final component list plus a SpecificityAndFlags (the computed specificity and a SelectorFlags bitset).
  • SelectorFlags (bitflags, defined in src/css/selectors/parser.rs) records whether the selector contains a pseudo-element, ::slotted() or ::part(). HAS_PSEUDO is what Selector::has_pseudo_element() reads when deciding whether style rules can be merged; the flags also take part in selector equality and hashing.
  • mordant-baseline.toml is 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 deleting (or decrementing) its entry.

build() only used its parsed_pseudo, parsed_slotted and parsed_part
arguments to OR the matching SelectorFlags bits together, so take the
flags directly. parse_selector builds them from the parsing state and
Selector::from_component_in passes SelectorFlags::empty() in place of
build(false, false, false). No behavior change.

Drops the cleared bare_bool_args entry for src/css/selectors/builder.rs
from mordant-baseline.toml.
@robobun

robobun commented Aug 15, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:12 PM PT - Aug 15th, 2026

🔄 @robobun, the build for your commit 302ae8e6 (Build #98337) was cancelled — waiting for the next build...

@coderabbitai

coderabbitai Bot commented Aug 15, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 57 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8463ef2e-ba8e-478e-9db2-8af3d7ab119b

📥 Commits

Reviewing files that changed from the base of the PR and between 302ae8e and 455d22a.

📒 Files selected for processing (1)
  • mordant-baseline.toml

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fb8c7894-0d68-46bf-bc01-bb51ef96ba94

📥 Commits

Reviewing files that changed from the base of the PR and between 88a6398 and 302ae8e.

📒 Files selected for processing (3)
  • mordant-baseline.toml
  • src/css/selectors/builder.rs
  • src/css/selectors/parser.rs
💤 Files with no reviewable changes (1)
  • mordant-baseline.toml

Walkthrough

The selector builder now accepts a unified SelectorFlags value. The parser derives and passes the flags for pseudo-elements, slotted, and part selectors. Single-component selectors pass an empty flag set.

Changes

Selector flags propagation

Layer / File(s) Summary
Builder flags contract
src/css/selectors/builder.rs
SelectorBuilder::build now accepts SelectorFlags directly and forwards it with specificity data.
Parser integration
src/css/selectors/parser.rs, mordant-baseline.toml
parse_selector builds the unified flag set from parser state. Single-component selectors use SelectorFlags::empty(). The obsolete boolean-argument baseline entry is removed.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main API change from three boolean parameters to SelectorFlags.
Description check ✅ Passed The description explains the problem, fix, behavior impact, and verification results, including the reported stress-test timeouts.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — mechanical refactor replacing three positional bools with the SelectorFlags bitset they were already being ORed into.

What was reviewed:

  • Confirmed both builder.build(...) call sites in src/css/selectors/parser.rs are updated and there are no others.
  • Verified the flag construction in parse_selector is bit-for-bit identical to the removed logic in build(); from_component_in's (false, false, false) → SelectorFlags::empty() is equivalent.
  • The removed mordant-baseline.toml entry had count 1 and this was the only multi-bool function in builder.rs.
Extended reasoning...

Overview

This PR changes SelectorBuilder::build in src/css/selectors/builder.rs from taking three bool parameters (parsed_pseudo, parsed_slotted, parsed_part) to taking a single SelectorFlags bitset. The three-bool-to-bitflags OR chain that used to live inside build() is moved verbatim into parse_selector (the caller that derives those flags from parsing state), and the other caller from_component_in now passes SelectorFlags::empty() instead of (false, false, false). The corresponding bare_bool_args entry is dropped from mordant-baseline.toml.

Security risks

None. This is a signature refactor in the CSS selector parser with no change to input handling, allocation, or control flow.

Level of scrutiny

Low. This is a pure, mechanical, behavior-preserving refactor motivated by a lint. The transformation is trivially verifiable by inspection: each of the three former bools maps 1:1 to the same SelectorFlags bit under the same condition, and SelectorFlags::empty() is exactly what three false values produced. SelectorFlags is defined in parser.rs itself so no import changes are needed. Grep confirms the two updated sites are the only callers of builder.build(...).

Other factors

The PR description documents that the CSS test suites (test/js/bun/css/, test/bundler/css/, test/bundler/esbuild/css.test.ts) pass, including DeduplicateRules which exercises selector equality/hashing over these flags. No new test is expected for a no-behavior-change refactor. The baseline file was hand-edited to remove exactly one line, which matches the single finding cleared.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Replace boolean flag parameters with two-variant enums #37180 - Rewrites the same SelectorBuilder::build(parsed_pseudo, parsed_slotted, parsed_part) signature and the same two call sites in src/css/selectors/parser.rs to remove the bare bools, using three bool_enum! types instead of a single SelectorFlags parameter.

🤖 Generated with Claude Code

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Related rather than a duplicate. #37180 is a workspace-wide pass (539 files) that retypes this same build() signature with three bool_enum! types (ParsedPseudo, ParsedSlotted, ParsedPart) and keeps the translation into SelectorFlags inside build(). This PR covers only this site and removes that translation instead, since SelectorFlags is already the named form of those three inputs. If #37180 lands first this PR can be closed; if this lands first, #37180 picks up a small conflict in builder.rs and parser.rs.

@robobun

robobun commented Aug 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI status: 177 of the 179 jobs in build 98337 passed, including the other half of the darwin 14 aarch64 test lane. The one job that never ran was the second parallel job of :darwin: 14 aarch64 - test-bun, whose attempts expired before any agent picked them up: the test-darwin queue serving that lane was several hours behind today and builds from earlier in the afternoon show the same expired job. The build has since been canceled along with other builds that were waiting on that lane; the passed jobs are still listed on it. Nothing in this change affects scheduling, so I am not re-running the build; the diff itself is green.

@alii
alii merged commit dc7800c into main Aug 15, 2026
9 of 10 checks passed
@alii
alii deleted the farm/03c445e7/css-selector-build-flags branch August 15, 2026 19:32

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — mechanical refactor moving the bool→SelectorFlags OR chain from build() into its caller.

What was reviewed:

  • Confirmed the only two .build() call sites in src/css/selectors/ are both updated; the parse_selector flag construction is bit-for-bit the old body of build(), and from_component_in's SelectorFlags::empty() is exactly what build(false, false, false) produced.
  • SelectorFlags is defined in parser.rs itself, so no import changes needed.
  • mordant-baseline.toml entry removal matches: builder.rs no longer has any multi-bool-param function.
Extended reasoning...

Overview

Three files touched. src/css/selectors/builder.rs drops the three bool parameters from SelectorBuilder::build and takes a SelectorFlags directly (net -15 lines). src/css/selectors/parser.rs updates the two callers: parse_selector now builds the SelectorFlags from parsing state using the exact same three if conditions that were previously inside build(), and from_component_in passes SelectorFlags::empty() in place of (false, false, false). mordant-baseline.toml removes the now-cleared bare_bool_args:src/css/selectors/builder.rs entry.

Security risks

None. This is a pure signature refactor in the CSS selector parser with no change to input validation, allocation, or control flow.

Level of scrutiny

Low. The transformation is mechanical: the three if parsed_X { flags |= HAS_X } statements move verbatim from callee to the one caller that computed them, and the other caller's (false, false, false) becomes empty(). I grepped src/css/selectors/ for .build( and confirmed exactly two call sites, both in the diff. SelectorFlags is defined in parser.rs (bitflags struct at line ~2752) and already re-exported into builder.rs, so no scoping concerns.

Other factors

The PR description documents bun bd test runs across test/js/bun/css/, test/bundler/css/, and test/bundler/esbuild/css.test.ts (including DeduplicateRules, which exercises selector equality/hashing on these flags). CI build 98337 had 177/179 jobs pass with the remaining job an infrastructure expiry on the darwin queue. No prior review comments to address; the duplicate-PR bot flagged #37180, and the author's response (this is the narrower, more direct fix using the existing SelectorFlags type rather than three new bool_enum! types) is reasonable — either can land independently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants