Skip to content

css: report parser warnings with 1-based line numbers - #37911

Closed
robobun wants to merge 1 commit into
mainfrom
farm/84c0d8ef/css-warning-line-numbers
Closed

robobun wants to merge 1 commit into
mainfrom
farm/84c0d8ef/css-warning-line-numbers

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Every warning the CSS parser reports (invalid @ rule encountered, unsupported pseudo-class/element, deprecated @nest, the two css-modules composes warnings and the note attached to one of them) points one line too early. A warning on the first line of a file is reported as line 0, which the logger prints as at x.module.css:0 (no column), which Bun.build() exposes as position.line === 0, and which the dev server's error overlay payload cannot represent (see bake: serialize bundler messages with line 0 as having no location #37885 for the writer side of that).
  • Errors in the same file are correct. src/css/error.rs ErrorLocation::to_location adds 1 when it builds the logger location, because the parser's SourceLocation / Location lines are 0-based (src/css/lib.rs, "line is 0-based") while bun_ast::Location.line is 1-based with line <= 0 meaning "no position" (src/ast/lib.rs).
  • The warning paths skipped that conversion: ParserOptions::warn / warn_fmt / warn_fmt_with_notes (src/css/css_parser.rs) passed the raw line into Log::add_warning_fmt_line_col[_with_notes] (src/ast/lib.rs), which stored it verbatim, and SourceLocation::to_logger_location (used for the composes note in src/css/declaration.rs) did the same.
$ printf '@unknownrule foo;\n.a { color: red; }\n\n@other bar;\n' > x.module.css
$ bun build ./x.module.css
warn: invalid @ rule encountered: '@unknownrule'
   at x.module.css:0          <- line 1
warn: invalid @ rule encountered: '@other'
   at x.module.css:3:7        <- line 4

