diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 77b4f33d014a..335ad86feb51 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -51,10 +51,46 @@ pub use bv2_impl::JSBundleCompletionTask; /// `jsc::api::JSBundler::FileMap` — re-exported from the canonical def below. pub use api::JSBundler::FileMap; +/// An onResolve answer, queued in `resolve_tasks_waiting_for_import_source_index`. #[derive(Clone, Copy)] -pub struct PendingImport { - pub(crate) to_source_index: Index, - pub(crate) import_record_index: u32, +pub enum PendingImport { + SourceIndex { + import_record_index: u32, + to_source_index: Index, + }, + /// `path` borrows bytes parked on `BundleV2::free_list`. + ExternalPath { + import_record_index: u32, + path: bun_paths::fs::Path<'static>, + }, +} + +impl PendingImport { + pub(crate) fn import_record_index(self) -> u32 { + match self { + Self::SourceIndex { + import_record_index, + .. + } + | Self::ExternalPath { + import_record_index, + .. + } => import_record_index, + } + } + + pub(crate) fn apply(self, import_record: &mut bun_ast::ImportRecord) { + match self { + Self::SourceIndex { + to_source_index, .. + } => import_record.source_index = to_source_index, + Self::ExternalPath { path, .. } => { + import_record.path = path; + // The path map pass may have matched the source specifier to a module. + import_record.source_index = Index::INVALID; + } + } + } } pub struct BundleV2<'a> { @@ -4706,27 +4742,24 @@ pub mod bv2_impl { } jsc_api::JSBundler::ResolveValue::Success(result) => { let mut out_source_index: Option = None; + // SAFETY: `result.{path,namespace}` are `Box<[u8]>`. Every arm below + // either moves both boxes into `this.free_list`, which outlives the + // graph, before it stores `path` (`!found_existing`, external), or + // drops them without storing `path` (`found_existing`, entry point). + // So the erased `'static` borrow is never read after the bytes are freed. + let (result_path_static, result_ns_static): (&'static [u8], &'static [u8]) = unsafe { + ( + &*std::ptr::from_ref::<[u8]>(result.path.as_ref()), + &*std::ptr::from_ref::<[u8]>(result.namespace.as_ref()), + ) + }; + let mut path = Fs::Path::init(result_path_static); + if result.namespace.is_empty() || result.namespace.as_ref() == b"file" { + path.namespace = b"file"; + } else { + path.namespace = result_ns_static; + } if !result.external { - // SAFETY: `result.{path,namespace}` are `Box<[u8]>` whose heap - // allocations are moved into `this.free_list` below (in the - // `!found_existing` branch) and thus outlive `BundleV2`. Erase - // to `'static` so `Fs::Path<'static>` can borrow them across - // `path_with_pretty_initialized` / `ParseTask`. In the `found_existing`/`external` - // branches `path` is dead before the boxes drop, so the dangling - // `'static` is never observed. - let (result_path_static, result_ns_static): (&'static [u8], &'static [u8]) = unsafe { - ( - &*std::ptr::from_ref::<[u8]>(result.path.as_ref()), - &*std::ptr::from_ref::<[u8]>(result.namespace.as_ref()), - ) - }; - let mut path = Fs::Path::init(result_path_static); - if result.namespace.is_empty() || result.namespace.as_ref() == b"file" { - path.namespace = b"file"; - } else { - path.namespace = result_ns_static; - } - // SAFETY: `GetOrPutResult` borrows `&mut this` for its whole // lifetime, blocking the `free_list`/`graph` accesses below. // Capture `value_ptr` as a raw ptr + `found_existing` and drop @@ -4846,23 +4879,32 @@ pub mod bv2_impl { drop(result.namespace); drop(result.path); } - } else { - if resolve.import_record.kind == ImportKind::EntryPointBuild { - let log = this.log_for_resolution_failures( - &resolve.import_record.source_file, - resolve.import_record.original_target.bake_graph(), - ); - log.add_error_fmt( - None, - bun_ast::Loc::EMPTY, - format_args!( - "The entry point {} cannot be marked as external", - bun_core::fmt::quote(&resolve.import_record.specifier), - ), - ); - } + } else if resolve.import_record.kind == ImportKind::EntryPointBuild { + let log = this.log_for_resolution_failures( + &resolve.import_record.source_file, + resolve.import_record.original_target.bake_graph(), + ); + log.add_error_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!( + "The entry point {} cannot be marked as external", + bun_core::fmt::quote(&resolve.import_record.specifier), + ), + ); drop(result.namespace); drop(result.path); + } else { + // Like esbuild, the external import is printed with the plugin's path. + this.free_list.push(result.namespace); + this.free_list.push(result.path); + this.apply_or_defer_pending_import( + resolve.import_record.importer_source_index, + PendingImport::ExternalPath { + import_record_index: resolve.import_record.import_record_index, + path: path_as_static(&path), + }, + ); } if let Some(source_index) = out_source_index { @@ -4878,29 +4920,13 @@ pub mod bv2_impl { .entry_point_original_names .put(source_index.get(), &resolve.import_record.specifier); } else { - let source_import_records = - &mut this.graph.ast.items_import_records_mut() - [resolve.import_record.importer_source_index as usize]; - if source_import_records.len() as u32 - <= resolve.import_record.import_record_index - { - let entry = this - .resolve_tasks_waiting_for_import_source_index - .get_or_put(resolve.import_record.importer_source_index) - .expect("oom"); - if !entry.found_existing { - *entry.value_ptr = Vec::new(); - } - let _ = entry.value_ptr.push(PendingImport { - to_source_index: source_index, + this.apply_or_defer_pending_import( + resolve.import_record.importer_source_index, + PendingImport::SourceIndex { import_record_index: resolve.import_record.import_record_index, - }); - } else { - let import_record: &mut ImportRecord = &mut source_import_records - .as_mut_slice() - [resolve.import_record.import_record_index as usize]; - import_record.source_index = source_index; - } + to_source_index: source_index, + }, + ); } } } @@ -6797,6 +6823,26 @@ pub mod bv2_impl { } impl<'a> BundleV2<'a> { + /// Defers to `patch_import_record_source_indices` until the importer's records are on the graph. + fn apply_or_defer_pending_import(&mut self, importer: IndexInt, pending: PendingImport) { + let import_records = &mut self.graph.ast.items_import_records_mut()[importer as usize]; + if let Some(import_record) = import_records + .as_mut_slice() + .get_mut(pending.import_record_index() as usize) + { + pending.apply(import_record); + return; + } + let entry = self + .resolve_tasks_waiting_for_import_source_index + .get_or_put(importer) + .expect("oom"); + if !entry.found_existing { + *entry.value_ptr = Vec::new(); + } + let _ = entry.value_ptr.push(pending); + } + /// Patch source_index on import records from pathToSourceIndexMap and /// resolve_tasks_waiting_for_import_source_index. Called after /// processResolveQueue has registered new modules. @@ -6815,22 +6861,30 @@ pub mod bv2_impl { || ctx.loader == Loader::Html || ctx.loader.is_css(); - if let Some(idx) = self + let pending = self .resolve_tasks_waiting_for_import_source_index .get_index(&ctx.source_index.get()) - { - let (_, value) = self - .resolve_tasks_waiting_for_import_source_index - .swap_remove_at(idx); - for to_assign in value.slice() { - if save_import_record_source_index - || input_file_loaders[to_assign.to_source_index.get() as usize].is_css() + .map(|idx| { + self.resolve_tasks_waiting_for_import_source_index + .swap_remove_at(idx) + .1 + }); + if let Some(pending) = &pending { + for to_assign in pending.slice() { + if let PendingImport::SourceIndex { + import_record_index, + to_source_index, + } = *to_assign { - import_records.as_mut_slice()[to_assign.import_record_index as usize] - .source_index = to_assign.to_source_index; + if save_import_record_source_index + || input_file_loaders[to_source_index.get() as usize].is_css() + { + to_assign.apply( + &mut import_records.as_mut_slice()[import_record_index as usize], + ); + } } } - drop(value); } // Inlined `self.path_to_source_index_map(ctx.target)` (== `&mut self.graph.build_graphs[target]`) @@ -6854,6 +6908,18 @@ pub mod bv2_impl { } } } + + // After the map pass: an external path must not be looked up as a module. + if let Some(pending) = pending { + for to_assign in pending.slice() { + if matches!(to_assign, PendingImport::ExternalPath { .. }) { + to_assign.apply( + &mut import_records.as_mut_slice() + [to_assign.import_record_index() as usize], + ); + } + } + } } fn generate_server_html_module( diff --git a/src/js/builtins/BundlerPlugin.ts b/src/js/builtins/BundlerPlugin.ts index 9f05e17b5988..28243b1634f1 100644 --- a/src/js/builtins/BundlerPlugin.ts +++ b/src/js/builtins/BundlerPlugin.ts @@ -441,16 +441,18 @@ export function runOnResolvePlugins(this: BundlerPlugin, specifier, inputNamespa throw new TypeError("onResolve plugins 'namespace' field must be a string if provided"); } + if (typeof external !== "boolean" && !$isUndefinedOrNull(external)) { + throw new TypeError('onResolve plugins "external" field must be boolean or unspecified'); + } + if (!path) { - continue; + if (external) path = inputPath; + else continue; } if (!userNamespace) { userNamespace = inputNamespace; } - if (typeof external !== "boolean" && !$isUndefinedOrNull(external)) { - throw new TypeError('onResolve plugins "external" field must be boolean or unspecified'); - } if (!external) { if (userNamespace === "file") { diff --git a/test/bundler/bundler_plugin.test.ts b/test/bundler/bundler_plugin.test.ts index 3e357e85c298..dfe98b2bc89a 100644 --- a/test/bundler/bundler_plugin.test.ts +++ b/test/bundler/bundler_plugin.test.ts @@ -441,6 +441,186 @@ describe("bundler", () => { }, }; }); + // https://github.com/oven-sh/bun/issues/2805 + itBundled("plugin/ResolveExternalRewritesPath", { + files: { + "index.ts": /* ts */ ` + import React from "react"; + import { createRoot } from "react-dom/client"; + export { h } from "preact"; + export * from "mobx"; + console.log(React, createRoot, await import("lodash")); + `, + }, + plugins(builder) { + builder.onResolve({ filter: /^(react|react-dom\/client|preact|mobx|lodash)$/ }, args => { + return { path: "https://esm.sh/" + args.path, external: true }; + }); + }, + onAfterBundle(api) { + const contents = api.readFile("/out.js"); + expect(contents).toContain(`from "https://esm.sh/react"`); + expect(contents).toContain(`from "https://esm.sh/react-dom/client"`); + expect(contents).toContain(`from "https://esm.sh/preact"`); + expect(contents).toContain(`from "https://esm.sh/mobx"`); + expect(contents).toContain(`import("https://esm.sh/lodash")`); + for (const original of ["react", "react-dom/client", "preact", "mobx", "lodash"]) { + expect(contents).not.toContain(`"${original}"`); + } + }, + }); + itBundled("plugin/ResolveExternalSamePath", { + files: { + "index.ts": /* ts */ ` + import React from "react"; + console.log(React); + `, + }, + plugins(builder) { + builder.onResolve({ filter: /^react$/ }, args => { + return { path: args.path, external: true }; + }); + }, + onAfterBundle(api) { + expect(api.readFile("/out.js")).toContain(`from "react"`); + }, + }); + // The path map is keyed by path text alone. Once the "virt" answer has put + // "react" in it, the map pass over late.js matches its import of "react" to the + // virt module before the plugin answers external for it. late.js is loaded + // only after the virt answer is queued, so that order is fixed. Released Bun + // keeps that match and prints `__INVALID__REF__` and `__toESM(, 1)`. + for (const [name, answer] of [ + ["WithPath", (args: { path: string }) => ({ path: args.path, external: true })], + ["WithoutPath", () => ({ external: true })], + ] as const) { + itBundled(`plugin/ResolveExternalClearsModuleMatchedBySpecifier${name}`, () => { + const virtAnswered = Promise.withResolvers(); + return { + files: { + "/entry.js": /* js */ ` + import { x } from "virt"; + import "./late.js"; + console.log(x); + `, + "/late.js": ``, + }, + plugins(builder) { + builder.onResolve({ filter: /^virt$/ }, () => { + virtAnswered.resolve(); + return { path: "react", namespace: "virt" }; + }); + builder.onLoad({ filter: /.*/, namespace: "virt" }, () => { + return { contents: `export const x = "x";`, loader: "js" }; + }); + builder.onResolve({ filter: /^react$/ }, answer); + builder.onLoad({ filter: /late\.js$/ }, async () => { + await virtAnswered.promise; + return { contents: `import React from "react"; console.log(React);`, loader: "js" }; + }); + }, + onAfterBundle(api) { + const contents = api.readFile("/out.js"); + expect(contents).toContain(`from "react"`); + expect(contents).toContain(`"x"`); + expect(contents).not.toContain(`__toESM(,`); + expect(contents).not.toContain(`__INVALID__REF__`); + }, + }; + }); + } + // A barrel's records are resolved and patched again each time a consumer + // un-defers one of them. The rewritten external must survive both: it must + // not be resolved as the vendored specifier, and its path must not be matched + // against the bundled "virt" module that shares the same path text. late.js is + // loaded only after both answers are queued, so its un-defer of `a` runs after + // the rewrite landed. + itBundled("plugin/ResolveExternalRewriteSurvivesBarrelRevisit", () => { + const reactAnswered = Promise.withResolvers(); + const virtAnswered = Promise.withResolvers(); + return { + files: { + "/entry.js": /* js */ ` + import { React, x } from "barrel"; + import "./late.js"; + console.log(React, x); + `, + "/late.js": ``, + "/node_modules/barrel/package.json": JSON.stringify({ + name: "barrel", + main: "./index.js", + sideEffects: false, + }), + "/node_modules/barrel/index.js": /* js */ ` + export { default as React } from "react"; + export { x } from "virt"; + export { a } from "./a.js"; + export { b } from "./b.js"; + `, + "/node_modules/barrel/a.js": `export const a = "a";`, + "/node_modules/barrel/b.js": `export const b = "b";`, + }, + plugins(builder) { + builder.onResolve({ filter: /^react$/ }, () => { + reactAnswered.resolve(); + return { path: "react-vendored", external: true }; + }); + builder.onResolve({ filter: /^virt$/ }, () => { + virtAnswered.resolve(); + return { path: "react-vendored", namespace: "virt" }; + }); + builder.onLoad({ filter: /.*/, namespace: "virt" }, () => { + return { contents: `export const x = "x";`, loader: "js" }; + }); + builder.onLoad({ filter: /late\.js$/ }, async () => { + await Promise.all([reactAnswered.promise, virtAnswered.promise]); + return { contents: `import { a } from "barrel"; console.log(a);`, loader: "js" }; + }); + }, + onAfterBundle(api) { + const contents = api.readFile("/out.js"); + expect(contents).toContain(`from "react-vendored"`); + expect(contents).toContain(`"x"`); + expect(contents).not.toContain(`"react"`); + }, + }; + }); + itBundled("plugin/ResolveExternalRewritesPathRequire", { + files: { + "index.ts": /* ts */ ` + const React = require("react"); + console.log(React); + `, + }, + format: "cjs", + plugins(builder) { + builder.onResolve({ filter: /^react$/ }, () => { + return { path: "./vendor/react.cjs", external: true }; + }); + }, + onAfterBundle(api) { + const contents = api.readFile("/out.js"); + expect(contents).toContain(`require("./vendor/react.cjs")`); + expect(contents).not.toContain(`require("react")`); + }, + }); + itBundled("plugin/ResolveExternalWithoutPath", { + files: { + "index.ts": /* ts */ ` + import lodash from "lodash"; + console.log(lodash); + `, + }, + plugins(builder) { + builder.onResolve({ filter: /^lodash$/ }, () => { + return { external: true }; + }); + }, + onAfterBundle(api) { + const contents = api.readFile("/out.js"); + expect(contents).toContain(`from "lodash"`); + }, + }); itBundled("plugin/ResolveOverrideFile", ({ root }) => { return { files: { diff --git a/test/regression/issue/29264.test.ts b/test/regression/issue/29264.test.ts index a2f1112758c4..8796398cc4e8 100644 --- a/test/regression/issue/29264.test.ts +++ b/test/regression/issue/29264.test.ts @@ -12,7 +12,13 @@ test("#29264 bundler survives external + missing imports in same file", { timeou { name: "mark-bare-external", setup(build) { - build.onResolve({ filter: /^[^.]/ }, () => ({ external: true })); + build.onResolve({ filter: /^[^.]/ }, args => { + if (args.kind === "entry-point-build") return; + if (args.path === "src") return { external: true }; + // "other": fall through to NoMatch -> run_resolver so the + // unchecked import_records[..] access there is still + // exercised against the error-path store (#29264). + }); }, }, ], @@ -27,6 +33,7 @@ test("#29264 bundler survives external + missing imports in same file", { timeou `, "index.js": /* js */ ` import "src"; + import "other"; import "./src"; `, }); @@ -43,11 +50,9 @@ test("#29264 bundler survives external + missing imports in same file", { timeou // Before the fix, the child crashed in Bun.build — segfault (release) or // index-out-of-bounds panic (debug/ASAN) — so "DONE:caught" never printed. - // We deliberately don't assert on the bare "src" import; whether the - // plugin's `{ external: true }` (with no `path`) falls through to a - // resolver error is plugin semantics, not what this test guards against. const combined = stdout + stderr; expect(combined).toContain("DONE:caught"); expect(combined).toContain('Could not resolve: "./src"'); + expect(combined).toContain('Could not resolve: "other"'); expect(exitCode).toBe(0); });