From eaa5cc5c95192d9c1dba3fa98404bed905e3d2e6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 Oct 2019 18:38:50 +0200 Subject: [PATCH 1/3] module: warn on using unfinished circular dependency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. (/tmp/b.js:4:3) [...] --- benchmark/module/module-loader-circular.js | 34 +++++++++++++ lib/internal/modules/cjs/loader.js | 49 +++++++++++++++++++ test/fixtures/cycles/warning-a.js | 1 + test/fixtures/cycles/warning-b.js | 1 + .../cycles/warning-moduleexports-a.js | 2 + .../cycles/warning-moduleexports-b.js | 1 + .../cycles/warning-moduleexports-class-a.js | 11 +++++ .../cycles/warning-moduleexports-class-b.js | 1 + ...test-module-circular-dependency-warning.js | 26 ++++++++++ 9 files changed, 126 insertions(+) create mode 100644 benchmark/module/module-loader-circular.js create mode 100644 test/fixtures/cycles/warning-a.js create mode 100644 test/fixtures/cycles/warning-b.js create mode 100644 test/fixtures/cycles/warning-moduleexports-a.js create mode 100644 test/fixtures/cycles/warning-moduleexports-b.js create mode 100644 test/fixtures/cycles/warning-moduleexports-class-a.js create mode 100644 test/fixtures/cycles/warning-moduleexports-class-b.js create mode 100644 test/parallel/test-module-circular-dependency-warning.js diff --git a/benchmark/module/module-loader-circular.js b/benchmark/module/module-loader-circular.js new file mode 100644 index 00000000000000..6d392e0e1908db --- /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 3580614481acc2..273d8eff622302 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -656,6 +656,47 @@ 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) { + // 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 @@ -676,6 +717,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]; @@ -687,6 +730,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; } @@ -728,6 +773,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 00000000000000..dea4c53a267af6 --- /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 00000000000000..6044be4099f52d --- /dev/null +++ b/test/fixtures/cycles/warning-b.js @@ -0,0 +1 @@ +require('./warning-a.js').missingPropB; diff --git a/test/fixtures/cycles/warning-moduleexports-a.js b/test/fixtures/cycles/warning-moduleexports-a.js new file mode 100644 index 00000000000000..b37504b1b8ad51 --- /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 00000000000000..8d2292934d2d64 --- /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 00000000000000..e23654d7f36c64 --- /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 00000000000000..3fe1763eb7256f --- /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 00000000000000..c19d75aff58965 --- /dev/null +++ b/test/parallel/test-module-circular-dependency-warning.js @@ -0,0 +1,26 @@ +'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 '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'); From dd1cdfa7801429935ee1e866c89e32ce514427f5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 Oct 2019 20:04:26 +0200 Subject: [PATCH 2/3] fixup! module: warn on using unfinished circular dependency --- test/fixtures/cycles/warning-b.js | 4 +++- test/parallel/test-module-circular-dependency-warning.js | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/test/fixtures/cycles/warning-b.js b/test/fixtures/cycles/warning-b.js index 6044be4099f52d..3be73bfd0af264 100644 --- a/test/fixtures/cycles/warning-b.js +++ b/test/fixtures/cycles/warning-b.js @@ -1 +1,3 @@ -require('./warning-a.js').missingPropB; +const a = require('./warning-a.js'); +a.missingPropB; +a[Symbol('someSymbol')]; diff --git a/test/parallel/test-module-circular-dependency-warning.js b/test/parallel/test-module-circular-dependency-warning.js index c19d75aff58965..62e8c197dabb62 100644 --- a/test/parallel/test-module-circular-dependency-warning.js +++ b/test/parallel/test-module-circular-dependency-warning.js @@ -8,6 +8,8 @@ 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'] ] From 3ba43138c35f91a097522c6391830fa842ac7dee Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 Oct 2019 22:38:33 +0200 Subject: [PATCH 3/3] fixup! module: warn on using unfinished circular dependency --- lib/internal/modules/cjs/loader.js | 7 ++++++- test/fixtures/cycles/warning-esm-transpiled-a.js | 2 ++ test/fixtures/cycles/warning-esm-transpiled-b.js | 1 + test/parallel/test-module-circular-dependency-warning.js | 5 +++++ 4 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/cycles/warning-esm-transpiled-a.js create mode 100644 test/fixtures/cycles/warning-esm-transpiled-b.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 273d8eff622302..d7ec3dff92f0c8 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -689,7 +689,12 @@ const PublicObjectPrototype = global.Object.prototype; function getExportsForCircularRequire(module) { if (module.exports && - Object.getPrototypeOf(module.exports) === PublicObjectPrototype) { + 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); } 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 00000000000000..fe70e745d38e08 --- /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 00000000000000..a44a63ce2c95b3 --- /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/parallel/test-module-circular-dependency-warning.js b/test/parallel/test-module-circular-dependency-warning.js index 62e8c197dabb62..10893978829ff5 100644 --- a/test/parallel/test-module-circular-dependency-warning.js +++ b/test/parallel/test-module-circular-dependency-warning.js @@ -26,3 +26,8 @@ assert.strictEqual(Object.getPrototypeOf(requiredWithModuleExportsOverridden), 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);