Fix

  • ParserOptions::logger_location(SourceLocation) is now the single place a parser location becomes a bun_ast::Location for a warning; it adds 1 like ErrorLocation::to_location does. warn and warn_fmt funnel into warn_fmt_with_notes, which uses it, and the composes note uses it too. warn_fmt / warn_fmt_with_notes take a SourceLocation instead of a bare line, column pair so a caller cannot hand the logger an unconverted line again.
  • Log::add_warning_fmt_line_col / add_warning_fmt_line_col_with_notes (only the CSS parser used them) are replaced by add_warning_fmt_with_location(Location, args, notes): the logger stores a Location whose field documentation defines the contract instead of a line number of unspecified base.
  • This is correct because it makes warnings follow the same 0-based to 1-based conversion the crate already applies at its other boundaries (ErrorLocation::to_location, dependencies::Location::from_source_location), and it matches lightningcss, which this parser is a port of: lightningcss@1.30.2 transform() reports the two warnings from the repro above at { line: 1, column: 13 } and { line: 4, column: 7 }, exactly what bun prints with this change (its Warning.loc type documents line as 1-based). Columns were already 1-based and are unchanged.
  • Side effects: the 0-based BuildMessage.line getter (which subtracts 1) becomes correct as well, and a first-line CSS warning no longer produces a line 0 location for the dev server overlay serializer. The terminal output of a first-line warning changes from at x.module.css:0 to at x.module.css:1:13.
  • Not changed here (pre-existing, reported separately): warnings in non-module .css files still carry no file name because the bundler only sets ParserOptions.filename for css modules, and warnings still have no line_text, so they print without a code frame.
  • Verified with three tests added to test/bundler/css/css-modules.test.ts: one Bun.build() test covering all three emission paths plus the note in one .module.css file (every expected line is exactly one higher than what 1.4.0 reports), one for a non-module stylesheet, and one checking the bun build stderr format. All three fail with USE_SYSTEM_BUN=1 and pass with bun bd test.
  • Also run on the debug build: test/bundler/esbuild/css.test.ts, the other test/bundler/css/*.test.ts files, test/js/bun/css/css.test.ts, test/bake/dev/css.test.ts; cargo clippy -p bun_css -p bun_ast is clean.

Background

  • bun_ast::Log is the diagnostics sink shared by the bundler, the CLI and Bun.build(); a Msg carries a Data { text, location } plus notes (more Data). A Location is file / 1-based line / column / optional line_text; the printer emits file:line:column only when line > 0, BuildMessage.position exposes it as is, and the dev server overlay serializes it per message.
  • The CSS parser (src/css, a port of lightningcss on top of a cssparser-style tokenizer) tracks positions as SourceLocation { line (0-based), column (1-based) }. Fatal problems come back as Err<ParserError> and the bundler converts them with ErrorLocation::to_location(source); non-fatal ones are written into the Log during parsing through ParserOptions.logger, which is the path this PR fixes.
Fail-before output of the new tests on bun 1.4.0
(fail) css parser warning locations > warnings and notes report 1-based lines, like errors
-     "line": 1,      +     "line": 0,     invalid @ rule encountered: '@unknownrule'
-     "line": 4,      +     "line": 3,     "composes" is not allowed inside nested selectors
-     "line": 10,     +     "line": 9,     "composes" only works inside single class selectors
-         "line": 8,  +         "line": 7, (note) The parent selector is not a single class selector ...
-     "line": 12,     +     "line": 11,    Unsupported pseudo-class or pseudo-element 'custom-thing'
-     "line": 13,     +     "line": 12,    invalid @ rule encountered: '@other'
(fail) css parser warning locations > warnings in a stylesheet that is not a CSS module report 1-based lines
(fail) css parser warning locations > bun build prints warning locations as file:line:column
-    at styles.module.css:1:13
+    at styles.module.css:0
-    at styles.module.css:3:7
+    at styles.module.css:2:7

The CSS parser counts lines from 0 (cssparser convention) while
bun_ast::Location counts from 1 and treats line 0 as "no location".
Parse errors were converted in ErrorLocation::to_location, but the
warning paths (ParserOptions::warn, warn_fmt, warn_fmt_with_notes and
the composes note) passed the 0-based line straight through, so every
CSS warning pointed one line too early and a first-line warning printed
as "file.css:0".

Route all of them through ParserOptions::logger_location, which builds
the logger Location (line + 1), and replace the logger's line/column
passthrough helpers with one that takes a Location.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next review available in: 1 minute

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: 517f162f-98f9-4618-bd85-1bdf3c700471

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and 03a330c.

📒 Files selected for processing (4)
  • src/ast/lib.rs
  • src/css/css_parser.rs
  • src/css/declaration.rs
  • test/bundler/css/css-modules.test.ts

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

@robobun

robobun commented Aug 12, 2026 •

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

❌ @robobun, your commit 03a330c has 3 failures in Build #93483 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37911

That installs a local version of the PR into your bun-37911 executable, so you can run:

bun-37911 --bun

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on bun 1.4.0 with bun build x.module.css (at x.module.css:0 for a first-line warning, :3:7 for a warning on line 4) and with Bun.build() (position.line one less than the real line for all five warning kinds and the note). Fix and tests are in this PR; self-review in progress.

@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 — focused off-by-one fix aligning CSS parser warnings with the existing error-path conversion.

What was reviewed:

  • Verified logger_location's line + 1 matches ErrorLocation::to_location (src/css/error.rs:190) and dependencies::Location::from_source_location (src/css/dependencies.rs:31), and that SourceLocation.line is documented 0-based (src/css/lib.rs:299).
  • Confirmed no remaining callers of the removed add_warning_fmt_line_col* / to_logger_location; all warn_fmt* call sites (declaration.rs) updated to the new SourceLocation signature.
  • Checked that warn → warn_fmt → warn_fmt_with_notes funneling preserves the moved SAFETY comment on the single remaining unsafe deref.
  • Tests cover every warning emission path (invalid @rule, both composes variants + note, unsupported pseudo) via both Bun.build() and the CLI, for module and non-module CSS.
Extended reasoning...

Overview

This PR fixes an off-by-one in CSS parser warning line numbers. The parser's internal SourceLocation uses 0-based lines (documented in src/css/lib.rs:299), while bun_ast::Location uses 1-based lines. The error path (ErrorLocation::to_location) already added 1; the warning path did not. The fix introduces ParserOptions::logger_location as the single conversion point, funnels warn/warn_fmt through warn_fmt_with_notes, and replaces the two ad-hoc Log::add_warning_fmt_line_col* helpers with one add_warning_fmt_with_location that takes a pre-built Location. Four files touched: src/ast/lib.rs, src/css/css_parser.rs, src/css/declaration.rs, and new tests in test/bundler/css/css-modules.test.ts.

Security risks

None. This is diagnostic metadata (line numbers in warning messages) with no effect on parsing, output, or control flow.

Level of scrutiny

Low-to-medium. The change is mechanical and mirrors two existing in-crate conversions (ErrorLocation::to_location, dependencies::Location::from_source_location) that already do line + 1. I grepped for stale callers of every removed/re-signatured function (add_warning_fmt_line_col, add_warning_fmt_line_col_with_notes, to_logger_location, warn_fmt, warn_fmt_with_notes) — all are either gone or updated. ParseError.location is SourceLocation, so warn()'s new delegation type-checks. The SAFETY comment on the unsafe { &mut *lg.as_ptr() } was preserved (moved from the deleted warn body to the remaining warn_fmt_with_notes). Columns are unchanged (already 1-based per the SourceLocation doc).

Other factors

The tests are thorough: one Bun.build() test exercises every warning-emission code path in a single fixture (invalid @rule via warn, nested-composes via warn_fmt, non-single-class-composes via warn_fmt_with_notes including the note's location, unsupported pseudo via warn), a second covers non-module .css, and a third asserts the CLI file:line:column format. The PR description documents fail-before output on 1.4.0 and lists the additional test files run on the debug build. The refactor also improves the API shape (callers can no longer hand the logger an unconverted 0-based line as a bare u32), which follows the review guideline of moving the guard into the shared helper.

@robobun

robobun commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: #45787 supersedes this. It gives a CSS parser warning its file, its 1-based line, its column and its line text from the sheet's Source, so the line number change here is part of it.

@robobun robobun closed this Oct 17, 2026
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.

1 participant