css modules: drop a rejected composes instead of failing the file at print time

[robobun]

Problem

• A composes declaration the CSS modules parser rejects (inside a nested rule, inside an at-rule nested in a rule, or on a selector that is not a single class) gets its warning, and then bun build still exits 1 with error: Failed to generate CSS for this file (PrintError).
• Cause: parse_declaration_impl (src/css/declaration.rs) warns but still pushes the declaration, and StyleRule::to_css_base (src/css/rules/style.rs) validated composes a second time at print time (Printer::is_nested, CssModule::handle_composes) and failed the whole stylesheet.
• The print-time check was also stricter than the parser. is_nested() is only "indent > 2", so an accepted composes failed with no warning at all when its rule is printed inside any block:
  • @media (min-width: 1px) { .b { composes: c } } (accepted by the parser, exported by the bundler, then PrintError).
  • a valid module reached through @import "./x.module.css" layer(foo); (or a media / supports condition): the bundler wraps the file's rules in the matching @layer / @media / @supports block itself (prepareCssAstsForChunk.rs), so the file can never print.
• A rejected composes: x from "./other.module.css" kept its import record, so other.module.css was bundled (or failed resolution) on behalf of a declaration that has no effect. The same happened, without any warning, for a composes written outside a style rule of a module (@keyframes, @page, ...), which was also printed as if it were a real property.
• esbuild builds all of these: rejected declarations get a warning and are dropped, composes in a rule inside a top-level at-rule works.

Fix

