From 9fa4631bec264ac8af771418a8e017f9ca4acfda Mon Sep 17 00:00:00 2001 From: sangwook Date: Sat, 23 May 2026 23:32:19 +0900 Subject: [PATCH 1/5] module: add requireStack to all error paths Previously, `requireStack` was attached only on the bottom fallback path of `Module._resolveFilename`. Errors thrown from `tryPackage`, `trySelf`, the `#imports` branch, `finalizeEsmResolution`, and from within `Module._findPath` (exports/imports resolution failures) all escaped without it, leaving the diagnostic uneven depending on which inner path failed. This change makes the diagnostic uniform across those paths via two helpers: * `buildRequireStack(parent)` walks the parent chain. It is called lazily on the error path only, so successful resolves keep their original cost and a pathological parent-chain cycle from `module.parent = module` cannot hang the hot path. * `attachRequireStack(err, parent)` applies the stack to an error when (a) the error code is in a `SafeSet` of module-resolution codes (`MODULE_NOT_FOUND`, `ERR_MODULE_NOT_FOUND`, `ERR_PACKAGE_PATH_NOT_EXPORTED`, `ERR_PACKAGE_IMPORT_NOT_DEFINED`, `ERR_INVALID_MODULE_SPECIFIER`, `ERR_INVALID_PACKAGE_TARGET`, `ERR_INVALID_PACKAGE_CONFIG`), (b) `err.requireStack` is not already an array, and (c) the error object is extensible. The last check protects against monkey-patched `Module._findPath` implementations that may throw frozen errors. The catch blocks in the `#imports`, `trySelf`, and `Module._findPath` paths delegate to `attachRequireStack`, and the bottom fallback uses `buildRequireStack` directly. Resolves two long-standing `TODO(BridgeAR): Add the requireStack as well` comments in `lib/internal/modules/cjs/loader.js`, originally deferred in #26823 (2019) and carried through #34744 (2020). Tests cover the `#imports`-to-missing-file case, the `ERR_PACKAGE_PATH_NOT_EXPORTED` case via self-require, a frozen- error monkey-patch (must not crash), and a preset non-array `requireStack` value (must be replaced). Existing assertions in `test-module-loading.js` and `test-module-loading-error.js` are extended to verify the new property on errors that previously lacked it. Signed-off-by: sangwook --- lib/internal/modules/cjs/loader.js | 84 +++++++++-- .../packages/missing-imports/index.js | 5 + .../packages/missing-imports/package.json | 6 + .../packages/restrictive-exports/index.js | 2 + .../packages/restrictive-exports/package.json | 6 + .../restrictive-self-require/index.js | 5 + .../restrictive-self-require/package.json | 6 + test/parallel/test-module-loading-error.js | 3 +- .../test-module-require-stack-paths.js | 130 ++++++++++++++++++ test/sequential/test-module-loading.js | 1 + 10 files changed, 234 insertions(+), 14 deletions(-) create mode 100644 test/fixtures/packages/missing-imports/index.js create mode 100644 test/fixtures/packages/missing-imports/package.json create mode 100644 test/fixtures/packages/restrictive-exports/index.js create mode 100644 test/fixtures/packages/restrictive-exports/package.json create mode 100644 test/fixtures/packages/restrictive-self-require/index.js create mode 100644 test/fixtures/packages/restrictive-self-require/package.json create mode 100644 test/parallel/test-module-require-stack-paths.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 824214b55a2cb5..362ccaaae31ada 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -43,6 +43,7 @@ const { ObjectGetOwnPropertyDescriptor, ObjectGetPrototypeOf, ObjectHasOwn, + ObjectIsExtensible, ObjectKeys, ObjectPrototype, ObjectPrototypeHasOwnProperty, @@ -562,7 +563,6 @@ function tryPackage(requestPath, exts, isMain, originalPath) { err.code = 'MODULE_NOT_FOUND'; err.path = pjsonPath; err.requestPath = originalPath; - // TODO(BridgeAR): Add the requireStack as well. throw err; } else { process.emitWarning( @@ -1424,6 +1424,54 @@ Module._load = function(request, parent, isMain, internalOptions = kEmptyObject) return module.exports; }; +// Error codes emitted by the module-resolution pipeline. The CJS loader +// attaches `requireStack` to errors with any of these codes, so that the +// diagnostic is uniform regardless of which inner helper produced the error. +const kModuleResolutionErrorCodes = new SafeSet([ + 'MODULE_NOT_FOUND', + 'ERR_MODULE_NOT_FOUND', + 'ERR_PACKAGE_PATH_NOT_EXPORTED', + 'ERR_PACKAGE_IMPORT_NOT_DEFINED', + 'ERR_INVALID_MODULE_SPECIFIER', + 'ERR_INVALID_PACKAGE_TARGET', + 'ERR_INVALID_PACKAGE_CONFIG', +]); + +/** + * Walk the parent chain and produce the require stack. + * Called lazily on the error path so that successful resolves do not pay + * the cost (and so that a pathological parent chain - e.g. a cycle from + * `module.parent = module` - cannot hang the success path). + * @param {Module|null|undefined} parent + * @returns {string[]} + */ +function buildRequireStack(parent) { + const requireStack = []; + for (let cursor = parent; + cursor; + // TODO(joyeecheung): it makes more sense to use kLastModuleParent here. + cursor = cursor[kFirstModuleParent]) { + ArrayPrototypePush(requireStack, cursor.filename || cursor.id); + } + return requireStack; +} + +/** + * Attach `requireStack` to a module-resolution error if appropriate. + * Skips errors whose code is unrelated, errors that already carry an + * array `requireStack`, and frozen/sealed errors (e.g. from a + * monkey-patched `Module._findPath`) so that the assignment never + * masks the original failure with a TypeError. + * @param {Error} err + * @param {Module|null|undefined} parent + */ +function attachRequireStack(err, parent) { + if (!kModuleResolutionErrorCodes.has(err.code)) { return; } + if (ArrayIsArray(err.requireStack)) { return; } + if (!ObjectIsExtensible(err)) { return; } + err.requireStack = buildRequireStack(parent); +} + /** * Given a `require` string and its context, get its absolute file path. * @param {string} request The specifier to resolve @@ -1487,15 +1535,22 @@ Module._resolveFilename = function(request, parent, isMain, options) { ); } catch (e) { if (e.code === 'ERR_MODULE_NOT_FOUND') { - throw createEsmNotFoundErr(request); + throw createEsmNotFoundErr(request, undefined, buildRequireStack(parent)); } + attachRequireStack(e, parent); throw e; } } } // Try module self resolution first - const selfResolved = trySelf(parentPath, request, conditions); + let selfResolved; + try { + selfResolved = trySelf(parentPath, request, conditions); + } catch (e) { + attachRequireStack(e, parent); + throw e; + } if (selfResolved) { const cacheKey = request + '\x00' + (paths.length === 1 ? paths[0] : ArrayPrototypeJoin(paths, '\x00')); @@ -1504,15 +1559,15 @@ Module._resolveFilename = function(request, parent, isMain, options) { } // Look up the filename first, since that's the cache key. - const filename = Module._findPath(request, paths, isMain, conditions); - if (filename) { return filename; } - const requireStack = []; - for (let cursor = parent; - cursor; - // TODO(joyeecheung): it makes more sense to use kLastModuleParent here. - cursor = cursor[kFirstModuleParent]) { - ArrayPrototypePush(requireStack, cursor.filename || cursor.id); + let filename; + try { + filename = Module._findPath(request, paths, isMain, conditions); + } catch (e) { + attachRequireStack(e, parent); + throw e; } + if (filename) { return filename; } + const requireStack = buildRequireStack(parent); let message = `Cannot find module '${request}'`; if (requireStack.length > 0) { message = message + '\nRequire stack:\n- ' + @@ -1552,16 +1607,19 @@ function finalizeEsmResolution(resolved, parentPath, pkgPath) { * Creates an error object for when a requested ES module cannot be found. * @param {string} request The name of the requested module * @param {string} [path] The path to the requested module + * @param {string[]} [requireStack] The require stack at the time of the error * @returns {Error} */ -function createEsmNotFoundErr(request, path) { +function createEsmNotFoundErr(request, path, requireStack) { // eslint-disable-next-line no-restricted-syntax const err = new Error(`Cannot find module '${request}'`); err.code = 'MODULE_NOT_FOUND'; if (path) { err.path = path; } - // TODO(BridgeAR): Add the requireStack as well. + if (requireStack) { + err.requireStack = requireStack; + } return err; } diff --git a/test/fixtures/packages/missing-imports/index.js b/test/fixtures/packages/missing-imports/index.js new file mode 100644 index 00000000000000..5ed914349abfa6 --- /dev/null +++ b/test/fixtures/packages/missing-imports/index.js @@ -0,0 +1,5 @@ +'use strict'; +// Triggers the `#imports` resolution branch in Module._resolveFilename. +// The mapped target file does not exist, so finalizeEsmResolution throws +// a MODULE_NOT_FOUND error that the loader must enrich with requireStack. +module.exports = require('#missing'); diff --git a/test/fixtures/packages/missing-imports/package.json b/test/fixtures/packages/missing-imports/package.json new file mode 100644 index 00000000000000..cb8a7edad6a8fe --- /dev/null +++ b/test/fixtures/packages/missing-imports/package.json @@ -0,0 +1,6 @@ +{ + "name": "missing-imports", + "imports": { + "#missing": "./does-not-exist.js" + } +} diff --git a/test/fixtures/packages/restrictive-exports/index.js b/test/fixtures/packages/restrictive-exports/index.js new file mode 100644 index 00000000000000..df51a8e59e4ae4 --- /dev/null +++ b/test/fixtures/packages/restrictive-exports/index.js @@ -0,0 +1,2 @@ +'use strict'; +module.exports = { ok: true }; diff --git a/test/fixtures/packages/restrictive-exports/package.json b/test/fixtures/packages/restrictive-exports/package.json new file mode 100644 index 00000000000000..55a5d676c3cf5d --- /dev/null +++ b/test/fixtures/packages/restrictive-exports/package.json @@ -0,0 +1,6 @@ +{ + "name": "restrictive-exports", + "exports": { + ".": "./index.js" + } +} diff --git a/test/fixtures/packages/restrictive-self-require/index.js b/test/fixtures/packages/restrictive-self-require/index.js new file mode 100644 index 00000000000000..8e202e8589d304 --- /dev/null +++ b/test/fixtures/packages/restrictive-self-require/index.js @@ -0,0 +1,5 @@ +'use strict'; +// Self-require of a subpath that is not listed in this package's +// `exports` map. `trySelf` resolves the bare specifier through +// `packageExportsResolve`, which throws ERR_PACKAGE_PATH_NOT_EXPORTED. +module.exports = require('restrictive-self-require/not-in-exports'); diff --git a/test/fixtures/packages/restrictive-self-require/package.json b/test/fixtures/packages/restrictive-self-require/package.json new file mode 100644 index 00000000000000..007dbb43e31733 --- /dev/null +++ b/test/fixtures/packages/restrictive-self-require/package.json @@ -0,0 +1,6 @@ +{ + "name": "restrictive-self-require", + "exports": { + ".": "./index.js" + } +} diff --git a/test/parallel/test-module-loading-error.js b/test/parallel/test-module-loading-error.js index 3496a4104df090..b26e395373e042 100644 --- a/test/parallel/test-module-loading-error.js +++ b/test/parallel/test-module-loading-error.js @@ -91,6 +91,7 @@ assert.throws( () => { require('../fixtures/packages/is-dir'); }, common.isAIX ? { code: 'ERR_INVALID_PACKAGE_CONFIG' } : { code: 'MODULE_NOT_FOUND', - message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/ + message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/, + requireStack: [__filename], } ); diff --git a/test/parallel/test-module-require-stack-paths.js b/test/parallel/test-module-require-stack-paths.js new file mode 100644 index 00000000000000..eef83b8eefd266 --- /dev/null +++ b/test/parallel/test-module-require-stack-paths.js @@ -0,0 +1,130 @@ +'use strict'; + +// Verifies that `requireStack` is attached uniformly across the +// module-resolution error paths the CJS loader walks. The historical +// behavior attached `requireStack` only on the bottom fallback path of +// `Module._resolveFilename`; the inner helpers (`tryPackage`, `trySelf`, +// `Module._findPath`, `createEsmNotFoundErr` via the `#imports` branch +// and `finalizeEsmResolution`) all threw without it. + +require('../common'); +const assert = require('assert'); +const Module = require('module'); + +// 1. `#imports` resolves to a non-existent file. finalizeEsmResolution +// throws a MODULE_NOT_FOUND error from within the `#imports` branch's +// inner try/catch. The branch must enrich the error with requireStack +// before re-throwing, otherwise it escapes via the unrelated-code +// fall-through path. +{ + const entry = require.resolve('../fixtures/packages/missing-imports'); + assert.throws( + () => require('../fixtures/packages/missing-imports'), + (err) => { + assert.strictEqual(err.code, 'MODULE_NOT_FOUND'); + assert.ok(Array.isArray(err.requireStack), + `expected array requireStack, got ${err.requireStack}`); + assert.ok(err.requireStack.includes(entry), + `requireStack ${JSON.stringify(err.requireStack)} ` + + `did not contain ${entry}`); + assert.ok(err.requireStack.includes(__filename), + `requireStack ${JSON.stringify(err.requireStack)} ` + + `did not contain ${__filename}`); + return true; + }, + ); +} + +// 2. A self-require of a subpath that is not present in the package's +// `exports` map produces ERR_PACKAGE_PATH_NOT_EXPORTED from +// `packageExportsResolve` inside `trySelf`. The new wrapper must +// attach requireStack for this code as well, not only for +// MODULE_NOT_FOUND. A relative require would bypass the exports +// enforcement entirely, so this case uses a self-require to reach +// `packageExportsResolve`. +{ + const entry = require.resolve('../fixtures/packages/restrictive-self-require'); + assert.throws( + () => require('../fixtures/packages/restrictive-self-require'), + (err) => { + assert.strictEqual(err.code, 'ERR_PACKAGE_PATH_NOT_EXPORTED'); + assert.ok(Array.isArray(err.requireStack), + `expected array requireStack, got ${err.requireStack}`); + assert.ok(err.requireStack.includes(entry), + `requireStack ${JSON.stringify(err.requireStack)} ` + + `did not contain ${entry}`); + assert.ok(err.requireStack.includes(__filename), + `requireStack ${JSON.stringify(err.requireStack)} ` + + `did not contain ${__filename}`); + return true; + }, + ); +} + +// 3. `restrictive-exports` itself resolves cleanly. This is the happy path +// that proves the requireStack work does not regress successful +// resolutions (the eager-build version had a measurable cost here). +{ + const mod = require('../fixtures/packages/restrictive-exports'); + assert.deepStrictEqual(mod, { ok: true }); +} + +// 4. A monkey-patched `Module._findPath` that throws a frozen +// MODULE_NOT_FOUND error must propagate cleanly. The loader's attempt +// to attach requireStack must not throw a TypeError on the +// non-extensible error and replace the original failure. +{ + const originalFindPath = Module._findPath; + const frozenMessage = 'frozen module not found from monkey-patch'; + Module._findPath = function() { + const err = new Error(frozenMessage); + err.code = 'MODULE_NOT_FOUND'; + Object.freeze(err); + throw err; + }; + + try { + assert.throws( + () => require('this-module-does-not-exist-and-never-will'), + (err) => { + assert.strictEqual(err.code, 'MODULE_NOT_FOUND'); + assert.strictEqual(err.message, frozenMessage); + assert.ok(!(err instanceof TypeError), + 'loader masked the original error with a TypeError'); + return true; + }, + ); + } finally { + Module._findPath = originalFindPath; + } +} + +// 5. A monkey-patched `Module._findPath` that throws an error whose +// `requireStack` is already set as a non-array sentinel must not be +// silently overwritten with a stale value, and must not crash the +// loader. The defensive `ArrayIsArray` check controls this. +{ + const originalFindPath = Module._findPath; + Module._findPath = function() { + const err = new Error('preset non-array requireStack'); + err.code = 'MODULE_NOT_FOUND'; + err.requireStack = null; + throw err; + }; + + try { + assert.throws( + () => require('another-module-that-does-not-exist'), + (err) => { + assert.strictEqual(err.code, 'MODULE_NOT_FOUND'); + // Loader replaces the non-array sentinel with the actual chain. + assert.ok(Array.isArray(err.requireStack), + `expected array requireStack, got ${err.requireStack}`); + assert.ok(err.requireStack.includes(__filename)); + return true; + }, + ); + } finally { + Module._findPath = originalFindPath; + } +} diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js index 1e2efc6fd26a6b..118fcb1ff18d42 100644 --- a/test/sequential/test-module-loading.js +++ b/test/sequential/test-module-loading.js @@ -123,6 +123,7 @@ assert.throws( message: /packages[/\\]missing-main-no-index[/\\]doesnotexist\.js'\. Please.+package\.json.+valid "main"/, path: /fixtures[/\\]packages[/\\]missing-main-no-index[/\\]package\.json/, requestPath: /^\.\.[/\\]fixtures[/\\]packages[/\\]missing-main-no-index$/, + requireStack: [__filename], } ); From 42a7cd5b4c70d069fb0608968f189df42587a00f Mon Sep 17 00:00:00 2001 From: sangwook Date: Sat, 23 May 2026 23:39:00 +0900 Subject: [PATCH 2/5] meta: re-run CI Signed-off-by: sangwook From 34c128991369890482afe97ba955db6edb52da93 Mon Sep 17 00:00:00 2001 From: sangwook Date: Sat, 23 May 2026 23:51:31 +0900 Subject: [PATCH 3/5] meta: re-run CI Signed-off-by: sangwook From 8aee4112a9a1789e946763cd85aa99a5974070ca Mon Sep 17 00:00:00 2001 From: sangwook Date: Sun, 24 May 2026 09:39:59 +0900 Subject: [PATCH 4/5] module: drop unused code from requireStack filter `ERR_MODULE_NOT_FOUND` is always converted to `MODULE_NOT_FOUND` before reaching `attachRequireStack`, so the entry is unreachable. --- lib/internal/modules/cjs/loader.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 362ccaaae31ada..dff89910994cef 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1429,7 +1429,6 @@ Module._load = function(request, parent, isMain, internalOptions = kEmptyObject) // diagnostic is uniform regardless of which inner helper produced the error. const kModuleResolutionErrorCodes = new SafeSet([ 'MODULE_NOT_FOUND', - 'ERR_MODULE_NOT_FOUND', 'ERR_PACKAGE_PATH_NOT_EXPORTED', 'ERR_PACKAGE_IMPORT_NOT_DEFINED', 'ERR_INVALID_MODULE_SPECIFIER', From 08ebddd3e69fa01914827c8c83bfb544b80110eb Mon Sep 17 00:00:00 2001 From: sangwook Date: Sun, 24 May 2026 22:37:39 +0900 Subject: [PATCH 5/5] meta: re-run CI Signed-off-by: sangwook