Skip to content
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

Normative: Add Source Phase Imports #3094

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

lucacasonato
Copy link
Member

</table>
</emu-table>

<emu-note type="editor">In general, this proposal replaces places where module specifiers are passed around with ModuleRequest Records. For example, several syntax-directed operations, such as ModuleRequests produce Lists of ModuleRequest Records rather than Lists of Strings which are interpreted as module specifiers. Some algorithms like ImportEntries and ImportEntriesForModule pass around ModuleRequest Records rather than Strings, in a way which doesn't require any particular textual change. Additionally, record fields in Cyclic Module Records and Source Text Module Records which contained Lists of Strings are replaced by Lists of ModuleRequest Records, as indicated above.</emu-note>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exact layout of this record depend on #3057. There is cleanup to be done around how and where exactly phases are passed around (and if they are part of this record), but this will be less merge conflict prone once Nicolo's PR is merged.

The PR is technically correct as is, so I'll defer this work until Import Attributes is landed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll also have to add [[Phase]] to LoadedModuleRequest Records.

@lucacasonato lucacasonato force-pushed the source_phase_imports branch from 07332fb to 52fed39 Compare June 8, 2023 18:33
@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. labels Jun 8, 2023
<li>along with its corresponding prototype object, provides common properties that are inherited by module source constructors and their instances.</li>
<li>does not have a global name or appear as a property of the global object.</li>
<li>acts as the abstract superclass of the _ModuleSource_ constructor.</li>
<li>will throw an error when invoked, because it is an abstract class constructor. The module source constructors do not perform a `super` call to it.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure we have the concept of an "abstract" class constructor; can't it be a normal constructor that throws both when [[Call]]ed or when it's its own new.target?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb Do you mean you want it to be possible to do like

let AbstractModuleSource = /* some coe which gets AbstractModuleSource somehow */;
class A extends AbstractModuleSource {}

new A()

and have that not throw?

If so, that can't be done while keeping the property mentioned above, that these things are supposed to all have a [[ModuleSourceRecord]] internal slot holding a ModuleSource Record - there's no way to provide one when constructing from user code.


Or do you just mean you don't like the wording "abstract class constructor" here, and are OK with the behavior but want different wording?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantics are fine, i indeed just mean the word "abstract" doesn't appear in the spec in this context afaik, and I don't think we should introduce it short of a proposal to add some kind of language feature for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't it be a normal constructor that throws both when [[Call]]ed or when it's its own new.target?

Yes, we should do this, and we should get rid of

The module source constructors do not perform a super call to it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the resolution of this? The current version I'm reading has an %AbstractModuleSource% constructor that unconditionally throws for both [[Call]] and [[Construct]].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here is the following:

  • User code can not [[Call]] and [[Construct]] the %AbstractModuleSource% constructor.
  • Host objects can inherit from %AbstractModuleSource%.
  • Host objects (like WebAssembly.Module) that inherit from %AbstractModuleSource% should be able to be constructed by user code (if the relevant host object constructor allows this).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User code should be able to class extends AbstractModuleSource, though, otherwise you can't self-host a JS implementation and get the right internal slot.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since only hosts are able to implement custom module sources, and not user code, this is explicitly designed to ensure class extends AbstractModuleSource doesn't work.

spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
GetModuleSource()
</td>
<td>
<p>It returns either a normal completion completion for the ModuleSource Object corresponding to this source Module Record's source phase (<emu-xref href="#sec-module-source-objects"></emu-xref>), or a throw completion.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below this is spelled "Module Source Object", rather than "ModuleSource Object". It should be consistent (and ideally <dfn>'d).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a space in tc39/proposal-source-phase-imports#52. I don't want to call this a definition since these are a class of objects as opposed to a specific object here.

<li>along with its corresponding prototype object, provides common properties that are inherited by module source constructors and their instances.</li>
<li>does not have a global name or appear as a property of the global object.</li>
<li>acts as the abstract superclass of the _ModuleSource_ constructor.</li>
<li>will throw an error when invoked, because it is an abstract class constructor. The module source constructors do not perform a `super` call to it.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb Do you mean you want it to be possible to do like

