module: add requireStack to all error paths

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,53 @@ 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_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 +1534,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 +1558,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 +1606,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;
 }
 

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');

test/fixtures/packages/missing-imports/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "missing-imports",
+  "imports": {
+    "#missing": "./does-not-exist.js"
+  }
+}

test/fixtures/packages/restrictive-exports/index.js

@@ -0,0 +1,2 @@
+'use strict';
+module.exports = { ok: true };

test/fixtures/packages/restrictive-exports/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "restrictive-exports",
+  "exports": {
+    ".": "./index.js"
+  }
+}

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');

test/fixtures/packages/restrictive-self-require/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "restrictive-self-require",
+  "exports": {
+    ".": "./index.js"
+  }
+}

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],
   }
 );

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;
+  }
+}

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],
   }
 );