From 1f9dfcb0ea2afa84d67242a13c320d572c0afa22 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 15 Oct 2026 21:25:01 +0000 Subject: [PATCH 1/3] logger: locate a diagnostic at the start of its CR LF pair or UTF-8 sequence An error at the end of a file with CR LF line ends was reported one line past the last line, at column 0, with an empty line text and no code frame. The scan stopped on the LF of the final pair, after it had read the CR. - error_stop_offset replaces clamp_error_offset. It moves every Loc to the first byte of the unit that holds it: the CR of a CR LF pair, the first byte of a UTF-8 sequence. Location::init_or_null_impl calls it once. - The CR arm of the scanner starts the next line at column 1, as the LF, U+2028 and U+2029 arms do. A file with lone CR line ends no longer has columns that count from 0. - LineColumnTracker no longer rescans from byte 0 for an offset on a continuation byte: no scan stops inside a sequence now. - bun_core::strings::wtf8_codepoint_start is the code_point_start of the lint's regex subject, moved so that the logger can call it too. --- src/ast/lib.rs | 222 ++++++++++++++---- src/bun_core/string/immutable.rs | 5 +- src/bun_core/string/immutable/wtf8_text.rs | 8 + src/lint/regex/subject.rs | 2 +- src/lint/regex/wtf8.rs | 8 - test/bake/dev/bundle.test.ts | 20 ++ .../bun/transpiler/parse-error-column.test.ts | 158 ++++++++++++- 7 files changed, 369 insertions(+), 54 deletions(-) diff --git a/src/ast/lib.rs b/src/ast/lib.rs index f4c86093762d..ac442184999d 100644 --- a/src/ast/lib.rs +++ b/src/ast/lib.rs @@ -653,8 +653,7 @@ pub struct Location { // TODO: move to `bun.Ordinal` pub line: i32, // TODO: figure out how this is interpreted, convert to `bun.Ordinal` - // original docs: 0-based, in bytes. - // but there is a place where this is emitted in output, implying one based character offset + // 1-based, in UTF-16 code units, when `init_or_null` computes it from a `Source`. pub column: i32, } @@ -821,17 +820,16 @@ impl Location { offset: 0, }); } + let offset = error_stop_offset(&source.contents, r.loc); let data = match tracker { - Some(tracker) => tracker.error_position(source, r.loc), - None => source.init_error_position(r.loc), + Some(tracker) => tracker.error_position(source, offset), + None => source.init_error_position(offset), }; let mut full_line = &source.contents[data.line_start..data.line_end]; // Window a long line to ~120 bytes around the error. Bounds are // BYTE offsets; the gate keeps the original shape (no left trim for // an error in the last 80 bytes) so `write_format`'s caret aligns. - let offset_in_line = clamp_error_offset(&source.contents, r.loc) - .saturating_sub(data.line_start) - .min(full_line.len()); + let offset_in_line = offset.saturating_sub(data.line_start).min(full_line.len()); if full_line.len() > 80 + offset_in_line { let mut lo = offset_in_line.saturating_sub(40); let mut hi = (offset_in_line + 80).min(full_line.len()); @@ -846,6 +844,7 @@ impl Location { full_line = &full_line[lo..hi]; } + debug_assert!(data.column_count >= 1); return Some(Location { file: Cow::Borrowed(source.path.text), namespace: Cow::Borrowed(source.path.namespace), @@ -2509,10 +2508,7 @@ impl Default for ErrorPositionState { } impl ErrorPositionState { - /// Consume the codepoints of `contents[from..to]`, updating line/column - /// exactly the way `Source::init_error_position` always has (`\r` resets - /// the column to 0, `\r\n` counts as a single line break, U+2028/U+2029 - /// are line breaks). Returns `true` if a line break was crossed. + /// Consume the codepoints of `contents[from..to]`. Returns `true` if a line break was crossed. fn advance(&mut self, contents: &[u8], from: usize, to: usize) -> bool { use bun_core::strings::{CodepointIterator, Cursor}; let iter_ = CodepointIterator::init(&contents[from..to]); @@ -2532,7 +2528,7 @@ impl ErrorPositionState { } 0x0D => { // '\r' - self.column_number = 0; + self.column_number = 1; self.line_start = from + iter.width as usize + iter.i as usize; self.line_count += 1; crossed_line_break = true; @@ -2588,11 +2584,20 @@ fn scan_line_end(contents: &[u8], offset: usize) -> usize { contents.len() } -/// Clamp a diagnostic `Loc` into `contents` the way `init_error_position` -/// always has. +/// The `\n` of a `\r\n` pair and a UTF-8 continuation byte have no line and column: their unit's first byte has. #[inline] -fn clamp_error_offset(contents: &[u8], offset_loc: Loc) -> usize { - (usize::try_from(offset_loc.start).expect("int cast")).min(contents.len().max(1) - 1) +fn error_stop_offset(contents: &[u8], offset_loc: Loc) -> usize { + use bun_core::strings; + debug_assert!(!offset_loc.is_empty()); + let offset = + (usize::try_from(offset_loc.start).expect("int cast")).min(contents.len().max(1) - 1); + match contents.get(offset) { + Some(b'\n') if strings::js_line_break_len_back(&contents[..=offset]) == 2 => offset - 1, + Some(&byte) if !strings::is_utf8_char_boundary(byte) => { + strings::wtf8_codepoint_start(contents, offset) + } + _ => offset, + } } /// Incremental line/column scanner for diagnostics (the esbuild @@ -2643,21 +2648,17 @@ impl LineColumnTracker { /// [`Source::init_error_position`], resuming from the previous call's /// offset when possible instead of rescanning from the start. - pub(crate) fn error_position(&mut self, source: &Source, offset_loc: Loc) -> ErrorPosition { - debug_assert!(!offset_loc.is_empty()); + pub(crate) fn error_position(&mut self, source: &Source, offset: usize) -> ErrorPosition { let contents: &[u8] = &source.contents; - let offset = clamp_error_offset(contents, offset_loc); + // A cursor left inside a unit would decode the rest of it as a unit of its own. + debug_assert_eq!(offset, error_stop_offset(contents, usize2loc(offset))); if !self.matches(source) { self.reset_for(source); } - if offset < contents.len() && contents[offset] & 0xC0 == 0x80 { - return source.init_error_position(offset_loc); - } - let Some(index) = self.cursors.iter().position(|c| c.offset <= offset) else { - return source.init_error_position(offset_loc); + return source.init_error_position(offset); }; let cursor = &mut self.cursors[index]; @@ -2824,10 +2825,9 @@ impl Source { Range { loc, len: 0 } } - pub(crate) fn init_error_position(&self, offset_loc: Loc) -> ErrorPosition { - debug_assert!(!offset_loc.is_empty()); + /// The line and column of `offset`, a stop from [`error_stop_offset`]. + pub(crate) fn init_error_position(&self, offset: usize) -> ErrorPosition { let contents: &[u8] = &self.contents; - let offset = clamp_error_offset(contents, offset_loc); let mut state = ErrorPositionState::default(); state.advance(contents, 0, offset); @@ -3436,6 +3436,19 @@ mod range_of_identifier_tests { mod line_column_tracker_tests { use super::*; + /// The position of the `Loc` at `at`, from a scan from byte 0 and from `tracker`. + fn locate( + source: &Source, + tracker: &mut LineColumnTracker, + at: usize, + ) -> (ErrorPosition, ErrorPosition) { + let offset = error_stop_offset(source.contents(), usize2loc(at)); + ( + source.init_error_position(offset), + tracker.error_position(source, offset), + ) + } + fn check_corpus_entry(contents: &[u8]) { let source = Source::init_path_string(b"tracker-test.js" as &[u8], contents); let len = contents.len(); @@ -3443,10 +3456,12 @@ mod line_column_tracker_tests { // Non-decreasing offsets (the parser/lexer pattern): every result must // match a from-scratch scan. let mut tracker = LineColumnTracker::default(); - for offset in 0..len.max(1) { - let loc = usize2loc(offset); - let expected = source.init_error_position(loc); - let got = tracker.error_position(&source, loc); + for offset in 0..=len + 1 { + let (expected, got) = locate(&source, &mut tracker, offset); + assert!( + got.column_count >= 1, + "column 0 at offset {offset} of {contents:?}" + ); assert_eq!( ( expected.line_start, @@ -3470,9 +3485,7 @@ mod line_column_tracker_tests { offsets.extend((0..len.max(1)).rev().step_by(7)); offsets.extend([0, len.max(1) - 1, 0, len / 2, len / 2]); for offset in offsets { - let loc = usize2loc(offset); - let expected = source.init_error_position(loc); - let got = tracker.error_position(&source, loc); + let (expected, got) = locate(&source, &mut tracker, offset); assert_eq!( ( expected.line_start, @@ -3505,6 +3518,15 @@ mod line_column_tracker_tests { "unicode \u{2028} separator \u{2029} and emoji \u{1F600}\u{1F600} plus \u{00E9}\u{4E2D}\u{6587}\nsecond line \u{00E9}\n".as_bytes(), b"mixed\ninvalid \xff\xfe bytes \x80 here\r\nlast line", b"[{\"\":[{\"\":[{\"\":[{\"\":[{\"\":[{\"\":[{\"\":[{\"\":[{\"\":[{\"\":", + b"crlf\r\nend\r\n", + b"cr crlf\r\r\n", + "ls end\u{2028}".as_bytes(), + "2 bytes \u{00E9}".as_bytes(), + "3 bytes \u{4E2D}".as_bytes(), + "4 bytes \u{1F600}".as_bytes(), + b"cut short \xE4\xB8A", + b"\xF0\x9F\xE4\xB8\xAD", + b"lone lead \xC3", ]; for contents in corpus { check_corpus_entry(contents); @@ -3537,9 +3559,7 @@ mod line_column_tracker_tests { let mut tracker = LineColumnTracker::default(); for offset in offsets { - let loc = usize2loc(offset); - let expected = source.init_error_position(loc); - let got = tracker.error_position(&source, loc); + let (expected, got) = locate(&source, &mut tracker, offset); assert_eq!( ( expected.line_start, @@ -3575,9 +3595,7 @@ mod line_column_tracker_tests { (&b, 45), (&a, 2), ] { - let loc = usize2loc(offset); - let expected = source.init_error_position(loc); - let got = tracker.error_position(source, loc); + let (expected, got) = locate(source, &mut tracker, offset); assert_eq!( ( expected.line_start, @@ -3604,14 +3622,134 @@ mod line_column_tracker_tests { // and this returned 17. let src = "const a = \"\u{1F600}\u{1F600}\"; ]".as_bytes(); let source = Source::init_path_string(b"t.js" as &[u8], src); - let pos = source.init_error_position(usize2loc(src.len() - 1)); + let pos = source.init_error_position(src.len() - 1); assert_eq!(pos.column_count, 19); // BMP-only control: `é` is one UTF-16 unit, so astral vs BMP lines // with the same layout must agree. let bmp = "const a = \"\u{00E9}\u{00E9}\u{00E9}\u{00E9}\"; ]".as_bytes(); let bmp_source = Source::init_path_string(b"t.js" as &[u8], bmp); - let bmp_pos = bmp_source.init_error_position(usize2loc(bmp.len() - 1)); + let bmp_pos = bmp_source.init_error_position(bmp.len() - 1); assert_eq!(bmp_pos.column_count, 19); } + + /// The line, column and line text that a diagnostic at `at` reports. + fn located(contents: &[u8], at: usize) -> (i32, i32, bstr::BString) { + let source = Source::init_path_string(b"t.js" as &[u8], contents); + let range = Range { + loc: usize2loc(at), + len: 0, + }; + let location = Location::init_or_null(Some(&source), range).unwrap(); + ( + location.line, + location.column, + location.line_text.unwrap().into_owned().into(), + ) + } + + fn with_line_break(lf: &[u8], line_break: &[u8]) -> Vec { + let mut spelled = Vec::new(); + for &byte in lf { + if byte == b'\n' { + spelled.extend_from_slice(line_break); + } else { + spelled.push(byte); + } + } + spelled + } + + #[test] + fn crlf_and_lone_cr_are_located_like_lf() { + let sources = [ + "a;\nf(", + "a;\nf(\n", + "a;\nf(\n\n", + "\n\nf(a,\n", + "s = '\u{00E9}\u{4E2D}\u{1F600}';\n];\n", + ]; + for lf in sources.map(str::as_bytes) { + let crlf = with_line_break(lf, b"\r\n"); + let cr = with_line_break(lf, b"\r"); + let source = bstr::BStr::new(lf); + let mut line_breaks_before = 0; + // Through `lf.len()`, the `Loc` of an error at the end of the file. + for at in 0..=lf.len() { + let expected = located(lf, at); + let in_crlf = at + line_breaks_before; + assert_eq!( + located(&cr, at), + expected, + "lone CR, Loc {at} of {source:?}" + ); + assert_eq!( + located(&crlf, in_crlf), + expected, + "CR LF, Loc {at} of {source:?}" + ); + if lf.get(at) == Some(&b'\n') { + assert_eq!( + located(&crlf, in_crlf + 1), + expected, + "the LF of a CR LF pair, Loc {at} of {source:?}" + ); + line_breaks_before += 1; + } + } + } + } + + #[test] + fn a_loc_inside_a_unit_is_located_at_its_first_byte() { + // As after a final `\n`, the end of the file is the end of the last line. + let separator = "const port = 3000;\nfunction start() {\u{2028}".as_bytes(); + assert_eq!( + located(separator, separator.len()), + (2, 19, "function start() {".into()) + ); + + // With no final line break it is the last character. + let cjk = "foo(\u{4E2D}".as_bytes(); + assert_eq!(located(cjk, cjk.len()), (1, 5, "foo(\u{4E2D}".into())); + + let text = "f(\u{00E9}\u{4E2D}\u{1F600})".as_bytes(); + for (first, len) in [(2, 2), (4, 3), (7, 4)] { + for inside in first + 1..first + len { + assert_eq!(located(text, inside), located(text, first), "Loc {inside}"); + } + } + + // No sequence holds these continuation bytes, so each has a column of its own. + let stray = b"a\x80\x80b"; + assert_eq!([1, 2, 3].map(|at| located(stray, at).1), [2, 3, 4]); + } + + /// The bytes `tracker` scans for every `Loc` of `contents` in order, and its scans from byte 0. + fn scan_cost(contents: &[u8]) -> (usize, usize) { + let source = Source::init_path_string(b"tracker-test.js" as &[u8], contents); + let mut tracker = LineColumnTracker::default(); + let (mut scanned, mut rescans) = (0, 0); + for at in 0..=contents.len() { + let offset = error_stop_offset(contents, usize2loc(at)); + let before: usize = tracker.cursors.iter().map(|cursor| cursor.offset).sum(); + tracker.error_position(&source, offset); + if tracker.cursors.iter().any(|cursor| cursor.offset == offset) { + let after: usize = tracker.cursors.iter().map(|cursor| cursor.offset).sum(); + scanned += after - before; + } else { + scanned += offset; + rescans += 1; + } + } + (scanned, rescans) + } + + #[test] + fn locs_in_order_are_located_in_one_pass() { + let crlf = b"const a = 1;\r\n".repeat(32); + assert_eq!(scan_cost(&crlf), (crlf.len() - b"\r\n".len(), 0)); + let cjk = "\u{4E2D}".repeat(32).into_bytes(); + assert_eq!(scan_cost(&cjk), (cjk.len() - "\u{4E2D}".len(), 0)); + } } diff --git a/src/bun_core/string/immutable.rs b/src/bun_core/string/immutable.rs index e90c5e05e030..d29961fdb692 100644 --- a/src/bun_core/string/immutable.rs +++ b/src/bun_core/string/immutable.rs @@ -57,8 +57,9 @@ use wtf8_text::wtf8_decoded_units; pub use wtf8_text::{ Wtf8Codepoints, codepoint_len_utf16, push_codepoint_wtf8_joined, push_wtf8, push_wtf8_well_formed, utf8_decoded_len_utf16, utf8_lossy_len_utf16, wtf8_codepoint_at, - wtf8_codepoint_count, wtf8_codepoints, wtf8_first_codepoint, wtf8_has_surrogate, - wtf8_len_utf16, wtf8_offset_of_utf16_index, wtf8_slice_by_utf16, wtf8_to_utf16, wtf16_to_wtf8, + wtf8_codepoint_count, wtf8_codepoint_start, wtf8_codepoints, wtf8_first_codepoint, + wtf8_has_surrogate, wtf8_len_utf16, wtf8_offset_of_utf16_index, wtf8_slice_by_utf16, + wtf8_to_utf16, wtf16_to_wtf8, }; // UTF-16 surrogate primitives. The single implementation lives in the diff --git a/src/bun_core/string/immutable/wtf8_text.rs b/src/bun_core/string/immutable/wtf8_text.rs index 6a24ebd4a2ee..54cc2c760b6c 100644 --- a/src/bun_core/string/immutable/wtf8_text.rs +++ b/src/bun_core/string/immutable/wtf8_text.rs @@ -31,6 +31,14 @@ pub fn wtf8_codepoint_at(text: &[u8], at: usize) -> (u32, usize) { (UNICODE_REPLACEMENT, 1) } +/// Where the code point starts that `at` is in the middle of. `at` itself if it is not. +pub fn wtf8_codepoint_start(text: &[u8], at: usize) -> usize { + (1..=3) + .filter_map(|back| at.checked_sub(back)) + .find(|&start| wtf8_codepoint_at(text, start).1 > at - start) + .unwrap_or(at) +} + /// The code points of `text`, each with the offset in bytes where it starts: `[...text]`. See /// [`wtf8_codepoint_at`]. #[inline] diff --git a/src/lint/regex/subject.rs b/src/lint/regex/subject.rs index 487ceec8ce40..f13d8eb9f026 100644 --- a/src/lint/regex/subject.rs +++ b/src/lint/regex/subject.rs @@ -96,7 +96,7 @@ impl<'t> Subject<'t> { if self.is_ascii { return offset; } - wtf8::code_point_start(self.bytes, offset) + strings::wtf8_codepoint_start(self.bytes, offset) } /// The index in [`Subject::text`] of the unit that starts at or after the byte offset `offset`. diff --git a/src/lint/regex/wtf8.rs b/src/lint/regex/wtf8.rs index abca006b70b0..9a1eaf5ba712 100644 --- a/src/lint/regex/wtf8.rs +++ b/src/lint/regex/wtf8.rs @@ -18,14 +18,6 @@ pub(crate) fn code_point_at(s: &[u8], i: usize) -> (u32, usize) { } } -/// If `i` is in the middle of a code point, returns that code point's start. Otherwise returns `i`. -pub(crate) fn code_point_start(s: &[u8], i: usize) -> usize { - (1..=3) - .filter_map(|back| i.checked_sub(back)) - .find(|start| code_point_at(s, *start).1 > i - start) - .unwrap_or(i) -} - #[inline] pub(crate) fn lead_surrogate(cp: u32) -> u32 { 0xD800 + ((cp - 0x10000) >> 10) diff --git a/test/bake/dev/bundle.test.ts b/test/bake/dev/bundle.test.ts index 04d4e00788b8..662fa5ab6407 100644 --- a/test/bake/dev/bundle.test.ts +++ b/test/bake/dev/bundle.test.ts @@ -1776,3 +1776,23 @@ devTest("recovers from an unresolved css url() and html script src longer than t expect((await dev.fetch("/")).status).toBe(200); }, }); +const braceLeftOpenCRLF = "const port = 3000;\r\nfunction start() {\r\n"; +devTest("a brace left open in a CR LF file is shown on the file's last line", { + files: { + "index.html": emptyHtmlFile({ + styles: [], + scripts: ["index.ts"], + }), + // Not a string: the harness would trim the final line break. + "index.ts": Buffer.from(braceLeftOpenCRLF), + }, + async test(dev) { + const errors = ["index.ts:2:19: error: Unexpected end of file"]; + await using c = await dev.client("/", { errors }); + await c.expectReload(async () => { + await dev.write("index.ts", `console.log("loaded");`); + }); + await c.expectMessage("loaded"); + await dev.write("index.ts", braceLeftOpenCRLF, { dedent: false, errors }); + }, +}); diff --git a/test/js/bun/transpiler/parse-error-column.test.ts b/test/js/bun/transpiler/parse-error-column.test.ts index c48ac17bb863..27ef47951837 100644 --- a/test/js/bun/transpiler/parse-error-column.test.ts +++ b/test/js/bun/transpiler/parse-error-column.test.ts @@ -1,5 +1,5 @@ import { expect, test } from "bun:test"; -import { bunEnv, bunExe, tempDir } from "harness"; +import { bunEnv, bunExe, normalizeBunSnapshot, tempDir } from "harness"; import { join } from "node:path"; // JS/TS/TOML parse diagnostics must count columns in UTF-16 code units, the @@ -185,3 +185,159 @@ test.concurrent("CLI caret counts a tab the same with and without non-ASCII text // "1 | " is 4 columns. Each of the two wide characters takes 2. expect({ ascii, accent, wide }).toEqual({ ascii: 4 + 18, accent: 4 + 18, wide: 4 + 20 }); }); + +// A diagnostic that the logger locates from a byte offset has the same line, +// column and line text for every way to end a line. Each source is written from +// explicit bytes: a fixture file would get LF. +const lineBreaks = { LF: "\n", CRLF: "\r\n", CR: "\r" }; + +async function diagnostics(file: string, source: string) { + await Bun.write(file, source); + const { logs } = await Bun.build({ entrypoints: [file], throw: false }); + return logs.map(({ message, position }) => ({ + message, + line: position?.line, + column: position?.column, + lineText: position?.lineText, + })); +} + +async function diagnosticsPerLineBreak(ext: string, lines: string[], names = Object.keys(lineBreaks)) { + using dir = tempDir("parse-col-line-breaks", {}); + const found: Record = {}; + for (const name of names) { + const lineBreak = lineBreaks[name as keyof typeof lineBreaks]; + found[name] = await diagnostics(join(String(dir), `${name}.${ext}`), lines.join(lineBreak) + lineBreak); + } + return found; +} + +test.concurrent.each([ + { ext: "js", lines: ["const port = 3000;", "function start() {"], message: "Unexpected end of file", column: 19 }, + { ext: "jsx", lines: ["const port = 3000;", "function start() {"], message: "Unexpected end of file", column: 19 }, + { ext: "ts", lines: ["const port = 3000;", "listen(port,"], message: "Unexpected end of file", column: 13 }, + { + ext: "tsx", + lines: ["const port = 3000;", "class Server {"], + message: 'Expected "}" but found end of file', + column: 15, + }, + { ext: "json", lines: ["{", ' "port": 3000,'], message: "Expected string but found end of file", column: 16 }, + { + ext: "jsonc", + lines: ["{ // the port", ' "port": 3000,'], + message: "Expected string but found end of file", + column: 16, + }, + { ext: "json5", lines: ["{", " port: 3000,"], message: "Unexpected end of input", column: 14 }, + // TOML has no lone CR line break. CSS is not here: its tokenizer locates its own errors (test/js/bun/css/). + { + ext: "toml", + lines: ["port = 3000", "hosts = [1,"], + message: "Unterminated array; expected ']'", + column: 12, + names: ["LF", "CRLF"], + }, + { ext: "yaml", lines: ["port: 3000", "hosts: [1,"], message: "Unexpected token", column: 11 }, + { ext: "xml", lines: ["", "3000"], message: "Missing closing tag for element 'port'", column: 11 }, +])( + "$ext: an error at the end of the file is on the last line for every line break", + async ({ ext, lines, message, column, names = Object.keys(lineBreaks) }) => { + const onLastLine = [{ message, line: 2, column, lineText: lines[1] }]; + expect(await diagnosticsPerLineBreak(ext, lines, names)).toEqual( + Object.fromEntries(names.map(name => [name, onLastLine])), + ); + }, +); + +test.concurrent.each([ + ["];", 1], + ["const host = ];", 14], +])("the column of the error in %j on line 2 counts from 1 for every line break", async (secondLine, column) => { + const found = [{ message: "Unexpected ]", line: 2, column, lineText: secondLine }]; + expect(await diagnosticsPerLineBreak("js", ["const port = 3000;", secondLine])).toEqual({ + LF: found, + CRLF: found, + CR: found, + }); +}); + +test.concurrent("both errors for an unclosed ${ are on the last line for every line break", async () => { + // The lexer reports the second one at the last byte of the file, not at its end. + const lines = ["const port = 3000;", "const url = `http://localhost:${port"]; + const found = ['Expected "}" but found end of file', "Unterminated string literal"].map(message => ({ + message, + line: 2, + column: 37, + lineText: lines[1], + })); + expect(await diagnosticsPerLineBreak("js", lines)).toEqual({ LF: found, CRLF: found, CR: found }); +}); + +test.concurrent("an error at the end of the file is not located inside a multi-byte sequence", async () => { + using dir = tempDir("parse-col-sequence", {}); + const at = (name: string, source: string) => diagnostics(join(String(dir), name), source); + expect({ + lineSeparator: await at("ls.js", "const port = 3000;\nfunction start() {\u2028"), + paragraphSeparator: await at("ps.js", "const port = 3000;\nfunction start() {\u2029"), + lastCharacter: await at("cjk.js", "foo(\u4E2D"), + lastCharacterTwice: await at("template.js", "const s = `${a\u4E2D"), + }).toEqual({ + lineSeparator: [{ message: "Unexpected end of file", line: 2, column: 19, lineText: "function start() {" }], + paragraphSeparator: [{ message: "Unexpected end of file", line: 2, column: 19, lineText: "function start() {" }], + // With no final line break the error is at the last character, as it is for ASCII. + lastCharacter: [{ message: 'Expected ")" but found end of file', line: 1, column: 5, lineText: "foo(\u4E2D" }], + lastCharacterTwice: [ + { message: 'Expected "}" but found end of file', line: 1, column: 15, lineText: "const s = `${a\u4E2D" }, + { message: "Unterminated string literal", line: 1, column: 15, lineText: "const s = `${a\u4E2D" }, + ], + }); +}); + +const braceLeftOpenCRLF = "const port = 3000;\r\nfunction start() {\r\n"; + +test.concurrent("Bun.Transpiler and import() locate the end of a CR LF source on its last line", async () => { + using dir = tempDir("parse-col-crlf-import", { "crlf.js": braceLeftOpenCRLF }); + const position = (thrown: any) => { + const { line, column, lineText } = (thrown.errors?.[0] ?? thrown).position; + return { line, column, lineText }; + }; + let transformed, imported; + try { + new Bun.Transpiler({ loader: "js" }).transformSync(braceLeftOpenCRLF); + } catch (thrown) { + transformed = position(thrown); + } + try { + await import(join(String(dir), "crlf.js")); + } catch (thrown) { + imported = position(thrown); + } + const onLastLine = { line: 2, column: 19, lineText: "function start() {" }; + expect({ transformed, imported }).toEqual({ transformed: onLastLine, imported: onLastLine }); +}); + +test.concurrent.each(["build", "run"])( + "bun %s prints the last line of a CR LF file for a brace left open", + async command => { + using dir = tempDir("parse-col-crlf-cli", { "crlf.js": braceLeftOpenCRLF }); + await using proc = Bun.spawn({ + cmd: [bunExe(), command, "crlf.js"], + env: { ...bunEnv, NO_COLOR: "1" }, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, frame: normalizeBunSnapshot(stderr, String(dir)).split("\n").slice(0, 4), exitCode }).toEqual({ + stdout: "", + frame: [ + "2 | function start() {", + " ^", + "error: Unexpected end of file", + " at /crlf.js:2:19", + ], + exitCode: 1, + }); + }, +); From b93e5a4b6a873f612ddd823aabdb78ef6def5376 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:44:40 +0000 Subject: [PATCH 2/3] logger tests: spell the line breaks with strings::replace_owned --- src/ast/lib.rs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/ast/lib.rs b/src/ast/lib.rs index ac442184999d..e58a2541530b 100644 --- a/src/ast/lib.rs +++ b/src/ast/lib.rs @@ -3648,18 +3648,6 @@ mod line_column_tracker_tests { ) } - fn with_line_break(lf: &[u8], line_break: &[u8]) -> Vec { - let mut spelled = Vec::new(); - for &byte in lf { - if byte == b'\n' { - spelled.extend_from_slice(line_break); - } else { - spelled.push(byte); - } - } - spelled - } - #[test] fn crlf_and_lone_cr_are_located_like_lf() { let sources = [ @@ -3670,8 +3658,8 @@ mod line_column_tracker_tests { "s = '\u{00E9}\u{4E2D}\u{1F600}';\n];\n", ]; for lf in sources.map(str::as_bytes) { - let crlf = with_line_break(lf, b"\r\n"); - let cr = with_line_break(lf, b"\r"); + let crlf = bun_core::strings::replace_owned(lf, b"\n", b"\r\n"); + let cr = bun_core::strings::replace_owned(lf, b"\n", b"\r"); let source = bstr::BStr::new(lf); let mut line_breaks_before = 0; // Through `lf.len()`, the `Loc` of an error at the end of the file. From 076be3ed78e07aded73d32f29bb6200b5be16e65 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 17 Oct 2026 18:02:15 +0000 Subject: [PATCH 3/3] css: log parser warnings with the Source and a byte position A warning of the CSS parser had no file for a plain sheet, no line text, and a 0-based line. ParserOptions::warn* had a line and a column only, and logged through Log::add_warning_fmt_line_col, the one logger entry that takes no Source. The parse entry points now take the Source, and each warn site passes the byte offset of the tokenizer to ParserOptions::warn_at. The logger builds the file, the namespace, the line, the column and the line text, as it does for the diagnostics of every other parser. Warnings go through the new Log::add_warning_fmt_no_long_excerpt. It drops an excerpt of 120 bytes or more: the logger cuts such a line around the position and pads the caret by the whole column, so the warnings of a one-line sheet would print in the order of N squared bytes. ParserOptions::warn(&ParseError) stays for the errors that error_recovery skips. A ParseError has no byte position, so such a warning has the file and no line. The warning for a nested "composes" is now at the declaration. It was at the start of a rule, and not always of the rule that holds the declaration. Log::add_warning_fmt_line_col, Log::add_warning_fmt_line_col_with_notes and SourceLocation::to_logger_location are deleted. --- src/ast/lib.rs | 109 +++++++---- src/bundler/ParseTask.rs | 3 +- src/bundler/transpiler.rs | 3 +- src/css/css_parser.rs | 165 ++++++++-------- src/css/declaration.rs | 27 ++- src/css/selectors/parser.rs | 46 ++--- src/css_jsc/css_internals.rs | 8 +- src/runtime/cli/pm_diff_normalize.rs | 3 +- test/bake/dev/css.test.ts | 24 +++ test/bundler/css/css-modules.test.ts | 277 ++++++++++++++++++++++++++- 10 files changed, 494 insertions(+), 171 deletions(-) diff --git a/src/ast/lib.rs b/src/ast/lib.rs index e58a2541530b..a4b8c4322d9f 100644 --- a/src/ast/lib.rs +++ b/src/ast/lib.rs @@ -659,7 +659,7 @@ pub struct Location { // NOT `#[derive(Clone)]`. `file` / `namespace` / `line_text` are // `Cow<'static, [u8]>` whose `Borrowed` arm may carry a lifetime-erased view -// into a `Source` (see `init_or_null`, `css_parser.rs`, `error.rs`, +// into a `Source` (see `init_or_null`, `error.rs`, // `JSBundler.rs`). The derived `Cow::clone` would re-borrow that pointer, so a // `BuildMessage` cloned via `Option::clone()` / `Vec::clone()` // could outlive the source buffer and read poisoned memory. Instead, @@ -803,6 +803,10 @@ impl Location { Self::init_or_null_impl(_source, r, Some(tracker)) } + /// The bytes of a long line that an excerpt keeps before and after the position. + const EXCERPT_BEFORE: usize = 40; + const EXCERPT_AFTER: usize = 80; + fn init_or_null_impl( _source: Option<&Source>, r: Range, @@ -830,9 +834,9 @@ impl Location { // BYTE offsets; the gate keeps the original shape (no left trim for // an error in the last 80 bytes) so `write_format`'s caret aligns. let offset_in_line = offset.saturating_sub(data.line_start).min(full_line.len()); - if full_line.len() > 80 + offset_in_line { - let mut lo = offset_in_line.saturating_sub(40); - let mut hi = (offset_in_line + 80).min(full_line.len()); + if full_line.len() > Self::EXCERPT_AFTER + offset_in_line { + let mut lo = offset_in_line.saturating_sub(Self::EXCERPT_BEFORE); + let mut hi = (offset_in_line + Self::EXCERPT_AFTER).min(full_line.len()); while lo > 0 && !bun_core::strings::is_utf8_char_boundary(full_line[lo]) { lo -= 1; } @@ -1971,46 +1975,38 @@ impl Log { } } + /// A warning and its note. An excerpt stays only if it starts where its line starts. #[cold] - pub fn add_warning_fmt_line_col( + pub fn add_warning_fmt_no_long_excerpt( &mut self, - filepath: Str, - line: u32, - col: u32, + source: Option<&Source>, + loc: Loc, args: fmt::Arguments<'_>, + note: Option<(Loc, fmt::Arguments<'_>)>, ) { - self.add_warning_fmt_line_col_with_notes(filepath, line, col, args, Box::default()) - } + // Every excerpt that `Location::init_or_null` cut on the left is at least this long. + const CUT_EXCERPT_MIN: usize = Location::EXCERPT_BEFORE + Location::EXCERPT_AFTER; - #[cold] - pub fn add_warning_fmt_line_col_with_notes( - &mut self, - filepath: Str, - line: u32, - col: u32, - args: fmt::Arguments<'_>, - notes: Box<[Data]>, - ) { if !Kind::Warn.should_print(self.level) { return; } self.warnings += 1; - - // TODO: do this properly - - let data = Data { - text: alloc_print(args), - location: Some(Location { - // `Location.file` borrows the lifetime-erased `Str`; see the - // module-level OWNERSHIP note. - file: Cow::Borrowed(filepath), - line: i32::try_from(line).expect("int cast"), - column: i32::try_from(col).expect("int cast"), - ..Default::default() - }), - } - .clone_line_text(self.clone_line_text); - + let mut located = |loc, args| { + let range = Range { loc, len: 0 }; + let mut data = self.tracked_range_data(source, range, alloc_print(args)); + if let Some(location) = &mut data.location + && let Some(excerpt) = &location.line_text + && excerpt.len() >= CUT_EXCERPT_MIN + { + location.line_text = None; + } + data + }; + let notes: Box<[Data]> = note + .map(|(loc, args)| located(loc, args)) + .into_iter() + .collect(); + let data = located(loc, args); self.add_msg(Msg { kind: Kind::Warn, data, @@ -3741,3 +3737,46 @@ mod line_column_tracker_tests { assert_eq!(scan_cost(&cjk), (cjk.len() - "\u{4E2D}".len(), 0)); } } + +#[cfg(test)] +mod no_long_excerpt_tests { + use super::*; + + #[test] + fn a_warning_keeps_only_an_excerpt_that_starts_where_its_line_starts() { + let long_line = b"short\n".len(); + let contents = [b"short\n" as &[u8], &[b'a'; 200]].concat(); + let source = Source::init_path_string_owned(b"t.css" as &[u8], contents); + let mut log = Log::default(); + for at in [0, long_line + 39, long_line + 40, long_line + 199] { + log.add_warning_fmt_no_long_excerpt( + Some(&source), + usize2loc(at), + format_args!("warning"), + Some((usize2loc(long_line), format_args!("note"))), + ); + } + + let located = |data: &Data| { + let location = data.location.as_ref().unwrap(); + let excerpt_len = location.line_text.as_ref().map(|excerpt| excerpt.len()); + (location.line, location.column, excerpt_len) + }; + let found: Vec<_> = log + .msgs + .iter() + .map(|msg| (located(&msg.data), located(&msg.notes[0]))) + .collect(); + let note = (2, 1, Some(80)); + assert_eq!( + found, + [ + ((1, 1, Some(5)), note), + ((2, 40, Some(119)), note), + ((2, 41, None), note), + ((2, 200, None), note), + ] + ); + assert_eq!(log.warnings, 4); + } +} diff --git a/src/bundler/ParseTask.rs b/src/bundler/ParseTask.rs index 1c20cf221d97..f087b80fe74b 100644 --- a/src/bundler/ParseTask.rs +++ b/src/bundler/ParseTask.rs @@ -1287,7 +1287,6 @@ pub mod parse_worker { Loader::Css => { // make css ast let mut import_records = Vec::::default(); - let source_code = &source.contents; let mut temp_log = Log::init(); // `temp_log` is flushed into `log` on every exit path via linear // control flow (scopeguard would alias `log`/`temp_log`). @@ -1307,7 +1306,7 @@ pub mod parse_worker { let (mut css_ast, extra) = match bun_css::BundlerStyleSheet::parse_bundler( bump, - source_code, + source, parser_options, &mut import_records, bun_ast::Index::source(source.index.0), diff --git a/src/bundler/transpiler.rs b/src/bundler/transpiler.rs index e7dd5a0b4ef7..d55ff4c0365b 100644 --- a/src/bundler/transpiler.rs +++ b/src/bundler/transpiler.rs @@ -3070,9 +3070,10 @@ impl<'a> Transpiler<'a> { // `'bump`-threading note). let alloc: &'static Arena = unsafe { bun_ptr::detach_lifetime_ref::(self.arena) }; + let source = bun_ast::Source::init_path_string(file_path_text, entry.contents()); let (mut sheet, extra) = match bun_css::StyleSheet::::parse( alloc, - entry.contents(), + &source, opts, None, bun_ast::Index::source(0u32), diff --git a/src/css/css_parser.rs b/src/css/css_parser.rs index 54f041ba317d..4a5806ad8de3 100644 --- a/src/css/css_parser.rs +++ b/src/css/css_parser.rs @@ -152,15 +152,6 @@ impl VendorPrefix { pub use crate::SourceLocation; impl SourceLocation { - pub(crate) fn to_logger_location(self, file: &'static [u8]) -> bun_ast::Location { - bun_ast::Location { - file: std::borrow::Cow::Borrowed(file), - line: i32::try_from(self.line).expect("int cast"), - column: i32::try_from(self.column).expect("int cast"), - ..Default::default() - } - } - /// Create a new BasicParseError at this location for an unexpected token pub(crate) fn new_basic_unexpected_token_error(self, token: Token) -> ParseError { BasicParseError { @@ -360,7 +351,10 @@ fn parse_custom_at_rule_prelude( // SAFETY: `name` points into the parser's source/arena, which outlives every // `AtRulePrelude`/warning produced from this parser (see `src_str`). let name: &'static [u8] = unsafe { src_str(name) }; - options.warn(&input.new_error(BasicParseErrorKind::at_rule_invalid(name))); + options.warn_at( + input.loc(), + format_args!("{}", BasicParseErrorKind::at_rule_invalid(name)), + ); input.skip_whitespace(); let tokens = TokenListFns::parse(input, options, 0)?; Ok(AtRulePrelude::Unknown { name, tokens }) @@ -985,11 +979,13 @@ impl<'a, AtRuleParserT: CustomAtRuleParser> TopLevelRuleParser<'a, AtRuleParserT // ───────────────────────────── NestedRuleParser ───────────────────────────── +/// Whether a style rule can hold `composes`. #[derive(Clone, Copy)] pub enum ComposesState { - Allow(SourceLocation), - DisallowNested(SourceLocation), - DisallowNotSingleClass(SourceLocation), + Allow, + DisallowNested, + /// Where that rule starts, for the note of the warning. + DisallowNotSingleClass(bun_ast::Loc), DisallowEntirely, } @@ -1403,17 +1399,13 @@ mod rule_parsers { ) -> CssResult<(DeclarationBlock<'static>, CssRuleList)> { // TODO: think about memory management in error cases let mut rules = CssRuleList::::default(); - let composes_state = if self.is_in_style_rule - && matches!(self.composes_state, ComposesState::Allow(_)) - { - let ComposesState::Allow(l) = self.composes_state else { - unreachable!() + let composes_state = + if self.is_in_style_rule && matches!(self.composes_state, ComposesState::Allow) { + ComposesState::DisallowNested + } else { + // ComposesState is Copy. + self.composes_state }; - ComposesState::DisallowNested(l) - } else { - // ComposesState is Copy. - self.composes_state - }; // SAFETY: see `TopLevelRuleParser::nested` — `'static` erasure of the // parser arena. let bump: &'static Bump = unsafe { bun_ptr::detach_lifetime_ref(self.arena) }; @@ -1633,7 +1625,10 @@ mod rule_parsers { break 'brk AtRulePrelude::Scope { scope_start, scope_end }; }, b"nest" => if this.is_in_style_rule { - this.options.warn(&input.new_custom_error(ParserError::deprecated_nest_rule)); + this.options.warn_at( + input.loc(), + format_args!("{}", ParserError::deprecated_nest_rule), + ); let mut selector_parser = selector_parser::SelectorParser { is_nesting_allowed: true, options: this.options, @@ -1992,45 +1987,29 @@ mod rule_parsers { // - NOT in nested style rules // - AND there is only one class selector if input.flags.css_modules() { + let rule_start = bun_ast::usize2loc(start.position); 'out: { if this.is_in_style_rule { - this.composes_state = ComposesState::DisallowNested(SourceLocation { - line: loc.line, - column: loc.column, - }); + this.composes_state = ComposesState::DisallowNested; break 'out; } if selectors.v.len() != 1 { - this.composes_state = - ComposesState::DisallowNotSingleClass(SourceLocation { - line: loc.line, - column: loc.column, - }); + this.composes_state = ComposesState::DisallowNotSingleClass(rule_start); break 'out; } let sel = &selectors.v.slice()[0]; if sel.components.len() != 1 { - this.composes_state = - ComposesState::DisallowNotSingleClass(SourceLocation { - line: loc.line, - column: loc.column, - }); + this.composes_state = ComposesState::DisallowNotSingleClass(rule_start); break 'out; } let comp = &sel.components[0]; if let Some(r) = comp.as_class() { let ref_ = r.as_ref().unwrap(); this.composes_refs.append(ref_); - this.composes_state = ComposesState::Allow(SourceLocation { - line: loc.line, - column: loc.column, - }); + this.composes_state = ComposesState::Allow; break 'out; } - this.composes_state = ComposesState::DisallowNotSingleClass(SourceLocation { - line: loc.line, - column: loc.column, - }); + this.composes_state = ComposesState::DisallowNotSingleClass(rule_start); } } let location = input.position(); @@ -2038,7 +2017,7 @@ mod rule_parsers { // We parsed a style rule with the `composes` property. Track which // properties it used so we can validate it later. - if matches!(this.composes_state, ComposesState::Allow(_)) { + if matches!(this.composes_state, ComposesState::Allow) { let len = input.position() - location; let mut usage = PropertyBitset::init_empty(); let mut custom_properties: Vec<&'static [u8]> = Vec::new(); @@ -2537,10 +2516,10 @@ mod stylesheet_impl { return Ok(ToCssResult { code: dest }); } - pub fn parse( + pub fn parse<'a>( arena: &'static Bump, - code: &[u8], - options: ParserOptions<'_>, + source: &'a bun_ast::Source, + options: ParserOptions<'a>, import_records: Option<&mut Vec>, source_index: SrcIndex, ) -> Maybe<(StyleSheet, StylesheetExtra), Err> { @@ -2556,7 +2535,7 @@ mod stylesheet_impl { }; StyleSheet::::parse_with( arena, - code, + source, options, &mut at_rule_parser, import_records.map(core::ptr::NonNull::from), @@ -2564,15 +2543,16 @@ mod stylesheet_impl { ) } - /// Parse a style sheet from a string. - pub(crate) fn parse_with>( + /// Parse the style sheet in `source`. + pub(crate) fn parse_with<'a, P: CustomAtRuleParser>( arena: &'static Bump, - code: &[u8], - options: ParserOptions<'_>, + source: &'a bun_ast::Source, + mut options: ParserOptions<'a>, at_rule_parser: &mut P, import_records: Option>>, source_index: SrcIndex, ) -> Maybe<(Self, StylesheetExtra), Err> { + options.source = Some(source); // TODO: 'bump lifetime threading — every arena-backed slice the // parser hands back is currently detached to `'static` (matching the // crate-wide erasure on `DeclarationBlock<'static>`/`Token` payloads). @@ -2580,7 +2560,7 @@ mod stylesheet_impl { // returned `StyleSheet`. // TODO(refactor): re-thread the lifetime through `CssRuleList<'bump, R>` // and drop the `'static` bound on `arena`. - check_input_len(code)?; + check_input_len(source.contents())?; let mut composes = ComposesMap::default(); let mut parser_extra = ParserExtra { local_scope: LocalScope::default(), @@ -2589,7 +2569,7 @@ mod stylesheet_impl { }; let mut local_properties = LocalPropertyUsage::default(); - let mut input = ParserInput::new(code, arena); + let mut input = ParserInput::new(source.contents(), arena); let mut parser = Parser::new( &mut input, import_records, @@ -2673,23 +2653,24 @@ mod stylesheet_impl { } impl StyleAttribute { - pub fn parse( + pub fn parse<'a>( arena: &'static Bump, - code: &[u8], - options: &ParserOptions, + source: &'a bun_ast::Source, + mut options: ParserOptions<'a>, import_records: &mut Vec, source_index: SrcIndex, ) -> Maybe> { + options.source = Some(source); // TODO: 'bump lifetime threading — `DeclarationBlock<'static>` in // `StyleAttribute` vs `Parser<'a>` here; `arena: &'static Bump` // matches the crate-wide erasure (see `parse_with`). - check_input_len(code)?; + check_input_len(source.contents())?; let mut parser_extra = ParserExtra { local_scope: LocalScope::default(), symbols: SymbolList::default(), source_index, }; - let mut input = ParserInput::new(code, arena); + let mut input = ParserInput::new(source.contents(), arena); let mut parser = Parser::new( &mut input, Some(core::ptr::NonNull::from(import_records)), @@ -2702,7 +2683,7 @@ mod stylesheet_impl { ); let sources: Vec> = vec![options.filename.into()]; Ok(StyleAttribute { - declarations: match DeclarationBlock::parse(&mut parser, options) { + declarations: match DeclarationBlock::parse(&mut parser, &options) { Ok(v) => v, Err(e) => return Err(Err::from_parse_error(e, b"")), }, @@ -2743,10 +2724,10 @@ mod stylesheet_impl { } impl StyleSheet { - pub fn parse_bundler( + pub fn parse_bundler<'a>( arena: &'static Bump, - code: &[u8], - options: ParserOptions<'_>, + source: &'a bun_ast::Source, + options: ParserOptions<'a>, import_records: &mut Vec, source_index: SrcIndex, ) -> Maybe<(Self, StylesheetExtra), Err> { @@ -2768,7 +2749,7 @@ mod stylesheet_impl { }; Self::parse_with( arena, - code, + source, options, &mut at_rule_parser, Some(import_records_ptr), @@ -2915,47 +2896,47 @@ pub struct ParserOptions<'a> { /// — see PORTING.md §Forbidden patterns. Only `default` sets it, from a /// `&'a mut Log`. logger: Option>, + /// The sheet that is parsed, set by the parse entry points: a warning is located in it. + source: Option<&'a bun_ast::Source>, /// Feature flags to enable. pub flags: ParserFlags, _lt: core::marker::PhantomData<&'a mut Log>, } impl<'a> ParserOptions<'a> { + /// Log an error that `error_recovery` skipped, with the file only: it has no byte position. pub(crate) fn warn(&self, warning: &ParseError) { + self.warn_at(bun_ast::Loc::EMPTY, format_args!("{}", warning.kind)); + } + + /// Log a warning at byte position `at` of the sheet. + #[cold] + #[inline(never)] + pub(crate) fn warn_at(&self, at: bun_ast::Loc, text: fmt::Arguments<'_>) { if let Some(lg) = self.logger { // SAFETY: `logger` was constructed from a unique `&'a mut Log` (see // `default`); the pointee outlives `'a` and no other borrow of the // Log exists for the duration of parsing. let lg: &mut Log = unsafe { &mut *lg.as_ptr() }; - lg.add_warning_fmt_line_col( - self.filename, - warning.location.line, - warning.location.column, - format_args!("{}", warning.kind), - ); + lg.add_warning_fmt_no_long_excerpt(self.source, at, text, None); } } - pub(crate) fn warn_fmt(&self, args: fmt::Arguments<'_>, line: u32, column: u32) { - if let Some(lg) = self.logger { - // SAFETY: see `warn` — `logger` carries `*mut Log` provenance from a - // unique `&'a mut Log`; no other borrow exists during this call. - let lg: &mut Log = unsafe { &mut *lg.as_ptr() }; - lg.add_warning_fmt_line_col(self.filename, line, column, args); - } - } - - pub(crate) fn warn_fmt_with_notes( + /// [`warn_at`](Self::warn_at), with a note at byte position `note_at`. + #[cold] + #[inline(never)] + pub(crate) fn warn_with_note( &self, - args: fmt::Arguments<'_>, - line: u32, - column: u32, - notes: Box<[bun_ast::Data]>, + at: bun_ast::Loc, + text: fmt::Arguments<'_>, + note_at: bun_ast::Loc, + note_text: fmt::Arguments<'_>, ) { if let Some(lg) = self.logger { - // SAFETY: see `warn`. + // SAFETY: see `warn_at`. let lg: &mut Log = unsafe { &mut *lg.as_ptr() }; - lg.add_warning_fmt_line_col_with_notes(self.filename, line, column, args, notes); + let note = Some((note_at, note_text)); + lg.add_warning_fmt_no_long_excerpt(self.source, at, text, note); } } @@ -2966,6 +2947,7 @@ impl<'a> ParserOptions<'a> { source_index: 0, error_recovery: false, logger: log.map(core::ptr::NonNull::from), + source: None, flags: ParserFlags::default(), _lt: core::marker::PhantomData, } @@ -3579,6 +3561,11 @@ impl<'a> Parser<'a> { self.input.tokenizer.position } + /// [`Parser::position`] as a position for the logger. + pub(crate) fn loc(&self) -> bun_ast::Loc { + bun_ast::usize2loc(self.position()) + } + fn parse_empty(_: &mut Parser) -> CssResult<()> { Ok(()) } diff --git a/src/css/declaration.rs b/src/css/declaration.rs index 810640064580..9b1b3063af23 100644 --- a/src/css/declaration.rs +++ b/src/css/declaration.rs @@ -335,7 +335,6 @@ where ) { delimiters |= css::Delimiters::CURLY_BRACKET; } - let source_location = input.current_source_location(); let mut property = input.parse_until_before(delimiters, |input2: &mut css::Parser| { css::Property::parse(property_id, input2, options) })?; @@ -351,27 +350,23 @@ where if let css::Property::Composes(composes) = &mut property { match composes_ctx.composes_state() { css::ComposesState::DisallowEntirely => {} - css::ComposesState::Allow(_) => { + css::ComposesState::Allow => { composes_ctx.record_composes(composes); } - css::ComposesState::DisallowNested(info) => { - options.warn_fmt( + css::ComposesState::DisallowNested => { + options.warn_at( + composes.loc, format_args!("\"composes\" is not allowed inside nested selectors"), - info.line, - info.column, ); } - css::ComposesState::DisallowNotSingleClass(info) => { - options.warn_fmt_with_notes( + css::ComposesState::DisallowNotSingleClass(rule_start) => { + options.warn_with_note( + composes.loc, format_args!("\"composes\" only works inside single class selectors"), - source_location.line, - source_location.column, - Box::new([bun_ast::Data { - text: b"The parent selector is not a single class selector because of the syntax here:" - .as_slice() - .into(), - location: Some(info.to_logger_location(options.filename)), - }]), + rule_start, + format_args!( + "The parent selector is not a single class selector because of the syntax here:" + ), ); } } diff --git a/src/css/selectors/parser.rs b/src/css/selectors/parser.rs index 5e8df3665529..05012c2066f6 100644 --- a/src/css/selectors/parser.rs +++ b/src/css/selectors/parser.rs @@ -1213,6 +1213,17 @@ impl<'a> SelectorParser<'a> { Some(unsafe { crate::arena_str(prefix.v) }) } + fn warn_unsupported_pseudo(&self, at: bun_ast::Loc, name: Str) { + self.options.warn_at( + at, + format_args!( + "{}", + SelectorParseErrorKind::UnsupportedPseudoClassOrElement(name) + .into_default_parser_error() + ), + ); + } + pub(crate) fn parse_functional_pseudo_element( &mut self, name: Str, @@ -1277,12 +1288,7 @@ impl<'a> SelectorParser<'a> { _ => {} } if !strings::starts_with(name, b"-") { - self.options.warn( - &input.new_custom_error( - SelectorParseErrorKind::UnsupportedPseudoClassOrElement(name) - .into_default_parser_error(), - ), - ); + self.warn_unsupported_pseudo(input.loc(), name); } let mut args: Vec = Vec::new(); @@ -1307,9 +1313,11 @@ impl<'a> SelectorParser<'a> { }} } + /// `loc` (for an error) and `position` (for a warning) are where the pseudo-class starts. pub(crate) fn parse_non_ts_pseudo_class( &mut self, loc: css::SourceLocation, + position: usize, name: Str, ) -> CResult { // @compileError(css.todo_stuff.match_ignore_ascii_case); @@ -1318,9 +1326,7 @@ impl<'a> SelectorParser<'a> { break 'pseudo_class pseudo; } if strings::starts_with_char(name, b'_') { - self.options.warn(&loc.new_custom_error( - SelectorParseErrorKind::UnsupportedPseudoClassOrElement(name), - )); + self.warn_unsupported_pseudo(bun_ast::usize2loc(position), name); } else if (self.options.css_modules.is_some() && strings::eql_case_insensitive_ascii_check_length(name, b"local")) || strings::eql_case_insensitive_ascii_check_length(name, b"global") @@ -1371,12 +1377,7 @@ impl<'a> SelectorParser<'a> { }, _ => { if !strings::starts_with_char(name, b'-') { - self.options.warn( - &parser.new_custom_error( - SelectorParseErrorKind::UnsupportedPseudoClassOrElement(name) - .into_default_parser_error(), - ), - ); + self.warn_unsupported_pseudo(parser.loc(), name); } let mut args: Vec = Vec::new(); css::TokenListFns::parse_raw(parser, &mut args, self.options, 0)?; @@ -1423,16 +1424,15 @@ impl<'a> SelectorParser<'a> { ParseErrorRecovery::IgnoreInvalidSelector } + /// `position` is the byte offset where the pseudo-element starts. pub(crate) fn parse_pseudo_element( &mut self, - loc: css::SourceLocation, + position: usize, name: Str, ) -> CResult { let pseudo_element = lookup_pseudo_element(name).unwrap_or_else(|| { if !strings::starts_with_char(name, b'-') { - self.options.warn(&loc.new_custom_error( - SelectorParseErrorKind::UnsupportedPseudoClassOrElement(name), - )); + self.warn_unsupported_pseudo(bun_ast::usize2loc(position), name); } PseudoElement::Custom { name } }); @@ -3257,6 +3257,7 @@ pub(crate) fn parse_one_simple_selector( } Token::Colon => { let location = input.current_source_location(); + let position = input.position(); let (is_single_colon, next_token): (bool, Token) = match *input.next_including_whitespace()? { Token::Colon => (false, *input.next_including_whitespace()?), @@ -3327,7 +3328,7 @@ pub(crate) fn parse_one_simple_selector( parser.parse_functional_pseudo_element(name, i) })? } else { - parser.parse_pseudo_element(location, name)? + parser.parse_pseudo_element(position, name)? }; if state.contains(SelectorParsingState::AFTER_SLOTTED) @@ -3343,7 +3344,7 @@ pub(crate) fn parse_one_simple_selector( parse_functional_pseudo_class::(parser, input2, name, state) })? } else { - parse_simple_pseudo_class::(parser, location, name, *state)? + parse_simple_pseudo_class::(parser, location, position, name, *state)? }; return Ok(Some(S::SimpleSelector(pseudo_class))); } @@ -3639,6 +3640,7 @@ pub(crate) fn parse_functional_pseudo_class( pub(crate) fn parse_simple_pseudo_class( parser: &mut SelectorParser, location: css::SourceLocation, + position: usize, name: Str, state: SelectorParsingState, ) -> CResult> { @@ -3673,7 +3675,7 @@ pub(crate) fn parse_simple_pseudo_class( } } - let pseudo_class = parser.parse_non_ts_pseudo_class(location, name)?; + let pseudo_class = parser.parse_non_ts_pseudo_class(location, position, name)?; if state.contains(SelectorParsingState::AFTER_WEBKIT_SCROLLBAR) { if !pseudo_class.is_valid_after_webkit_scrollbar() { return Err(location.new_custom_error( diff --git a/src/css_jsc/css_internals.rs b/src/css_jsc/css_internals.rs index a7141d16c9f6..2acda5ff7f3c 100644 --- a/src/css_jsc/css_internals.rs +++ b/src/css_jsc/css_internals.rs @@ -159,9 +159,10 @@ fn testing_impl( }; let mut import_records = Vec::::default(); + let sheet = bun_ast::Source::init_path_string(b"", source.slice()); match StyleSheet::::parse( alloc, - source.slice(), + &sheet, parser_options, Some(&mut import_records), bun_ast::Index::INVALID, @@ -319,10 +320,11 @@ pub fn attr_test(global: &JSGlobalObject, frame: &CallFrame) -> JsResult::default(); + let attribute = bun_ast::Source::init_path_string(b"", source.slice()); match StyleAttribute::parse( alloc, - source.slice(), - &parser_options, + &attribute, + parser_options, &mut import_records, bun_ast::Index::INVALID, ) { diff --git a/src/runtime/cli/pm_diff_normalize.rs b/src/runtime/cli/pm_diff_normalize.rs index ca4042e0e740..9815c82217d0 100644 --- a/src/runtime/cli/pm_diff_normalize.rs +++ b/src/runtime/cli/pm_diff_normalize.rs @@ -924,9 +924,10 @@ fn normalize_css(path: &[u8], bytes: &[u8]) -> Option { let mut opts = bun_css::ParserOptions::default(None); opts.filename = alloc.alloc_slice_copy(path); let mut import_records = Vec::::default(); + let source = bun_ast::Source::init_path_string(path, bytes); let (sheet, _extra) = bun_css::StyleSheet::::parse( alloc, - bytes, + &source, opts, Some(&mut import_records), bun_ast::Index::INVALID, diff --git a/test/bake/dev/css.test.ts b/test/bake/dev/css.test.ts index 52c8735b7e16..200de67818a0 100644 --- a/test/bake/dev/css.test.ts +++ b/test/bake/dev/css.test.ts @@ -95,6 +95,30 @@ devTest("css file with initial syntax error gets recovered", { ); }, }); +devTest("css file with a warning on its first line and a syntax error shows both", { + files: { + "index.html": emptyHtmlFile({ + styles: ["styles.css"], + body: `hello world`, + }), + "styles.css": ` + @unknown-rule x; + body { + color: red; + }} + `, + }, + async test(dev) { + // The overlay reads line 0 as a message with no position. A warning on + // the first line had line 0 and a position after it. + await using c = await dev.client("/", { + errors: [ + "styles.css:1:14: warn: invalid @ rule encountered: '@unknown-rule'", + "styles.css:4:3: error: Unexpected end of input", + ], + }); + }, +}); devTest("add new css import later", { files: { "index.html": emptyHtmlFile({ diff --git a/test/bundler/css/css-modules.test.ts b/test/bundler/css/css-modules.test.ts index 849065706a6c..746c8fd0075b 100644 --- a/test/bundler/css/css-modules.test.ts +++ b/test/bundler/css/css-modules.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, tempDir } from "harness"; -import { join } from "node:path"; +import { bunEnv, bunExe, normalizeBunSnapshot, tempDir } from "harness"; +import { join, relative } from "node:path"; import { itBundled } from "../expectBundled"; describe("css", () => { @@ -672,3 +672,276 @@ test("a resolve error in CSS covers the whole url() and composes path", async () ); expect(ranges).toEqual([`url('/fonts/missing.woff2')`, `url("./missing.png")`, `'./missing.css'`]); }); + +// A warning of the CSS parser says where it is, as an error of the same sheet +// does: the file, the 1-based line, the column and the text of the line. +describe.concurrent("css parser warning positions", () => { + // The `.notes` getter is not declared in bun-types yet. + type Log = BuildMessage & { notes?: BuildMessage[] }; + + function positionIn(dir: string, log: Log) { + const { file, namespace, line, column, lineText } = log.position!; + return { file: relative(dir, file), namespace, line, column, lineText }; + } + + async function buildLogs(name: string, sheet: string) { + using dir = tempDir("css-warning-position", { [name]: sheet }); + const result = await Bun.build({ entrypoints: [join(String(dir), name)], throw: false }); + return (result.logs as Log[]).map(log => ({ + level: log.level, + message: log.message, + ...positionIn(String(dir), log), + notes: (log.notes ?? []).map(note => ({ message: note.message, ...positionIn(String(dir), note) })), + })); + } + + const unknownAtRule = (name: string, line: number, lineText = "@unknown-rule x;", column = 14) => ({ + level: "warn", + message: "invalid @ rule encountered: '@unknown-rule'", + file: name, + namespace: "file", + line, + column, + lineText, + notes: [], + }); + + test("each kind of warning, and the note, in a CSS module", async () => { + const sheet = [ + /* 1 */ "@unknownrule foo;", + /* 2 */ ".base { color: red; }", + /* 3 */ ".outer {", + /* 4 */ " .inner {", + /* 5 */ " composes: base;", + /* 6 */ " }", + /* 7 */ "}", + /* 8 */ ".a .b {", + /* 9 */ " color: blue;", + /* 10 */ " composes: base;", + /* 11 */ "}", + /* 12 */ ".c::custom-thing { color: green; }", + /* 13 */ ".d:_internal { color: green; }", + /* 14 */ ".e::custom-fn(x) { color: green; }", + /* 15 */ ".f:custom-fn(x) { color: green; }", + /* 16 */ ".p { @nest .q & { color: red } }", + /* 17 */ "@other bar;", + ]; + const warning = (message: string, line: number, column: number, notes: object[] = []) => ({ + level: "warn", + message, + file: "styles.module.css", + namespace: "file", + line, + column, + lineText: sheet[line - 1], + notes, + }); + const unsupported = (name: string) => `Invalid selector. Unsupported pseudo-class or pseudo-element '${name}'`; + + const logs = await buildLogs("styles.module.css", sheet.join("\n")); + // The misplaced `composes` declarations also make printing the sheet fail. + expect(logs.filter(log => log.level !== "error")).toEqual([ + warning("invalid @ rule encountered: '@unknownrule'", 1, 13), + warning('"composes" is not allowed inside nested selectors', 5, 14), + warning('"composes" only works inside single class selectors', 10, 12, [ + { + message: "The parent selector is not a single class selector because of the syntax here:", + file: "styles.module.css", + namespace: "file", + line: 8, + column: 1, + lineText: sheet[7], + }, + ]), + warning(unsupported("custom-thing"), 12, 4), + warning(unsupported("_internal"), 13, 4), + warning(unsupported("custom-fn"), 14, 15), + warning(unsupported("custom-fn"), 15, 14), + warning("The @nest rule is deprecated, use standard CSS nesting instead", 16, 11), + warning("invalid @ rule encountered: '@other'", 17, 7), + ]); + }); + + test.each([ + ["in a nested rule", [".outer {", " .inner {", " composes: base;", " }", "}"], 3, 14], + ["two levels deep", [".a {", " .b {", " .c {", " composes: base;", " }", " }", "}"], 4, 16], + ["in a nested @media", [".outer {", " @media (min-width: 1px) {", " composes: base;", " }", "}"], 3, 14], + ])('the warning for a "composes" %s is on the declaration', async (_, rules, line, column) => { + const logs = await buildLogs("styles.module.css", [...rules, ".base { color: red; }"].join("\n")); + expect(logs.filter(log => log.level !== "error")).toEqual([ + { + level: "warn", + message: '"composes" is not allowed inside nested selectors', + file: "styles.module.css", + namespace: "file", + line, + column, + lineText: rules[line - 1], + notes: [], + }, + ]); + }); + + test("a sheet that is not a CSS module", async () => { + const rules = ".a { color: red }\n.b { color: blue }\n"; + expect(await buildLogs("styles.css", rules + "@unknown-rule x;\n")).toEqual([unknownAtRule("styles.css", 3)]); + }); + + test.each([ + ["on the first line", "@unknown-rule x;\n.a { color: red }\n", 1, "@unknown-rule x;"], + ["after a CRLF line break", ".a { color: red }\r\n@unknown-rule x;\r\n", 2, "@unknown-rule x;"], + ["after a lone CR line break", ".a { color: red }\r@unknown-rule x;\r", 2, "@unknown-rule x;"], + ["on a last line with no line break", ".a { color: red }\n@unknown-rule x;", 2, "@unknown-rule x;"], + ])("a warning %s", async (_, sheet, line, lineText) => { + expect(await buildLogs("styles.css", sheet)).toEqual([unknownAtRule("styles.css", line, lineText)]); + }); + + test("a warning at the end of the sheet", async () => { + const sheet = ".a { color: red }\n@unknown-rule"; + using dir = tempDir("css-warning-position", { "styles.css": sheet }); + const { logs } = await Bun.build({ entrypoints: [join(String(dir), "styles.css")], throw: false }); + // The logger has no column after the last character of a source: it reports that character. + expect(logs.map(({ position }) => position)).toEqual([ + expect.objectContaining({ line: 2, column: 13, offset: sheet.length, lineText: "@unknown-rule" }), + ]); + }); + + // The logger cuts a long line around the position, or keeps it whole for a position in its last + // 80 bytes, and indents the caret by the whole column. Such a warning has its place and no line + // text. The first warning of each sheet is on a cut line and the second on a whole one. + test("warnings on a long line have their place and no line text", async () => { + const rules = Buffer.alloc(130, ".a{color:red}").toString(); + const logs = await buildLogs("styles.css", rules + "@unknown-rule x;" + rules + "@unknown-rule x;\n"); + expect(logs).toEqual([unknownAtRule("styles.css", 1, "", 144), unknownAtRule("styles.css", 1, "", 290)]); + }); + + test("bun build prints the place and no code frame for warnings on a long line", async () => { + const rules = Buffer.alloc(130, ".a{color:red}").toString(); + using dir = tempDir("css-warning-long-line", { + "styles.css": rules + "@unknown-rule x;" + rules + "@unknown-rule x;\n", + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "build", "./styles.css"], + env: bunEnv, + cwd: String(dir), + stdout: "ignore", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + expect({ stderr: normalizeBunSnapshot(stderr, String(dir)), exitCode }).toEqual({ + stderr: [ + "warn: invalid @ rule encountered: '@unknown-rule'", + " at /styles.css:1:144", + "", + "warn: invalid @ rule encountered: '@unknown-rule'", + " at /styles.css:1:290", + ].join("\n"), + exitCode: 0, + }); + }); + + test("a warning has the file and the namespace that an error of the same sheet has", async () => { + const logs = await buildLogs("styles.css", "@unknown-rule x;\n.a { color: red }\n}\n"); + expect(logs.map(({ level, file, namespace, line }) => ({ level, file, namespace, line }))).toEqual([ + { level: "warn", file: "styles.css", namespace: "file", line: 1 }, + { level: "error", file: "styles.css", namespace: "file", line: 4 }, + ]); + }); + + test("a sheet that a plugin loads in another namespace, and an in-memory sheet", async () => { + const sheet = ".a { color: red }\n@unknown-rule x;\n"; + using dir = tempDir("css-warning-namespace", { "entry.ts": `import "virtual:sheet.css";` }); + + const plugin = await Bun.build({ + entrypoints: [join(String(dir), "entry.ts")], + throw: false, + plugins: [ + { + name: "virtual", + setup(builder) { + builder.onResolve({ filter: /^virtual:/ }, args => ({ + path: args.path.slice("virtual:".length), + namespace: "virtual", + })); + builder.onLoad({ filter: /.*/, namespace: "virtual" }, () => ({ contents: sheet, loader: "css" })); + }, + }, + ], + }); + expect(plugin.logs.map(log => ({ level: log.level, message: log.message, ...log.position }))).toEqual([ + { + level: "warn", + message: "invalid @ rule encountered: '@unknown-rule'", + file: "sheet.css", + namespace: "virtual", + line: 2, + column: 14, + length: 0, + offset: sheet.indexOf(" x;"), + lineText: "@unknown-rule x;", + }, + ]); + + // The same in-memory path, once with a warning and once with an error. + const inMemory = (contents: string) => + Bun.build({ entrypoints: ["/memory/sheet.css"], files: { "/memory/sheet.css": contents }, throw: false }); + const [warned, failed] = await Promise.all([inMemory(sheet), inMemory(".a { color: red }\n}\n")]); + const place = ({ level, position }: BuildMessage) => ({ + level, + file: position!.file, + namespace: position!.namespace, + line: position!.line, + }); + const error = place(failed.logs[0] as BuildMessage); + expect([place(warned.logs[0] as BuildMessage), error]).toEqual([ + { level: "warn", file: error.file, namespace: "file", line: 2 }, + { level: "error", file: expect.stringContaining("sheet.css"), namespace: "file", line: 3 }, + ]); + }); + + test("a sheet in a data: URL has the URL as its file", async () => { + const url = "data:text/css,.a{color:red}%0A@unknown-rule%20x;"; + using dir = tempDir("css-warning-data-url", { "styles.css": `@import "${url}";\n` }); + const { logs } = await Bun.build({ entrypoints: [join(String(dir), "styles.css")], throw: false }); + expect(logs.map(log => ({ level: log.level, ...log.position }))).toEqual([ + { + level: "warn", + file: url, + namespace: "dataurl", + line: 2, + column: 14, + length: 0, + offset: ".a{color:red}\n@unknown-rule".length, + lineText: "@unknown-rule x;", + }, + ]); + }); + + test.each([ + ["bun build", "styles.css", []], + ["bun build", "styles.module.css", []], + ["bun build --no-bundle", "styles.css", ["--no-bundle"]], + ["bun build --no-bundle", "styles.module.css", ["--no-bundle"]], + ])("%s of %s prints the line and the place of a warning", async (_, name, flags) => { + using dir = tempDir("css-warning-position-cli", { + [name]: ".a { color: red }\n.b { color: blue }\n@unknown-rule x;\n", + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "build", ...flags, "./" + name], + env: bunEnv, + cwd: String(dir), + stdout: "ignore", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + expect(normalizeBunSnapshot(stderr, String(dir))).toBe( + [ + "3 | @unknown-rule x;", + " ^", + "warn: invalid @ rule encountered: '@unknown-rule'", + ` at /${name}:3:14`, + ].join("\n"), + ); + expect(exitCode).toBe(0); + }); +});