let AbstractModuleSource = /* some coe which gets AbstractModuleSource somehow */;
class A extends AbstractModuleSource {}

new A()

and have that not throw?

If so, that can't be done while keeping the property mentioned above, that these things are supposed to all have a [[ModuleSourceRecord]] internal slot holding a ModuleSource Record - there's no way to provide one when constructing from user code.


Or do you just mean you don't like the wording "abstract class constructor" here, and are OK with the behavior but want different wording?

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@jmdyck
Copy link
Collaborator

jmdyck commented Jun 12, 2023

Rather than making a bunch of suggestions here, I've created a PR against this PR's branch: lucacasonato#1

<emu-clause id="sec-static-semantics-modulerequests" oldids="sec-module-semantics-static-semantics-modulerequests,sec-imports-static-semantics-modulerequests,sec-exports-static-semantics-modulerequests" type="sdo">
<h1>Static Semantics: ModuleRequests ( ): a List of Strings</h1>
<h1>Static Semantics: ModuleRequests ( ): a List of ModuleRequest Records</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ExportEntries(), there's an invocation of ModuleRequests that needs to accommodate this change in the return-type.

spec.html Outdated
GetModuleSource()
</td>
<td>
<p>It returns either a normal completion completion for the ModuleSource Object corresponding to this source Module Record's source phase (<emu-xref href="#sec-module-source-objects"></emu-xref>), or a throw completion.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p>It returns either a normal completion completion for the ModuleSource Object corresponding to this source Module Record's source phase (<emu-xref href="#sec-module-source-objects"></emu-xref>), or a throw completion.</p>
<p>It returns either a normal completion containing the Module Source Object corresponding to this Module Record, or a throw completion.</p>

Assuming we <dfn> "Module Source Object" below.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
<h1>Module Source Objects</h1>
<p>Module Source Objects represent modules in their source import phase, which are not linked, instantiated or executed.</p>
<p>All Module Source Objects should have a prototype of %AbstractModuleSource%.prototype and must define a [[ModuleSourceRecord]] internal slot.</p>
<p>Hosts may define their own Module Source subclasses for custom module record types.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "record" mean here?

Suggested change
<p>Hosts may define their own Module Source subclasses for custom module record types.</p>
<p>Hosts may define their own Module Source subclasses for custom module types.</p>

Also, wouldn't they be %AbstractModuleSource% subclasses?

Suggested change
<p>Hosts may define their own Module Source subclasses for custom module record types.</p>
<p>Hosts may define their own %AbstractModuleSource% subclasses for custom module types.</p>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we're trying to say "Hosts may define their own %AbstractModuleSource% subclasses for custom Module Record subtypes/subclasses/subrecords."

Not sure about that last word.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % some questions.

P.S. Be sure to run npm run format.

spec.html Outdated
@@ -18754,6 +18754,7 @@ <h2>Syntax</h2>

ImportCall[Yield, Await] :
`import` `(` AssignmentExpression[+In, ?Yield, ?Await] `)`
`import` `.` `source` `(` AssignmentExpression[+In, ?Yield, ?Await] `)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking as a delegate, not editor, I've gotten feedback that there is a somewhat strong intuition that source refers to unparsed code. Are champions still open to bikeshedding the phase name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the unparsed source will be import.asset in the future. I don't know if there is a better name for this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Wasm uses module, but within the module harmony group we are of the opinion that module refers to a module instance (source + context).

This is reflected in the designs for Module expressions, Module declarations, and Module loaders.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internally in V8 we bikshedded handle and instantiable.

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
<li>along with its corresponding prototype object, provides common properties that are inherited by module source constructors and their instances.</li>
<li>does not have a global name or appear as a property of the global object.</li>
<li>acts as the abstract superclass of the _ModuleSource_ constructor.</li>
<li>will throw an error when invoked, because it is an abstract class constructor. The module source constructors do not perform a `super` call to it.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the resolution of this? The current version I'm reading has an %AbstractModuleSource% constructor that unconditionally throws for both [[Call]] and [[Construct]].

Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder that I created lucacasonato#1 a month ago.

[[Specifier]]
</td>
<td>
String
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
String
a String

@guybedford
Copy link

This is now superseded by #3492.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants