From d1b94d291d3abc411631607fcc72173d8d3acdee Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 03:43:37 +0000 Subject: [PATCH 1/6] bundler: dispatch plugin requests through the pointer the pass keeps Resolve::dispatch and Load::dispatch took &mut self, linked the request into Graph::outstanding_* through one reborrow of it and posted a second one (ptr::from_mut(self)) to the plugins' JS thread. The next dispatch writes the request's link through the list's pointer, which invalidates the posted one under the aliasing model before the answer is consumed through it; the &mut self argument also claims exclusive access for a call during which the JS thread may already be writing the request. Both functions now take the request as `this: *mut Self` and pass that same pointer to the list and to Task::init. The three callers bind the arena slot as a raw pointer; Resolve no longer needs a Default impl to pre-allocate the slot. The source lint checks the receiver shape and that the body links and posts the same pointer. --- src/bundler/bundle_v2.rs | 107 +++++---- ...undler-plugin-dispatch-raw-request.test.ts | 227 ++++++++++++++++++ 2 files changed, 284 insertions(+), 50 deletions(-) create mode 100644 test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 0af0e0ce265..8412fe04dde 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -1092,17 +1092,6 @@ pub mod bv2_impl { /// holds (`Graph::outstanding_resolves`); bundle thread only. pub(crate) outstanding: crate::Graph::OutstandingLink, } - impl Default for Resolve { - fn default() -> Self { - Self { - bv2: core::ptr::null_mut(), - import_record: MiniImportRecord::default(), - value: ResolveValue::Pending, - task: bun_event_loop::AnyTaskWithExtraContext::AnyTaskWithExtraContext::default(), - outstanding: Default::default(), - } - } - } impl bun_event_loop::Taskable for Resolve { const TAG: bun_event_loop::TaskTag = bun_event_loop::task_tag::BundleV2PluginResolve; @@ -1122,15 +1111,33 @@ pub mod bv2_impl { outstanding: Default::default(), } } - /// Hops to the JS thread to call the `onResolve` plugin chain — - /// unless the pass is already cancelled (that VM is stopping and - /// will never answer): then the request fails here and now. - pub(crate) fn dispatch(&mut self) { - // SAFETY: `bv2` is a valid backref set by `init`; plugins is - // Some (asserted by `enqueue_on_js_loop_for_plugins`). + /// Bundle thread. Hops to the JS thread to call the `onResolve` + /// plugin chain, unless the pass is already cancelled (that VM is + /// stopping and will never answer): then the request fails here + /// and now. + /// + /// Takes the pointer, not `&mut self`: the same pointer goes into + /// `Graph::outstanding_resolves` and into the task, because from + /// here on the request is reached through both. The next push + /// writes this request's link through the list's pointer and the + /// answer comes back through the task's; were those two reborrows + /// of a `&mut self`, that write would invalidate the task's + /// pointer under the aliasing model (Miri rejects the answer's + /// reborrow), and the `&mut self` itself would claim exclusivity + /// for a call during which the JS thread may already be writing + /// `value`. + /// + /// # Safety + /// `this` is an arena slot holding an `init`ed, not yet dispatched + /// request, and the caller does not use it afterwards: the pass + /// gets it back through the list or the answer. + pub(crate) unsafe fn dispatch(this: *mut Self) { + // SAFETY: `this` is live (fn contract) and this is the thread + // that owns the pass `bv2` (set by `init`) points at; plugins + // is Some (asserted by `enqueue_on_js_loop_for_plugins`). unsafe { - let bv2 = &mut *self.bv2; - bv2.graph.outstanding_resolves.push(self); + let bv2 = &mut *(*this).bv2; + bv2.graph.outstanding_resolves.push(this); if bv2.graph.cancelled { // Failed by `is_done` at the loop's top level (not // here, mid-caller); make sure it runs again. @@ -1138,7 +1145,7 @@ pub mod bv2_impl { return; } let task = bun_event_loop::ConcurrentTask::ConcurrentTask::create( - bun_event_loop::Task::init(std::ptr::from_mut::(self)), + bun_event_loop::Task::init(this), ); bv2.enqueue_on_js_loop_for_plugins(task); } @@ -1257,14 +1264,17 @@ pub mod bv2_impl { pub(crate) fn bake_graph(&self) -> crate::bake_types::Graph { self.parse_task().known_target.bake_graph() } - /// Hops to the JS thread to call the `onLoad` plugin chain — - /// unless the pass is already cancelled: see `Resolve::dispatch`. - pub(crate) fn dispatch(&mut self) { - // SAFETY: `bv2` is a valid backref; plugins is Some (asserted - // by `enqueue_on_js_loop_for_plugins`). + /// Bundle thread. Hops to the JS thread to call the `onLoad` + /// plugin chain, unless the pass is already cancelled. Pointer + /// receiver for the reason given on `Resolve::dispatch`. + /// + /// # Safety + /// As `Resolve::dispatch`. + pub(crate) unsafe fn dispatch(this: *mut Self) { + // SAFETY: as `Resolve::dispatch`. unsafe { - let bv2 = &mut *self.bv2; - bv2.graph.outstanding_loads.push(self); + let bv2 = &mut *(*this).bv2; + bv2.graph.outstanding_loads.push(this); if bv2.graph.cancelled { // Failed by `is_done` at the loop's top level (not // here, mid-caller); make sure it runs again. @@ -1273,7 +1283,7 @@ pub mod bv2_impl { } let concurrent_task = bun_event_loop::ConcurrentTask::ConcurrentTask::create( - bun_event_loop::Task::init(std::ptr::from_mut::(self)), + bun_event_loop::Task::init(this), ); bv2.enqueue_on_js_loop_for_plugins(concurrent_task); } @@ -5628,13 +5638,7 @@ pub mod bv2_impl { ); self.increment_scan_counter(); - // Arena-owned; the dispatch - // chain holds the raw `*mut Resolve` until the JS thread calls - // back, at which point the bundle pass is still alive. - // SAFETY: arena outlives the bundle pass. - let resolve: &mut jsc_api::JSBundler::Resolve = - self.arena_create(jsc_api::JSBundler::Resolve::default()); - *resolve = jsc_api::JSBundler::Resolve::init( + let resolve = jsc_api::JSBundler::Resolve::init( self, jsc_api::JSBundler::MiniImportRecord { kind: import_record.kind, @@ -5647,8 +5651,12 @@ pub mod bv2_impl { original_target, }, ); - - resolve.dispatch(); + // Arena-owned: the slot lives as long as the pass, which + // holds it in `outstanding_resolves` until the JS thread + // answers (or the pass is cancelled). + let resolve: *mut jsc_api::JSBundler::Resolve = self.arena_create(resolve); + // SAFETY: fresh slot, `init`ed above, not used again here. + unsafe { jsc_api::JSBundler::Resolve::dispatch(resolve) }; return true; } } @@ -5671,13 +5679,9 @@ pub mod bv2_impl { bstr::BStr::new(entry_point) ); - // Arena-owned. - // SAFETY: arena outlives the bundle pass. - let resolve: &mut jsc_api::JSBundler::Resolve = - self.arena_create(jsc_api::JSBundler::Resolve::default()); self.increment_scan_counter(); - *resolve = jsc_api::JSBundler::Resolve::init( + let resolve = jsc_api::JSBundler::Resolve::init( self, jsc_api::JSBundler::MiniImportRecord { kind: ImportKind::EntryPointBuild, @@ -5690,8 +5694,10 @@ pub mod bv2_impl { original_target: target, }, ); - - resolve.dispatch(); + // Arena-owned; see `enqueue_on_resolve_plugin_if_needed`. + let resolve: *mut jsc_api::JSBundler::Resolve = self.arena_create(resolve); + // SAFETY: fresh slot, `init`ed above, not used again here. + unsafe { jsc_api::JSBundler::Resolve::dispatch(resolve) }; return true; } } @@ -5748,12 +5754,13 @@ pub mod bv2_impl { bstr::BStr::new(&parse.path.namespace), bstr::BStr::new(&parse.path.text) ); - // Arena-owned; the dispatch - // chain holds the raw `*mut Load` until the JS thread calls back. - let load_val = jsc_api::JSBundler::Load::init(self, parse); - // SAFETY: arena outlives the bundle pass. - let load: &mut jsc_api::JSBundler::Load = self.arena_create(load_val); - load.dispatch(); + let load = jsc_api::JSBundler::Load::init(self, parse); + // Arena-owned: the slot lives as long as the pass, which + // holds it in `outstanding_loads` until the JS thread + // answers (or the pass is cancelled). + let load: *mut jsc_api::JSBundler::Load = self.arena_create(load); + // SAFETY: fresh slot, `init`ed above, not used again here. + unsafe { jsc_api::JSBundler::Load::dispatch(load) }; return true; } } diff --git a/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts b/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts new file mode 100644 index 00000000000..200c95e12fa --- /dev/null +++ b/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts @@ -0,0 +1,227 @@ +import { file } from "bun"; +import { expect, test } from "bun:test"; +import { realpathSync } from "fs"; +import path from "path"; +import { globAllSources } from "../../../scripts/glob-sources.ts"; + +// The bundler's plugin requests are handed to the plugins' thread by the +// pointer the pass keeps, never through a `self` receiver. +// +// `Resolve::dispatch` and `Load::dispatch` (src/bundler/bundle_v2.rs) each +// link one arena-allocated request into `Graph::outstanding_resolves` / +// `outstanding_loads` and post it to the JS thread that runs the plugins +// (`enqueue_on_js_loop_for_plugins`). From the moment the post lands, the +// request is used from two threads through two stored pointers: the JS thread +// writes its `value` (and posts it back) through the one in the task, while +// the bundle thread writes its `outstanding` link through the one in the list +// whenever a neighbouring request is pushed or unlinked, and fails the request +// through it if the pass is cancelled. Under the aliasing model that only +// works if the list and the task hold the same pointer. When the two +// functions took `&mut self`, the list got one reborrow of the receiver +// (`push(self)`) and the task another (`Task::init(ptr::from_mut(self))`): +// dispatching the next request wrote this one's link through the list's +// pointer, which invalidated the task's, and the answer was then consumed +// through the task's (Miri rejects that reborrow under Tree Borrows and under +// Stacked Borrows). The `&mut self` argument also asserted exclusive access +// for a call during which the other thread may already have been writing the +// request. (The free variant of the same mistake is +// self-receiver-reclaim.test.ts.) +// +// So the two functions take `this: *mut Self`, and the body passes `this` +// itself both to the list and to `Task::init`. Banned inside them: a `self` +// receiver, and forming a reference to the request (`&mut *this`, `&*this`, +// `ptr::from_mut` / `from_ref`), which is the same two-pointer shape spelled +// differently. The pass itself (`&mut *(*this).bv2`) is not covered: dispatch +// runs on the thread that owns it. +// +// The answer path (`on_resolve_async` / `on_load_async` and the thunks in +// src/runtime/api/JSBundler.rs) and `DeferredBatchTask::schedule` are +// separate populations and not covered here. + +const root = path.resolve(import.meta.dir, "..", "..", ".."); +const bundlerSources = globAllSources().rust.filter( + p => p.endsWith(".rs") && path.relative(root, p).replaceAll(path.sep, "/").startsWith("src/bundler/"), +); + +// Only scan files tracked in HEAD (a `git stash` round-trip can leave stray +// `.rs` files in the working tree; CI runs on a clean checkout). Same guard as +// dead-code-escapes.test.ts. +const tracked: Set | null = (() => { + const r = Bun.spawnSync({ + cmd: ["git", "-C", root, "ls-tree", "-r", "--name-only", "-z", "HEAD"], + stdout: "pipe", + stderr: "ignore", + }); + if (!r.success) return null; + return new Set(r.stdout.toString().split("\0").filter(Boolean)); +})(); + +const DISPATCH = /\bfn\s+dispatch\s*\(([^)]*)\)/g; +// `this: *mut Self` (any parameter name; `*mut Resolve` / `*mut Load` would do +// as well). Captures the name so the body checks can look for it. +const RAW_REQUEST_PARAM = /^\s*(\w+)\s*:\s*\*\s*mut\s+\w+\s*$/; + +function lineOf(text: string, index: number): number { + return text.slice(0, index).split("\n").length; +} + +/** The `{ .. }` block starting at the first `{` at or after `from`. */ +function blockAfter(text: string, from: number): { start: number; body: string } | null { + const start = text.indexOf("{", from); + if (start < 0) return null; + let depth = 0; + for (let i = start; i < text.length; i++) { + const c = text[i]; + if (c === "{") depth++; + else if (c === "}" && --depth === 0) return { start, body: text.slice(start, i + 1) }; + } + return null; +} + +/** Strip comments so prose about the banned shapes (this file's own header + * is paraphrased in the doc comments) does not count. Full-line comments + * first, keeping the newline so line numbers hold; then trailing ones. The + * bodies contain no string literals, which is what this would misread. */ +function stripComments(text: string): string { + return text.replace(/^[ \t]*\/\/.*$/gm, "").replace(/[ \t]\/\/.*$/gm, ""); +} + +/** Every `fn dispatch` in `text` and what is wrong with each (nothing, for a + * conforming one). */ +function audit(text: string): { line: number; complaints: string[] }[] { + const stripped = stripComments(text); + const out: { line: number; complaints: string[] }[] = []; + for (const m of stripped.matchAll(DISPATCH)) { + const line = lineOf(stripped, m.index); + const complaints: string[] = []; + const params = m[1]; + const raw = RAW_REQUEST_PARAM.exec(params); + if (/\bself\b/.test(params)) { + complaints.push(`takes a self receiver: \`${params.trim()}\``); + } else if (raw === null) { + complaints.push(`does not take the request as its one \`*mut\` parameter: \`${params.trim()}\``); + } + const block = blockAfter(stripped, m.index + m[0].length); + if (block === null) { + complaints.push("could not find the body"); + } else { + const { body } = block; + if (/\bself\b/.test(body)) complaints.push("body uses `self`"); + if (raw !== null) { + const name = raw[1]; + const reborrow = new RegExp(String.raw`&\s*(?:mut\s+)?\*\s*${name}\b`); + if (reborrow.test(body) || /\bfrom_(?:mut|ref)\b/.test(body)) { + complaints.push("body forms a reference to the request"); + } + const linked = new RegExp(String.raw`\boutstanding_\w+\s*\.\s*push\(\s*${name}\s*\)`); + if (!linked.test(body)) complaints.push(`body does not link \`${name}\` itself into the outstanding list`); + const posted = new RegExp(String.raw`\bTask::init\(\s*${name}\s*\)`); + if (!posted.test(body)) complaints.push(`body does not post \`${name}\` itself`); + } + } + out.push({ line, complaints }); + } + return out; +} + +const found: string[] = []; +const offenders: string[] = []; +let scanned = 0; +for (const abs of bundlerSources) { + const source = path.relative(root, abs).replaceAll(path.sep, "/"); + if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; + if (tracked !== null && !tracked.has(source)) continue; + scanned++; + for (const { line, complaints } of audit(await file(abs).text())) { + found.push(source); + for (const c of complaints) offenders.push(`${source}:${line}: ${c}`); + } +} + +test("scans the tracked sources of the bundler crate", () => { + // Guards against the tracked/realpath filters above over-firing and leaving + // nothing to scan, which would make the assertions below pass vacuously. + expect(scanned).toBeGreaterThan(0); +}); + +test("Resolve::dispatch and Load::dispatch are still where this lint looks for them", () => { + // If this changes, a request type was added, removed or moved: update this + // list (and the header) rather than the checks below. + expect(found).toEqual(["src/bundler/bundle_v2.rs", "src/bundler/bundle_v2.rs"]); +}); + +test("the audit recognizes the shapes it claims to", () => { + const conforming = ` + pub(crate) unsafe fn dispatch(this: *mut Self) { + // SAFETY: see above; \`&mut *this\` is mentioned here only in prose. + unsafe { + let bv2 = &mut *(*this).bv2; + bv2.graph.outstanding_resolves.push(this); + if bv2.graph.cancelled { + bv2.wake_own_loop(); + return; + } + let task = bun_event_loop::ConcurrentTask::ConcurrentTask::create( + bun_event_loop::Task::init(this), + ); + bv2.enqueue_on_js_loop_for_plugins(task); + } + }`; + expect(audit(conforming)).toEqual([{ line: 2, complaints: [] }]); + // Another parameter name and a spelled-out type are fine too. + expect(audit("unsafe fn dispatch(load: *mut Load) { outstanding_loads.push(load); Task::init(load); }")).toEqual([ + { line: 1, complaints: [] }, + ]); + + const complaintsOf = (snippet: string) => audit(snippet).flatMap(f => f.complaints); + + // The shape this lint was written against: the receiver gets reborrowed once + // for the list and once for the task. + expect( + complaintsOf(` + pub(crate) fn dispatch(&mut self) { + unsafe { + let bv2 = &mut *self.bv2; + bv2.graph.outstanding_loads.push(self); + let task = ConcurrentTask::create(Task::init(std::ptr::from_mut::(self))); + bv2.enqueue_on_js_loop_for_plugins(task); + } + }`), + ).toEqual(["takes a self receiver: `&mut self`", "body uses `self`"]); + expect(complaintsOf("fn dispatch(self: &mut Self) {}")).toContain("takes a self receiver: `self: &mut Self`"); + expect(complaintsOf("fn dispatch(this: &mut Self) {}")).toContain( + "does not take the request as its one `*mut` parameter: `this: &mut Self`", + ); + expect(complaintsOf("fn dispatch(this: *mut Self, bv2: &mut BundleV2) {}")).toContain( + "does not take the request as its one `*mut` parameter: `this: *mut Self, bv2: &mut BundleV2`", + ); + // Raw receiver, but the body reintroduces a second pointer derivation. + expect( + complaintsOf(` + unsafe fn dispatch(this: *mut Self) { + let this_ref = &mut *this; + bv2.graph.outstanding_resolves.push(this); + Task::init(this); + }`), + ).toEqual(["body forms a reference to the request"]); + expect( + complaintsOf(` + unsafe fn dispatch(this: *mut Self) { + bv2.graph.outstanding_resolves.push(this); + Task::init(std::ptr::from_mut::(&mut *this)); + }`), + ).toEqual(["body forms a reference to the request", "body does not post `this` itself"]); + expect( + complaintsOf(` + unsafe fn dispatch(this: *mut Self) { + let node = this.cast::(); + bv2.graph.outstanding_resolves.push(node); + Task::init(this); + }`), + ).toEqual(["body does not link `this` itself into the outstanding list"]); + expect(complaintsOf("unsafe fn dispatch(this: *mut Self)")).toContain("could not find the body"); +}); + +test("plugin requests are dispatched through the pointer the pass keeps", () => { + expect(offenders).toEqual([]); +}); From 4da690ddbc078776d6a17970638ca08cf962bce7 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 09:08:52 +0000 Subject: [PATCH 2/6] ci: retrigger From dc8b5bad9be03f525e6adafc45fbcf67a2bfb288 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 09:17:41 +0000 Subject: [PATCH 3/6] bundler: shorten the dispatch docs and drop the caller notes --- src/bundler/bundle_v2.rs | 36 ++++++++---------------------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 8412fe04dde..675a3cacf59 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -1111,26 +1111,15 @@ pub mod bv2_impl { outstanding: Default::default(), } } - /// Bundle thread. Hops to the JS thread to call the `onResolve` - /// plugin chain, unless the pass is already cancelled (that VM is - /// stopping and will never answer): then the request fails here - /// and now. - /// - /// Takes the pointer, not `&mut self`: the same pointer goes into - /// `Graph::outstanding_resolves` and into the task, because from - /// here on the request is reached through both. The next push - /// writes this request's link through the list's pointer and the - /// answer comes back through the task's; were those two reborrows - /// of a `&mut self`, that write would invalidate the task's - /// pointer under the aliasing model (Miri rejects the answer's - /// reborrow), and the `&mut self` itself would claim exclusivity - /// for a call during which the JS thread may already be writing - /// `value`. + /// Bundle thread: links the request into the pass and hops to the JS + /// thread for the `onResolve` chain (a cancelled pass only links it, + /// for `is_done` to fail). Pointer receiver because `this` itself + /// has to be what both the list and the task hold; the reasoning is + /// in test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts. /// /// # Safety - /// `this` is an arena slot holding an `init`ed, not yet dispatched - /// request, and the caller does not use it afterwards: the pass - /// gets it back through the list or the answer. + /// `this` is a fresh arena slot holding an `init`ed request, and the + /// caller does not use it again. pub(crate) unsafe fn dispatch(this: *mut Self) { // SAFETY: `this` is live (fn contract) and this is the thread // that owns the pass `bv2` (set by `init`) points at; plugins @@ -1264,9 +1253,7 @@ pub mod bv2_impl { pub(crate) fn bake_graph(&self) -> crate::bake_types::Graph { self.parse_task().known_target.bake_graph() } - /// Bundle thread. Hops to the JS thread to call the `onLoad` - /// plugin chain, unless the pass is already cancelled. Pointer - /// receiver for the reason given on `Resolve::dispatch`. + /// Bundle thread: as `Resolve::dispatch`, for the `onLoad` chain. /// /// # Safety /// As `Resolve::dispatch`. @@ -5651,9 +5638,6 @@ pub mod bv2_impl { original_target, }, ); - // Arena-owned: the slot lives as long as the pass, which - // holds it in `outstanding_resolves` until the JS thread - // answers (or the pass is cancelled). let resolve: *mut jsc_api::JSBundler::Resolve = self.arena_create(resolve); // SAFETY: fresh slot, `init`ed above, not used again here. unsafe { jsc_api::JSBundler::Resolve::dispatch(resolve) }; @@ -5694,7 +5678,6 @@ pub mod bv2_impl { original_target: target, }, ); - // Arena-owned; see `enqueue_on_resolve_plugin_if_needed`. let resolve: *mut jsc_api::JSBundler::Resolve = self.arena_create(resolve); // SAFETY: fresh slot, `init`ed above, not used again here. unsafe { jsc_api::JSBundler::Resolve::dispatch(resolve) }; @@ -5755,9 +5738,6 @@ pub mod bv2_impl { bstr::BStr::new(&parse.path.text) ); let load = jsc_api::JSBundler::Load::init(self, parse); - // Arena-owned: the slot lives as long as the pass, which - // holds it in `outstanding_loads` until the JS thread - // answers (or the pass is cancelled). let load: *mut jsc_api::JSBundler::Load = self.arena_create(load); // SAFETY: fresh slot, `init`ed above, not used again here. unsafe { jsc_api::JSBundler::Load::dispatch(load) }; From 03cac5410b718c65388f5fabfc441688a84c7d68 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:11:29 +0000 Subject: [PATCH 4/6] bundler: allocate, link and post plugin requests in one helper BundleV2::dispatch_plugin_request takes the request by value, parks it in the arena and hands the one resulting pointer to both the outstanding list and the task, so no unsafe is needed anywhere on the dispatch path and the two request types' dispatch fns only forward to it. OutstandingNode gains the accessor for the list a request type lives in. The lint now checks the helper's shape and that every dispatch forwards to it, and fails if the tree-wide publish lint still ratchets bundle_v2.rs. --- src/bundler/Graph.rs | 2 + src/bundler/bundle_v2.rs | 100 +++-- ...undler-plugin-dispatch-raw-request.test.ts | 355 +++++++++++------- 3 files changed, 264 insertions(+), 193 deletions(-) diff --git a/src/bundler/Graph.rs b/src/bundler/Graph.rs index aa13706b082..dc2bd2a6749 100644 --- a/src/bundler/Graph.rs +++ b/src/bundler/Graph.rs @@ -276,6 +276,8 @@ impl Default for OutstandingLink { } pub trait OutstandingNode: Sized { fn link(&mut self) -> &mut OutstandingLink; + /// The pass's list of outstanding requests of this type. + fn outstanding<'g>(graph: &'g mut Graph<'_>) -> &'g mut OutstandingList; } /// A bundle pass's outstanding plugin requests; single-threaded (bundle thread). pub struct OutstandingList { diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 675a3cacf59..41506d1a6d9 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -1111,33 +1111,9 @@ pub mod bv2_impl { outstanding: Default::default(), } } - /// Bundle thread: links the request into the pass and hops to the JS - /// thread for the `onResolve` chain (a cancelled pass only links it, - /// for `is_done` to fail). Pointer receiver because `this` itself - /// has to be what both the list and the task hold; the reasoning is - /// in test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts. - /// - /// # Safety - /// `this` is a fresh arena slot holding an `init`ed request, and the - /// caller does not use it again. - pub(crate) unsafe fn dispatch(this: *mut Self) { - // SAFETY: `this` is live (fn contract) and this is the thread - // that owns the pass `bv2` (set by `init`) points at; plugins - // is Some (asserted by `enqueue_on_js_loop_for_plugins`). - unsafe { - let bv2 = &mut *(*this).bv2; - bv2.graph.outstanding_resolves.push(this); - if bv2.graph.cancelled { - // Failed by `is_done` at the loop's top level (not - // here, mid-caller); make sure it runs again. - bv2.wake_own_loop(); - return; - } - let task = bun_event_loop::ConcurrentTask::ConcurrentTask::create( - bun_event_loop::Task::init(this), - ); - bv2.enqueue_on_js_loop_for_plugins(task); - } + /// Bundle thread: hands the request to the `onResolve` plugin chain. + pub(crate) fn dispatch(self, bv2: &mut BundleV2<'_>) { + bv2.dispatch_plugin_request(self); } pub fn run_on_js_thread(&mut self) { let kind = self.import_record.kind; @@ -1253,27 +1229,9 @@ pub mod bv2_impl { pub(crate) fn bake_graph(&self) -> crate::bake_types::Graph { self.parse_task().known_target.bake_graph() } - /// Bundle thread: as `Resolve::dispatch`, for the `onLoad` chain. - /// - /// # Safety - /// As `Resolve::dispatch`. - pub(crate) unsafe fn dispatch(this: *mut Self) { - // SAFETY: as `Resolve::dispatch`. - unsafe { - let bv2 = &mut *(*this).bv2; - bv2.graph.outstanding_loads.push(this); - if bv2.graph.cancelled { - // Failed by `is_done` at the loop's top level (not - // here, mid-caller); make sure it runs again. - bv2.wake_own_loop(); - return; - } - let concurrent_task = - bun_event_loop::ConcurrentTask::ConcurrentTask::create( - bun_event_loop::Task::init(this), - ); - bv2.enqueue_on_js_loop_for_plugins(concurrent_task); - } + /// Bundle thread: hands the request to the `onLoad` plugin chain. + pub(crate) fn dispatch(self, bv2: &mut BundleV2<'_>) { + bv2.dispatch_plugin_request(self); } pub fn run_on_js_thread(&mut self) { let is_server_side = self.bake_graph() != crate::bake_types::Graph::Client; @@ -1306,11 +1264,21 @@ pub mod bv2_impl { fn link(&mut self) -> &mut crate::Graph::OutstandingLink { &mut self.outstanding } + fn outstanding<'g>( + graph: &'g mut crate::Graph::Graph<'_>, + ) -> &'g mut crate::Graph::OutstandingList { + &mut graph.outstanding_loads + } } impl crate::Graph::OutstandingNode for Resolve { fn link(&mut self) -> &mut crate::Graph::OutstandingLink { &mut self.outstanding } + fn outstanding<'g>( + graph: &'g mut crate::Graph::Graph<'_>, + ) -> &'g mut crate::Graph::OutstandingList { + &mut graph.outstanding_resolves + } } } } @@ -1519,6 +1487,29 @@ pub mod bv2_impl { pub use super::{BakeOptions, BundleV2, PendingImport}; impl<'a> BundleV2<'a> { + /// Bundle thread: parks an onResolve / onLoad request in the arena, links it + /// into the pass and posts it to the plugins' thread (a cancelled pass only + /// links it, for `is_done` to fail). Taking the request by value makes the + /// pointer created here the only one: the list, the JS thread and the + /// answer all hold it. Guarded by bundler-plugin-dispatch-raw-request.test.ts. + pub(crate) fn dispatch_plugin_request(&mut self, request: T) + where + T: bun_event_loop::Taskable + crate::Graph::OutstandingNode, + { + let request: *mut T = self.arena_create(request); + T::outstanding(&mut self.graph).push(request); + if self.graph.cancelled { + // Failed by `is_done` at the loop's top level (not here, + // mid-caller); make sure it runs again. + self.wake_own_loop(); + return; + } + let task = bun_event_loop::ConcurrentTask::ConcurrentTask::create( + bun_event_loop::Task::init(request), + ); + self.enqueue_on_js_loop_for_plugins(task); + } + /// Folds the JS-loop lookup + enqueue so the bundler never dereferences /// `JSBundleCompletionTask` (its layout lives in `bun_runtime`); the /// `completion` handle carries the `&'static` vtable. @@ -5638,9 +5629,7 @@ pub mod bv2_impl { original_target, }, ); - let resolve: *mut jsc_api::JSBundler::Resolve = self.arena_create(resolve); - // SAFETY: fresh slot, `init`ed above, not used again here. - unsafe { jsc_api::JSBundler::Resolve::dispatch(resolve) }; + resolve.dispatch(self); return true; } } @@ -5678,9 +5667,7 @@ pub mod bv2_impl { original_target: target, }, ); - let resolve: *mut jsc_api::JSBundler::Resolve = self.arena_create(resolve); - // SAFETY: fresh slot, `init`ed above, not used again here. - unsafe { jsc_api::JSBundler::Resolve::dispatch(resolve) }; + resolve.dispatch(self); return true; } } @@ -5737,10 +5724,7 @@ pub mod bv2_impl { bstr::BStr::new(&parse.path.namespace), bstr::BStr::new(&parse.path.text) ); - let load = jsc_api::JSBundler::Load::init(self, parse); - let load: *mut jsc_api::JSBundler::Load = self.arena_create(load); - // SAFETY: fresh slot, `init`ed above, not used again here. - unsafe { jsc_api::JSBundler::Load::dispatch(load) }; + jsc_api::JSBundler::Load::init(self, parse).dispatch(self); return true; } } diff --git a/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts b/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts index 200c95e12fa..fe2d29ccebe 100644 --- a/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts +++ b/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts @@ -1,42 +1,41 @@ import { file } from "bun"; import { expect, test } from "bun:test"; -import { realpathSync } from "fs"; +import { existsSync, realpathSync } from "fs"; import path from "path"; import { globAllSources } from "../../../scripts/glob-sources.ts"; -// The bundler's plugin requests are handed to the plugins' thread by the -// pointer the pass keeps, never through a `self` receiver. +// A bundler plugin request (`Resolve` / `Load` in src/bundler/bundle_v2.rs) is +// known by exactly one pointer from the moment it is dispatched. // -// `Resolve::dispatch` and `Load::dispatch` (src/bundler/bundle_v2.rs) each -// link one arena-allocated request into `Graph::outstanding_resolves` / -// `outstanding_loads` and post it to the JS thread that runs the plugins -// (`enqueue_on_js_loop_for_plugins`). From the moment the post lands, the -// request is used from two threads through two stored pointers: the JS thread -// writes its `value` (and posts it back) through the one in the task, while -// the bundle thread writes its `outstanding` link through the one in the list -// whenever a neighbouring request is pushed or unlinked, and fails the request -// through it if the pass is cancelled. Under the aliasing model that only -// works if the list and the task hold the same pointer. When the two -// functions took `&mut self`, the list got one reborrow of the receiver -// (`push(self)`) and the task another (`Task::init(ptr::from_mut(self))`): -// dispatching the next request wrote this one's link through the list's -// pointer, which invalidated the task's, and the answer was then consumed -// through the task's (Miri rejects that reborrow under Tree Borrows and under -// Stacked Borrows). The `&mut self` argument also asserted exclusive access -// for a call during which the other thread may already have been writing the -// request. (The free variant of the same mistake is -// self-receiver-reclaim.test.ts.) +// Once dispatched, a request is linked in `Graph::outstanding_resolves` / +// `outstanding_loads` and posted to the thread that runs the plugins, and is +// used through both: the JS thread writes its `value` and posts it back +// through the task's pointer, while the bundle thread writes its +// `outstanding` link through the list's pointer whenever a neighbouring +// request is dispatched or answered, and fails it through the list's pointer +// if the pass is cancelled. Under the aliasing model the list and the task +// therefore have to hold the same pointer. `Resolve::dispatch(&mut self)` and +// `Load::dispatch(&mut self)` used to give them two reborrows of the receiver +// (`push(self)` and `Task::init(ptr::from_mut(self))`): dispatching the next +// request wrote this one's link through the first and invalidated the second, +// and the answer was then consumed through the second. Miri rejects that +// reborrow under Tree Borrows and under Stacked Borrows. // -// So the two functions take `this: *mut Self`, and the body passes `this` -// itself both to the list and to `Task::init`. Banned inside them: a `self` -// receiver, and forming a reference to the request (`&mut *this`, `&*this`, -// `ptr::from_mut` / `from_ref`), which is the same two-pointer shape spelled -// differently. The pass itself (`&mut *(*this).bv2`) is not covered: dispatch -// runs on the thread that owns it. +// So `BundleV2::dispatch_plugin_request` takes the request by value, parks it +// in the arena, and passes the one `*mut` it gets back both to the list and to +// `Task::init`; there is no other pointer for anything to hold. `Resolve:: +// dispatch` / `Load::dispatch` only forward to it. This lint checks both halves: +// the helper's body has that shape (one `arena_create` binding, the same local +// into `.push(..)` and `Task::init(..)`, no reference formed to it), and every +// `fn dispatch` in the crate takes `self` by value and forwards instead of +// linking or posting anything itself. // -// The answer path (`on_resolve_async` / `on_load_async` and the thunks in -// src/runtime/api/JSBundler.rs) and `DeferredBatchTask::schedule` are -// separate populations and not covered here. +// Not covered: the answer path (`on_resolve_async` / `on_load_async` and the +// thunks in src/runtime/api/JSBundler.rs) and `DeferredBatchTask::schedule`, +// which posts a field of the pass rather than an arena request. The tree-wide +// guard for posting `self` is self-receiver-publish.test.ts, whose ratchet +// entry for bundle_v2.rs this conversion retires; the last test below keeps +// the two from landing out of step. const root = path.resolve(import.meta.dir, "..", "..", ".."); const bundlerSources = globAllSources().rust.filter( @@ -56,75 +55,110 @@ const tracked: Set | null = (() => { return new Set(r.stdout.toString().split("\0").filter(Boolean)); })(); -const DISPATCH = /\bfn\s+dispatch\s*\(([^)]*)\)/g; -// `this: *mut Self` (any parameter name; `*mut Resolve` / `*mut Load` would do -// as well). Captures the name so the body checks can look for it. -const RAW_REQUEST_PARAM = /^\s*(\w+)\s*:\s*\*\s*mut\s+\w+\s*$/; +type Finding = { line: number; complaints: string[] }; function lineOf(text: string, index: number): number { return text.slice(0, index).split("\n").length; } /** The `{ .. }` block starting at the first `{` at or after `from`. */ -function blockAfter(text: string, from: number): { start: number; body: string } | null { +function blockAfter(text: string, from: number): string | null { const start = text.indexOf("{", from); if (start < 0) return null; let depth = 0; for (let i = start; i < text.length; i++) { const c = text[i]; if (c === "{") depth++; - else if (c === "}" && --depth === 0) return { start, body: text.slice(start, i + 1) }; + else if (c === "}" && --depth === 0) return text.slice(start, i + 1); } return null; } -/** Strip comments so prose about the banned shapes (this file's own header - * is paraphrased in the doc comments) does not count. Full-line comments - * first, keeping the newline so line numbers hold; then trailing ones. The - * bodies contain no string literals, which is what this would misread. */ +/** Strip comments (full-line ones first, keeping the newline so line numbers + * hold; then trailing ones) so the doc comments paraphrasing this header do + * not count. The bodies involved contain no string literals. */ function stripComments(text: string): string { return text.replace(/^[ \t]*\/\/.*$/gm, "").replace(/[ \t]\/\/.*$/gm, ""); } -/** Every `fn dispatch` in `text` and what is wrong with each (nothing, for a - * conforming one). */ -function audit(text: string): { line: number; complaints: string[] }[] { +// `fn dispatch_plugin_request(&mut self, request: T)`; the `<..>` may be +// absent or carry bounds, the `where` clause is outside the parens. +const HELPER = /\bfn\s+dispatch_plugin_request\b(?:<[^()]*?>)?\s*\(([^)]*)\)/g; +// `&mut self, : ` where the type is a plain path, i.e. the request +// is taken by value and not as a pointer or reference. +const HELPER_PARAMS = /^\s*&mut\s+self\s*,\s*(\w+)\s*:\s*[\w:]+\s*,?\s*$/; + +/** Every `dispatch_plugin_request` definition in `text`, audited. */ +function auditHelper(text: string): Finding[] { + const stripped = stripComments(text); + const out: Finding[] = []; + for (const m of stripped.matchAll(HELPER)) { + const complaints: string[] = []; + const params = HELPER_PARAMS.exec(m[1]); + if (params === null) complaints.push(`does not take the request by value: \`${m[1].trim()}\``); + const body = blockAfter(stripped, m.index + m[0].length); + if (body === null) { + complaints.push("could not find the body"); + } else if (params !== null) { + const request = params[1]; + const bindings = [ + ...body.matchAll( + new RegExp( + String.raw`\blet\s+(\w+)\s*(?::\s*\*\s*mut\s+[\w:]+\s*)?=\s*self\s*\.\s*arena_create\(\s*${request}\s*\)`, + "g", + ), + ), + ]; + const allocations = body.match(/\barena_create\(/g)?.length ?? 0; + if (allocations !== 1) { + complaints.push(`expected exactly one arena_create, found ${allocations}`); + } else if (bindings.length !== 1) { + complaints.push(`does not bind the arena slot of \`${request}\` as a raw pointer`); + } else { + const slot = bindings[0][1]; + if (!new RegExp(String.raw`\.push\(\s*${slot}\s*\)`).test(body)) + complaints.push(`\`${slot}\` is not what gets linked`); + if (!new RegExp(String.raw`\bTask::init\(\s*${slot}\s*\)`).test(body)) + complaints.push(`\`${slot}\` is not what gets posted`); + if (new RegExp(String.raw`&\s*(?:mut\s+)?\*\s*${slot}\b`).test(body) || /\bfrom_(?:mut|ref)\b/.test(body)) { + complaints.push(`forms a reference to \`${slot}\``); + } + } + } + out.push({ line: lineOf(stripped, m.index), complaints }); + } + return out; +} + +const DISPATCH = /\bfn\s+dispatch\s*\(([^)]*)\)/g; + +/** Every `fn dispatch` in `text`, audited: by-value `self`, and a body that + * forwards rather than linking or posting. */ +function auditDispatch(text: string): Finding[] { const stripped = stripComments(text); - const out: { line: number; complaints: string[] }[] = []; + const out: Finding[] = []; for (const m of stripped.matchAll(DISPATCH)) { - const line = lineOf(stripped, m.index); const complaints: string[] = []; const params = m[1]; - const raw = RAW_REQUEST_PARAM.exec(params); - if (/\bself\b/.test(params)) { - complaints.push(`takes a self receiver: \`${params.trim()}\``); - } else if (raw === null) { - complaints.push(`does not take the request as its one \`*mut\` parameter: \`${params.trim()}\``); + if (!/^\s*(?:mut\s+)?self\s*(?:,|$)/.test(params)) { + complaints.push(`does not take the request by value: \`${params.trim()}\``); } - const block = blockAfter(stripped, m.index + m[0].length); - if (block === null) { + const body = blockAfter(stripped, m.index + m[0].length); + if (body === null) { complaints.push("could not find the body"); } else { - const { body } = block; - if (/\bself\b/.test(body)) complaints.push("body uses `self`"); - if (raw !== null) { - const name = raw[1]; - const reborrow = new RegExp(String.raw`&\s*(?:mut\s+)?\*\s*${name}\b`); - if (reborrow.test(body) || /\bfrom_(?:mut|ref)\b/.test(body)) { - complaints.push("body forms a reference to the request"); - } - const linked = new RegExp(String.raw`\boutstanding_\w+\s*\.\s*push\(\s*${name}\s*\)`); - if (!linked.test(body)) complaints.push(`body does not link \`${name}\` itself into the outstanding list`); - const posted = new RegExp(String.raw`\bTask::init\(\s*${name}\s*\)`); - if (!posted.test(body)) complaints.push(`body does not post \`${name}\` itself`); - } + if (!/\.dispatch_plugin_request\(\s*self\s*\)/.test(body)) + complaints.push("does not forward to dispatch_plugin_request"); + if (/\.push\(|\bTask::init\(|\barena_create\(/.test(body)) + complaints.push("links, allocates or posts on its own"); } - out.push({ line, complaints }); + out.push({ line: lineOf(stripped, m.index), complaints }); } return out; } -const found: string[] = []; +const helpers: string[] = []; +const dispatches: string[] = []; const offenders: string[] = []; let scanned = 0; for (const abs of bundlerSources) { @@ -132,9 +166,14 @@ for (const abs of bundlerSources) { if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; if (tracked !== null && !tracked.has(source)) continue; scanned++; - for (const { line, complaints } of audit(await file(abs).text())) { - found.push(source); - for (const c of complaints) offenders.push(`${source}:${line}: ${c}`); + const content = await file(abs).text(); + for (const { line, complaints } of auditHelper(content)) { + helpers.push(source); + for (const c of complaints) offenders.push(`${source}:${line}: dispatch_plugin_request ${c}`); + } + for (const { line, complaints } of auditDispatch(content)) { + dispatches.push(source); + for (const c of complaints) offenders.push(`${source}:${line}: dispatch ${c}`); } } @@ -144,41 +183,85 @@ test("scans the tracked sources of the bundler crate", () => { expect(scanned).toBeGreaterThan(0); }); -test("Resolve::dispatch and Load::dispatch are still where this lint looks for them", () => { - // If this changes, a request type was added, removed or moved: update this - // list (and the header) rather than the checks below. - expect(found).toEqual(["src/bundler/bundle_v2.rs", "src/bundler/bundle_v2.rs"]); +test("the helper and the two request types' dispatch are where this lint looks for them", () => { + // If this changes, the helper moved or a request type was added, removed or + // moved: update this (and the header) rather than the checks below. + expect({ helpers, dispatches }).toEqual({ + helpers: ["src/bundler/bundle_v2.rs"], + dispatches: ["src/bundler/bundle_v2.rs", "src/bundler/bundle_v2.rs"], + }); }); -test("the audit recognizes the shapes it claims to", () => { - const conforming = ` - pub(crate) unsafe fn dispatch(this: *mut Self) { - // SAFETY: see above; \`&mut *this\` is mentioned here only in prose. - unsafe { - let bv2 = &mut *(*this).bv2; - bv2.graph.outstanding_resolves.push(this); - if bv2.graph.cancelled { - bv2.wake_own_loop(); - return; - } - let task = bun_event_loop::ConcurrentTask::ConcurrentTask::create( - bun_event_loop::Task::init(this), - ); - bv2.enqueue_on_js_loop_for_plugins(task); - } - }`; - expect(audit(conforming)).toEqual([{ line: 2, complaints: [] }]); - // Another parameter name and a spelled-out type are fine too. - expect(audit("unsafe fn dispatch(load: *mut Load) { outstanding_loads.push(load); Task::init(load); }")).toEqual([ - { line: 1, complaints: [] }, - ]); +test("the audits recognize the shapes they claim to", () => { + const complaintsOfHelper = (s: string) => auditHelper(s).flatMap(f => f.complaints); + const complaintsOfDispatch = (s: string) => auditDispatch(s).flatMap(f => f.complaints); - const complaintsOf = (snippet: string) => audit(snippet).flatMap(f => f.complaints); + const helper = (body: string, params = "&mut self, request: T") => + `pub(crate) fn dispatch_plugin_request(${params})\nwhere\n T: Taskable + OutstandingNode,\n{\n${body}\n}`; + const conformingBody = ` + // SAFETY-style prose mentioning &mut *request is stripped before auditing. + let request: *mut T = self.arena_create(request); + T::outstanding(&mut self.graph).push(request); + if self.graph.cancelled { + self.wake_own_loop(); + return; + } + let task = bun_event_loop::ConcurrentTask::ConcurrentTask::create( + bun_event_loop::Task::init(request), + ); + self.enqueue_on_js_loop_for_plugins(task);`; + expect(auditHelper(helper(conformingBody))).toEqual([{ line: 1, complaints: [] }]); + // A different local name and an untyped binding are fine. + expect( + complaintsOfHelper( + helper("let slot = self.arena_create(req); list(self).push(slot); Task::init(slot);", "&mut self, req: R"), + ), + ).toEqual([]); + // The request must arrive by value: a pointer or a reference means someone + // else already holds a pointer to it. + expect(complaintsOfHelper(helper(conformingBody, "&mut self, request: *mut T"))).toEqual([ + "does not take the request by value: `&mut self, request: *mut T`", + ]); + expect(complaintsOfHelper(helper(conformingBody, "&mut self, request: &mut T"))).toEqual([ + "does not take the request by value: `&mut self, request: &mut T`", + ]); + // Two derivations of the slot, in the spellings main used. + expect( + complaintsOfHelper( + helper(` + let slot: &mut T = self.arena_create(request); + T::outstanding(&mut self.graph).push(slot); + Task::init(std::ptr::from_mut::(slot));`), + ), + ).toEqual(["does not bind the arena slot of `request` as a raw pointer"]); + expect( + complaintsOfHelper( + helper(` + let slot: *mut T = self.arena_create(request); + let again: *mut T = self.arena_create(T::default()); + T::outstanding(&mut self.graph).push(slot); + Task::init(again);`), + ), + ).toEqual(["expected exactly one arena_create, found 2"]); + expect( + complaintsOfHelper( + helper(` + let slot: *mut T = self.arena_create(request); + let node = &mut *slot; + T::outstanding(&mut self.graph).push(node); + Task::init(std::ptr::from_mut::(node));`), + ), + ).toEqual(["`slot` is not what gets linked", "`slot` is not what gets posted", "forms a reference to `slot`"]); + expect(complaintsOfHelper("fn dispatch_plugin_request(&mut self, request: T)")).toEqual([ + "could not find the body", + ]); - // The shape this lint was written against: the receiver gets reborrowed once - // for the list and once for the task. expect( - complaintsOf(` + auditDispatch("pub(crate) fn dispatch(self, bv2: &mut BundleV2<'_>) {\n bv2.dispatch_plugin_request(self);\n}"), + ).toEqual([{ line: 1, complaints: [] }]); + // The shape this lint was written against. + expect( + complaintsOfDispatch(` pub(crate) fn dispatch(&mut self) { unsafe { let bv2 = &mut *self.bv2; @@ -187,41 +270,43 @@ test("the audit recognizes the shapes it claims to", () => { bv2.enqueue_on_js_loop_for_plugins(task); } }`), - ).toEqual(["takes a self receiver: `&mut self`", "body uses `self`"]); - expect(complaintsOf("fn dispatch(self: &mut Self) {}")).toContain("takes a self receiver: `self: &mut Self`"); - expect(complaintsOf("fn dispatch(this: &mut Self) {}")).toContain( - "does not take the request as its one `*mut` parameter: `this: &mut Self`", - ); - expect(complaintsOf("fn dispatch(this: *mut Self, bv2: &mut BundleV2) {}")).toContain( - "does not take the request as its one `*mut` parameter: `this: *mut Self, bv2: &mut BundleV2`", - ); - // Raw receiver, but the body reintroduces a second pointer derivation. - expect( - complaintsOf(` - unsafe fn dispatch(this: *mut Self) { - let this_ref = &mut *this; - bv2.graph.outstanding_resolves.push(this); - Task::init(this); - }`), - ).toEqual(["body forms a reference to the request"]); - expect( - complaintsOf(` - unsafe fn dispatch(this: *mut Self) { - bv2.graph.outstanding_resolves.push(this); - Task::init(std::ptr::from_mut::(&mut *this)); - }`), - ).toEqual(["body forms a reference to the request", "body does not post `this` itself"]); - expect( - complaintsOf(` - unsafe fn dispatch(this: *mut Self) { - let node = this.cast::(); - bv2.graph.outstanding_resolves.push(node); - Task::init(this); - }`), - ).toEqual(["body does not link `this` itself into the outstanding list"]); - expect(complaintsOf("unsafe fn dispatch(this: *mut Self)")).toContain("could not find the body"); + ).toEqual([ + "does not take the request by value: `&mut self`", + "does not forward to dispatch_plugin_request", + "links, allocates or posts on its own", + ]); + // Other receivers that hand out a second pointer. + expect(complaintsOfDispatch("fn dispatch(&self, bv2: &mut BundleV2) { bv2.dispatch_plugin_request(self) }")).toEqual([ + "does not take the request by value: `&self, bv2: &mut BundleV2`", + ]); + expect(complaintsOfDispatch("unsafe fn dispatch(this: *mut Self) { Task::init(this); }")).toEqual([ + "does not take the request by value: `this: *mut Self`", + "does not forward to dispatch_plugin_request", + "links, allocates or posts on its own", + ]); + // By value, but doing the work inline instead of through the helper. + expect(complaintsOfDispatch("fn dispatch(self, bv2: &mut BundleV2) { let p = bv2.arena_create(self); }")).toEqual([ + "does not forward to dispatch_plugin_request", + "links, allocates or posts on its own", + ]); }); -test("plugin requests are dispatched through the pointer the pass keeps", () => { +test("plugin requests are allocated, linked and posted in one place, through one pointer", () => { expect(offenders).toEqual([]); }); + +test("the tree-wide publish lint no longer ratchets bundle_v2.rs", async () => { + // self-receiver-publish.test.ts (the tree-wide guard against posting `self`) + // was written while Resolve::dispatch / Load::dispatch still did that, and + // allowlists bundle_v2.rs with an exact count; this conversion is what + // retires that entry. The two changes touch different files, so nothing + // else stops them from landing with the entry still in place, which would + // fail that lint's ratchet on main. Passes trivially while that lint does + // not exist in this checkout. + const publishLint = path.join(import.meta.dir, "self-receiver-publish.test.ts"); + if (!existsSync(publishLint)) return; + const entries = stripComments(await file(publishLint).text()).match( + /^[ \t]*["']src\/bundler\/bundle_v2\.rs["'][ \t]*:[ \t]*\d+/gm, + ); + expect(entries).toBeNull(); +}); From 0f43e9549247c5a21032fd0d6cd7f70e475e38a3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:21:26 +0000 Subject: [PATCH 5/6] bundler: shorter doc on dispatch_plugin_request --- src/bundler/bundle_v2.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 41506d1a6d9..98fc645b796 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -1488,10 +1488,9 @@ pub mod bv2_impl { impl<'a> BundleV2<'a> { /// Bundle thread: parks an onResolve / onLoad request in the arena, links it - /// into the pass and posts it to the plugins' thread (a cancelled pass only - /// links it, for `is_done` to fail). Taking the request by value makes the - /// pointer created here the only one: the list, the JS thread and the - /// answer all hold it. Guarded by bundler-plugin-dispatch-raw-request.test.ts. + /// into the pass and posts it to the plugins' thread. By value, so that the + /// pointer made here is the only one: the list, the JS thread and the answer + /// all hold it. pub(crate) fn dispatch_plugin_request(&mut self, request: T) where T: bun_event_loop::Taskable + crate::Graph::OutstandingNode, From 3fd2e18ba9ffe066e50378f4f95581c74267686f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 15:05:57 +0000 Subject: [PATCH 6/6] source-lints: key the plugin dispatch lint on linking, not on a method name Any push onto an outstanding list outside dispatch_plugin_request is now the offence, whatever the method is called, instead of auditing every fn named dispatch in the crate. The landing-order check against the publish lint's allowlist is dropped: that lint's own exact-count ratchet already fails in any tree that has both changes. --- ...undler-plugin-dispatch-raw-request.test.ts | 294 ++++++++---------- 1 file changed, 136 insertions(+), 158 deletions(-) diff --git a/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts b/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts index fe2d29ccebe..55521d8bf09 100644 --- a/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts +++ b/test/internal/source-lints/bundler-plugin-dispatch-raw-request.test.ts @@ -1,6 +1,6 @@ import { file } from "bun"; import { expect, test } from "bun:test"; -import { existsSync, realpathSync } from "fs"; +import { realpathSync } from "fs"; import path from "path"; import { globAllSources } from "../../../scripts/glob-sources.ts"; @@ -23,19 +23,17 @@ import { globAllSources } from "../../../scripts/glob-sources.ts"; // // So `BundleV2::dispatch_plugin_request` takes the request by value, parks it // in the arena, and passes the one `*mut` it gets back both to the list and to -// `Task::init`; there is no other pointer for anything to hold. `Resolve:: -// dispatch` / `Load::dispatch` only forward to it. This lint checks both halves: -// the helper's body has that shape (one `arena_create` binding, the same local -// into `.push(..)` and `Task::init(..)`, no reference formed to it), and every -// `fn dispatch` in the crate takes `self` by value and forwards instead of -// linking or posting anything itself. +// `Task::init`; there is no other pointer for anything to hold. This lint +// checks that the helper's body has that shape (one `arena_create` binding, +// the same local into `.push(..)` and `Task::init(..)`, no reference formed to +// it), and that nothing else in the crate links a request into an outstanding +// list: linking is what makes something a dispatched request, so a method +// that links its own receiver again, under whatever name, shows up here. // // Not covered: the answer path (`on_resolve_async` / `on_load_async` and the // thunks in src/runtime/api/JSBundler.rs) and `DeferredBatchTask::schedule`, -// which posts a field of the pass rather than an arena request. The tree-wide -// guard for posting `self` is self-receiver-publish.test.ts, whose ratchet -// entry for bundle_v2.rs this conversion retires; the last test below keeps -// the two from landing out of step. +// which posts a field of the pass rather than an arena request; the general +// "posting `self`" spelling is the tree-wide lint's business (#37723). const root = path.resolve(import.meta.dir, "..", "..", ".."); const bundlerSources = globAllSources().rust.filter( @@ -55,28 +53,26 @@ const tracked: Set | null = (() => { return new Set(r.stdout.toString().split("\0").filter(Boolean)); })(); -type Finding = { line: number; complaints: string[] }; - function lineOf(text: string, index: number): number { return text.slice(0, index).split("\n").length; } -/** The `{ .. }` block starting at the first `{` at or after `from`. */ -function blockAfter(text: string, from: number): string | null { +/** Bounds of the `{ .. }` block starting at the first `{` at or after `from`. */ +function blockAfter(text: string, from: number): { start: number; end: number } | null { const start = text.indexOf("{", from); if (start < 0) return null; let depth = 0; for (let i = start; i < text.length; i++) { const c = text[i]; if (c === "{") depth++; - else if (c === "}" && --depth === 0) return text.slice(start, i + 1); + else if (c === "}" && --depth === 0) return { start, end: i + 1 }; } return null; } /** Strip comments (full-line ones first, keeping the newline so line numbers * hold; then trailing ones) so the doc comments paraphrasing this header do - * not count. The bodies involved contain no string literals. */ + * not count. The code involved contains no string literals. */ function stripComments(text: string): string { return text.replace(/^[ \t]*\/\/.*$/gm, "").replace(/[ \t]\/\/.*$/gm, ""); } @@ -87,78 +83,64 @@ const HELPER = /\bfn\s+dispatch_plugin_request\b(?:<[^()]*?>)?\s*\(([^)]*)\)/g; // `&mut self, : ` where the type is a plain path, i.e. the request // is taken by value and not as a pointer or reference. const HELPER_PARAMS = /^\s*&mut\s+self\s*,\s*(\w+)\s*:\s*[\w:]+\s*,?\s*$/; +// Linking a request: a push onto one of the graph's lists, reached either as +// the field or through the `OutstandingNode::outstanding` accessor. +const LINK = /(?:\boutstanding_\w+|::outstanding\([^)]*\))\s*\.\s*push\(/g; + +type Audit = { + /** Each helper definition and what is wrong with it (nothing, if conforming). */ + helpers: { line: number; complaints: string[] }[]; + /** Lines of every link site, and of those outside a helper body. */ + links: number[]; + strayLinks: number[]; +}; -/** Every `dispatch_plugin_request` definition in `text`, audited. */ -function auditHelper(text: string): Finding[] { +function audit(text: string): Audit { const stripped = stripComments(text); - const out: Finding[] = []; + const out: Audit = { helpers: [], links: [], strayLinks: [] }; + const bodies: { start: number; end: number }[] = []; for (const m of stripped.matchAll(HELPER)) { const complaints: string[] = []; const params = HELPER_PARAMS.exec(m[1]); if (params === null) complaints.push(`does not take the request by value: \`${m[1].trim()}\``); - const body = blockAfter(stripped, m.index + m[0].length); - if (body === null) { + const bounds = blockAfter(stripped, m.index + m[0].length); + if (bounds === null) { complaints.push("could not find the body"); - } else if (params !== null) { - const request = params[1]; - const bindings = [ - ...body.matchAll( - new RegExp( - String.raw`\blet\s+(\w+)\s*(?::\s*\*\s*mut\s+[\w:]+\s*)?=\s*self\s*\.\s*arena_create\(\s*${request}\s*\)`, - "g", - ), - ), - ]; - const allocations = body.match(/\barena_create\(/g)?.length ?? 0; - if (allocations !== 1) { - complaints.push(`expected exactly one arena_create, found ${allocations}`); - } else if (bindings.length !== 1) { - complaints.push(`does not bind the arena slot of \`${request}\` as a raw pointer`); - } else { - const slot = bindings[0][1]; - if (!new RegExp(String.raw`\.push\(\s*${slot}\s*\)`).test(body)) - complaints.push(`\`${slot}\` is not what gets linked`); - if (!new RegExp(String.raw`\bTask::init\(\s*${slot}\s*\)`).test(body)) - complaints.push(`\`${slot}\` is not what gets posted`); - if (new RegExp(String.raw`&\s*(?:mut\s+)?\*\s*${slot}\b`).test(body) || /\bfrom_(?:mut|ref)\b/.test(body)) { - complaints.push(`forms a reference to \`${slot}\``); - } - } + } else { + bodies.push(bounds); + if (params !== null) complaints.push(...auditBody(stripped.slice(bounds.start, bounds.end), params[1])); } - out.push({ line: lineOf(stripped, m.index), complaints }); + out.helpers.push({ line: lineOf(stripped, m.index), complaints }); + } + for (const m of stripped.matchAll(LINK)) { + const line = lineOf(stripped, m.index); + out.links.push(line); + if (!bodies.some(b => m.index >= b.start && m.index < b.end)) out.strayLinks.push(line); } return out; } -const DISPATCH = /\bfn\s+dispatch\s*\(([^)]*)\)/g; - -/** Every `fn dispatch` in `text`, audited: by-value `self`, and a body that - * forwards rather than linking or posting. */ -function auditDispatch(text: string): Finding[] { - const stripped = stripComments(text); - const out: Finding[] = []; - for (const m of stripped.matchAll(DISPATCH)) { - const complaints: string[] = []; - const params = m[1]; - if (!/^\s*(?:mut\s+)?self\s*(?:,|$)/.test(params)) { - complaints.push(`does not take the request by value: \`${params.trim()}\``); - } - const body = blockAfter(stripped, m.index + m[0].length); - if (body === null) { - complaints.push("could not find the body"); - } else { - if (!/\.dispatch_plugin_request\(\s*self\s*\)/.test(body)) - complaints.push("does not forward to dispatch_plugin_request"); - if (/\.push\(|\bTask::init\(|\barena_create\(/.test(body)) - complaints.push("links, allocates or posts on its own"); - } - out.push({ line: lineOf(stripped, m.index), complaints }); +function auditBody(body: string, request: string): string[] { + const allocations = body.match(/\barena_create\(/g)?.length ?? 0; + if (allocations !== 1) return [`expected exactly one arena_create, found ${allocations}`]; + const binding = new RegExp( + String.raw`\blet\s+(\w+)\s*(?::\s*\*\s*mut\s+[\w:]+\s*)?=\s*self\s*\.\s*arena_create\(\s*${request}\s*\)`, + ).exec(body); + if (binding === null) return [`does not bind the arena slot of \`${request}\` as a raw pointer`]; + const slot = binding[1]; + const complaints: string[] = []; + if (!new RegExp(String.raw`\.push\(\s*${slot}\s*\)`).test(body)) + complaints.push(`\`${slot}\` is not what gets linked`); + if (!new RegExp(String.raw`\bTask::init\(\s*${slot}\s*\)`).test(body)) + complaints.push(`\`${slot}\` is not what gets posted`); + if (new RegExp(String.raw`&\s*(?:mut\s+)?\*\s*${slot}\b`).test(body) || /\bfrom_(?:mut|ref)\b/.test(body)) { + complaints.push(`forms a reference to \`${slot}\``); } - return out; + return complaints; } const helpers: string[] = []; -const dispatches: string[] = []; +let links = 0; const offenders: string[] = []; let scanned = 0; for (const abs of bundlerSources) { @@ -166,15 +148,14 @@ for (const abs of bundlerSources) { if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; if (tracked !== null && !tracked.has(source)) continue; scanned++; - const content = await file(abs).text(); - for (const { line, complaints } of auditHelper(content)) { + const result = audit(await file(abs).text()); + links += result.links.length; + for (const { line, complaints } of result.helpers) { helpers.push(source); for (const c of complaints) offenders.push(`${source}:${line}: dispatch_plugin_request ${c}`); } - for (const { line, complaints } of auditDispatch(content)) { - dispatches.push(source); - for (const c of complaints) offenders.push(`${source}:${line}: dispatch ${c}`); - } + for (const line of result.strayLinks) + offenders.push(`${source}:${line}: links a request outside dispatch_plugin_request`); } test("scans the tracked sources of the bundler crate", () => { @@ -183,23 +164,19 @@ test("scans the tracked sources of the bundler crate", () => { expect(scanned).toBeGreaterThan(0); }); -test("the helper and the two request types' dispatch are where this lint looks for them", () => { - // If this changes, the helper moved or a request type was added, removed or - // moved: update this (and the header) rather than the checks below. - expect({ helpers, dispatches }).toEqual({ - helpers: ["src/bundler/bundle_v2.rs"], - dispatches: ["src/bundler/bundle_v2.rs", "src/bundler/bundle_v2.rs"], - }); +test("there is one helper, and it is the one place a request gets linked", () => { + // If this changes, the helper moved or was split, or linking is spelled in a + // way LINK does not see: update this (and the header) rather than the ban + // below. `links` counting the helper's own push is what keeps the stray-link + // check from going vacuous. + expect({ helpers, links }).toEqual({ helpers: ["src/bundler/bundle_v2.rs"], links: 1 }); }); -test("the audits recognize the shapes they claim to", () => { - const complaintsOfHelper = (s: string) => auditHelper(s).flatMap(f => f.complaints); - const complaintsOfDispatch = (s: string) => auditDispatch(s).flatMap(f => f.complaints); - +test("the audit recognizes the shapes it claims to", () => { const helper = (body: string, params = "&mut self, request: T") => `pub(crate) fn dispatch_plugin_request(${params})\nwhere\n T: Taskable + OutstandingNode,\n{\n${body}\n}`; const conformingBody = ` - // SAFETY-style prose mentioning &mut *request is stripped before auditing. + // Prose mentioning &mut *request or outstanding_loads.push( is stripped first. let request: *mut T = self.arena_create(request); T::outstanding(&mut self.graph).push(request); if self.graph.cancelled { @@ -210,24 +187,33 @@ test("the audits recognize the shapes they claim to", () => { bun_event_loop::Task::init(request), ); self.enqueue_on_js_loop_for_plugins(task);`; - expect(auditHelper(helper(conformingBody))).toEqual([{ line: 1, complaints: [] }]); + expect(audit(helper(conformingBody))).toEqual({ + helpers: [{ line: 1, complaints: [] }], + links: [8], + strayLinks: [], + }); // A different local name and an untyped binding are fine. expect( - complaintsOfHelper( - helper("let slot = self.arena_create(req); list(self).push(slot); Task::init(slot);", "&mut self, req: R"), + audit( + helper( + "let slot = self.arena_create(req); T::outstanding(&mut self.graph).push(slot); Task::init(slot);", + "&mut self, req: R", + ), ), - ).toEqual([]); + ).toMatchObject({ helpers: [{ complaints: [] }], strayLinks: [] }); + + const complaintsOf = (snippet: string) => audit(snippet).helpers.flatMap(h => h.complaints); // The request must arrive by value: a pointer or a reference means someone // else already holds a pointer to it. - expect(complaintsOfHelper(helper(conformingBody, "&mut self, request: *mut T"))).toEqual([ + expect(complaintsOf(helper(conformingBody, "&mut self, request: *mut T"))).toEqual([ "does not take the request by value: `&mut self, request: *mut T`", ]); - expect(complaintsOfHelper(helper(conformingBody, "&mut self, request: &mut T"))).toEqual([ + expect(complaintsOf(helper(conformingBody, "&mut self, request: &mut T"))).toEqual([ "does not take the request by value: `&mut self, request: &mut T`", ]); // Two derivations of the slot, in the spellings main used. expect( - complaintsOfHelper( + complaintsOf( helper(` let slot: &mut T = self.arena_create(request); T::outstanding(&mut self.graph).push(slot); @@ -235,16 +221,7 @@ test("the audits recognize the shapes they claim to", () => { ), ).toEqual(["does not bind the arena slot of `request` as a raw pointer"]); expect( - complaintsOfHelper( - helper(` - let slot: *mut T = self.arena_create(request); - let again: *mut T = self.arena_create(T::default()); - T::outstanding(&mut self.graph).push(slot); - Task::init(again);`), - ), - ).toEqual(["expected exactly one arena_create, found 2"]); - expect( - complaintsOfHelper( + complaintsOf( helper(` let slot: *mut T = self.arena_create(request); let node = &mut *slot; @@ -252,61 +229,62 @@ test("the audits recognize the shapes they claim to", () => { Task::init(std::ptr::from_mut::(node));`), ), ).toEqual(["`slot` is not what gets linked", "`slot` is not what gets posted", "forms a reference to `slot`"]); - expect(complaintsOfHelper("fn dispatch_plugin_request(&mut self, request: T)")).toEqual([ - "could not find the body", - ]); + expect( + complaintsOf( + helper(` + let slot: *mut T = self.arena_create(request); + let again: *mut T = self.arena_create(T::default()); + T::outstanding(&mut self.graph).push(slot); + Task::init(again);`), + ), + ).toEqual(["expected exactly one arena_create, found 2"]); + expect(complaintsOf("fn dispatch_plugin_request(&mut self, request: T)")).toEqual(["could not find the body"]); + // main's shape: no helper, and each request type links itself. + const mainShape = ` + impl Resolve { + pub(crate) fn dispatch(&mut self) { + unsafe { + let bv2 = &mut *self.bv2; + bv2.graph.outstanding_resolves.push(self); + let task = ConcurrentTask::create(Task::init(std::ptr::from_mut::(self))); + bv2.enqueue_on_js_loop_for_plugins(task); + } + } + } + impl Load { + pub(crate) fn dispatch(&mut self) { + unsafe { (*self.bv2).graph.outstanding_loads.push(self) } + } + }`; + expect(audit(mainShape)).toEqual({ helpers: [], links: [6, 14], strayLinks: [6, 14] }); + // The helper present, plus a method of any name linking its receiver again, + // through either spelling of the list. expect( - auditDispatch("pub(crate) fn dispatch(self, bv2: &mut BundleV2<'_>) {\n bv2.dispatch_plugin_request(self);\n}"), - ).toEqual([{ line: 1, complaints: [] }]); - // The shape this lint was written against. + audit( + helper(conformingBody) + + ` + impl Load { + fn redispatch(&mut self, bv2: &mut BundleV2) { + Load::outstanding(&mut bv2.graph).push(self); + } + fn relink(&mut self, graph: &mut Graph) { + graph.outstanding_loads.push(self); + } + }`, + ), + ).toMatchObject({ strayLinks: [20, 23] }); + // Other uses of the lists are not links. expect( - complaintsOfDispatch(` - pub(crate) fn dispatch(&mut self) { - unsafe { - let bv2 = &mut *self.bv2; - bv2.graph.outstanding_loads.push(self); - let task = ConcurrentTask::create(Task::init(std::ptr::from_mut::(self))); - bv2.enqueue_on_js_loop_for_plugins(task); - } - }`), - ).toEqual([ - "does not take the request by value: `&mut self`", - "does not forward to dispatch_plugin_request", - "links, allocates or posts on its own", - ]); - // Other receivers that hand out a second pointer. - expect(complaintsOfDispatch("fn dispatch(&self, bv2: &mut BundleV2) { bv2.dispatch_plugin_request(self) }")).toEqual([ - "does not take the request by value: `&self, bv2: &mut BundleV2`", - ]); - expect(complaintsOfDispatch("unsafe fn dispatch(this: *mut Self) { Task::init(this); }")).toEqual([ - "does not take the request by value: `this: *mut Self`", - "does not forward to dispatch_plugin_request", - "links, allocates or posts on its own", - ]); - // By value, but doing the work inline instead of through the helper. - expect(complaintsOfDispatch("fn dispatch(self, bv2: &mut BundleV2) { let p = bv2.arena_create(self); }")).toEqual([ - "does not forward to dispatch_plugin_request", - "links, allocates or posts on its own", - ]); + audit(` + this.graph.outstanding_resolves.unlink(resolve); + while let Some(load) = self.graph.outstanding_loads.pop() {} + let mut load = self.outstanding_loads.head; + pub(crate) fn push(&mut self, node: *mut T) {} + pending.push(self);`), + ).toEqual({ helpers: [], links: [], strayLinks: [] }); }); test("plugin requests are allocated, linked and posted in one place, through one pointer", () => { expect(offenders).toEqual([]); }); - -test("the tree-wide publish lint no longer ratchets bundle_v2.rs", async () => { - // self-receiver-publish.test.ts (the tree-wide guard against posting `self`) - // was written while Resolve::dispatch / Load::dispatch still did that, and - // allowlists bundle_v2.rs with an exact count; this conversion is what - // retires that entry. The two changes touch different files, so nothing - // else stops them from landing with the entry still in place, which would - // fail that lint's ratchet on main. Passes trivially while that lint does - // not exist in this checkout. - const publishLint = path.join(import.meta.dir, "self-receiver-publish.test.ts"); - if (!existsSync(publishLint)) return; - const entries = stripComments(await file(publishLint).text()).match( - /^[ \t]*["']src\/bundler\/bundle_v2\.rs["'][ \t]*:[ \t]*\d+/gm, - ); - expect(entries).toBeNull(); -});