module: warn on using unfinished circular dependency

Warn when a non-existent property of an unfinished module.exports
object is being accessed, as that very often indicates the presence
of a hard-to-detect and hard-to-debug problem.

This mechanism is only used if `module.exports` is still a
regular object at the point at which the second, circular `require()`
happens.

The downside is that, temporarily, `module.exports` will have a
prototype other than `Object.prototype`, and that there may
be valid uses of accessing non-existent properties of unfinished
`module.exports` objects.

Performance of circular require calls in general is not
noticeably impacted.

                                               confidence improvement accuracy (*)   (**)  (***)
     module/module-loader-circular.js n=10000                 3.96 %       ±5.12% ±6.82% ±8.89%

Example:

    $ cat a.js
    'use strict';
    const b = require('./b.js');

    exports.fn = () => {};
    $ cat b.js
    'use strict';
    const a = require('./a.js');

    a.fn();
    $ node a.js
    (node:1617) Warning: Accessing non-existent property 'fn' of module exports inside circular dependency
    /tmp/b.js:4
    a.fn();
      ^

    TypeError: a.fn is not a function
        at Object.<anonymous> (/tmp/b.js:4:3)
        [...]

PR-URL: https://github.com/nodejs/node/pull/29935
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

11 files changed, 143 insertions, 0 deletions

diff --git a/benchmark/module/module-loader-circular.js b/benchmark/module/module-loader-circular.js
new file mode 100644
index 0000000000..6d392e0e19
--- /dev/null
+++ b/benchmark/module/module-loader-circular.js
@@ -0,0 +1,34 @@
+'use strict';
+const fs = require('fs');
+const path = require('path');
+const common = require('../common.js');
+
+const tmpdir = require('../../test/common/tmpdir');
+const benchmarkDirectory =
+  path.resolve(tmpdir.path, 'benchmark-module-circular');
+
+const bench = common.createBenchmark(main, {
+  n: [1e4]
+});
+
+function main({ n }) {
+  tmpdir.refresh();
+
+  const aDotJS = path.join(benchmarkDirectory, 'a.js');
+  const bDotJS = path.join(benchmarkDirectory, 'b.js');
+
+  fs.mkdirSync(benchmarkDirectory);
+  fs.writeFileSync(aDotJS, 'require("./b.js");');
+  fs.writeFileSync(bDotJS, 'require("./a.js");');
+
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    require(aDotJS);
+    require(bDotJS);
+    delete require.cache[aDotJS];
+    delete require.cache[bDotJS];
+  }
+  bench.end(n);
+
+  tmpdir.refresh();
+}
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index 7df91ce4fd..8cec27e6c4 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -756,6 +756,52 @@ Module._resolveLookupPaths = function(request, parent) {
   return parentDir;
 };
 
+function emitCircularRequireWarning(prop) {
+  process.emitWarning(
+    `Accessing non-existent property '${String(prop)}' of module exports ` +
+    'inside circular dependency',
+    'Warning',
+    undefined,  // code
+    undefined,  // ctor
+    true);      // emit now
+}
+
+// A Proxy that can be used as the prototype of a module.exports object and
+// warns when non-existend properties are accessed.
+const CircularRequirePrototypeWarningProxy = new Proxy({}, {
+  get(target, prop) {
+    if (prop in target) return target[prop];
+    emitCircularRequireWarning(prop);
+    return undefined;
+  },
+
+  getOwnPropertyDescriptor(target, prop) {
+    if (ObjectPrototype.hasOwnProperty(target, prop))
+      return Object.getOwnPropertyDescriptor(target, prop);
+    emitCircularRequireWarning(prop);
+    return undefined;
+  }
+});
+
+// Object.prototype and ObjectProtoype refer to our 'primordials' versions
+// and are not identical to the versions on the global object.
+const PublicObjectPrototype = global.Object.prototype;
+
+function getExportsForCircularRequire(module) {
+  if (module.exports &&
+      Object.getPrototypeOf(module.exports) === PublicObjectPrototype &&
+      // Exclude transpiled ES6 modules / TypeScript code because those may
+      // employ unusual patterns for accessing 'module.exports'. That should be
+      // okay because ES6 modules have a different approach to circular
+      // dependencies anyway.
+      !module.exports.__esModule) {
+    // This is later unset once the module is done loading.
+    Object.setPrototypeOf(module.exports, CircularRequirePrototypeWarningProxy);
+  }
+
+  return module.exports;
+}
+
 // Check the cache for the requested file.
 // 1. If a module already exists in the cache: return its exports object.
 // 2. If the module is native: call
