Repository navigation
Conversation
…rd-code warnings `bun build` printed nothing for a direct `eval()` call in a bundled ES module. The bundler can rename the top-level names of such a file and it replaces import bindings, so the code that the eval runs can fail. The direct-eval block of `e_call` now calls a cold helper, `P::warn_direct_eval`, when the file is bundled and is an ES module. It adds one warning on the `eval` token, with a note that names `globalThis.eval(code)` and `new Function`. esbuild uses the same rule for its `direct-eval` warning. `P::warns_about_weird_code` is the guard of the visit-pass warnings about suspect code: no warning inside node_modules, and none on the second visit that the minifier makes after it moves a single-use value. The three private-name warnings go through it too (`warn_private_name`). Before, they printed for node_modules files, and twice with `--minify-syntax`. `Log::add_range_warning_fmt_with_notes` now checks the log level, like the other warning helpers. `default/WarningsInsideNodeModules` is no longer `todo`. Three `compile-argv` test cases bundle a direct eval in an ES module. They now declare the warning.
|
Updated 9:58 AM PT - Oct 25th, 2026
✅ @robobun, your commit cd9e6b010b019e8b0ef20a5c699174fa46dd2e67 passed in 🧪 To try this PR locally: bunx bun-pr 46890That installs a local version of the PR into your bun-46890 --bun |
|
Status: draft until #41658 and #43313 merge. Without those printer fixes, a warning on a long line prints one space per column, and the whole line when it is near the end of the line. Both are rebased and mergeable. CI on this PR is green. One maintainer decision is also open (first bullet of Downsides). How to reproduce: // entry.js
import { helper } from "./lib.js";
const secret = 42;
console.log(eval("secret + helper()"));
// lib.js
export function helper() { return 1; }On main, |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Warning emission and private-name diagnostics src/ast/lib.rs, src/js_parser/p.rs, src/js_parser/visit/visit_expr.rs, test/bundler/esbuild/default.test.ts |
Range warnings return without formatting when warnings are hidden by the log level. Private-name warnings use a gate that suppresses warnings during substitution revisits and for paths under node_modules. The tests check that minification does not duplicate these warnings. |
Direct-eval diagnostics and bundler coverage src/js_parser/p.rs, src/js_parser/visit/visit_expr.rs, test/bundler/bun-build-api.test.ts, test/bundler/compile-argv.test.ts, test/bundler/esbuild/default.test.ts |
Bundled direct eval produces a warning for files considered to have ESM exports or ES module syntax. Tests cover module forms, eval forms, warning notes, log-level suppression, minification, and node_modules behavior. |
Possibly related PRs
- oven-sh/bun#20435: Adds a minification regression test for duplicate private-name warnings, related to this PR’s suppression of warnings on substitution revisits.
- oven-sh/bun#45063: Changes warning handling based on
logLevel, related to this PR’s range-warning log-level check.
Suggested reviewers: dylan-conway, alii
Merge Risk: 
cd9e6
-
The warning and its suppression behavior have no identified issue requiring a fix before merge.
Pre-merge checks: 
-
Check name Status Explanation Title check The title names the primary change: a bundler warning for direct eval in ES modules. It also names the guard for weird-code warnings, which the PR adds. The title is specific and matches the changeset… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description clearly explains the problem, implementation, expected behavior, downsides, and verification. It does not use the exact template headings, but the required content is present through t… Full details: Title check
-
Explanation
The title names the primary change: a bundler warning for direct eval in ES modules. It also names the guard for weird-code warnings, which the PR adds. The title is specific and matches the changeset. It is long at 85 characters, but it is not vague or unrelated.
Full details: Description check
-
Explanation
The description clearly explains the problem, implementation, expected behavior, downsides, and verification. It does not use the exact template headings, but the required content is present through the Problem, Fix, Verified, and Suites sections.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked a few things and ruled them out: every other add_*warning* method on Log in src/ast/lib.rs already carries the Kind::Warn.should_print guard, so the new guard on add_range_warning_fmt_with_notes closes the last gap rather than leaving siblings behind; the gate is_file_considered_to_have_esm_exports || has_es_module_syntax lines up with esbuild's isFileConsideredESM (export/TLA/package type, plus import statements and import.meta via esm_import_keyword, which is restored for import() and import x = require()); and un-todo'ing default/WarningsInsideNodeModules is consistent with Bun emitting no delete-super warning at all, so the exact per-file CLI check does not depend on an unported message. The default-on CLI warning is a user-visible output change that a maintainer should sign off on.
Extended reasoning...
The change adds esbuild's direct-eval bundler warning in the Rust parser, routes the three private-name warnings and the new warning through a shared node_modules/revisit gate, and adds a missing log-level guard; it touches no security-sensitive surface. Tests cover a per-file variant matrix on the CLI backend, the JS API, logLevel suppression, and the second-build node_modules case. Two confirmed findings are posted inline, and the new warning changes default bun build stderr for ESM files that call eval, so a human decision on that default is still needed.
| /// esbuild's `direct-eval` warning. The caller checks that the file is a bundled ES module. | ||
| #[cold] | ||
| #[inline(never)] | ||
| pub(crate) fn warn_direct_eval(&mut self, loc: bun_ast::Loc) { | ||
| if !self.warns_about_weird_code() { | ||
| return; | ||
| } | ||
|
|
||
| let r = js_lexer::range_of_identifier(self.source, loc); |
There was a problem hiding this comment.
🔴 Users who bundle a minified ES module that calls eval outside node_modules now get a stderr line of the whole minified source plus one space per column, per eval call. The new warning in warn_direct_eval at src/js_parser/p.rs:1043 carries a range on the source line, and the printer at src/ast/lib.rs:1052-1065 echoes the full line text and then pads to the column. Fix: make the warning safe on long lines before it ships, either by landing the printer cap the PR description names (#41658) first or by truncating lineText and the padding in write_format around a window of the column, so every range warning on a minified line stays bounded. The description calls this an ordering dependency, but nothing in this diff enforces it. [also at: src/js_parser/p.rs:1044 - Users who bundle a minified ES module that lives outside node_modules (a vendored .min.js, or the output of a first bun build --minify fed into a second build or --compile) now get a ~120-byte window of the line echoed to stderr once per direct eval, plus one space per column, where the base branch printed nothing.]
Why this was flagged
A project bundles a prebuilt, minified ESM file that is not under node_modules (for example a vendored lib.min.mjs, or a bundle output from bun build --minify re-bundled). The file has export or import and a direct eval( call at column N on a one-line source. visit_expr.rs:1924 now calls warn_direct_eval for every such call, and p.rs:1043 attaches a range on that line. Log printing at src/ast/lib.rs:1052-1065 writes the full line text and then write_n_bytes(to, b' ', line_offset_for_second_line), so stderr gets roughly N bytes of source plus N spaces per eval call, with no cap in this diff. On the base branch this file produces no warning so none of this is printed. The author's note says #41658 must merge first, but this checkout has no such cap, so merging in the other order ships the cost.
Verification: normal — acknowledged in diff: the bound is accurate for the padding, and nothing in this PR's own commits caps it. Trigger: bun build (CLI, logLevel >= warn) of a bundled ES module not under node_modules whose source is one long line with a direct eval( call. write_n_bytes(to, b' ', line_offset_for_second_line) (lib.rs:1065) then writes that many spaces.
There was a problem hiding this comment.
This is correct. It is the second bullet of Downsides.
The cost comes from the log printer and not from this warning. Location::init_or_null_impl cuts the excerpt to about 120 bytes but keeps the column of the whole line, and Data::write_format pads the caret to that column. It also keeps the whole line when the position is in its last 80 bytes. Each located diagnostic on a long line pays this on main. #41658 and #43313 are the fixes. Both need a rebase, and I asked for that.
I did not add a bound in this diff:
- A cap or a shortened location in
warn_direct_evalfixes one diagnostic and leaves the printer as it is. - A copy of the printer fix here competes with logger: indent the caret relative to the windowed line excerpt #41658.
To make the order a property of the PR and not only a sentence in its body, this PR is now a draft. It leaves draft when #41658 is merged. I leave this thread open until then.
There was a problem hiding this comment.
Correction to my reply above: this PR waits for #43313 too, not only for #41658. #41658 bounds the caret padding. #43313 bounds the excerpt when the position is in the last 80 bytes of the line.
Measured on main with a warning that exists today (a read of a setter-only private name) on a 200 KB line: 200,268 B of stderr in the middle of the line, 400,196 B at its end.
Both PRs are rebased and mergeable now. This PR stays a draft until both are merged.
Related to #2875
Problem
bun buildprints nothing for a directeval()in a bundled ES module. The file ofevaluses global scope instead of local scope #2875 builds with--minify, then throwsReferenceError: bar is not defined.e_callended in aTODO(src/js_parser/visit/visit_expr.rs:1961). esbuild printsUsing direct eval with a bundler is not recommended and may cause problemsthere.node_modulesfiles, and twice with--minify-syntax.Fix
P::warn_direct_eval. Its note namesglobalThis.eval(code)andnew Function.warns_about_weird_codekeeps these warnings out ofnode_modulesand off the second visit of the minifier. The private-name warnings use it too.test/bundler/esbuild/default.test.ts,test/bundler/bun-build-api.test.ts(9 cases, all fail on main), then all oftest/bundler/.Background
eval(x)is a direct eval and runsxin the scope of the call.globalThis.eval(x)is indirect and runs in the global scope.Downsides
logLevel = "error"hides it. A maintainer must decide if this default is acceptable.bun run+0 instructions,.text+768 B,visit_expr_in_outstack frame +16 B.Notes
Where this comes from. A maintainer asked for this warning at the
TODO. No issue asks for it. #2875 (closed) is the one report of the shape that it covers: its file still fails with--minifyon main, and #40518 left the top-level names of a flat ES module renameable on purpose, as in esbuild. #13194, #14891, #19908 and #25433 are CommonJS packages undernode_modules. They get no warning and this PR does not change them. #35961 was the first attempt at this line, with a debug-level note. The 90-day stale sweep closed it and no maintainer commented on it.The rule, next to esbuild. esbuild gives the message the warning level when
mode == bundle && isFileConsideredESM && !suppressWarningsAboutWeirdCode, and the debug level in all other cases. This PR emits the warning-level case only.isFileConsideredESMisis_file_considered_to_have_esm_exports || has_es_module_syntax.prepare_for_visit_passsets the two fields before the visit starts.suppress_warnings_about_weird_codeistrueat each place that builds parser options (Remove dead code from bun_bundler, the JS parser, the help command, and PackageJSONEditor #44687 deletes it). The guard tests the whole path withpath_contains_node_modules_folder.Path::is_node_module()looks only at the directory part, so it misses a file directly innode_modules/. Its 12 callers keep that gap.bun buildhas no flag that changes the level for one message.Counts compared with esbuild 0.25.10 (
--bundle --format=esm, the same in both): import only 1,export1,import.metaonly 1, top-level await only 1,.mjswith no ESM syntax 1,import typein.ts1,export {}after the eval in.ts1,importin.cjs1, three calls in one file (top level, nested function, dead branch) 3, a plain script under"type": "module"1,module.exports = eval()under"type": "module"1,.mjsunder"type": "commonjs"1. Zero in both: CommonJS-shaped, plain script,.cjsunder"type": "module",import x = require(),export =, a file with onlyimport(), indirect forms ((0, eval)(x),eval?.(x),globalThis.eval(x), an alias,new Function), a package undernode_modules, a file directly innode_modules/.Where Bun warns but keeps the names. Bun wraps a file that has
importbut no export in a CommonJS closure when it has a direct eval, and pins its top-level names. For such a file only the imports are at risk. For a file with onlyimport.metaor onlyimport type, and formodule.exports = eval()under"type": "module", nothing is at risk. The warning still prints there, as in esbuild. The note makes no claim about renamed names for this reason.The note.
Use globalThis.eval(code) when the code reads only global variables. To give the code a variable, an import or require from this file, pass it as an argument: new Function("name", code)(name). I ran the two forms with no flag,--minify-syntax,--minifyandbun run: no warning, andglobalThis.eval(code)stays indirect.(0, eval)(code)is not in the note: the second visit of the minifier printsconst r = (0, eval)(code); return r + "!"as a direct eval, on main too. #45507 removes the second visit and theis_revisit_for_substitutionfield. The PR that merges second drops the field from the guard. An indirect eval runs the code as a non-strict script, so itsvarand function declarations become globals. The note does not say that.What a direct eval costs when it works. It keeps every top-level declaration of its file in the bundle. A file with 200 unused functions and one exported function that evaluates a string: 7,655 B with
eval(code), 88 B withglobalThis.eval(code)(4,892 B and 58 B with--minify).Where the warning shows.
bun buildprints it, also with--compile.Bun.build()puts it inlogsand prints nothing.Bun.serveHTML routes and the dev server drop the log of a build that succeeds, as they do for each bundler warning. A file that is parsed for two targets (server entry and HTML import) gives the warning 2 times. A second build of an ES module bundle warns about a direct eval that came fromnode_modules(esbuild does the same). A test pins that.Each clause has a test that fails without it. I removed one clause at a time from a local build: each of the two ES module fields, the guard call in each helper, each of the two conditions of the guard, and the log level check.
Measurements. Release builds of the merge-base (179664e) and of this PR, linux x64.
f(1)and 26 -> 26 forabcd(1). Private-name read and write that do not warn: 25 -> 25 and 26 -> 26. Method: gdbstepi, 3 runs each.node_modulesES module direct eval: +131 instructions (6 -> 137), 1 cold call, 0 allocations.logs.lengthis 0, 1, 101 for 0, 1, 101 calls, and CLI stderr has the same count. 31 allocations per warning end to end (mimalloc entry points counted in gdb over a CLI build, 5 runs). A file with no direct eval: 9614 -> 9614 allocations (4 of 5 runs of the PR build, one run gave 10538).warn_direct_evalis 400 B andwarn_private_nameis 295 B, each at 1 address.visit_expr_in_outis 17881 -> 17610 B in each of its 2 instantiations. Framesub rsp0x238 -> 0x248.compile-argvcases).itBundledfixtures on the API backend that gain alogsentry which the harness does not check: 9.default/WarningsInsideNodeModulesis no longertodo.Self-review, what is not done.
Pand gives counts that differ from esbuild.ifafter the CommonJS block would avoid a one-hunk conflict with bundler: count a direct eval literal that names __dirname or __filename as a reference #45498. It costs 3 instructions for each direct eval onbun run(4 -> 7), so the block is nested.Suites. All of
test/bundler/,test/js/bun/module-graph/module-graph-compile.test.ts,test/js/node/disallow-code-generation-from-strings.test.ts,test/napi/napi.test.ts,test/cli/lint/glob.test.ts,test/js/node/module/node-module-module.test.js,test/js/node/worker_threads/worker_threads.test.tsandtest/regression/issue/07740.test.tson the release build. The changed test files on the debug build.Found on the way, not changed here.
BuildMessage.levelis"warn"at run time.packages/bun-types/globals.d.tsanddocs/bundler/index.mdxsay"warning", and the type has nonotes.test/bundler/bun-build-compile.test.ts:2141writesapp-with-resources(240 MB) into the working directory.bundler_bytecode_portable.test.ts"encoder output is identical on every platform" fails on a release build of main in my container.no test proof · iteration 6 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts