From cd9e6b010b019e8b0ef20a5c699174fa46dd2e67 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 25 Oct 2026 08:57:21 +0000 Subject: [PATCH] bundler: warn about direct eval in ES modules, with one guard for weird-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. --- src/ast/lib.rs | 3 + src/js_parser/p.rs | 54 ++++++++ src/js_parser/visit/visit_expr.rs | 61 +++------ test/bundler/bun-build-api.test.ts | 99 +++++++++++++++ test/bundler/compile-argv.test.ts | 3 + test/bundler/esbuild/default.test.ts | 177 ++++++++++++++++++++++++++- 6 files changed, 353 insertions(+), 44 deletions(-) diff --git a/src/ast/lib.rs b/src/ast/lib.rs index f4c86093762d..012d4177e171 100644 --- a/src/ast/lib.rs +++ b/src/ast/lib.rs @@ -2068,6 +2068,9 @@ impl Log { notes: Box<[Data]>, args: fmt::Arguments<'_>, ) { + if !Kind::Warn.should_print(self.level) { + return; + } let text = alloc_print(args); self.add_formatted_msg(Kind::Warn, source, r, text, notes, false) } diff --git a/src/js_parser/p.rs b/src/js_parser/p.rs index d981eb71862d..f4b383d9cc97 100644 --- a/src/js_parser/p.rs +++ b/src/js_parser/p.rs @@ -1001,6 +1001,60 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O Ok(()) } + /// A visit-pass warning about valid but suspect code is silent inside node_modules, as in + /// esbuild, and on the minifier's second visit of a statement. + fn warns_about_weird_code(&self) -> bool { + !self.is_revisit_for_substitution + && !bun_paths::strings::path_contains_node_modules_folder(self.source.path.text) + } + + /// `problem` is the start of the sentence, such as "Writing to read-only method". + #[cold] + #[inline(never)] + pub(crate) fn warn_private_name( + &mut self, + problem: &'static str, + name: &[u8], + loc: bun_ast::Loc, + ) { + if !self.warns_about_weird_code() { + return; + } + + let r = bun_ast::Range { + loc, + len: i32::try_from(name.len()).expect("int cast"), + }; + self.log().add_range_warning_fmt( + Some(self.source), + r, + format_args!("{} \"{}\" will throw", problem, bstr::BStr::new(name)), + ); + } + + /// 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); + self.log().add_range_warning_fmt_with_notes( + Some(self.source), + r, + Box::new([bun_ast::range_data( + None, + bun_ast::Range::NONE, + "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)", + )]), + format_args!( + "Using direct eval with a bundler is not recommended and may cause problems" + ), + ); + } + // ─────────────────────────────────────────────────────────────────────── // Inlined ExpressionTransposer recursion. // Defined as `&mut self` methods so the only live `&mut` is the caller's diff --git a/src/js_parser/visit/visit_expr.rs b/src/js_parser/visit/visit_expr.rs index 61e97b7a79a0..aa2dce06ef25 100644 --- a/src/js_parser/visit/visit_expr.rs +++ b/src/js_parser/visit/visit_expr.rs @@ -941,49 +941,20 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O && (kind == js_ast::symbol::Kind::PrivateMethod || kind == js_ast::symbol::Kind::PrivateStaticMethod) { - let r = bun_ast::Range { - loc: e_.index.loc, - len: i32::try_from(name.len()).expect("int cast"), - }; - p.log().add_range_warning_fmt( - Some(p.source), - r, - format_args!( - "Writing to read-only method \"{}\" will throw", - BStr::new(name) - ), - ); + p.warn_private_name("Writing to read-only method", name, e_.index.loc); } else if in_.assign_target != js_ast::AssignTarget::None && (kind == js_ast::symbol::Kind::PrivateGet || kind == js_ast::symbol::Kind::PrivateStaticGet) { - let r = bun_ast::Range { - loc: e_.index.loc, - len: i32::try_from(name.len()).expect("int cast"), - }; - p.log().add_range_warning_fmt( - Some(p.source), - r, - format_args!( - "Writing to getter-only property \"{}\" will throw", - BStr::new(name) - ), - ); + p.warn_private_name("Writing to getter-only property", name, e_.index.loc); } else if in_.assign_target != js_ast::AssignTarget::Replace && (kind == js_ast::symbol::Kind::PrivateSet || kind == js_ast::symbol::Kind::PrivateStaticSet) { - let r = bun_ast::Range { - loc: e_.index.loc, - len: i32::try_from(name.len()).expect("int cast"), - }; - p.log().add_range_warning_fmt( - Some(p.source), - r, - format_args!( - "Reading from setter-only property \"{}\" will throw", - BStr::new(name) - ), + p.warn_private_name( + "Reading from setter-only property", + name, + e_.index.loc, ); } } @@ -1941,12 +1912,18 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O { e_.is_direct_eval = true; - // Pessimistically assume that if this looks like a CommonJS module - // (e.g. no "export" keywords), a direct call to "eval" means that - // code could potentially access "module" or "exports". - if p.options.bundle && !p.is_file_considered_to_have_esm_exports { - p.record_usage(p.module_ref); - p.record_usage(p.exports_ref); + if p.options.bundle { + // Pessimistically assume that if this looks like a CommonJS module + // (e.g. no "export" keywords), a direct call to "eval" means that + // code could potentially access "module" or "exports". + if !p.is_file_considered_to_have_esm_exports { + p.record_usage(p.module_ref); + p.record_usage(p.exports_ref); + } + + if p.is_file_considered_to_have_esm_exports || p.has_es_module_syntax { + p.warn_direct_eval(e_.target.loc); + } } // Walk `Scope.parent` (`Option>`) via the safe @@ -1957,8 +1934,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O scope.contains_direct_eval = true; scope_iter = scope.parent; } - - // TODO: Log a build note for this like esbuild does } } Data::EDot(dot) => { diff --git a/test/bundler/bun-build-api.test.ts b/test/bundler/bun-build-api.test.ts index c7ca18450820..5e142ffb2400 100644 --- a/test/bundler/bun-build-api.test.ts +++ b/test/bundler/bun-build-api.test.ts @@ -745,6 +745,105 @@ describe("Bun.build", () => { Bun.gc(true); }); + test("warns about a direct eval in an ES module", async () => { + const evalLine = `console.log(eval("secret + helper()"));`; + using dir = tempDir("bun-build-api-direct-eval", { + "esm.js": `import { helper } from "./lib.js";\nconst secret = 42;\n${evalLine}\n`, + "lib.js": `export function helper() { return 1; }\n`, + "commonjs.js": `module.exports = eval("1");\n`, + }); + // `notes` is not in the `BuildMessage` type. + const plain = (message: any) => ({ level: message.level, message: message.message, position: message.position }); + const logsOf = async (entry: string) => { + const build = await Bun.build({ entrypoints: [join(String(dir), entry)] }); + return { + success: build.success, + logs: build.logs.map((log: any) => ({ ...plain(log), notes: log.notes.map(plain) })), + }; + }; + + expect(await logsOf("esm.js")).toEqual({ + success: true, + logs: [ + { + level: "warn", + message: "Using direct eval with a bundler is not recommended and may cause problems", + position: { + file: expect.stringMatching(/esm\.js$/), + namespace: "file", + line: 3, + column: evalLine.indexOf("eval") + 1, + length: "eval".length, + offset: expect.any(Number), + lineText: evalLine, + }, + notes: [ + { + level: "note", + message: + '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)', + position: null, + }, + ], + }, + ], + }); + expect(await logsOf("commonjs.js")).toEqual({ success: true, logs: [] }); + }); + + test('logLevel = "error" in bunfig.toml drops the direct eval warning', async () => { + const project = { + "entry.js": `export const value = eval("1");\n`, + "build.js": ` + const build = await Bun.build({ entrypoints: ["./entry.js"] }); + console.log(JSON.stringify({ success: build.success, logs: build.logs.map(log => log.message) })); + `, + }; + using dir = tempDir("bun-build-api-direct-eval-log-level", { + default: project, + quiet: { ...project, "bunfig.toml": `logLevel = "error"\n` }, + }); + const build = async (project: string) => { + await using proc = Bun.spawn({ + cmd: [bunExe(), "build.js"], + env: bunEnv, + cwd: join(String(dir), project), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout: JSON.parse(stdout), stderr, exitCode }; + }; + + expect({ default: await build("default"), quiet: await build("quiet") }).toEqual({ + default: { + stdout: { + success: true, + logs: ["Using direct eval with a bundler is not recommended and may cause problems"], + }, + stderr: "", + exitCode: 0, + }, + quiet: { stdout: { success: true, logs: [] }, stderr: "", exitCode: 0 }, + }); + }); + + test("a build of an ES module bundle warns about a direct eval that came from node_modules", async () => { + using dir = tempDir("bun-build-api-direct-eval-second-build", { + "entry.js": `import { run } from "dep";\nexport const value = run("1");\n`, + "node_modules/dep/package.json": `{ "name": "dep", "type": "module", "main": "index.js" }`, + "node_modules/dep/index.js": `export function run(code) { return eval(code); }\n`, + }); + const messages = (build: Awaited>) => build.logs.map(log => log.message); + + const first = await Bun.build({ entrypoints: [join(String(dir), "entry.js")], outdir: join(String(dir), "dist") }); + const second = await Bun.build({ entrypoints: [join(String(dir), "dist", "entry.js")] }); + expect({ first: messages(first), second: messages(second) }).toEqual({ + first: [], + second: ["Using direct eval with a bundler is not recommended and may cause problems"], + }); + }); + test("Bun.write(BuildArtifact)", async () => { Bun.gc(true); const tmpdir = tempDirWithFiles("bun-build-api-write", { diff --git a/test/bundler/compile-argv.test.ts b/test/bundler/compile-argv.test.ts index cc25ff384511..90604426f08f 100644 --- a/test/bundler/compile-argv.test.ts +++ b/test/bundler/compile-argv.test.ts @@ -83,6 +83,9 @@ describe("bundler", () => { `, "/imported.ts": /* js */ `export default 2;`, }, + bundleWarnings: { + "/see.ts": ["Using direct eval with a bundler is not recommended and may cause problems"], + }, entryPointsRaw: ["./entry.ts", "./worker.ts"], outfile: "dist/out", run: { diff --git a/test/bundler/esbuild/default.test.ts b/test/bundler/esbuild/default.test.ts index aae64e643e2e..18fd4a185223 100644 --- a/test/bundler/esbuild/default.test.ts +++ b/test/bundler/esbuild/default.test.ts @@ -2152,6 +2152,180 @@ describe.concurrent("bundler", () => { assert(!text.includes("renameMe"), "Should have renamed all `renameMe` variabled"); }, }); + const directEvalWarning = "Using direct eval with a bundler is not recommended and may cause problems"; + // The CLI backend checks the warnings of every file exactly: a file that is + // not a key of `bundleWarnings` must print none. esbuild 0.25.10 gives the same counts. + itBundled("default/DirectEvalWarning", { + files: { + "/entry.js": /* js */ ` + import "./import-only.js"; + import "./export.js"; + import "./import-meta.js"; + import "./top-level-await.js"; + import "./no-syntax.mjs"; + import "./import-type.ts"; + import "./ts-empty-export.ts"; + import "./import.cjs"; + import "./three-calls.js"; + import "./esm-pkg/no-syntax.js"; + import "./esm-pkg/commonjs.js"; + import "./esm-pkg/legacy.cjs"; + import "./cjs-pkg/module.mjs"; + import "./cjs-pkg/script.js"; + import "./commonjs.js"; + import "./script.js"; + import "./import-equals.ts"; + import "./export-equals.ts"; + import "./dynamic-import.js"; + import "./indirect.js"; + import "pkg"; + import "single"; + `, + "/import-only.js": /* js */ ` + import { helper } from "./lib.js"; + const secret = 42; + console.log(eval("secret + helper()")); + `, + "/lib.js": `export function helper() { return 1; }`, + "/export.js": `export const value = eval("1");`, + "/import-meta.js": `globalThis.a = [import.meta.url, eval("1")];`, + "/top-level-await.js": `await 0; globalThis.b = eval("1");`, + "/no-syntax.mjs": `globalThis.c = eval("1");`, + "/import-type.ts": `import type { T } from "./types"; globalThis.d = eval("1");`, + "/types.ts": `export type T = number;`, + "/ts-empty-export.ts": `globalThis.e = eval("1");\nexport {};`, + "/import.cjs": `import "./lib.js"; globalThis.f = eval("1");`, + "/three-calls.js": /* js */ ` + export const top = eval("1"); + export function nested() { return eval("2"); } + if (false) eval("3"); + `, + // The "type" of the nearest package.json makes a file with no module syntax an ES module. + "/esm-pkg/package.json": `{ "type": "module" }`, + "/esm-pkg/no-syntax.js": `globalThis.g = eval("1");`, + "/esm-pkg/commonjs.js": `module.exports = eval("1");`, + "/esm-pkg/legacy.cjs": `globalThis.h = eval("1");`, + "/cjs-pkg/package.json": `{ "type": "commonjs" }`, + "/cjs-pkg/module.mjs": `globalThis.i = eval("1");`, + "/cjs-pkg/script.js": `globalThis.j = eval("1");`, + "/commonjs.js": `module.exports = eval("1");`, + "/script.js": `globalThis.k = eval("1");`, + "/import-equals.ts": `import lib = require("./commonjs.js");\nglobalThis.l = [lib, eval("1")];`, + "/export-equals.ts": `const value = eval("1");\nexport = value;`, + "/dynamic-import.js": `globalThis.m = [import("./lib.js"), eval("1")];`, + "/indirect.js": /* js */ ` + export const a = (0, eval)("1"); + export const b = eval?.("1"); + export const c = globalThis.eval("1"); + const alias = eval; + export const d = alias("1"); + export const e = new Function("return 1")(); + `, + "/node_modules/pkg/package.json": `{ "name": "pkg", "type": "module", "main": "index.js" }`, + "/node_modules/pkg/index.js": `export const value = eval("1");`, + "/node_modules/single.js": `export const value = eval("1");`, + }, + bundleWarnings: { + "/import-only.js": [directEvalWarning], + "/export.js": [directEvalWarning], + "/import-meta.js": [directEvalWarning], + "/top-level-await.js": [directEvalWarning], + "/no-syntax.mjs": [directEvalWarning], + "/import-type.ts": [directEvalWarning], + "/ts-empty-export.ts": [directEvalWarning], + "/import.cjs": [directEvalWarning], + "/three-calls.js": [directEvalWarning, directEvalWarning, directEvalWarning], + "/esm-pkg/no-syntax.js": [directEvalWarning], + "/esm-pkg/commonjs.js": [directEvalWarning], + "/cjs-pkg/module.mjs": [directEvalWarning], + }, + }); + // The two forms that the note of the warning names. Neither warns, and both + // keep their meaning when the minifier moves a single-use value. + itBundled("default/DirectEvalWarningRemedies", { + files: { + "/entry.js": /* js */ ` + import { helper } from "./lib.js"; + const secret = 42; + const viaFunction = new Function("secret", "helper", "return secret + helper()")(secret, helper); + function viaGlobalEval(code) { const result = globalThis.eval(code); return result + "!"; } + console.log(viaFunction, viaGlobalEval("typeof code")); + export const control = eval("1"); + `, + "/lib.js": `export function helper() { return 1; }`, + }, + minifySyntax: true, + bundleWarnings: { + "/entry.js": [directEvalWarning], + }, + onAfterBundle(api) { + api.expectFile("/out.js").toContain("globalThis.eval(code)"); + }, + run: { + stdout: "43 undefined!", + }, + }); + // The minifier moves a single-use value into the next statement and visits that + // statement again. The call it then sees is `eval(...)`, but the source has an + // indirect eval. Only the direct eval at the end warns. + itBundled("default/DirectEvalWarningIndirectEvalAfterInlining", { + files: { + "/entry.js": /* js */ ` + export function viaAlias(x) { const e = eval; return typeof e(x); } + export function viaComma(code) { const r = (0, eval)(code); return r + "!"; } + export const control = eval("1"); + `, + }, + minifySyntax: true, + bundleWarnings: { + "/entry.js": [directEvalWarning], + }, + }); + // `eval("require")` works in cjs output and warns. The note says to pass + // `require` as an argument, which works with no warning. + itBundled("default/DirectEvalWarningRequire", { + files: { + "/entry.js": /* js */ ` + import { helper } from "./lib.js"; + const viaEval = eval("require"); + const viaFunction = new Function("require", "return require")(require); + console.log(typeof viaEval, typeof viaFunction, helper()); + `, + "/lib.js": `export function helper() { return 1; }`, + }, + target: "node", + format: "cjs", + bundleWarnings: { + "/entry.js": [directEvalWarning], + }, + run: { + stdout: "function function 1", + }, + }); + // The minifier visits the statement that receives a single-use value twice. + // A warning about a private name must not print twice. + itBundled("default/PrivateNameWarningAfterInlining", { + files: { + "/entry.js": /* js */ ` + export class Foo { + #method() {} + get #getter() { return 1; } + set #setter(v) {} + writeMethod(v) { const value = v; return value + (this.#method = 1); } + writeGetter(v) { const value = v; return value + (this.#getter = 1); } + readSetter(v) { const value = v; return value + this.#setter; } + } + `, + }, + minifySyntax: true, + bundleWarnings: { + "/entry.js": [ + `Writing to read-only method "#method" will throw`, + `Writing to getter-only property "#getter" will throw`, + `Reading from setter-only property "#setter" will throw`, + ], + }, + }); itBundled("default/ImportReExportES6ESBuildIssue149", { files: { "/app.jsx": /* jsx */ ` @@ -4007,9 +4181,9 @@ describe.concurrent("bundler", () => { "/write-getter.js": `class Foo { get #foo() {} foo() { this.#foo = 123 } }`, "/read-setter.js": `class Foo { set #foo(x) {} foo() { return this.#foo } }`, "/delete-super.js": `class Foo extends Bar { foo() { delete super.foo } }`, + "/direct-eval.js": `export default eval("1")`, }; itBundled("default/WarningsInsideNodeModules", { - todo: true, files: { "/entry.js": Object.keys(WarningsInsideNodeModules) .map(file => `import "./${file}"; import "./node_modules/${file}"; import "@plugin/${file}"`) @@ -4025,6 +4199,7 @@ describe.concurrent("bundler", () => { bundleWarnings: { "/write-getter.js": [`Writing to getter-only property "#foo" will throw`], "/read-setter.js": [`Reading from setter-only property "#foo" will throw`], + "/direct-eval.js": [directEvalWarning], }, }); itBundled("default/RequireResolve", {