@@ -776,6 +822,8 @@ Module._load = function(request, parent, isMain) {
       const cachedModule = Module._cache[filename];
       if (cachedModule !== undefined) {
         updateChildren(parent, cachedModule, true);
+        if (!cachedModule.loaded)
+          return getExportsForCircularRequire(cachedModule);
         return cachedModule.exports;
       }
       delete relativeResolveCache[relResolveCacheIdentifier];
@@ -787,6 +835,8 @@ Module._load = function(request, parent, isMain) {
   const cachedModule = Module._cache[filename];
   if (cachedModule !== undefined) {
     updateChildren(parent, cachedModule, true);
+    if (!cachedModule.loaded)
+      return getExportsForCircularRequire(cachedModule);
     return cachedModule.exports;
   }
 
@@ -828,6 +878,10 @@ Module._load = function(request, parent, isMain) {
       if (parent !== undefined) {
         delete relativeResolveCache[relResolveCacheIdentifier];
       }
+    } else if (module.exports &&
+               Object.getPrototypeOf(module.exports) ===
+                 CircularRequirePrototypeWarningProxy) {
+      Object.setPrototypeOf(module.exports, PublicObjectPrototype);
     }
   }
 
diff --git a/test/fixtures/cycles/warning-a.js b/test/fixtures/cycles/warning-a.js
new file mode 100644
index 0000000000..dea4c53a26
--- /dev/null
+++ b/test/fixtures/cycles/warning-a.js
@@ -0,0 +1 @@
+require('./warning-b.js');
diff --git a/test/fixtures/cycles/warning-b.js b/test/fixtures/cycles/warning-b.js
new file mode 100644
index 0000000000..3be73bfd0a
--- /dev/null
+++ b/test/fixtures/cycles/warning-b.js
@@ -0,0 +1,3 @@
+const a = require('./warning-a.js');
+a.missingPropB;
+a[Symbol('someSymbol')];
diff --git a/test/fixtures/cycles/warning-esm-transpiled-a.js b/test/fixtures/cycles/warning-esm-transpiled-a.js
new file mode 100644
index 0000000000..fe70e745d3
--- /dev/null
+++ b/test/fixtures/cycles/warning-esm-transpiled-a.js
@@ -0,0 +1,2 @@
+Object.defineProperty(exports, "__esModule", { value: true });
+require('./warning-esm-transpiled-b.js');
diff --git a/test/fixtures/cycles/warning-esm-transpiled-b.js b/test/fixtures/cycles/warning-esm-transpiled-b.js
new file mode 100644
index 0000000000..a44a63ce2c
--- /dev/null
+++ b/test/fixtures/cycles/warning-esm-transpiled-b.js
@@ -0,0 +1 @@
+require('./warning-esm-transpiled-a.js').missingPropESM;
diff --git a/test/fixtures/cycles/warning-moduleexports-a.js b/test/fixtures/cycles/warning-moduleexports-a.js
new file mode 100644
index 0000000000..b37504b1b8
--- /dev/null
+++ b/test/fixtures/cycles/warning-moduleexports-a.js
@@ -0,0 +1,2 @@
+module.exports = {};
+require('./warning-moduleexports-b.js');
diff --git a/test/fixtures/cycles/warning-moduleexports-b.js b/test/fixtures/cycles/warning-moduleexports-b.js
new file mode 100644
index 0000000000..8d2292934d
--- /dev/null
+++ b/test/fixtures/cycles/warning-moduleexports-b.js
@@ -0,0 +1 @@
+require('./warning-moduleexports-b.js').missingPropModuleExportsB;
diff --git a/test/fixtures/cycles/warning-moduleexports-class-a.js b/test/fixtures/cycles/warning-moduleexports-class-a.js
new file mode 100644
index 0000000000..e23654d7f3
--- /dev/null
+++ b/test/fixtures/cycles/warning-moduleexports-class-a.js
@@ -0,0 +1,11 @@
+const assert = require('assert');
+
+class Parent {}
+class A extends Parent {}
+
+module.exports = A;
+require('./warning-moduleexports-class-b.js');
+process.nextTick(() => {
+  assert.strictEqual(module.exports, A);
+  assert.strictEqual(Object.getPrototypeOf(module.exports), Parent);
+});
diff --git a/test/fixtures/cycles/warning-moduleexports-class-b.js b/test/fixtures/cycles/warning-moduleexports-class-b.js
new file mode 100644
index 0000000000..3fe1763eb7
--- /dev/null
+++ b/test/fixtures/cycles/warning-moduleexports-class-b.js
@@ -0,0 +1 @@
+require('./warning-moduleexports-class-a.js').missingPropModuleExportsClassB;
diff --git a/test/parallel/test-module-circular-dependency-warning.js b/test/parallel/test-module-circular-dependency-warning.js
new file mode 100644
index 0000000000..1089397882
--- /dev/null
+++ b/test/parallel/test-module-circular-dependency-warning.js
@@ -0,0 +1,33 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+
+common.expectWarning({
+  Warning: [
+    ["Accessing non-existent property 'missingPropB' " +
+     'of module exports inside circular dependency'],
+    ["Accessing non-existent property 'Symbol(someSymbol)' " +
+     'of module exports inside circular dependency'],
+    ["Accessing non-existent property 'missingPropModuleExportsB' " +
+     'of module exports inside circular dependency']
+  ]
+});
+const required = require(fixtures.path('cycles', 'warning-a.js'));
+assert.strictEqual(Object.getPrototypeOf(required), Object.prototype);
+
+const requiredWithModuleExportsOverridden =
+  require(fixtures.path('cycles', 'warning-moduleexports-a.js'));
+assert.strictEqual(Object.getPrototypeOf(requiredWithModuleExportsOverridden),
+                   Object.prototype);
+
+// If module.exports is not a regular object, no warning should be emitted.
+const classExport =
+  require(fixtures.path('cycles', 'warning-moduleexports-class-a.js'));
+assert.strictEqual(Object.getPrototypeOf(classExport).name, 'Parent');
+
+// If module.exports.__esModule is set, no warning should be emitted.
+const esmTranspiledExport =
+  require(fixtures.path('cycles', 'warning-esm-transpiled-a.js'));
+assert.strictEqual(esmTranspiledExport.__esModule, true);