Repository navigation
bundler: run no macro in a bundle made only for its import graph (test --changed, --analyze, bun create <component>)
#46281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4109,12 +4109,20 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O | |
| pub(crate) fn process_import_statement( | ||
| &mut self, | ||
| stmt_: S::Import, | ||
| path: ParsedPath<'a>, | ||
| mut path: ParsedPath<'a>, | ||
| loc: bun_ast::Loc, | ||
| was_originally_bare_import: bool, | ||
| ) -> Result<Stmt, crate::Error> { | ||
| let is_macro = | ||
| let mut is_macro = | ||
| Self::ALLOW_MACROS && (path.is_macro || crate::Macro::is_macro_path(path.text)); | ||
| if is_macro && self.options.features.macro_imports_are_plain { | ||
| // `MacroContext::call` resolves the path without the `macro:` prefix, so the ordinary import gets that path. | ||
| path.text = path | ||
| .text | ||
| .strip_prefix(crate::Macro::NAMESPACE_WITH_COLON) | ||
| .unwrap_or(path.text); | ||
| is_macro = false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Users of Why this was flaggedA repository has Verification: The PR description's Downsides bullet and the new pinned test "a file that only the result of a macro names does not select the test" in test/cli/test/test-changed.test.ts assert |
||
| } | ||
|
Comment on lines
+4116
to
+4125
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
rg -nP -C4 '\bfn get_remap\b' --type=rust
rg -nP -C3 'macros|remap' src/bundler/ParseTask.rs src/bundler/transpiler.rs 2>/dev/null | head -80Repository: oven-sh/bun Length of output: 6474 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- process_import_statement and remap branch ---'
nl -ba src/js_parser/p.rs | sed -n '4040,4285p'
printf '%s\n' '--- scan-only parser setup ---'
nl -ba src/bundler/ParseTask.rs | sed -n '2470,2605p'
printf '%s\n' '--- macro remap definition and lookup ---'
nl -ba src/js_parser_jsc/Macro.rs | sed -n '1,135p'
printf '%s\n' '--- remap consumers in parser ---'
rg -n -F -e 'macro_remap' -e 'get_remap(' -e 'macro_.refs' -e 'IS_UNUSED' src/js_parser src/js_parser_jsc src/bundlerRepository: oven-sh/bun Length of output: 32530 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- remap consumers ---'
nl -ba src/js_parser/p.rs | sed -n '4285,4435p'
printf '%s\n' '--- visit-time macro handling ---'
nl -ba src/js_parser/visit/visit_expr.rs | sed -n '660,730p'
nl -ba src/js_parser/visit/visit_expr.rs | sed -n '2265,2340p'
printf '%s\n' '--- scan-only parse/visit call path ---'
rg -n -F -e 'scan_only' -e 'visit' src/bundler/ParseTask.rs src/bundler/bundle_v2.rs | head -120Repository: oven-sh/bun Length of output: 17812 🏁 Script executed: #!/bin/bash
set -e
nl -ba src/js_parser/visit/visit_expr.rs | sed -n '2333,2385p'
nl -ba src/js_parser/visit/visit_expr.rs | sed -n '720,770p'Repository: oven-sh/bun Length of output: 5241 Disable macro remapping in scan-only parses. When 🐛 Proposed fix- let macro_remap = if Self::ALLOW_MACROS {
+ let macro_remap = if Self::ALLOW_MACROS && !self.options.features.macro_imports_are_plain {
self.options
.macro_context
.as_deref()
.and_then(|ctx| ctx.get_remap(path.text))
} else {
None
};🤖 Prompt for AI Agents
Comment on lines
+4118
to
+4125
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 pre-existing, not blocking: Users with a bunfig Why this was flaggedA project has Verification: Pre-existing for the macro execution itself (the base already runs bunfig-remapped macros in the |
||
| let mut stmt = stmt_; | ||
| if is_macro { | ||
| let id = self.add_import_record(ImportKind::Stmt, path.loc, path.text); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Users whose project runs fine now get exit 1 from
bun install --analyze,bun add --analyzeandbun create <component>when a macro file reaches the link step. Withscan_only = fetcher.is_some()at src/bundler/bundle_v2.rs:4464 the macro's file becomes an ordinary module and is linked, so arequire()of a module that calls a macro whose file has a top-level await fails withThis require call is not allowed because the transitive dependency "macro.ts" contains a top-level await. Fix: for the dependency scan, read import records without running the linker's export and TLA validation on files that only a macro import reaches, or return fromgenerate_from_clibefore the link whenfetcheris set, while keeping the link forbun create <component>which reads exports.Why this was flagged
A user runs
bun add --analyze ./entry-point.tswhere entry-point.ts doesrequire("./calls.ts"), calls.ts importsonefrom./macro.tswith{ type: "macro" }, and macro.ts hasawait 0;at top level (test/cli/install/bun-add.test.ts). On the base branch the scan calls the macro at parse time, the macro file never joins the graph, the program runs, andbun add --analyzeexits 0 and addsbaz. After this change src/bundler/ParseTask.rs:2551 setsmacro_imports_are_plainbecause src/bundler/bundle_v2.rs:4464 setsscan_onlyfor everyfetcherbundle, so src/js_parser/p.rs:4124 records the macro file as an ordinary import;generate_from_clithen links the bundle beforeget_all_dependencies(src/bundler/bundle_v2.rs:4380) reads records, and the linker's TLA validation rejects the require withthe transitive dependency "macro.ts" contains a top-level await, exit 1, nothing added. No safeguard skips link validation for scan-only bundles; the only mitigation is that TypeScript importers trim unused imports, which does not cover the require/TLA case.Verification: src/bundler/bundle_v2.rs:4464
this.scan_only = fetcher.is_some();-> src/bundler/ParseTask.rs:2551 -> src/js_parser/p.rs:4118-4125 setsis_macro = false. generate_from_cli runslinker.link(...)at bundle_v2.rs:4527 before consultingfetcherat 4532, so the linker's TLA validation (src/bundler/LinkerContext.rs:2235) emits an error and the command exits 1.