diff --git a/src/ast/lib.rs b/src/ast/lib.rs index 9235526d4d5c..8e839c4a31bb 100644 --- a/src/ast/lib.rs +++ b/src/ast/lib.rs @@ -1309,9 +1309,7 @@ impl Log { self.msgs.clear(); self.warnings = 0; self.errors = 0; - // Drop the per-source scan cache with the messages it was built for; - // its identity check is pointer-based, so don't let it outlive the - // logical reuse boundary. + // Drop the per-source scan cache with the messages it was built for. self.line_column_tracker = None; } @@ -2082,7 +2080,6 @@ pub fn usize2loc(loc: usize) -> Loc { // Source // ─────────────────────────────────────────────────────────────────────────── -#[derive(Clone)] pub struct Source<'a> { pub path: bun_paths::fs::Path<'a>, @@ -2096,6 +2093,9 @@ pub struct Source<'a> { pub identifier_name: Cow<'a, [u8]>, pub index: Index, + + /// Keys the [`Log`]'s line and column scan. Only a `Source` that borrows the same bytes has it. + pub id: SourceId, } impl Default for Source<'_> { @@ -2106,10 +2106,60 @@ impl Default for Source<'_> { contents_is_recycled: false, identifier_name: Cow::Borrowed(b""), index: Index::source(0), + id: SourceId::new(), + } + } +} + +impl Clone for Source<'_> { + fn clone(&self) -> Self { + Source { + path: self.path, + contents: self.contents.clone(), + contents_is_recycled: self.contents_is_recycled, + identifier_name: self.identifier_name.clone(), + index: self.index, + id: match self.contents { + Cow::Borrowed(_) => self.id, + // The clone has its own copy of the bytes. + Cow::Owned(_) => SourceId::new(), + }, } } } +/// Identifies a [`Source`]'s text. Its address cannot: a reused buffer holds the next text there. +#[derive(Clone, Copy)] +pub struct SourceId(u64); + +#[thread_local] +static NEXT_SOURCE_ID: core::cell::Cell = core::cell::Cell::new(0); + +impl SourceId { + /// A thread takes this many ids at a time, so that [`SourceId::new`] has no atomic operation. + const BLOCK: u64 = 1 << 32; + + #[inline] + fn new() -> SourceId { + let mut id = NEXT_SOURCE_ID.get(); + // A multiple of `BLOCK` is never an id. At one, this thread has no block or has used it up. + if id.is_multiple_of(Self::BLOCK) { + id = Self::next_block() + 1; + } + NEXT_SOURCE_ID.set(id + 1); + SourceId(id) + } + + #[cold] + #[inline(never)] + fn next_block() -> u64 { + use core::sync::atomic::{AtomicU64, Ordering}; + static NEXT_BLOCK: AtomicU64 = AtomicU64::new(0); + // Relaxed: a block only has to differ from every other block. + NEXT_BLOCK.fetch_add(Self::BLOCK, Ordering::Relaxed) + } +} + #[derive(Copy, Clone, Debug)] struct ErrorPosition { pub(crate) line_start: usize, @@ -2238,13 +2288,11 @@ fn clamp_error_offset(contents: &[u8], offset_loc: Loc) -> usize { /// are identical to `Source::init_error_position`. #[derive(Default)] pub struct LineColumnTracker { - /// Identity of the tracked source (`contents` and `path.text` pointers + - /// lengths, stored as `usize` so the tracker stays `Send`). A diagnostic - /// for a different source resets the scan. + /// The [`SourceId`] of the cursors' source. 0, the id of no `Source`, before the first one. + source_id: u64, + /// The address of that source's `contents` (a `usize`, so that the tracker is `Send`). contents_ptr: usize, contents_len: usize, - path_ptr: usize, - path_len: usize, cursors: [ScanCursor; 4], } @@ -2257,18 +2305,16 @@ struct ScanCursor { impl LineColumnTracker { fn matches(&self, source: &Source) -> bool { - self.contents_ptr == source.contents.as_ptr() as usize + self.source_id == source.id.0 + && self.contents_ptr == source.contents.as_ptr() as usize && self.contents_len == source.contents.len() - && self.path_ptr == source.path.text.as_ptr() as usize - && self.path_len == source.path.text.len() } fn reset_for(&mut self, source: &Source) { *self = LineColumnTracker { + source_id: source.id.0, contents_ptr: source.contents.as_ptr() as usize, contents_len: source.contents.len(), - path_ptr: source.path.text.as_ptr() as usize, - path_len: source.path.text.len(), ..Default::default() }; } @@ -2298,6 +2344,20 @@ impl LineColumnTracker { } cursor.offset = offset; + // A cursor that another text left does not agree with a scan from the start. + #[cfg(debug_assertions)] + if offset <= 4096 { + let place = |state: &ErrorPositionState| { + (state.line_start, state.line_count, state.column_number) + }; + let mut scanned = ErrorPositionState::default(); + scanned.advance(contents, 0, offset); + assert!( + place(&scanned) == place(&cursor.state), + "the text of this Source has changed" + ); + } + let line_end = match cursor.line_end { Some(line_end) => line_end, None => { @@ -3297,4 +3357,94 @@ mod line_column_tracker_tests { let bmp_pos = bmp_source.init_error_position(usize2loc(bmp.len() - 1)); assert_eq!(bmp_pos.column_count, 19); } + + #[test] + fn log_locates_each_file_that_one_read_buffer_holds() { + // One buffer holds each file in turn, so every `Source` has the same addresses and lengths. + let files: [(&[u8], usize, (i32, i32)); 3] = [ + (b"one\ntwo\nthree\nfour four", 19, (4, 6)), + (b"a single line, no break", 21, (1, 22)), + (b"\n\n\n\n\n\n\n\nlast line only!", 22, (9, 15)), + ]; + let path = b"package.json".to_vec(); + let mut buffer = vec![0u8; files[0].0.len()]; + let mut log = Log::init(); + let mut located = Vec::new(); + for (file, offset, _) in files { + buffer.copy_from_slice(file); + let source = Source::init_path_string(path.as_slice(), buffer.as_slice()); + log.add_warning_fmt_no_excerpt(&source, usize2loc(offset), format_args!("here")); + let location = log.msgs.last().unwrap().data.location.as_ref().unwrap(); + located.push((location.line, location.column)); + } + assert_eq!( + located, + files.map(|(_, _, line_and_column)| line_and_column) + ); + } + + #[test] + fn log_resumes_the_scan_for_a_source_and_its_borrowed_clone_only() { + let text = b"one\ntwo\nthree\nfour"; + let source = Source::init_path_string(b"a.js" as &[u8], &text[..]); + let mut log = Log::init(); + log.add_warning_fmt_no_excerpt(&source, usize2loc(5), format_args!("here")); + let tracker = log.line_column_tracker.as_deref().unwrap(); + assert!(tracker.matches(&source)); + assert!(tracker.matches(&source.clone())); + + // The same path and the same bytes at the same address, but not a copy of `source`. + assert!(!tracker.matches(&Source::init_path_string(b"a.js" as &[u8], &text[..]))); + + let mut edited = source.clone(); + edited.contents = Cow::Borrowed(b"one\ntwo\nthree\nfive"); + assert!(!tracker.matches(&edited)); + } + + #[test] + fn a_clone_with_its_own_copy_of_the_bytes_has_its_own_id() { + let owned = Source::init_path_string_owned(b"a.js" as &[u8], b"one\ntwo".to_vec()); + assert_ne!(owned.clone().id.0, owned.id.0); + let borrowed = Source::init_path_string(b"a.js" as &[u8], owned.contents()); + assert_eq!(borrowed.clone().id.0, borrowed.id.0); + } + + #[test] + #[cfg(debug_assertions)] + #[should_panic(expected = "the text of this Source has changed")] + fn a_debug_build_refuses_the_id_of_one_text_on_another() { + let mut buffer = b"one\ntwo\nthree".to_vec(); + let mut tracker = LineColumnTracker::default(); + let id = { + let source = Source::init_path_string(b"a.js" as &[u8], buffer.as_slice()); + tracker.error_position(&source, usize2loc(9)); + source.id + }; + buffer.copy_from_slice(b"a line, no br"); + let forged = Source { + id, + ..Source::init_path_string(b"a.js" as &[u8], buffer.as_slice()) + }; + tracker.error_position(&forged, usize2loc(11)); + } + + #[test] + fn source_ids_are_unique_across_threads_and_blocks() { + let draw = || { + let mut ids: Vec = (0..8).map(|_| SourceId::new().0).collect(); + // The last id of this thread's block, then the first id of the block it takes next. + NEXT_SOURCE_ID.set(ids[0].next_multiple_of(SourceId::BLOCK) - 1); + ids.extend([SourceId::new().0, SourceId::new().0]); + ids + }; + let mut ids = std::thread::spawn(draw).join().unwrap(); + ids.extend(draw()); + assert!( + ids.iter().all(|id| !id.is_multiple_of(SourceId::BLOCK)), + "{ids:?}" + ); + ids.sort_unstable(); + ids.dedup(); + assert_eq!(ids.len(), 20); + } } diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index c92db0195230..303577c92cfe 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -4215,14 +4215,7 @@ pub mod bv2_impl { let mut new_source = source_without_index; let source_index = self.graph.input_files.len(); new_source.index = bun_ast::Index(source_index as u32); - // `bun_ast::Source: !Clone` — manually dup the (all-Clone) fields. - let task_source = bun_ast::Source { - path: new_source.path, - contents: new_source.contents.clone(), - contents_is_recycled: new_source.contents_is_recycled, - identifier_name: new_source.identifier_name.clone(), - index: new_source.index, - }; + let task_source = new_source.clone(); self.graph.input_files.append(crate::Graph::InputFile { source: new_source, loader: Loader::Js, diff --git a/src/bundler/linker_context/generateCodeForFileInChunkJS.rs b/src/bundler/linker_context/generateCodeForFileInChunkJS.rs index 431592d8f91a..6b7e10133927 100644 --- a/src/bundler/linker_context/generateCodeForFileInChunkJS.rs +++ b/src/bundler/linker_context/generateCodeForFileInChunkJS.rs @@ -204,6 +204,7 @@ pub fn generate_code_for_file_in_chunk_js<'r, 'src>( contents_is_recycled: source_ref.contents_is_recycled, identifier_name: std::borrow::Cow::Borrowed(&source_ref.identifier_name), index: source_ref.index, + id: source_ref.id, }; &source_storage } else { diff --git a/src/install/npm.rs b/src/install/npm.rs index 78e9a5926797..72a180debc51 100644 --- a/src/install/npm.rs +++ b/src/install/npm.rs @@ -2447,11 +2447,7 @@ impl PackageManifest { // `initialize_mini_store` in lib.rs for why. let parsed = match JSON::ParsedJson::parse_npm_manifest(&source, log) { Ok(j) => j, - Err(_) => { - // It knows a source by its address, and `json_buffer` is about to be freed. - log.line_column_tracker = None; - return Ok(None); - } + Err(_) => return Ok(None), }; let json = parsed.root; diff --git a/src/resolver/package_json.rs b/src/resolver/package_json.rs index a51337546640..8df4901431cc 100644 --- a/src/resolver/package_json.rs +++ b/src/resolver/package_json.rs @@ -1050,6 +1050,7 @@ impl PackageJSON { contents_is_recycled: json_source.contents_is_recycled, identifier_name: json_source.identifier_name.clone(), index: json_source.index, + id: json_source.id, }; // See SAFETY note on `contents_static` above — move ownership of the // backing buffer into the returned struct (replaces the prior diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index 011399d13c7a..dd6e2368ffcb 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -24,6 +24,7 @@ import { toBeValidBin, toBeWorkspaceLink, toHaveBins, + withoutAsanQuarantine, } from "harness"; import { connect, createServer, type AddressInfo, type Socket as NetSocket } from "net"; import { basename, join, relative, resolve, sep } from "path"; @@ -781,6 +782,94 @@ describe.concurrent("bun-install", () => { }); }); + // bun locates what it finds in a file with a scan that goes on from its last finding. Each file below is as long + // as the one that bun read before it, at a path of the same length. + describe("the line and column in a file as long as the last one", () => { + // The package.json of every `file:` folder is read into one buffer. Each is 260 bytes here, and the finding is + // further into each file than into the one before. + // [name, line breaks before the finding, bytes of padding before it] + const folders = [ + ["a", 3, 0], + ["b", 0, 100], + ["c", 5, 120], + ["d", 0, 150], + ] as const; + const manifest = (name: string, lineBreaks: number, padding: number, finding: string) => { + const before = `{"name":"${name}","version":"1.0.0",${Buffer.alloc(lineBreaks, "\n")}"p":"${Buffer.alloc(padding, "x")}",`; + const after = `${finding},"q":"`; + return `${before}${after}${Buffer.alloc(260 - before.length - after.length - 2, "y")}"}`; + }; + const unnamed = `"dependencies":{"":"1.0.0"}`; + const duplicate = `"p":1`; + + it.each([ + ["an unnamed dependency", [unnamed, unnamed, unnamed, unnamed], ["a:4:24", "b:1:154", "c:6:144", "d:1:204"], 0], + ["a duplicate key", [duplicate, duplicate, duplicate, duplicate], ["a:4:8", "b:1:138", "c:6:128", "d:1:188"], 0], + ["a syntax error after a warning", [duplicate, "oops"], ["a:4:8", "b:1:138"], 1], + ] as const)("in a file: folder's package.json: %s", async (_, findings, places, exitCode) => { + await withContext(defaultOpts, async ctx => { + const names = folders.slice(0, findings.length).map(([name]) => name); + await Promise.all([ + writeProject(ctx, { + "package.json": { + name: "foo", + dependencies: Object.fromEntries(names.map(name => [name, `file:./${name}`])), + }, + }), + ...findings.map((finding, i) => + write(join(ctx.package_dir, names[i], "package.json"), manifest(...folders[i], finding)), + ), + ]); + + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "ignore", + stdin: "ignore", + stderr: "pipe", + env, + }); + const [err, exited] = await Promise.all([proc.stderr.text(), proc.exited]); + const located = Array.from( + err.matchAll(/[\\/]([a-d])[\\/]package\.json:(\d+:\d+)/g), + at => `${at[1]}:${at[2]}`, + ); + expect(located.sort()).toEqual([...places]); + expect(exited).toBe(exitCode); + }); + }); + + // The text of .bunfig.toml is freed before bunfig.toml is read, and a release build reads that to the same + // address. Each is 5000 bytes here. + it("in bunfig.toml after the global .bunfig.toml, with its token masked", async () => { + const token = "npm_" + "a1b2c3d4e5f6g7h8i9j0k1l2m3n4o5p6q7r8"; + const padded = (text: string) => `${text}${Buffer.alloc(5000 - text.length - 1, "#")}\n`; + using dir = tempDir("bunfig-as-long-as-the-global-one", { + "h/.bunfig.toml": padded(`#${Buffer.alloc(40, "g")}\n[macros]\n"./bad" = { x = "${Buffer.alloc(40, "y")}" }\n`), + "pp/bunfig.toml": padded(`[install.scopes]\n"@myorg" = { token = "${token}", url = 5 }\n`), + "pp/package.json": JSON.stringify({ name: "foo", version: "1.0.0" }), + }); + + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: join(String(dir), "pp"), + stdout: "ignore", + stdin: "ignore", + stderr: "pipe", + // ASAN's quarantine would keep the address of the first text out of use. + env: withoutAsanQuarantine({ ...env, XDG_CONFIG_HOME: join(String(dir), "h") }), + }); + const [err, exited] = await Promise.all([proc.stderr.text(), proc.exited]); + expect(err).not.toContain(token.slice(-8)); + expect(err).toContain(`2 | "@myorg" = { token = "${Buffer.alloc(token.length, "*")}", url = 5 }`); + expect(Array.from(err.matchAll(/[\\/](\.?bunfig\.toml):(\d+:\d+)/g), at => `${at[1]}:${at[2]}`)).toEqual([ + ".bunfig.toml:3:1", + "bunfig.toml:2:72", + ]); + expect(exited).toBe(1); + }); + }); + describe("chooses", () => { async function runTest(ctx: TestContext, latest: string, range: string, chosen = "0.0.5") { const exeName: string = {