-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
util: fix inspection of module namespaces #20962
Conversation
b3450e9
to
e16a700
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this break in 6.7? And this will likely have a minor influence on the performance.
lib/util.js
Outdated
|
||
if (symbols.length !== 0) { | ||
symbols = symbols.filter((key) => | ||
propertyIsEnumerable.call(value, key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fit on a single line as before.
lib/util.js
Outdated
try { | ||
keys = Object.keys(value); | ||
} catch (err) { | ||
if (err instanceof ReferenceError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other possibilities for this to throw a ReferenceError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not that I know of
lib/util.js
Outdated
keys = Object.keys(value); | ||
} catch (err) { | ||
if (err instanceof ReferenceError) { | ||
keys = Object.getOwnPropertyNames(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this not throw compared to Object.keys? Because of https://www.ecma-international.org/ecma-262/6.0/#sec-enumerableownnames 5.a.i.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[[GetOwnProperty]] -> [[Get]] -> [[GetBindingValue]] which throws
lib/util.js
Outdated
keys = Object.keys(value); | ||
if (symbols.length !== 0) | ||
symbols = symbols.filter((key) => propertyIsEnumerable.call(value, key)); | ||
// this might throw if `value` is a Module Namespace Object from an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use upper cases for the first letter of a sentence.
@BridgeAR v8 fixed a bug where they weren't throwing a reference error and they should have been |
For reference: https://bugs.chromium.org/p/v8/issues/detail?id=7780 |
Yep, tests are passing on |
I suggest to take a completely different approach: I saw that there is already a custom inspect function for modules. So when calling that function it should just wrap the Btw: normally only the object should be returned instead of calling util.inspect in a custom inspect function. That reduces the overhead. In this specific case it is useful in case that call is wrapped as suggested. |
@BridgeAR edit: i misunderstood; module namespace objects can't have custom inspect functions |
Let's see how the benchmarks come out. I am not a fan of adding a lot of special handling for the namespace edge case. |
lib/util.js
Outdated
try { | ||
keys = Object.keys(value); | ||
} catch (err) { | ||
if (err instanceof ReferenceError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of relying on the reference error here, we might just test for types.isModuleNamespaceObject nevertheless.
\cc @ljharb On v8/issues/detail?id=7780 are you planning to file a spec bug? Update: I opened issue tc39/ecma262#1209. |
lib/util.js
Outdated
types.isModuleNamespaceObject(value)) { | ||
keys = Object.getOwnPropertyNames(value); | ||
} else { | ||
throw err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ the Object.getOwnPropertyNames(value)
fallback should filter non-enumerable to be equiv to keys
Update:
Not required since this is now restricted to namespace objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you can't have non-enumerable keys on a module namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe the only keys on a module namespace will ever (atm) be enumerable strings for the exports, and Symbol.toStringTag
.
If this is going to land on older branches, we should be sure to run benchmarks for those branches first. If there are performance regressions, we should try to see about minimizing the impact (for example the try-catch will probably have a noticeable impact in node v6.x). |
@mscdex luckily i don't think module namespace objects exist on 6.x |
keys = Object.keys(value); | ||
// This might throw if `value` is a Module Namespace Object from an | ||
// unevaluated module, but we don't want to perform the actual type | ||
// check because it's expensive. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
7d66a91
to
f056724
Compare
lib/util.js
Outdated
if (err instanceof ReferenceError && | ||
types.isModuleNamespaceObject(value)) { | ||
keys = Object.getOwnPropertyNames(value); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\cc @bmeurer A pattern in much of Node's code is to extract try-catches into their own functions. In this case it would be named tryKeys()
. Is this practice still beneficial in the V8 of today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc it hasn't been needed since like v6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not necessary anymore. It used to prevent a function from optimizing and this is not the case anymore with turbofan.
Is there any strong -1 here or can we land this? |
@mmarchini ci still appears to have issues so I'm not running it yet. |
@devsnek regarding our infrastructure I think the issues are fixed. There are only flaky tests we need to fix now. |
// check because it's expensive. | ||
// TODO(devsnek): track https://github.com/tc39/ecma262/issues/1209 | ||
// and modify this logic as needed. | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.keys
andObject.getOwnPropertyNames
references should be cherry-picked above.- The
instanceof ReferenceError
check will fail from errors of a different realm (context).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the first point, we purposely leave those as-is because some polyfills for things use them to mess with util.
for the second point, i guess types.isNativeError(err) && err.name === 'ReferenceError'
would work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the first point, we purposely leave those as-is because some polyfills for things use them to mess with util.
Ah cool! Do you have any examples at hand?
for the second point, i guess types.isNativeError(err) && err.name === 'ReferenceError' would work?
Yep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what package does it, but it was brought up during discussions about hardening core against tampering with builtin prototypes.
f056724
to
a9a0643
Compare
@@ -772,7 +788,7 @@ function formatNamespaceObject(ctx, value, recurseTimes, keys) { | |||
try { | |||
output[i] = formatProperty(ctx, value, recurseTimes, keys[i], 0); | |||
} catch (err) { | |||
if (!(err instanceof ReferenceError)) { | |||
if (!(types.isNativeError(err) && err.name === 'ReferenceError')) { | |||
throw err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the error checks be simplified all up? If namespace objects can only error by throwing a reference error then we can assume that the error accessing it is because of the reference error and remove the error checks and rethrow code above. We could also simplify the initial check by doing:
try {
keys = Object.keys(value);
} catch (err) {
if (types.isModuleNamespaceObject(value)) {
keys = Object.getOwnPropertyNames(value);
} else {
throw err;
}
}
Here erroring should be rare and when it does error it is likely a namespace object so the check up-front on error should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think its best to explicitly check if its a reference error, do you feel strongly about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dig it if code could be simpler, and still correct, that it be simpler is all.
This is blocking V8 6.7 landing @jdalton is it ok for this to land as is and be simplified in a follow up? |
Yep, yep. |
FreeBSD failure is a flaky and I have no idea why it's saying OSX failed. Running another one: https://ci.nodejs.org/job/node-test-pull-request/15169/ |
OSX should work now (thanks @refack!). 5th time's a charm! https://ci.nodejs.org/job/node-test-pull-request/15170/ |
CI is yellow. Landing! |
Landed in a25730b |
PR-URL: #20962 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #20962 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
unbreaks 6.7 upgrade
/cc @targos i don't have 6.7 checked out or the room to do so, can you confirm this patch fixes it?
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes