module: warn on using unfinished circular dependency #29935
Conversation
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)
[...]
|
maybe we could start with warning if module.loaded is false, and look into more advanced messages in future changes? |
|
@devsnek This does only warn if If you want to warn every time that there’s a cyclic dependency, that would be a non-starter to me because I assume it’s going to be very, very annoying to have a ton of warnings about something that’s perfectly fine if done right. |
|
@addaleax yeah i mean starting with just detecting all circulars. I think most of it is probably working by chance, not by intention. (I'm not blocking anything here though) |
|
@devsnek So … maybe this is just my experience, but I think circular dependencies aren’t all that uncommon, and if they are working, it’s often not just by chance but because somebody figured out that they didn’t work on the first try and then spent a while fixing them. (I definitely had that experience in the past, and iirc the same thing happened with circular dependencies in Node’s internal modules – e.g. 413fcad or 355523f) So if we do want to warn on all circular modules, I would not make that the default mode, and I would like to have something that warns about potential issues that is on by default. |
|
would this somehow impact code that clears the contents the require cache? I don’t really have time rn to think of and/or show examples of how to break this, but I am a bit concerned about unintended consequences of changing the prototype, and somewhat less concerned about false positives. mainly around modules like proxyquire, rewire, esm, etc |
|
I suspect that these warnings may be more common than we expect in running code adding to noise on existing apps, but apart from that this approach seems fine to me. |
|
@addaleax have you tried running node with this change on an app, or using some CLI tools? |
|
If there was a |
@boneskull I don't think so. The only interaction with the require cache that I can see is what would happen if a module deleted itself from the require cache while it is being loaded (whyever one would do that), but even in that scenario all that this PR would change is that it would not print a warning.
I can understand that, and I was a bit worried about this when writing the code, but after thinking it through I'm not really seeing this as a major concern. The prototype is always reset after the module is loaded, and only if it hasn't been changed in the meantime anyway, and all methods on it are still there during the whole process. The main way this is observable, besides the warnings, would be when somebody checks whether
@guybedford Yes, that would be my biggest concern here too. I've run CITGM with that in mind, and I also like @targos' suggestion of turning this a throw and then running CITGM with that.
Not yet, no. |
|
Actually, it looks like this interferes with TypeScript enums, where something like export const enum Foo { A = 1, B = 2 };can result in something like (function (Foo) {
Foo[Foo["A"] = 1] = "A";
Foo[Foo["B"] = 2] = "B";
})(Foo = exports.Foo || (exports.Foo = {}));so that might actually require getting a bit fancier here… maybe we need to exclude the module file itself from these warnings. |
|
Thinking about it, I think it’s a good idea to exclude transpiled TS/ESM code altogether, at least for now. ESM kind of has a different idea of how to deal with cyclic dependencies anyway, and already throws an explicit error for TDZ accesses. Here’s a CITGM for this code + throwing instead of warning so that we get meaningful results: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2051/ |
`lib/pack.js` and `lib/config/figgy-config.js` load each other, making `figgy-config.js` grab the original `module.exports` value and not the intended one. In particular, this always sets the `dirPacker` value to `undefined` in the config generation step. Fix this by setting `module.exports` early. Refs: nodejs/node#29935
|
So … the bad news is, CITGM is failing because The good news is: it’s not a false positive, but rather a genuine bug with npm of the exact kind that this PR is supposed to catch. Yay? :) Here’s another CITGM with a hack to silence the warning for that instance in npm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2052/ |
|
I would gladly welcome a warning like this. People can easily pass the Here's an issue that I'm not sure how to resolve, though. Say someone is already using |
@DerekNonGeneric So … yes, for the moment that would be the case – I think that brings up an interesting point though, namely whether we should have a way to disable specific kinds of warnings… maybe that would be helpful? |
|
Sifting through the CITGM results:
The other failures seem like unrelated, pre-existing CITGM failures. I will open a PR against @nodejs/tsc Thoughts on that approach? |
Node.js is currently considering printing a warning when a non-existent property of `module.exports` is accessed while in a circular `require()` dependency, in order to make it easier to catch issues with circular dependencies. In order to avoid printing these warnings for shelljs, checking for the property’s existence rather than its truthiness suffices. Refs: nodejs/node#29935
|
This would definitely be nice to have, at the very least as an opt-in feature. |
Node.js is currently considering printing a warning when a non-existent property of `module.exports` is accessed while in a circular `require()` dependency, in order to make it easier to catch issues with circular dependencies. In order to avoid printing these warnings for shelljs, checking for the property’s existence rather than its truthiness suffices. Refs: nodejs/node#29935
addaleax
added
the
semver-major
|
Can I get another @nodejs/tsc review? I’ve labelled this |
`lib/pack.js` and `lib/config/figgy-config.js` load each other, making `figgy-config.js` grab the original `module.exports` value and not the intended one. In particular, this always sets the `dirPacker` value to `undefined` in the config generation step. Fix this by setting `module.exports` early. Refs: nodejs/node#29935 PR-URL: #266 Credit: @addaleax Close: #266 Reviewed-by: @mikemimik
addaleax
added
the
author ready
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: #29935
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
|
Landed in d7452b7 |
Since this property access is performed by generated code, and not used for accessing the actual exports of a module (and because transpilers generally define it as the first key of `module.exports` when it *is* present), it should be okay to allow it. Refs: nodejs#29935 Fixes: nodejs#33046
Node.js is currently considering printing a warning when a non-existent property of `module.exports` is accessed while in a circular `require()` dependency, in order to make it easier to catch issues with circular dependencies. In order to avoid printing these warnings for shelljs, checking for the property’s existence rather than its truthiness suffices. Refs: nodejs/node#29935
Since this property access is performed by generated code, and not used for accessing the actual exports of a module (and because transpilers generally define it as the first key of `module.exports` when it *is* present), it should be okay to allow it. Refs: #29935 Fixes: #33046 PR-URL: #33048 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>
Since this property access is performed by generated code, and not used for accessing the actual exports of a module (and because transpilers generally define it as the first key of `module.exports` when it *is* present), it should be okay to allow it. Refs: #29935 Fixes: #33046 PR-URL: #33048 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>
Since this property access is performed by generated code, and not used for accessing the actual exports of a module (and because transpilers generally define it as the first key of `module.exports` when it *is* present), it should be okay to allow it. Refs: #29935 Fixes: #33046 PR-URL: #33048 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>
Since this property access is performed by generated code, and not used for accessing the actual exports of a module (and because transpilers generally define it as the first key of `module.exports` when it *is* present), it should be okay to allow it. Refs: #29935 Fixes: #33046 PR-URL: #33048 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>
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.exportsis still aregular object at the point at which the second, circular
require()happens.
The downside is that, temporarily,
module.exportswill have aprototype other than
Object.prototype, and that there maybe valid uses of accessing non-existent properties of unfinished
module.exportsobjects.Performance of circular require calls in general is not
noticeably impacted.
Example:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes