tc39 / proposal-source-phase-imports

Proposal to enable importing modules at the source phase
https://tc39.es/proposal-source-phase-imports/
MIT License
132 stars 8 forks source link

Minimum viable module source #36

Closed guybedford closed 1 year ago

guybedford commented 1 year ago

Here's a first pass at what we've been discussing with respect to a minimum viable module source.

We define %AbstractModuleSource% as an intrinsic base class for module source class instances. It defines the toStringTag getter returning the internal [[SourceModuleRecord]]'s [[SourceClassName]] string (where [[SourceModuleRecord]] also has a [[HostSourceMetadata]] slot). This allows carrying host-context through this object back to the compilation phase with an abstract method that can be called by other specs within HostLoadImportedModule, where [[HostDefined]] on the instance record can be recreated on new instances in this way.

A new GetModuleSource object is used to provide the module source via Cyclic Module Record, and can be redefined by suitable cyclic module subclasses as necessary.

Feedback very welcome!

guybedford commented 1 year ago

@nicolo-ribaudo I've updated this PR to use your phase generalization model.

ljharb commented 1 year ago

@nicolo-ribaudo not precisely; the best approach for my use cases is if the base abstract class has a getter that returns the string contents of a slot, and subclasses override the slot but not the getter.

guybedford commented 1 year ago

Thanks @nicolo-ribaudo for the feedback.

I've gone back to a toStringTag getter, that checks the [[SourceClassName]] string, the typed array example was a great help, so it's now doing exactly that. This now exactly does as @ljharb describes - defining the toStringTag return entirely through the internal slot, and not through subclassing.

Then yes this currently specifies (2) as you list above. And yes I went with the simplification of not defining the %AbstractModuleSourcePrototype% constructor, since it is not necessary.

In terms of exposing any sourceType() in future, thinking about this further, perhaps we should rather expose attributes on the record in future via a moduleSource.attributes getter. If we did that one would be able to read the source type that way, which is distinct from the exact class anyway. And I think we nicely leave the door open for all of those kinds of things, to then be justified on their own merit.

lucacasonato commented 1 year ago

Just want to make sure we are all aware that there will be two @toStringTag for WebAssembly.Module. One will be on %AbstractModuleSource% (the one defined here). The other will be on WebAssembly.Module.prototype (defined by WebIDL). The latter will shadow the former. Both will return WebAssembly.Module. One can always access the former one through WebAssembly.Module.prototype.prototype[Symbol.toStringTag].

nicolo-ribaudo commented 1 year ago

I have some other observations about what ECMA-262 currently does.

  1. (@kriskowal) I don't particularly care about whether or not we have the %AbstractModuleSource% constructor, other than for a general tendency of reducing useless objects. The precedent goes in both directions:

    • %ArrayIteratorPrototype%, %AsyncFromSyncIteratorPrototype%, %AsyncIteratorPrototype%, %IteratorPrototype%, %MapIteratorPrototype%, %RegExpStringIteratorPrototype%, %SetIteratorPrototype%, %StringIteratorPrototype% are all constructor-less prototypes.
    • %AsyncFunction%, %AsyncGeneratorFunction% and %GeneratorFunction% are all non-exposed constructors (there isn't just the prototype), however they can actually be used to eval code.
    • %TypedArray% is the only non-exposed non-constructable constructor that exists, where just having the prototype object would have been enough.
    • the Itertor Helpers proposal introduces a new %Iterator% constructor (exposed as globalThis.Iterator), so that user-defined classes can extends Iterator.

    I have a slight preference here for not having %AbstractModuleSource% yet, and only add it if we'll need it for user-defined subclasses (i.e. by the virtual module sources proposal), but also just having it here following the TypedArray precedent is fine.

  2. (@lucacasonato) the Itertor Helpers introduces a new Iterator.prototype[@@toStringTag] property, and all the built-in iterator subclasses overwrite it. So that will create another place where we have multiple @@toStringTags in the prototype chain of an object, although in that case they would have different values.
guybedford commented 1 year ago

@nicolo-ribaudo I've handled the requests problem by piping the ModuleRequest record through the host resolve hooks.

Instead of explicitly creating ModuleSourceObject, the expectation of the HostLoadImportedModule hook is that when the phase is source, it must then populate the ModuleSourceObject. If it doesn't we then throw the reference error.

As far as I can tell this refactoring does in fact better align with import attributes. It also seems useful to provide the phase information to the host hook, so far as the preloader needs to be aware of the phasing, which seems critical information as well.

Some other minor edits were needed to get the piping to work, there may well be further future simplifications on this stuff (eg ModuleSource Records as first class host hooks, resources / internal registry as first class keys), but for now this seems like a reasonable compromise on the design while aligning with the other spec changes for the most part.

Let me know your thoughts further!

guybedford commented 1 year ago

Per @lucacasonato's feedback I've removed the global ModuleSource for now in this PR. His argument was that defining an object that does not have any use at all would not be reasonable at this point. At the very least we can still define AbstractModuleSource and make this reachable through WebAssembly.Module for the WebAssembly use case. JS sources would then continue to throw for now during link time, while having a path for support in future.

kriskowal commented 1 year ago

I’m reminded that we at Agoric take a strong position that every proposed new intrinsic like %AbstractModuleSource% or %AbstractModuleSource.prototype% must be reachable by transitive property access starting with globalThis and that this is a good enough reason for globalThis.ModuleSource to exist even if it is initially inert.

littledan commented 1 year ago

How are we doing on making the call of sourceType vs @@toStringTag type checks? I'd be OK with either outcome personally (as long as we don't have multiple strong type checks).

guybedford commented 1 year ago

@littledan the way that ended up last week was that we now have a toStringTag getter function on AbstractModuleRecord which checks a [[SourceClassName]] internal slot from the object and returns that as its value, exactly mirroring TypedArray as suggested by @nicolo-ribaudo.

ljharb commented 1 year ago

(most builtins have multiple slot checks, but any additional ones added to module things would, i hope, have an actual purpose beyond that)

guybedford commented 1 year ago

@nicolo-ribaudo I believe I have addressed all of your feedback, I think it would be useful to carefully distinguish editorial suggestions from normative changes at this point in the review process. Specifically editorial changes can be discussed after this PR is merged, but I know you'll agree getting this merged is the priority at this point.

I will go ahead and land this today, unless there are any further objections.

nicolo-ribaudo commented 1 year ago

@erights @kriskowal I added exposing %AbstractModuleSource% to the agenda for the next SES meeting, if that's ok.

bakkot commented 1 year ago

Heads up that 262 editors haven't had / probably aren't going to have time to review this before next week, so it's probably not going to be able to achieve stage 3 at this meeting for that reason.