• parse_declaration_impl drops every rejected composes after warning (the previously silent "outside a style rule" case now warns "composes" is not valid here, esbuild's wording), and Composes::discard marks the import record of its from "<file>" specifier IS_UNUSED (the existing flag for "the parser created this record and then decided it is not needed"; the bundler neither resolves nor orders such records).
• StyleRule::to_css_base only omits composes from the output. handle_composes, Printer::is_nested, the two printer error kinds and Composes::cssparser_loc had no other users and are removed.
• Why this is correct: the parser's verdict is the only one the bundler uses (StyleSheet::composes feeds generateCodeForLazyExport / scanImportsAndExports; the printer never emitted the property), so the print-time validation only ever turned an already reported warning into a build failure, or, in the wrapped cases, rejected rules the parser had correctly accepted. With rejected declarations dropped at parse time, the only composes left in the AST are accepted ones in style rules, so there is nothing left for the printer to check. This matches esbuild for .module.css files, including not bundling the from file of a rejected declaration. (composes in plain, non-module .css files is still parsed as a property and bundles its from file; that is a separate pre-existing divergence, tracked separately, and untouched here.)
• test/bundler/expectBundled.ts: the warning parser assumed every warning location is an absolute path inside the test directory and threw on CSS parser warnings, which only carry the file's basename (at styles.module.css:1:6). Those are now keyed as /<basename>, so bundleWarnings can assert the exact list of CSS warnings. Warnings that did parse before parse exactly as before.
• Verified with the seven new css-module/Composes* cases in test/bundler/css/css-modules.test.ts: the three rejected shapes (each with one declaration that shares a rule with a real property and one that is alone, so the rule holding only the rejected composes has to disappear from the output), @keyframes / @page, a rejected from that must neither bundle an existing file nor fail on a missing one, @media / @supports + @layer acceptance, and a module imported with conditions. All seven fail on the unfixed build (the builds exit 1, or in the @keyframes / @page case bundle other.module.css without a warning) and pass with the fix.
• test/bundler/cli.test.ts "fails instead of emitting a truncated stylesheet" used a rejected composes as its PrintError trigger, which no longer exists; it now uses the nesting expansion limit (passes before and after, it is not the proof for this change).
• Also ran test/bundler/esbuild/{css,default,importstar,packagejson,splitting}.test.ts, test/bundler/bundler_jsx.test.ts and test/bundler/bundler_edgecase.test.ts (every suite that asserts warnings or errors through the harness; all green), test/js/bun/css/{css,doesnt_crash,css-loader,nested-selector-expansion}.test.ts, cargo clippy -p bun_css, cargo fmt.
• Overlaps textually with css modules: keep composes working after a nested rule in the same block #38520 (different bug: per-block composes state) in declaration.rs and the test file; whichever lands second rebases trivially.

Background

• CSS modules composes: .a { composes: b } makes the JS export for a the string "b_<hash> a_<hash>". Bun implements it in the parser: when a style rule is opened, NestedRuleParser::parse_block (css_parser.rs) computes a ComposesState from the selector shape and nesting depth (declaration blocks that are not style rules get DisallowEntirely); parse_declaration_impl then either records the declaration into StyleSheet::composes (Allow) or warns. The bundler builds the exports object from that map. The property itself is never printed.
• lightningcss, which this printer is ported from, has no such parser-side map; it resolves composes while printing, which is why its printer validated the selector and nesting. In Bun those checks duplicated the parser's, and the failure surfaced as a generic PrintError because the bundler discards printer error details (generateCompileResultForCssChunk.rs).
• ImportRecordFlags::IS_UNUSED: a record the parser created and later decided is unnecessary (TypeScript type-only imports use it). resolve_import_records skips such records, so they keep an invalid source_index, and findImportedFilesInCSSOrder only follows composes records with a valid one.
• The @keyframes fixture only produces the @page warning: keyframe bodies are parsed with a fresh ParserOptions that has no logger (rules/keyframes.rs), so warnings raised there are dropped today. The declaration is still removed, which the test's stylesheet snapshot shows; making those warnings visible is tracked separately.

Repro and esbuild comparison

entry.js does import styles from "./w.module.css"; bun build entry.js --outdir out (bun 1.4.0 and main before this change):

.c { color: red }
.a { .z { composes: c } }                       -> warn + "Failed to generate CSS for this file (PrintError)", exit 1
.a .b { composes: c }                           -> warn + PrintError, exit 1
.a { @media (min-width: 1px) { composes: c } }  -> warn + PrintError, exit 1
@media (min-width: 1px) { .b { composes: c } }  -> PrintError only, exit 1
.a .b { composes: x from "./missing.css" }      -> error: Could not resolve "./missing.css"
@keyframes k { from { composes: x from "./other.module.css" } }
                                                -> no warning, other.module.css bundled, composes printed

entry.css with @import "./w.module.css" layer(foo); where w.module.css is just .c { color: red } .b { composes: c } -> PrintError, exit 1.

esbuild 0.25 on the same inputs: the rejected shapes warn and export {c, a, z} / {c, a, b} / {c, a} with no composition, @media exports b: "w_c w_b", files named by rejected declarations are never resolved, and every build succeeds. With this change Bun produces the same exports and exit codes for all of them.

Earlier revision of this PR: the first push left the "outside a style rule" case untouched and tested the rest by spawning bun build directly, because the harness could not parse CSS warnings; the self-review pointed out both, which the second commit addresses.

---

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/cli.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 seconds

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: 504d4542-f362-40a5-ab7c-0885fc3b0e4d

📥 Commits

Reviewing files that changed from the base of the PR and between eabb96d and 5a038bc.

📒 Files selected for processing (10)

• src/css/css_modules.rs
• src/css/css_parser.rs
• src/css/declaration.rs
• src/css/error.rs
• src/css/printer.rs
• src/css/properties/css_modules.rs
• src/css/rules/style.rs
• test/bundler/cli.test.ts
• test/bundler/css/css-modules.test.ts
• test/bundler/expectBundled.ts

---

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

[robobun]

Status

• Reproduced on bun 1.4.0 and main: bun build entry.js --outdir out with entry.js importing a .module.css containing .a { .z { composes: c } }, .a .b { composes: c } or .a { @media (min-width: 1px) { composes: c } } (warning, then Failed to generate CSS for this file (PrintError), exit 1), @media (min-width: 1px) { .b { composes: c } } (PrintError with no warning), and @import "./x.module.css" layer(foo); of a valid module (PrintError).
• Fix and tests are in this PR. After self-review the PR also drops a composes written outside a style rule of a module (@keyframes, @page, ...) instead of bundling its from file, and the harness change in expectBundled.ts lets the tests assert the exact CSS warnings; the seven css-module/Composes* cases fail on the unfixed build and pass with it.
• CI: the diff is green on every lane that ran it. What is red in build 96715 is unrelated to this change: the Windows aarch64 / one Windows x64 / verify-baseline jobs never got an agent, three x64-asan shards each failed on an unrelated file (test/cli/run/require-cache.test.ts, vendor/elysia/test/response/stream.test.ts, test/js/bun/util/inspect-error-leak.test.js, all reported for triage separately); everything else, the darwin shards included, passed (166 of 179 jobs). The first build (96132, before the follow-up commits) had 177 of 179 jobs green with the same two darwin shards expired. Ready for review.
• Split off into their own fixes: composes in plain (non-module) .css files, warnings inside @keyframes bodies being dropped, and the unrelated @supports parsing failure and :global(...) exports issues found along the way.

[claude]

I reviewed this PR and didn't find any bugs. Because it removes print-time validation on the argument that the parser is now the sole source of truth for composes acceptance — a behavior change that turns previously-failing builds into warnings — a human look at that reasoning would still be worthwhile.

Checked: the removed handle_composes / is_nested / cssparser_loc / invalid_composes_* have no remaining users; mark_import_record_unused's .expect() is safe because Specifier::ImportRecordIndex can only be produced when add_import_record had a records list; IS_UNUSED is honored by resolve_import_records (bundle_v2.rs) and linker.rs; the DisallowEntirely arm's unchanged fall-through only reaches non-tracking contexts (NoComposesCtx). The replacement PrintError trigger in cli.test.ts hits the real maximum_nesting_expansion bound.

Extended reasoning...

Overview

This PR fixes CSS modules composes handling: rejected declarations (nested rule, non-single-class selector, at-rule inside a style rule) previously warned and then failed the whole build with a generic PrintError because the printer re-validated them; accepted declarations inside a top-level @media/@supports/@layer (or a module reached via a conditional @import) failed with no warning at all because is_nested() was "indent > 2". The fix drops rejected declarations at parse time (after warning), marks any from "<file>" import record IS_UNUSED so the target isn't resolved/bundled, and reduces the printer to a plain "skip composes" — deleting handle_composes, Printer::is_nested, two PrinterErrorKind variants, and the cssparser_loc field. 6 new tests cover the three rejected shapes, top-level at-rule acceptance, conditional-@import wrapping, and rejected-from not pulling in files; cli.test.ts's CSS PrintError test is repointed at the nesting-expansion limit.

Security risks

None. No untrusted-input parsing changes, no auth/crypto/permissions surface. The one new unsafe block (mark_import_record_unused) dereferences Parser.import_records under the same documented Stacked-Borrows invariant as add_import_record immediately above it, and the .expect() is guarded by the invariant that Specifier::ImportRecordIndex only exists when add_import_record succeeded (which requires import_records.is_some()).

Level of scrutiny

Medium-high. This is a user-visible behavior change to bun build: builds that previously exited 1 now exit 0 with a warning, and files referenced by rejected composes: x from "..." are no longer resolved or bundled. The correctness argument — that the parser's ComposesState verdict is the only one the bundler ever consumed (via StyleSheet::composes), so the printer's re-check was strictly redundant-or-wrong — is well-argued and matches what I traced through css_parser.rs, but it removes a check rather than adding one, so a maintainer familiar with the CSS pipeline should confirm the analysis.

Other factors

• Verified no remaining references to the removed symbols (is_nested hits in css_parser.rs are an unrelated local parameter).
• IS_UNUSED is respected by resolve_import_records (bundle_v2.rs:5950) and the linker loop (linker.rs:403), matching the PR's claim.
• The DisallowEntirely match arm still falls through and pushes the property; that path is only reached from NoComposesCtx (style attribute / @page / keyframes), which don't print via StyleRule::to_css_base, so behavior there is unchanged.
• Tests are thorough, use inline snapshots with exact exports/CSS, and the PR states they were run alongside the broader CSS suites plus USE_SYSTEM_BUN verification.
• The PR notes textual overlap with #38520 in the same files.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes user-facing bundler behavior (builds that previously failed now succeed), removes the printer's composes validation layer, and adjusts the shared expectBundled.ts warning parser, a human look would still be worthwhile.

What was reviewed:

• Confirmed handle_composes, Printer::is_nested, invalid_composes_* error kinds, and Composes::cssparser_loc have no remaining references after removal.
• Checked mark_import_record_unused: the .expect() on import_records is safe because a Specifier::ImportRecordIndex can only exist if add_import_record succeeded, which requires import_records to be Some.
• Traced the errorOrWarnParser slice-offset change ("\n at ".length → " at ".length): the absolute-path branch still works because its regex searches rather than anchors, and the new bare-basename branch needs the corrected offset for its ^-anchored match.

Extended reasoning...

Overview

This PR fixes CSS modules composes handling so that a rejected declaration (nested rule, non-single-class selector, or outside a style rule) is dropped at parse time with a warning instead of failing the whole stylesheet at print time. It touches 10 files: six in src/css/ (parser, printer, declaration handling, error kinds, css_modules helpers, style rule printing) and three test files plus the expectBundled.ts harness. The printer's redundant validation layer is removed entirely, along with the now-dead handle_composes, is_nested, two PrinterErrorKind variants, and the cssparser_loc field. A new Composes::discard marks the from "..." import record IS_UNUSED so the bundler neither resolves nor bundles it.

Security risks

None identified. This is CSS-modules build-time behavior with no auth, crypto, or untrusted-input parsing changes beyond what already existed. The unsafe in mark_import_record_unused follows the same pattern and SAFETY justification as the adjacent add_import_record.

Level of scrutiny

Medium-high. While the mechanism is well-explained and well-tested (seven new itBundled cases covering nested rules, non-single-class selectors, at-rules-in-rules, @keyframes/@page, rejected from imports, top-level at-rule acceptance, and @import with conditions), this is a deliberate design change: the printer no longer validates composes at all, relying on the parser having dropped every rejected declaration. The PR description argues this is correct because Bun (unlike lightningcss) records composes at parse time and the printer never emitted the property anyway — but confirming that no other code path can inject a Property::Composes into a rule the parser didn't validate is worth a human's eye. The harness change to errorOrWarnParser also affects every bundler test that asserts warnings.

Other factors

• The comment-cop bot flagged verbose comments across five files; the author trimmed them in follow-up commits (3d3908d, ca67465) and left a justified three-line note in style.rs explaining why the lightningcss nesting check must not be re-added.
• The cli.test.ts PrintError trigger was replaced (nesting expansion limit instead of rejected composes) since the old trigger no longer errors — the test still validates the same contract.
• The PR notes it overlaps textually with #38520 in declaration.rs and the test file.
• Test coverage is thorough and the PR description states each case fails on the unfixed build, plus the broader bundler warning-asserting suites were run.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes user-facing bundler behavior (builds that previously failed now succeed), removes the printer-side composes validation entirely, and touches the shared expectBundled.ts warning parser, a human look would still be worthwhile.

What was reviewed:

• mark_import_record_unused's .expect() is safe — add_import_record errors when import_records is None, so an ImportRecordIndex implies the pointer is set; resolve_import_records (bundle_v2.rs:6002) skips IS_UNUSED records so the dropped from file is neither resolved nor bundled.
• The errorOrWarnParser slice-length change ("\n at " → " at ") leaves 4 extra leading chars in source for absolute-path warnings, but the downstream /bun-build-tests[\/\\].../ regex is unanchored so existing warning assertions parse the same; the PR ran every bundleWarnings-using suite green.
• NoComposesCtx returns DisallowEntirely, so @page/@keyframes bodies (which use plain parse_declaration) now hit the new warn-and-drop arm; the removed cssparser_loc field and is_nested()/handle_composes had no other callers.

Extended reasoning...

Overview

This PR fixes CSS modules composes handling: rejected composes declarations (nested, non-single-class selector, or outside a style rule) are now dropped at parse time with a warning instead of being pushed into the AST and then failing the whole stylesheet at print time with a generic PrintError. It touches 6 Rust files in src/css/ (parser, printer, declaration handling, error kinds, css_modules struct) plus 3 test files. The change removes ~50 lines of print-time validation (handle_composes, Printer::is_nested, two PrinterErrorKind variants, the cssparser_loc field) and adds ~20 lines of parse-time drop logic plus a new mark_import_record_unused helper. Seven new itBundled cases with inline snapshots cover every rejected shape and the previously-broken accepted shapes (@media/@layer wrapping).

Security risks

None identified. The change operates on already-parsed CSS AST nodes; no new untrusted-input parsing paths, no filesystem or network surface. The one unsafe block added (mark_import_record_unused) follows the exact same SharedRW-provenance pattern as the adjacent add_import_record, with the same SAFETY comment referencing the field doc. The .expect() on import_records is invariant-guarded: a Specifier::ImportRecordIndex can only exist when add_import_record succeeded, which requires import_records.is_some().

Level of scrutiny

Medium-high. This is a user-facing behavior change to bun build for CSS modules — inputs that previously exited 1 now exit 0 with a warning, matching esbuild. The PR removes a validation layer entirely (print-time composes checks), which is a design decision the author justified thoroughly (the parser already decides everything the bundler exports; the printer never emitted the property; the print-time check was both redundant with the parser and stricter in ways that broke valid inputs). The reasoning is sound and matches the esbuild reference behavior, but removing validation and changing exit codes is the kind of thing a maintainer should sign off on. The expectBundled.ts change is small but affects every bundler test that asserts warnings.

Other factors

• All comment-cop bot feedback has been addressed (comments trimmed in ca67465 and 3d3908d; all threads resolved).
• Test coverage is thorough: 7 new cases covering nested rules, non-single-class selectors, at-rules-in-rules, @keyframes/@page, rejected from imports (both existing and missing files), top-level @media/@supports/@layer acceptance, and @import with conditions. Each rejected-shape test includes both a declaration sharing a rule with a real property and one alone (so the empty-rule-removal path is exercised). The author verified all seven fail on the unfixed build.
• The rewritten cli.test.ts case now uses the nesting-expansion limit as its PrintError trigger (the maximum_nesting_expansion variant), which is orthogonal to this fix and passes before and after.
• The PR notes textual overlap with #38520; whichever lands second rebases.
• I verified IS_UNUSED is honored by resolve_import_records and that the errorOrWarnParser offset change is harmless for existing absolute-path warnings (unanchored regex).