[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. |