[defer-import-eval] deferred import evaluation for ES modules [v8/v8 : main]
Caio Lima (Gerrit)
Caio Lima added 3 comments![]( "Open in Gerrit")
This new CL includes all the operations that should trigger an module evaluation.
case LookupIterator::DEFERRED_MODULE_NAMESPACE:Caio LimaThis should be reachable via `Object.hasOwn()`. Please write a test for this.
It should evaluate the module and `continue`.
Igor SheludkoI was planning to do other operations [[Has]], [[Set]], [[Delete]], [[GetOwnProperty]], etc in a separate patch to make it easier we review each case, but I can do them here if you prefer.
Caio LimaLet's try to put it into one CL as long as it's not huge.
Acknowledged
it->GetName());Caio LimaCan we make it so that here we do just `JSDeferredModuleNamespace::EvaluateModuleSync(isolate, holder);` + exception handling, and let the regular path for `JSModuleNamespace` objects handle the rest?
Caio LimaYes. This would mean basically to inline `JSDeferredModuleNamespace::GetProperty` here. In this new Patchset, once a module is evaluated, it will never get through `DEFERRED_MODULE_NAMESPACE` anymore, as you suggested above.
Acknowledged
Related details
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
case LookupIterator::DEFERRED_MODULE_NAMESPACE:Caio LimaThis case might be reachable via `ns.propertyIsEnumerable(..)`.
Thanks for pointing this out. I'm adding a test case for it as well. I don't know why I missed this comment before.
When using `Object.prototype.propertyIsEnumerable`, it actually goes to `JSReceiver::GetOwnPropertyAttributes` (ref: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-object.cc;l=31;drc=d26244cc105a77ea0708ee85fd8cf2e3ab33c608), that is analogous to `[[GetOwnProperty]]` from spec (ref: https://tc39.es/ecma262/#sec-object.prototype.propertyisenumerable).
Also, it's important to notice that `ns.propertyIsEnumerable` will trigger evaluation due to `[[Get]]` first, so at this point, this `DEFERRED_MODULE_NAMESPACE` state should be unexpected as well. Another thing that's important is that namespace's prototype is null, and since it's also non-extensible, it can't be changed. The way to execute `propertyIsEnumrable` is through `Object.prototype.propertyIsEnumerable(ns, ...)`.
Related details
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 5 comments![]( "Open in Gerrit")
I am basically happy. Only one question left on my last pass over the CL.
LoadHandler::LoadModuleExport(isolate(), value_index);Caio LimaI would like to see a CSA_DCHECK that this load never happens on an unevaluated object.
This would go here: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/ic/accessor-assembler.cc;l=1000;drc=adf1c0b0a7acf756e039e4203f41018e4bae22d0;bpv=0;bpt=1
Caio LimaHere I'd need something that asserts `IsJSDeferredModuleNamespace(receiver) && module->status() == kEvaluated`. Would it make sense? The thing is that we can have accesses to ordinary module namespaces while it's module is not evaluated yet, since they can be in a evaluating or evaluating-async` state if there's a dependency cycle.
Igor SheludkoThis code doesn't exist in the new version, but I'm wondering if we'd still want to add this CSA_DCHECK there as well.
Caio LimaIIUC the spec correctly that evaluated module does not require any more processing. I'd suggest to treat `LookupIterator::DEFERRED_MODULE_NAMESPACE` state as "not yet evaluated deferred module", and thus once it's evaluated we wouldn't even get into this case again.
Olivier FlückigerI think that's the behavior we have now, right? In the implementation of `JSDeferredModuleNamespace::TriggersEvaluation`, we do check if the module is evaluated or not. The only detail here is that if a module gets evaluated and transitions to `kErrored` estate because it throws, we still keep going through `LookupIterator::DEFERRED_MODULE_NAMESPACE`, because every future access to the `ns` needs to throws the same Exception, and it's stored in the module's `top_level_capability`. That's also the reason why we don't cache access in this case.
Acknowledged
case LookupIterator::DEFERRED_MODULE_NAMESPACE:When do we end up here?
// TODO(https://crbug.com/348660658): replace language mode```suggestion
// TODO(348660658): replace language mode
```
return true;Caio Limashould we return false if it is already evaluated? (unsure about the implications)
Caio LimaThe implication here is that once a module gets evaluated, we will never get `DEFERRED_MODULE_NAMESPACE` kind.
This would require that I install the accessors for exported names once, and behave like `JSModuleNamespace` now. I didn't think about that earlier, but this could be work. For now I'm mostly doing on `MaybeEvaluate`, however doing this here can simplify things a bit. This might better, since I don't have to think in how IC would work with `DEFERRED_MODULE_NAMESPACE, and it will behave pretty much like `JSModuleNamespace` does today. I'll give it a try.
Olivier FlückigerI changed it in this new Patchset. I feel this version is better than before, since we won't have much difference between an evaluated deferred module and an ordinary module anymore. Also the IC handling is pretty much like `do nothing if the module is not evaluated`. Once it's evaluated, it'll get the same IC behavior as ordinary module namespaces.
Acknowledged
Related details
- Caio Lima
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
case LookupIterator::DEFERRED_MODULE_NAMESPACE:When do we end up here?
One path that I see is coming from Object::NoSideEffectsToString. This is called with `"toString"` when formatting an error message, (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/objects.cc;l=636;drc=07576e5c96c664c421fcf6fb0dfdb940622f8aa3). This is a test case where this gets called (https://github.com/tc39/test262/blob/079b13f85da120049919365612163e4579678e89/test/language/import/import-defer/evaluation-triggers/ignore-symbol-toStringTag-delete.js). Looks like all the callers for it are somewhat internal operations, and we shouldn't trigger an evaluation here, otherwise, it'll break the spec.
Related details
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
// TODO(https://crbug.com/348660658): replace language mode```suggestion
// TODO(348660658): replace language mode
```
Fix applied.
Related details
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger voted and added 2 comments![]( "Open in Gerrit")
Votes added by Olivier Flückiger
| Code-Review | +1 |
2 comments
lgtm, thanks a lot and nice work!
@ish...@chromium.org could you do another pass please?
case LookupIterator::DEFERRED_MODULE_NAMESPACE:Caio LimaWhen do we end up here?
One path that I see is coming from Object::NoSideEffectsToString. This is called with `"toString"` when formatting an error message, (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/objects.cc;l=636;drc=07576e5c96c664c421fcf6fb0dfdb940622f8aa3). This is a test case where this gets called (https://github.com/tc39/test262/blob/079b13f85da120049919365612163e4579678e89/test/language/import/import-defer/evaluation-triggers/ignore-symbol-toStringTag-delete.js). Looks like all the callers for it are somewhat internal operations, and we shouldn't trigger an evaluation here, otherwise, it'll break the spec.
- Caio Lima
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
- Implement property operation triggers for [[Get]].This is out of date now.
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
- Implement property operation triggers for [[Get]].This is out of date now.
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 7 comments![]( "Open in Gerrit")
looks good, some nits and comments about [[Set]] and `!IsSymbolLikeNamespaceKey(P, O)` case.
if (isolate->has_exception()) {
return isolate->exception();
}Magic macro for such cases: `RETURN_FAILURE_IF_EXCEPTION(isolate);`
}Nit: I think the caller function `KeyAccumulator::CollectOwnKeys(..)` is a better place for handling this case. Right after `if (IsAccessCheckNeeded(*object) &&`.
if (it->HolderIsReceiverOrHiddenPrototype()) {This doesn't seem to be correct. As Nicolò Ribaudo pointed out in another CL, once we reached `[[Set]]` for deferred module, regardless of whether it happened for a direct store to a module of through a prototype chain of another object - we are done. So we shouldn't do this check.
BTW, IIUC, we should not be doing `IsSymbolLikeNamespaceKey(P, O)` step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for `!IsSymbolLikeNamespaceKey(P, O)` case and deferred module somewhere.
return Nothing<bool>();We should return `Just(false);`, `Nothing` is an indication that exception was thrown.
case LookupIterator::DEFERRED_MODULE_NAMESPACE:Caio LimaThis case might be reachable via `ns.propertyIsEnumerable(..)`.
Thanks for pointing this out. I'm adding a test case for it as well. I don't know why I missed this comment before.
When using `Object.prototype.propertyIsEnumerable`, it actually goes to `JSReceiver::GetOwnPropertyAttributes` (ref: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-object.cc;l=31;drc=d26244cc105a77ea0708ee85fd8cf2e3ab33c608), that is analogous to `[[GetOwnProperty]]` from spec (ref: https://tc39.es/ecma262/#sec-object.prototype.propertyisenumerable).
Also, it's important to notice that `ns.propertyIsEnumerable` will trigger evaluation due to `[[Get]]` first, so at this point, this `DEFERRED_MODULE_NAMESPACE` state should be unexpected as well. Another thing that's important is that namespace's prototype is null, and since it's also non-extensible, it can't be changed. The way to execute `propertyIsEnumrable` is through `Object.prototype.propertyIsEnumerable(ns, ...)`.
Ack!
Nit: please put this comment inside `case LookupIterator::DEFERRED_MODULE_NAMESPACE:`.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
if (it->HolderIsReceiverOrHiddenPrototype()) {This doesn't seem to be correct. As Nicolò Ribaudo pointed out in another CL, once we reached `[[Set]]` for deferred module, regardless of whether it happened for a direct store to a module of through a prototype chain of another object - we are done. So we shouldn't do this check.
BTW, IIUC, we should not be doing `IsSymbolLikeNamespaceKey(P, O)` step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for `!IsSymbolLikeNamespaceKey(P, O)` case and deferred module somewhere.
Oh right, I misunderstood the spec at this part. I just figured out now that the logic to install a new property is here (https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor), and it actually happens inside prototype.[[Set]].
BTW, IIUC, we should not be doing IsSymbolLikeNamespaceKey(P, O) step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for !IsSymbolLikeNamespaceKey(P, O) case and deferred module somewhere.
Thinking this a little deeper, I think we might have a bug for all module namespaces, both deferred and ordinary. The way we implement module namespace right now allows to a program like the one bellow to execute without errors, but it actually should throw when running on strict mode:
```
import * as ns from './foo_module.mjs'
let obj = Object.create(ns);
obj.missingProp = 10;
obj.missingProp; // returns 10, but this set should never happen.
```
The design I was thinking here was that any access that would trigger an evaluation, we actually throw an error. Otherwise fallback to ordinary ModuleNamespace logic - which shares the same [[Set]] we have for Deferred Modules (https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-set-p-v-receiver). I did that because I couldn't find a way to know if a Lookup is happening to a [[Set]] operation. However, since ordinary namespaces looks broken as well, it doesn't work.
Would it be too problematic if I add a configuration in LookupIterator to be able to know if it's a lookup for a [[Set]]?
Related details
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
if (it->HolderIsReceiverOrHiddenPrototype()) {Caio LimaThis doesn't seem to be correct. As Nicolò Ribaudo pointed out in another CL, once we reached `[[Set]]` for deferred module, regardless of whether it happened for a direct store to a module of through a prototype chain of another object - we are done. So we shouldn't do this check.
BTW, IIUC, we should not be doing `IsSymbolLikeNamespaceKey(P, O)` step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for `!IsSymbolLikeNamespaceKey(P, O)` case and deferred module somewhere.
Oh right, I misunderstood the spec at this part. I just figured out now that the logic to install a new property is here (https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor), and it actually happens inside prototype.[[Set]].
BTW, IIUC, we should not be doing IsSymbolLikeNamespaceKey(P, O) step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for !IsSymbolLikeNamespaceKey(P, O) case and deferred module somewhere.
Thinking this a little deeper, I think we might have a bug for all module namespaces, both deferred and ordinary. The way we implement module namespace right now allows to a program like the one bellow to execute without errors, but it actually should throw when running on strict mode:
```
import * as ns from './foo_module.mjs'let obj = Object.create(ns);
obj.missingProp = 10;
obj.missingProp; // returns 10, but this set should never happen.
```The design I was thinking here was that any access that would trigger an evaluation, we actually throw an error. Otherwise fallback to ordinary ModuleNamespace logic - which shares the same [[Set]] we have for Deferred Modules (https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-set-p-v-receiver). I did that because I couldn't find a way to know if a Lookup is happening to a [[Set]] operation. However, since ordinary namespaces looks broken as well, it doesn't work.
Would it be too problematic if I add a configuration in LookupIterator to be able to know if it's a lookup for a [[Set]]?
Doing such a way in lookup iterator, we would be able to probably fix the issue we have for ordinary Namespaces.
An alternative I can think could be to add a check in `NOT_FOUND` and check if the last check "holder" is a `JSModuleNamespace`, and if this is the case, we throw an error if we are in strict mode. The thing there is that we can't ever have a `JSModuleNamespace` in the middle of a prototype chain, since its prototype is null and it's non extensible. So it needs to be the last object of the chain.
Igor Sheludko (Gerrit)
Igor Sheludko added 1 comment![]( "Open in Gerrit")
if (it->HolderIsReceiverOrHiddenPrototype()) {Caio LimaThis doesn't seem to be correct. As Nicolò Ribaudo pointed out in another CL, once we reached `[[Set]]` for deferred module, regardless of whether it happened for a direct store to a module of through a prototype chain of another object - we are done. So we shouldn't do this check.
BTW, IIUC, we should not be doing `IsSymbolLikeNamespaceKey(P, O)` step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for `!IsSymbolLikeNamespaceKey(P, O)` case and deferred module somewhere.
Caio LimaOh right, I misunderstood the spec at this part. I just figured out now that the logic to install a new property is here (https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor), and it actually happens inside prototype.[[Set]].
BTW, IIUC, we should not be doing IsSymbolLikeNamespaceKey(P, O) step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for !IsSymbolLikeNamespaceKey(P, O) case and deferred module somewhere.
Thinking this a little deeper, I think we might have a bug for all module namespaces, both deferred and ordinary. The way we implement module namespace right now allows to a program like the one bellow to execute without errors, but it actually should throw when running on strict mode:
```
import * as ns from './foo_module.mjs'let obj = Object.create(ns);
obj.missingProp = 10;
obj.missingProp; // returns 10, but this set should never happen.
```The design I was thinking here was that any access that would trigger an evaluation, we actually throw an error. Otherwise fallback to ordinary ModuleNamespace logic - which shares the same [[Set]] we have for Deferred Modules (https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-set-p-v-receiver). I did that because I couldn't find a way to know if a Lookup is happening to a [[Set]] operation. However, since ordinary namespaces looks broken as well, it doesn't work.
Would it be too problematic if I add a configuration in LookupIterator to be able to know if it's a lookup for a [[Set]]?
Doing such a way in lookup iterator, we would be able to probably fix the issue we have for ordinary Namespaces.
An alternative I can think could be to add a check in `NOT_FOUND` and check if the last check "holder" is a `JSModuleNamespace`, and if this is the case, we throw an error if we are in strict mode. The thing there is that we can't ever have a `JSModuleNamespace` in the middle of a prototype chain, since its prototype is null and it's non extensible. So it needs to be the last object of the chain.
I think what would help is to have just one new `MODULE_NAMESPACE` state which would be set for both normal and deferred module namespace object. And the reaction to it would be to trigger evaluation of the deferred module if a particular operation requires it and/or continue lookup.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
if (it->HolderIsReceiverOrHiddenPrototype()) {Caio LimaThis doesn't seem to be correct. As Nicolò Ribaudo pointed out in another CL, once we reached `[[Set]]` for deferred module, regardless of whether it happened for a direct store to a module of through a prototype chain of another object - we are done. So we shouldn't do this check.
BTW, IIUC, we should not be doing `IsSymbolLikeNamespaceKey(P, O)` step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for `!IsSymbolLikeNamespaceKey(P, O)` case and deferred module somewhere.
Caio LimaOh right, I misunderstood the spec at this part. I just figured out now that the logic to install a new property is here (https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor), and it actually happens inside prototype.[[Set]].
BTW, IIUC, we should not be doing IsSymbolLikeNamespaceKey(P, O) step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for !IsSymbolLikeNamespaceKey(P, O) case and deferred module somewhere.
Thinking this a little deeper, I think we might have a bug for all module namespaces, both deferred and ordinary. The way we implement module namespace right now allows to a program like the one bellow to execute without errors, but it actually should throw when running on strict mode:
```
import * as ns from './foo_module.mjs'let obj = Object.create(ns);
obj.missingProp = 10;
obj.missingProp; // returns 10, but this set should never happen.
```The design I was thinking here was that any access that would trigger an evaluation, we actually throw an error. Otherwise fallback to ordinary ModuleNamespace logic - which shares the same [[Set]] we have for Deferred Modules (https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-set-p-v-receiver). I did that because I couldn't find a way to know if a Lookup is happening to a [[Set]] operation. However, since ordinary namespaces looks broken as well, it doesn't work.
Would it be too problematic if I add a configuration in LookupIterator to be able to know if it's a lookup for a [[Set]]?
Igor SheludkoDoing such a way in lookup iterator, we would be able to probably fix the issue we have for ordinary Namespaces.
An alternative I can think could be to add a check in `NOT_FOUND` and check if the last check "holder" is a `JSModuleNamespace`, and if this is the case, we throw an error if we are in strict mode. The thing there is that we can't ever have a `JSModuleNamespace` in the middle of a prototype chain, since its prototype is null and it's non extensible. So it needs to be the last object of the chain.
I think what would help is to have just one new `MODULE_NAMESPACE` state which would be set for both normal and deferred module namespace object. And the reaction to it would be to trigger evaluation of the deferred module if a particular operation requires it and/or continue lookup.
Is it fine if after a state `MODULE_NAMESPACE` happens and we call `LookupIterator::Next()` it will fallback to perform a `LookupInRegularHolder`? If so, I can try to do that. This would mean that I'll end up calling `continue` on pretty much all places when holder is not a JSDeferredModuleNamespace, but this can solve this issue.
I've implemented the version that checks on `NOT_FOUND` state of `SetPropertyInternal`, but thinking this through, I think it's not acceptable, since it'll slow down every `obj.prop = ...` operation that takes the slow path.
Related details
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 1 comment![]( "Open in Gerrit")
if (it->HolderIsReceiverOrHiddenPrototype()) {Caio LimaThis doesn't seem to be correct. As Nicolò Ribaudo pointed out in another CL, once we reached `[[Set]]` for deferred module, regardless of whether it happened for a direct store to a module of through a prototype chain of another object - we are done. So we shouldn't do this check.
BTW, IIUC, we should not be doing `IsSymbolLikeNamespaceKey(P, O)` step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for `!IsSymbolLikeNamespaceKey(P, O)` case and deferred module somewhere.
Caio LimaOh right, I misunderstood the spec at this part. I just figured out now that the logic to install a new property is here (https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor), and it actually happens inside prototype.[[Set]].
BTW, IIUC, we should not be doing IsSymbolLikeNamespaceKey(P, O) step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for !IsSymbolLikeNamespaceKey(P, O) case and deferred module somewhere.
Thinking this a little deeper, I think we might have a bug for all module namespaces, both deferred and ordinary. The way we implement module namespace right now allows to a program like the one bellow to execute without errors, but it actually should throw when running on strict mode:
```
import * as ns from './foo_module.mjs'let obj = Object.create(ns);
obj.missingProp = 10;
obj.missingProp; // returns 10, but this set should never happen.
```The design I was thinking here was that any access that would trigger an evaluation, we actually throw an error. Otherwise fallback to ordinary ModuleNamespace logic - which shares the same [[Set]] we have for Deferred Modules (https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-set-p-v-receiver). I did that because I couldn't find a way to know if a Lookup is happening to a [[Set]] operation. However, since ordinary namespaces looks broken as well, it doesn't work.
Would it be too problematic if I add a configuration in LookupIterator to be able to know if it's a lookup for a [[Set]]?
Igor SheludkoDoing such a way in lookup iterator, we would be able to probably fix the issue we have for ordinary Namespaces.
An alternative I can think could be to add a check in `NOT_FOUND` and check if the last check "holder" is a `JSModuleNamespace`, and if this is the case, we throw an error if we are in strict mode. The thing there is that we can't ever have a `JSModuleNamespace` in the middle of a prototype chain, since its prototype is null and it's non extensible. So it needs to be the last object of the chain.
Caio LimaI think what would help is to have just one new `MODULE_NAMESPACE` state which would be set for both normal and deferred module namespace object. And the reaction to it would be to trigger evaluation of the deferred module if a particular operation requires it and/or continue lookup.
Is it fine if after a state `MODULE_NAMESPACE` happens and we call `LookupIterator::Next()` it will fallback to perform a `LookupInRegularHolder`? If so, I can try to do that. This would mean that I'll end up calling `continue` on pretty much all places when holder is not a JSDeferredModuleNamespace, but this can solve this issue.
I've implemented the version that checks on `NOT_FOUND` state of `SetPropertyInternal`, but thinking this through, I think it's not acceptable, since it'll slow down every `obj.prop = ...` operation that takes the slow path.
Yes, it's totally fine, given that JSModuleNamespaces require such an orthogonal behavior.
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 5 comments![]( "Open in Gerrit")
if (isolate->has_exception()) {
return isolate->exception();
}Magic macro for such cases: `RETURN_FAILURE_IF_EXCEPTION(isolate);`
Nit: I think the caller function `KeyAccumulator::CollectOwnKeys(..)` is a better place for handling this case. Right after `if (IsAccessCheckNeeded(*object) &&`.
Done
Caio Lima// https://tc39.es/proposal-defer-import-eval/#sec-IsSymbolLikeNamespaceKey
Done
We should return `Just(false);`, `Nothing` is an indication that exception was thrown.
Acknowledged
case LookupIterator::DEFERRED_MODULE_NAMESPACE:Caio LimaThis case might be reachable via `ns.propertyIsEnumerable(..)`.
Igor SheludkoThanks for pointing this out. I'm adding a test case for it as well. I don't know why I missed this comment before.
When using `Object.prototype.propertyIsEnumerable`, it actually goes to `JSReceiver::GetOwnPropertyAttributes` (ref: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-object.cc;l=31;drc=d26244cc105a77ea0708ee85fd8cf2e3ab33c608), that is analogous to `[[GetOwnProperty]]` from spec (ref: https://tc39.es/ecma262/#sec-object.prototype.propertyisenumerable).
Also, it's important to notice that `ns.propertyIsEnumerable` will trigger evaluation due to `[[Get]]` first, so at this point, this `DEFERRED_MODULE_NAMESPACE` state should be unexpected as well. Another thing that's important is that namespace's prototype is null, and since it's also non-extensible, it can't be changed. The way to execute `propertyIsEnumrable` is through `Object.prototype.propertyIsEnumerable(ns, ...)`.
Ack!
Nit: please put this comment inside `case LookupIterator::DEFERRED_MODULE_NAMESPACE:`.
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
if (it->HolderIsReceiverOrHiddenPrototype()) {Caio LimaThis doesn't seem to be correct. As Nicolò Ribaudo pointed out in another CL, once we reached `[[Set]]` for deferred module, regardless of whether it happened for a direct store to a module of through a prototype chain of another object - we are done. So we shouldn't do this check.
BTW, IIUC, we should not be doing `IsSymbolLikeNamespaceKey(P, O)` step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for `!IsSymbolLikeNamespaceKey(P, O)` case and deferred module somewhere.
Caio LimaOh right, I misunderstood the spec at this part. I just figured out now that the logic to install a new property is here (https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor), and it actually happens inside prototype.[[Set]].
BTW, IIUC, we should not be doing IsSymbolLikeNamespaceKey(P, O) step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for !IsSymbolLikeNamespaceKey(P, O) case and deferred module somewhere.
Thinking this a little deeper, I think we might have a bug for all module namespaces, both deferred and ordinary. The way we implement module namespace right now allows to a program like the one bellow to execute without errors, but it actually should throw when running on strict mode:
```
import * as ns from './foo_module.mjs'let obj = Object.create(ns);
obj.missingProp = 10;
obj.missingProp; // returns 10, but this set should never happen.
```The design I was thinking here was that any access that would trigger an evaluation, we actually throw an error. Otherwise fallback to ordinary ModuleNamespace logic - which shares the same [[Set]] we have for Deferred Modules (https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-set-p-v-receiver). I did that because I couldn't find a way to know if a Lookup is happening to a [[Set]] operation. However, since ordinary namespaces looks broken as well, it doesn't work.
Would it be too problematic if I add a configuration in LookupIterator to be able to know if it's a lookup for a [[Set]]?
Igor SheludkoDoing such a way in lookup iterator, we would be able to probably fix the issue we have for ordinary Namespaces.
An alternative I can think could be to add a check in `NOT_FOUND` and check if the last check "holder" is a `JSModuleNamespace`, and if this is the case, we throw an error if we are in strict mode. The thing there is that we can't ever have a `JSModuleNamespace` in the middle of a prototype chain, since its prototype is null and it's non extensible. So it needs to be the last object of the chain.
Caio LimaI think what would help is to have just one new `MODULE_NAMESPACE` state which would be set for both normal and deferred module namespace object. And the reaction to it would be to trigger evaluation of the deferred module if a particular operation requires it and/or continue lookup.
Igor SheludkoIs it fine if after a state `MODULE_NAMESPACE` happens and we call `LookupIterator::Next()` it will fallback to perform a `LookupInRegularHolder`? If so, I can try to do that. This would mean that I'll end up calling `continue` on pretty much all places when holder is not a JSDeferredModuleNamespace, but this can solve this issue.
I've implemented the version that checks on `NOT_FOUND` state of `SetPropertyInternal`, but thinking this through, I think it's not acceptable, since it'll slow down every `obj.prop = ...` operation that takes the slow path.
Yes, it's totally fine, given that JSModuleNamespaces require such an orthogonal behavior.
I changed it to use `MODULE_NAMESPACE` now. The design idea here is that when in this state, it triggers evaluation for deferred module, and then continue the lookup to properly perform the operation. This looks better than before, because deferred and ordinary module namespaces share the same execution path, with just a difference of evaluation trigger when it's required.
Related details
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 10 comments![]( "Open in Gerrit")
DCHECK(ns->module()->status() != Module::kEvaluated);`DCHECK_EQ`
case LookupIterator::MODULE_NAMESPACE:I think the right thing would be to set state to NotFound and return undefined (as in JSPROXY case) for deferred module requiring evaluation.
JSDeferredModuleNamespace::EvaluateModuleSync(it->isolate(), holder);We need to handle exceptions:
```
RETURN_EXCEPTION_IF_EXCEPTION(it->isolate());
```
// TODO(348660658): replace language modePlease revert these changes, having a link eases opening the bug tracker.
case LookupIterator::MODULE_NAMESPACE:Please double-check this case. I think we can get here via `[[DefineOwnProperty]]` (EnforceDefineSemantics::kDefine).
RETURN_VALUE_IF_EXCEPTION(isolate_, Nothing<bool>());Nit: `RETURN_EXCEPTION_IF_EXCEPTION(isolate_);` would be nicer.
if (it->HolderIsReceiverOrHiddenPrototype()) {Caio LimaThis doesn't seem to be correct. As Nicolò Ribaudo pointed out in another CL, once we reached `[[Set]]` for deferred module, regardless of whether it happened for a direct store to a module of through a prototype chain of another object - we are done. So we shouldn't do this check.
BTW, IIUC, we should not be doing `IsSymbolLikeNamespaceKey(P, O)` step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for `!IsSymbolLikeNamespaceKey(P, O)` case and deferred module somewhere.
Caio LimaOh right, I misunderstood the spec at this part. I just figured out now that the logic to install a new property is here (https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor), and it actually happens inside prototype.[[Set]].
BTW, IIUC, we should not be doing IsSymbolLikeNamespaceKey(P, O) step for [[Set]] operation, it disallows ANY property store. I guess there should be a similar counter part for !IsSymbolLikeNamespaceKey(P, O) case and deferred module somewhere.
Thinking this a little deeper, I think we might have a bug for all module namespaces, both deferred and ordinary. The way we implement module namespace right now allows to a program like the one bellow to execute without errors, but it actually should throw when running on strict mode:
```
import * as ns from './foo_module.mjs'let obj = Object.create(ns);
obj.missingProp = 10;
obj.missingProp; // returns 10, but this set should never happen.
```The design I was thinking here was that any access that would trigger an evaluation, we actually throw an error. Otherwise fallback to ordinary ModuleNamespace logic - which shares the same [[Set]] we have for Deferred Modules (https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-set-p-v-receiver). I did that because I couldn't find a way to know if a Lookup is happening to a [[Set]] operation. However, since ordinary namespaces looks broken as well, it doesn't work.
Would it be too problematic if I add a configuration in LookupIterator to be able to know if it's a lookup for a [[Set]]?
Igor SheludkoDoing such a way in lookup iterator, we would be able to probably fix the issue we have for ordinary Namespaces.
An alternative I can think could be to add a check in `NOT_FOUND` and check if the last check "holder" is a `JSModuleNamespace`, and if this is the case, we throw an error if we are in strict mode. The thing there is that we can't ever have a `JSModuleNamespace` in the middle of a prototype chain, since its prototype is null and it's non extensible. So it needs to be the last object of the chain.
Caio LimaI think what would help is to have just one new `MODULE_NAMESPACE` state which would be set for both normal and deferred module namespace object. And the reaction to it would be to trigger evaluation of the deferred module if a particular operation requires it and/or continue lookup.
Igor SheludkoIs it fine if after a state `MODULE_NAMESPACE` happens and we call `LookupIterator::Next()` it will fallback to perform a `LookupInRegularHolder`? If so, I can try to do that. This would mean that I'll end up calling `continue` on pretty much all places when holder is not a JSDeferredModuleNamespace, but this can solve this issue.
I've implemented the version that checks on `NOT_FOUND` state of `SetPropertyInternal`, but thinking this through, I think it's not acceptable, since it'll slow down every `obj.prop = ...` operation that takes the slow path.
Caio LimaYes, it's totally fine, given that JSModuleNamespaces require such an orthogonal behavior.
I changed it to use `MODULE_NAMESPACE` now. The design idea here is that when in this state, it triggers evaluation for deferred module, and then continue the lookup to properly perform the operation. This looks better than before, because deferred and ordinary module namespaces share the same execution path, with just a difference of evaluation trigger when it's required.
Acknowledged
case LookupIterator::MODULE_NAMESPACE:This should be reachable if super constructor returns module.
```
class A { constructor() { return ns; } };
class B extends A { constructor() { super(); super.x = 14; super.y = 44; } };
```
case LookupIterator::MODULE_NAMESPACE:IIRC, by this moment deferred module should be evaluated... Please add a DCHECK for documenting purposes.
Related details
- Caio Lima
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 8 comments![]( "Open in Gerrit")
DCHECK(ns->module()->status() != Module::kEvaluated);Caio Lima`DCHECK_EQ`
Done
case LookupIterator::MODULE_NAMESPACE:I think the right thing would be to set state to NotFound and return undefined (as in JSPROXY case) for deferred module requiring evaluation.
Done
JSDeferredModuleNamespace::EvaluateModuleSync(it->isolate(), holder);We need to handle exceptions:
```
RETURN_EXCEPTION_IF_EXCEPTION(it->isolate());
```
Done
// TODO(348660658): replace language modePlease revert these changes, having a link eases opening the bug tracker.
Done
case LookupIterator::MODULE_NAMESPACE:Please double-check this case. I think we can get here via `[[DefineOwnProperty]]` (EnforceDefineSemantics::kDefine).
I'm going to add test for those.
When using `Object.defineProperty`, this goes through https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/js-objects.cc;l=1200;drc=26caa21b2ca37db2138418593e1db8a9832f0937 before reaching here.
The other case I also tested is the following:
```
class A { constructor() { return ns; } };
class B extends A {
prop1 = 42;
};
```
In this case, it will take the `DefineNamedOwnProperty` opcode and then fail before calling `DefineOwnPropertyIgnoreAttributes`. In this case, I think we should consider this to be unreachable for `MODULE_NAMESPACE`. WDYT?
RETURN_VALUE_IF_EXCEPTION(isolate_, Nothing<bool>());Nit: `RETURN_EXCEPTION_IF_EXCEPTION(isolate_);` would be nicer.
Done
case LookupIterator::MODULE_NAMESPACE:This should be reachable if super constructor returns module.
```
class A { constructor() { return ns; } };
class B extends A { constructor() { super(); super.x = 14; super.y = 44; } };
```
Done
case LookupIterator::MODULE_NAMESPACE:IIRC, by this moment deferred module should be evaluated... Please add a DCHECK for documenting purposes.
- Igor Sheludko
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
case LookupIterator::MODULE_NAMESPACE:Caio LimaPlease double-check this case. I think we can get here via `[[DefineOwnProperty]]` (EnforceDefineSemantics::kDefine).
I'm going to add test for those.
When using `Object.defineProperty`, this goes through https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/js-objects.cc;l=1200;drc=26caa21b2ca37db2138418593e1db8a9832f0937 before reaching here.
The other case I also tested is the following:
```
class A { constructor() { return ns; } };
class B extends A {
prop1 = 42;
};
```In this case, it will take the `DefineNamedOwnProperty` opcode and then fail before calling `DefineOwnPropertyIgnoreAttributes`. In this case, I think we should consider this to be unreachable for `MODULE_NAMESPACE`. WDYT?
I think I agree, except for builtins like %DefineKeyedOwnPropertyInLiteral. So as long we are not fuzzing them that should be ok.
Related details
- Caio Lima
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |