Normative: GetExportedNames adjustments for late-defined export names #1306
Conversation
ljharb
added
normative change
|
Since this is purely for downstream specifications and doesn't affects any mechanisms of Source Text Modules, shouldn't this be marked editorial? No behavior changes for anything in the JS semantics, just some extra data is being exposed for downstream. |
|
Requiring extra data to be exposed imo is a normative requirement. |
guybedford
force-pushed
the
dynamic-module-changes
branch
from
d99bdbd
to
143cb9a
Compare
How can that possibly be the case? A conforming ECMAScript implementation is not required to support anything other than ECMAScript Source Text Modules and isn't required to provide a mechanism to for extending itself with other kinds of modules or to expose any data to support such a mechanism. The whole abstract module hierarchy is there only to informatively say other implementation defined module types could exist and if they exist here is the data they must expose to interoperate with Source Text Modules. |
2018.09: add PR “GetExportedNames adjustments for Dynamic Modules” tc39/ecma262#1306
|
Ok, then that’s an interface contract - and updating a contract is a normative change. |
|
Ok, just looked at the Dynamic Modules Proposal readme for the first time... Here is my fundamental concern about the change to GetExportedNames. Some significant though went into the original versions of these algorithms to make sure that (at least for source text modules) module linking could be done ahead of time by a compiler or static linker. On implication of that is that there can't be any link-time dependencies upon ECMAScript runtime entities/state. Namespace exotic objects are runtime entities. They don't exist statically outside of a running ES engine. If nsModule is a reference to a namespace object then by making it a parameter to GetExportedNames (as defined by AbstractModuleRecords) you are introducing a runtime state dependency into the linking algorithms. However, just from the read me, I'm not sure whether or not that is what nsModule is actually a reference to, so I could be wrong. |
|
@allenwb thanks for taking a look. I previously called this |
|
@guybedford Some of this may come down to naming conventions. Names ending in Object are runtime entities that are reified to ECMAScript user land. Names ending in Record are state holders that aren't directly reified but whether they represent runtime state or static (ie, available outside runtime) state depends upon careful consideration of the nature of the state. For most of the language, the distinction between "Static Semantics" and "Runtime Semantics" is sufficient because the static semantics state is generally derivable by source code inspection of a single script/module. But module loading is in the middle ground because it occurs (at least in browsers) during runtime but is intended to primarily deal with static analyzable relationships between multiple perhaps dynamically acquired modules . It's still fundamentally about relationships that are fixed during module linking, even if the module was dynamically acquired or even dynamically constructed immediately before module linking. Ideally naming choices should help spec. readers/implementors better understand this distinction. |
guybedford
force-pushed
the
dynamic-module-changes
branch
from
143cb9a
to
0d8a128
Compare
|
Thanks for writing this up and for the reviews here. I'd classify this as a "Layering:" change, as it doesn't change the runtime semantics of anything now, but does add hooks for certain embedders. I'm looking forward to discussing this patch at TC39--getting it down to the minimal change that would need to be layered into ECMAScript really makes it easier for the committee to review whether we are comfortable with the change. |
|
Further to the last meeting where it was determined that implementation feedback was needed here, I went ahead and implemented the Dynamic Modules Spec (https://github.com/nodejs/dynamic-modules), which depends on this and it all worked out exactly as expected here, with the spec changes as in this PR. Complete implementation at https://chromium-review.googlesource.com/c/v8/v8/+/1303725. |
|
I've updated this PR to reflect the latest directions of the dynamic modules spec where instead of throwing an error when there are cycles with unexecuted dynamic module namespaces (which may be very hard to debug) instead the namespace is just treated as uninitialized through a null return here. |
spec.html
Outdated
| Any, default value is *undefined*. | ||
| </td> | ||
| <td> | ||
| Field reserved for use by host environments that need to associate additional information with a Module Namespace Exotic Object. |
There was a problem hiding this comment.
I'm a bit confused why this is needed - implementations can always add any number of internal slots (or side-channel metadata) to any value.
There was a problem hiding this comment.
I've applied this in https://github.com/nodejs/dynamic-modules/blob/master/spec.html#L203.
The nice thing about this is that it avoids having to declare the field in the dynamic modules spec, retaining the spec separation of simply implementing a subclass of module record and not changing any other objects or fields.
There was a problem hiding this comment.
Other suggestions welcome too.
There was a problem hiding this comment.
It seems like if the only spec to use it is the dynamic modules spec, then that's the place it should be declared :-/
There was a problem hiding this comment.
Are you saying that a spec can add fields to any object in the ECMA-262 spec while remaining valid against ECMA-262? I thought the point of [[HostDefined]] was to clarify these cases?
There was a problem hiding this comment.
I’m not sure how or why the spec could or would prevent it. Engines can always use a side channel, like a weak map - they’re already allowed to add any user-visible properties they want - i don’t see why they couldn’t add their own internal slots, as long as the names didn’t collide.
|
This isn’t about what implementations can do though, it’s about what
another spec can do. We just need to ensure that such a spec is fully
compatible with ECMA262. And extending a new field on an object in another
spec seems like exactly the use case [[HostDefined]] was created for to me.
Alternative approaches welcome though if I’m getting this wrong.
…On Wed, 14 Nov 2018 at 08:27, Jordan Harband ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec.html
<#1306 (comment)>:
> @@ -8450,6 +8450,17 @@ <h1>Module Namespace Exotic Objects</h1>
A List containing the String values of the exported names exposed as own properties of this object. The list is ordered as if an Array of those String values had been sorted using `Array.prototype.sort` using *undefined* as _comparefn_.
</td>
</tr>
+ <tr>
+ <td>
+ [[HostDefined]]
+ </td>
+ <td>
+ Any, default value is *undefined*.
+ </td>
+ <td>
+ Field reserved for use by host environments that need to associate additional information with a Module Namespace Exotic Object.
I’m not sure how or why the spec could or would prevent it. Engines can
always use a side channel, like a weak map - they’re already allowed to add
any user-visible properties they want - i don’t see why they couldn’t add
their own internal slots, as long as the names didn’t collide.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1306 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAkiysVntmWRqdVMbt28NJyRDKZpYemGks5uu7fKgaJpZM4WlqJ->
.
|
|
I'd love to hear others' thoughts on it, but there's nothing in the spec I'm aware of that restricts what hosts can do with respect to adding internal slots - especially considering the host, or another spec, could always put the object in a WeakMap to simulate an internal slot. |
|
Although technically hosts can do crazy things, it's much more friendly to spec readers and to implementers if integration points are called out explicitly and made to interoperate in the same way they would in implementations. I strongly support adding [[HostDefined]] fields where appropriate in this way to increase the fidelity and readability of the spec's layering. |
|
@domenic i agree in principle, but how do different hosts and specs use the HostDefined slot currently that requires interop? Why not implicitly define that slot on every value, if it’s needed? In this PR, the slot is initially set to |
|
I think @guybedford linked you to a pull request that explicitly uses the [[HostDefined]] slot to give better layering and implementability.
The point of my above post was that explicit is better than implicit, and that it's not needed on every value: it's needed on specific ones, which benefit from having clear specs emplacing them on those values.
The point of my above post is that hosts should not have to do wacky things like installing slots (which, remember, in ECMAScript are immutable at object creation time) as a monkey-patch onto other specs. They should instead explicitly coordinate with the base spec to create explicit, clear hook points (in this case, a [[HostDefined]] slot). |
This provides two features needed for supporting late-defined export names on custom module records. Firstly, a second argument to GetExportedNames of the namespace module being constructed by this call, allowing it to be tracked, and secondly permitting ResolveExport and GetExportedNames operations to provide abrupt completions corresponding to instantiation errors.
guybedford
force-pushed
the
dynamic-module-changes
branch
from
8617999
to
3805754
Compare
|
Over the course of Dec-Jan I raised a concern with the current dynamic module proposal because it mandates syntax like
However, with the current proposal SourceText Module Records will still populate their namespace objects during instantiation which means, even if we tackle I think to keep |
|
I don't think we should make any changes to how Source Text Module Records work. |
Are you suggesting the work be done in the Abstract Module Record or that late population of the namespace objects for non-dynamic modules is something you don't want to be possible? |
|
I think whatever goals are desired for this Node.js-specific module type should be done by not affecting the behavior of Source Text Module Records, and should not make STMRs dependent on the concepts of those environment-specific types. |
Ah okay. That diminishes the amount of possible interop between Dynamic and Source Text module records. Would you be open to instead having a hole in spec text enabling implementations the freedom to modify module records to accommodate additional record type needs (such as late-defined exports) outside of the ECMA262 as long as the interactions between Source Text and other spec'd module records, without outside Module record interaction, are spec compliant? |
|
I totally agree with the point @domenic has been making. Modules defined using ECMAScript code (ie, "Source Text Modules") were carefully designed around the concept of a statically determining and resolving a set of shared bindings. The complete set of shared bindings is determined before evaluation of such a module. The possibility of additional bindings becoming available for direct import (or indirectly available via modification of a syntactically imported module namespace) was excluded from the design so that ahead of time linking could be reliably performed. This is the semantics of an ECMAScript module. An ECMAScript module can (in theory) import bindings from something other than ECMAScript modules. But, to properly plug into the expectations of an ECMAScript module, such "foreign" modules must expose a fixed set of exported bindings at link time, just like an ECMAScript module. As an aside, I think the title of the "Dynamic Modules" proposal is terribly confusing and mislead given that we now also will have the "dynamic" Such "foreign modules" should be statically importable by a source text module as long as the foreign module implementation mechanism exposes a complete and fixed set of exported bindings to the ESM linking process. The fact that such a module is "built-in" or implemented in C++ or even in JS should be a problem, long as its definitional process is complete before any FromCause references to it are processed. I don't see any need to change the semantics of ECMAScript source modules to accommodate a reasonably specified foreign module mechanism. |
The issue we're facing in Node-land is CJS modules (dynamic-modules) don't have their This trick works OK but falls down for In my own experiments one way around it, that I've gotten to work, is in part due to deferring the Source Text Module namespace object population/finalization for cases of While deferring Source Text Module namespace object population/finalization is one possible way I'd ❤️ other ideas for, and for folks to noodle on, how to accommodate |
|
this consensus implied, to me, that if what’s required means that even if all named imports can’t be statically resolved at linking time, the committee is willing to bend on that part of the design for ESM in order to acoomodate node (iow, no commitment to any specific solution, but commitment to prioritizing “solving the problem for node” higher than “the initial design constraints for ESM”) |
|
It seems to me that, if Node can't uphold the semantics of a Source Text Module Record for whatever reason, they should instead use their own type of module record, instead of bringing everyone else down to their level by changing the core language semantics. Another way of looking at it would be that, as far as I know at least, the only environment so far that's managed to implement the Source Text Module Record is the browser. Maybe it'd be better to move the STMR spec into the web spec space, instead of trying to pretend it's part of a core, host-agnostic language semantic. In this way it would join the jobs infrastructure (#735) in the pile of things where ECMA-262 overreached and tried to specify something as "language" level, but it ended up not being suitable for realistic implementations. In this world, "Web JavaScript Module Records" and "Node.js Module Records" would be on equal footing, alongside WebAssembly Module Records, HTML Module Records, JSON Module Records, etc. They would all be extensions of Abstract Module Record. (Or, even better if #1311 ever lands, extensions of Cyclic Module Record.) |
|
There's a bunch going on in this discussion, so I want to try to break it out a bit, summarizing the above discussion to make next steps more clear. Let's consider individual Q/A questions here: Q: Given the goal of allowing static and dynamic modules to interact in Node.js, what does TC39 think of this proposal? A: Many TC39 delegates (e.g., at the November 2018 breakout sessions) expressed support for working on this problem, but skepticism (including @zenparsing above) about the semantics of the particular re-export case. This current PR seems to address those concerns. Q: What does TC39 think of integrating dynamic and static modules with this level of dynamism? A: @allenwb expressed some concerns about this integration above. We don't seem to have consensus as a committee as to whether this is a good thing to pursue, given that others expressed support. Offline, I have heard another angle of concern, that it would be unfortunate to implement this interoperability in Node.js when we don't expect it to ever make it to the Web. Q: For host environments that will only support source text module records, and have no plans to add dynamic module records, is TC39 OK with adding additional conditionals in the code path for source text module records? A: I think @domenic is arguing "no" above. Q: Is TC39 interested in layering patches for non-web environments? Are we interested in being consulted on host environment integration issues like this? A: (I think we should be open to both of these!) This PR is based on the assumption that the answer is yes, and no one is arguing explicitly against that, but I am not sure if we all agree on this point. Probably Node could move forward with a parallel type to Source Text Module Records to manage all of this, with the minor change, and leave all the rest in tact. The big change would be that some embedding environments would be modifying the The result of doing everything in a separate document would be either inconsistency/vagueness, a lot of duplication or a little bit of monkey patching, all of which @domenic and @annevk have done a lot of good work to reduce over time with the Web embedding. Many JS engines are used in both the Web and Node.js, so implementers of those engines need to be aware of the set of relevant spec documents regardless of whether it goes into ECMA-262 or not. If this is to be maintained as a separate document, I think the clearest way to present it would be ins tags around part of the TC39 text, like the current proposal. If we are, in general, not interested in such layering patches for other environments, and just for technical review, let's make this clear to help everyone save time. So, what are our recommendations for @guybedford from here? The options I see are:
|
|
@domenic
It isn't clear to me why CJS modules need to start acting as if they where ESMs. There is no backwards compatibility reason for a CJS module to export bindings to an ESM or to participate in circularities involving ESMs. There are speculative scenarios that can only exist in an environment that has both ESMs (conforming to the specified semantics) coexisting with CJS. But in such an environment if you want to export bindings or participate in circuaritwes why wouldn't you simply define a ESM rather than defining a new CJS module? |
|
@littledan |
|
@allenwb the reason is that both conceptually, and in practice for many years via babel, that’s how virtually the entire npm-using javascript community has expected to be able to import CJS modules. Whether you agree with it or not, this interoperability point is critical for some in the node modules working group to be able to ship ESM at all. If ESM with STMR can’t be implemented by a non-web implementation like node, then perhaps @domenic’s suggestion above would make sense. |
|
To follow up on @ljharb and +1 (I was writing when his reply came in). The goal of dynamic-module is to improve CJS+ESM interoperability by taking the implementation from its current experimental status of
which won't likely ship unflagged in Node. To one with named exports, which has a higher likelihood of shipping. One of the wins with dynamic-modules is that it more closely meets developer expectations of interoperability (established by Babel, TypeScript, Webpack, etc.).
It would be ideal to avoiding associating yet more metadata, and taxing developers with that backfilling/maintenance cost, so that the existing ecosystem of hundreds of thousands of CJS modules can be loaded. I think @domenic may be on to something with his suggestion:
|
|
I'm glad we're getting to the core of the argument. If I may clarify here very briefly, there are no changes to STMR in this PR, or in the previous version that did support export * handling. Nothing being discussed for either case in any hypothetical scenario would involve changes to STMR, so I'm not sure how this misunderstanding arose. |
|
I think we're generally discussing @jdalton's proposal to change it in #1306 (comment). Apologies if that was off-topic for this PR; it's unclear to what extent each of you represents the Node community's preferences and how his message ties to this PR's contents. |
|
@domenic note that this very PR with the guard on the export * case removed, would exactly support source text module records having their namespaces put into the same pending state in cycles (exactly as @jdalton is asking) to complete their exports on execution as dynamic modules get if they star export from dynamic modules. This is supported naturally through the way the algorithm uses a null return from GetExportedNames to indicate this. But this case he mentions doesn't come up at all with export * guarded as it is now. |
| @@ -22179,6 +22191,11 @@ <h1>ModuleDeclarationEnvironmentSetup ( _module_ )</h1> | |||
| 1. Let _resolution_ be ? _importedModule_.ResolveExport(_in_.[[ImportName]], « »). | |||
| 1. If _resolution_ is *null* or `"ambiguous"`, throw a *SyntaxError* exception. | |||
| 1. Call _envRec_.CreateImportBinding(_in_.[[LocalName]], _resolution_.[[Module]], _resolution_.[[BindingName]]). | |||
| 1. For each ExportEntry Record _e_ in _module_.[[StarExportEntries]], do | |||
There was a problem hiding this comment.
This is a change to Source Text Module Records, so I am unsure why you are saying there are no changes to STMR in this PR.
There was a problem hiding this comment.
Ah, yes, apologies I mispoke on that one.
| @@ -22026,14 +22036,16 @@ <h1>GetExportedNames ( _exportStarSet_ ) Concrete Method</h1> | |||
| 1. For each ExportEntry Record _e_ in _module_.[[StarExportEntries]], do | |||
| 1. Let _requestedModule_ be ? HostResolveImportedModule(_module_, _e_.[[ModuleRequest]]). | |||
| 1. Let _starNames_ be ? _requestedModule_.GetExportedNames(_exportStarSet_). | |||
| 1. If _starNames_ is *null*, then | |||
|
@linclark @guybedford do you think it would be possible to rebase against #1311 and extent cyclic module records instead of source text module record? @domenic would that appease your concerns related to modifying STMR? |
|
No, it's the modification to core ES infrastructure that's problematic, not the particular name "Source Text Module Record" vs. "Cyclic Module Record". |
|
Closing as this is blocked on various constraints. Here is a brief summary for future reference:
The only alternative to the above is to change the way execution order happens in cycles such that the dynamic modules namespaces in cycles are no longer observable at all in an unexecuted state. This was created at nodejs/dynamic-modules#3 if anyone wants to pick it up. Apart from the above, this means that defining export names after instantiation for any module record simply seems like it will never be possible in ECMA-262. As a result CommonJS must either execute during instantiation, or only define a default export. |
This PR provides the absolute minimum changes necessary for the Dynamic Modules proposal to be possible as a non-ECMA262 specification.
There are primarily two features:
This PR will hopefully be covered in more detail at the coming meeting.