diff --git a/src/bun.js/bindings/ErrorCode.ts b/src/bun.js/bindings/ErrorCode.ts index d721b2a4494..0a97cd691e8 100644 --- a/src/bun.js/bindings/ErrorCode.ts +++ b/src/bun.js/bindings/ErrorCode.ts @@ -216,6 +216,7 @@ const errors: ErrorCodeMapping = [ ["ERR_MYSQL_LIFETIME_TIMEOUT", Error, "MySQLError"], ["ERR_UNHANDLED_REJECTION", Error, "UnhandledPromiseRejection"], ["ERR_REQUIRE_ASYNC_MODULE", Error], + ["ERR_REQUIRE_CYCLE_MODULE", Error], ["ERR_S3_INVALID_ENDPOINT", Error], ["ERR_S3_INVALID_METHOD", Error], ["ERR_S3_INVALID_PATH", Error], diff --git a/src/bun.js/bindings/ZigGlobalObject.cpp b/src/bun.js/bindings/ZigGlobalObject.cpp index d4a2441587a..e0679844432 100644 --- a/src/bun.js/bindings/ZigGlobalObject.cpp +++ b/src/bun.js/bindings/ZigGlobalObject.cpp @@ -740,8 +740,40 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob bool entryExistedBefore = false; if (auto* entry = loader->registryEntry(key)) { entryExistedBefore = true; - if (isModuleEvaluated(entry->record())) { - auto* ns = entry->record()->getModuleNamespace(globalObject, false); + auto* record = entry->record(); + if (isModuleEvaluated(record)) { + auto* ns = record->getModuleNamespace(globalObject, false); + RETURN_IF_EXCEPTION(scope, {}); + return JSValue::encode(ns); + } + // The record already exists at status Evaluating, i.e. it is on an + // outer InnerModuleEvaluation stack that we are re-entering. Calling + // loadModuleSync here would reach CyclicModuleRecord::evaluate() and + // violate its precondition (status must be LINKED, EVALUATING-ASYNC, + // or EVALUATED), so handle this case directly instead. + if (auto* cyclic = dynamicDowncast(record); cyclic && cyclic->status() == JSC::CyclicModuleRecord::Status::Evaluating) { + // ExecuteModule sets the generator State field from Init -> Executing + // before entering the body. State == Init therefore means the body + // has not started: the module is still in the requested-modules + // loop of InnerModuleEvaluation, and let/const/class exports are + // TDZ. Returning the namespace here is the regression that surfaces + // as a confusing "x is not a function" far from the cycle. Match + // Node's ERR_REQUIRE_CYCLE_MODULE instead. + JSValue state = record->internalField(JSC::AbstractModuleRecord::Field::State).get(); + if (state.isInt32() && state.asInt32() == static_cast(JSC::AbstractModuleRecord::State::Init)) + return Bun::throwError(globalObject, scope, Bun::ErrorCode::ERR_REQUIRE_CYCLE_MODULE, makeString("Cannot require() ES Module "_s, keyString, " in a cycle."_s)); + // State != Init: ExecuteModule has been entered. For a non-TLA + // record that means the body ran to completion synchronously + // (status only flips to Evaluated once the SCC root pops the + // Tarjan stack). The namespace is fully populated; return it + // without re-driving the loader. For a TLA record reachable here + // the body is suspended at its first await (status flips to + // EvaluatingAsync only when the SCC root pops), so post-await + // bindings are TDZ — treat that the same as the EvaluatingAsync + // path below and surface the async-module error. + if (cyclic->hasTLA()) + return throwVMTypeError(globalObject, scope, makeString("require() async module \""_s, keyString, "\" is unsupported. use \"await import()\" instead."_s)); + auto* ns = record->getModuleNamespace(globalObject, false); RETURN_IF_EXCEPTION(scope, {}); return JSValue::encode(ns); } diff --git a/src/js/builtins/CommonJS.ts b/src/js/builtins/CommonJS.ts index c038fdaf0c8..abfdac8da62 100644 --- a/src/js/builtins/CommonJS.ts +++ b/src/js/builtins/CommonJS.ts @@ -103,16 +103,18 @@ export function overridableRequire(this: JSCommonJSModule, originalId: string, o // -1 means we need to lookup the module from the ESM registry. if (out === -1) { + let namespace; try { - out = $requireESM(id); + namespace = $requireESM(id); } catch (exception) { // Since the ESM code is mostly JS, we need to handle exceptions here. $requireMap.$delete(id); throw exception; } - // If we can pull out a ModuleNamespaceObject, let's do it. - const namespace = $esmNamespaceForCjs(id); + // $requireESM returns the namespace object directly (it throws on + // undefined). Use that — re-reading via $esmNamespaceForCjs would miss the + // Evaluating-but-body-ran case that $esmLoadSync handles. if (namespace !== undefined) { // In Bun, when __esModule is not defined, it's a CustomAccessor on the prototype. // Various libraries expect __esModule to be set when using ESM from require(). @@ -324,16 +326,18 @@ export function requireESM(this, resolved: string) { export function requireESMFromHijackedExtension(this: JSCommonJSModule, id: string) { $assert(this); + let namespace; try { - $requireESM(id); + namespace = $requireESM(id); } catch (exception) { // Since the ESM code is mostly JS, we need to handle exceptions here. $requireMap.$delete(id); throw exception; } - // If we can pull out a ModuleNamespaceObject, let's do it. - const namespace = $esmNamespaceForCjs(id); + // $requireESM returns the namespace object directly (it throws on + // undefined). Use that — re-reading via $esmNamespaceForCjs would miss the + // Evaluating-but-body-ran case that $esmLoadSync handles. if (namespace !== undefined) { // In Bun, when __esModule is not defined, it's a CustomAccessor on the prototype. // Various libraries expect __esModule to be set when using ESM from require(). diff --git a/test/js/bun/resolve/require-esm-cycle.test.ts b/test/js/bun/resolve/require-esm-cycle.test.ts new file mode 100644 index 00000000000..271ef13131d --- /dev/null +++ b/test/js/bun/resolve/require-esm-cycle.test.ts @@ -0,0 +1,108 @@ +// require(esm) on a module that is already on the InnerModuleEvaluation stack +// (status == Evaluating) used to fall through to loadModuleSync and return the +// namespace object before the body ran, exposing TDZ let/const/class exports. +// Now it must throw ERR_REQUIRE_CYCLE_MODULE — unless ExecuteModule has already +// run for that record (an SCC sibling whose body completed but whose status +// only flips to Evaluated once the SCC root pops), in which case the namespace +// is fully populated and is still returned. +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; + +test("require(esm) on a module mid-evaluation throws ERR_REQUIRE_CYCLE_MODULE", async () => { + using dir = tempDir("require-esm-cycle-throw", { + "a.mjs": ` + import "./b.mjs"; + export const value = "ok"; + `, + "b.mjs": ` + import { createRequire } from "node:module"; + const require = createRequire(import.meta.url); + let code = "no-throw"; + try { + const a = require("./a.mjs"); + // pre-fix: this access threw "Cannot access 'value' before initialization" + void a.value; + } catch (e) { + code = e.code ?? e.constructor.name; + } + console.log(code); + `, + "entry.mjs": `import "./a.mjs";`, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "entry.mjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("ERR_REQUIRE_CYCLE_MODULE"); + expect(exitCode).toBe(0); +}); + +test("require(esm) on an SCC sibling whose body already ran returns the populated namespace", async () => { + // entry imports c then d. c imports entry back, so c and entry share an SCC; + // after c.execute() returns c stays at status Evaluating until entry (the + // SCC root) finishes. d's body then require()s c — c is at Evaluating with + // generator State != Init (body ran), so the const export is readable + // without re-driving the loader. + using dir = tempDir("require-esm-cycle-sibling", { + "entry.mjs": ` + import "./c.mjs"; + import "./d.mjs"; + export const FROM_ENTRY = 1; + `, + "c.mjs": ` + import "./entry.mjs"; + export const FROM_C = "populated"; + `, + "d.mjs": ` + import { createRequire } from "node:module"; + const require = createRequire(import.meta.url); + const c = require("./c.mjs"); + console.log(c.FROM_C); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "entry.mjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("populated"); + expect(exitCode).toBe(0); +}); + +test("require(esm) cycle error names the requested module", async () => { + using dir = tempDir("require-esm-cycle-message", { + "a.mjs": ` + import "./b.mjs"; + export const value = 1; + `, + "b.mjs": ` + import { createRequire } from "node:module"; + const require = createRequire(import.meta.url); + try { + require("./a.mjs"); + } catch (e) { + console.log(e.code, e.message.includes("a.mjs")); + } + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "a.mjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("ERR_REQUIRE_CYCLE_MODULE true"); + expect(exitCode).toBe(0); +});