ecma262 icon indicating copy to clipboard operation
ecma262 copied to clipboard

Normative: Add Import Attributes

Open nicolo-ribaudo opened this issue 2 years ago • 18 comments

This proposal (https://tc39.es/proposal-import-attributes/, https://github.com/tc39/proposal-import-attributes) reached stage 3 during the March 2023 meeting, conditional on stage 3 reviews. I'm opening this PR before "real" stage 3 to help editors reviewing the full spec changes.

The proposal has already been integrated in HTML: https://html.spec.whatwg.org/#hostloadimportedmodule

PREVIEW: https://ci.tc39.es/preview/tc39/ecma262/pull/3057 DIFF: https://arai-a.github.io/ecma262-compare/?pr=3057 (note: the diff doesn't mark deprecated sections)

nicolo-ribaudo avatar May 08 '23 11:05 nicolo-ribaudo

@tc39/ecma262-editors Currently the only remaining condition for this proposal to be actually Stage 3 is the editorial review. It'd be great if you have time to do it :)

nicolo-ribaudo avatar May 22 '23 08:05 nicolo-ribaudo

I pushed two more normative commits, that are oversights from the assert->with migration:

  1. Added assert as an alternative to with also in export ... from statements
  2. Removed the [no LineTerminator here] restriction before with

nicolo-ribaudo avatar Jun 13 '23 12:06 nicolo-ribaudo

I pushed two new commits:

  1. Validate attrs when lodaing deps and not when parsing fixes https://github.com/tc39/proposal-import-attributes/issues/144. From an ECMA-262 point of view this is purely editorial, because:
    • it's not currently observable whether an error is thrown when parsing or when calling LoadImportedModules()
    • code that resulted in a syntax error still results in a syntax error, and vice versa. there might be a difference in the location of the reported syntax error (see the example in the linked issue), but that's already completely implementation-defined.
  2. Merge AssertClause into WithClause, and fix missing SDOs while fixing https://github.com/tc39/ecma262/pull/3057#pullrequestreview-1495910003 I realized that I had to duplicate every SDO using WithClause into exactly the same definition but using AssertClause. There are many of them, and copy-pasting risks introducing problems when we update one and forget to update the other one. I ended up merging the two productions as originally suggested by @bakkot in https://github.com/tc39/ecma262/pull/3057#discussion_r1225713248, but with a production parameter to only allow assert in the [no LineTerminator here] case.

nicolo-ribaudo avatar Jun 26 '23 10:06 nicolo-ribaudo

Why not just tweak @bakkot's suggestion into:

AttributesKeyword :
  `with`
  [no LineTerminator here] `assert`

That would eliminate the [DeprecatedAssert] parameter and the deprecated definitions of ImportDeclaration and ExportDeclaration.

jmdyck avatar Jun 26 '23 13:06 jmdyck

[no LineTerminator here] is never used at the beginning/end of a production currently, but if the editors like that I think it's a very good idea.

nicolo-ribaudo avatar Jun 26 '23 13:06 nicolo-ribaudo

I pushed @jmdyck's suggestion, to show how much it would simplify the spec.

nicolo-ribaudo avatar Jun 26 '23 13:06 nicolo-ribaudo

[no LineTerminator here] is never used at the beginning/end of a production currently,

A [no LineTerminator here] constraint is similar in flavor to a [lookahead ...] constraint, and we have lots of occurrences of those at the start + end of productions (4 at the start, 24 at the end).

And there's nothing in the definition of [no LineTerminator here] that would preclude its use at the start/end of a production.

So I don't think we should have any qualms about using it in this way.

jmdyck avatar Jun 26 '23 14:06 jmdyck

I think the ModuleRequests SDO needs rule(s) to handle:

`import` ModuleSpecifier WithClause? `;`

jmdyck avatar Jun 26 '23 16:06 jmdyck

PR updated to remove support for float and bigint literal keys, as per consensus in the TC39 2023-09 meeting.

nicolo-ribaudo avatar Sep 28 '23 04:09 nicolo-ribaudo

PR updated to remove the assert variant, as per consensus in the TC39 2024-07 meeting.

@tc39/ecma262-editors I'd like to open a PR for https://tc39.es/proposal-json-modules/, which I plan to propose for stage 4 at the next meeting. The PR for that proposal needs to be based on top of this one. Guy would also like to rebase https://github.com/tc39/ecma262/pull/3094 on top of this PR. What approach do you prefer?

  1. I push this PR's branch to a temporary branch in this repository, and we open PRs targeting it
  2. those PRs include commits from this one
  3. we open those PRs in my fork, and you review there

nicolo-ribaudo avatar Jul 30 '24 14:07 nicolo-ribaudo

@nicolo-ribaudo I prefer 1 but 3 is also fine.

michaelficarra avatar Jul 30 '24 15:07 michaelficarra

It turns out I don't have push access to this repo. Could somebody (@michaelficarra? @ljharb?) push this PR's branch to this repo so I can open a PR targeting it? :) Or give me push access, I don't know what's the policy.

nicolo-ribaudo avatar Aug 13 '24 15:08 nicolo-ribaudo

I did it for you at https://github.com/tc39/ecma262/tree/import-attributes.

ryzokuken avatar Aug 13 '24 18:08 ryzokuken

@jmdyck Done, thanks!

nicolo-ribaudo avatar Aug 23 '24 13:08 nicolo-ribaudo

@tc39/ecma262-editors Remember that this is going for stage 4 at the next meeting :)

nicolo-ribaudo avatar Sep 24 '24 04:09 nicolo-ribaudo

Could somebody restart CI?

nicolo-ribaudo avatar Oct 09 '24 09:10 nicolo-ribaudo

1 commits have an unknown author: 57f427b18bf7e629565ac2fcf2392ba7b7d0d8fb

guybedford avatar Oct 18 '24 20:10 guybedford

@guybedford Yeah the IPR check periodically gets out whack; don't worry about it. We'll fix before landing.

bakkot avatar Oct 18 '24 20:10 bakkot

What's the status of this?

linusg avatar Nov 20 '24 17:11 linusg

@linusg It's stage 4, editors just haven't had time to get it landed yet.

bakkot avatar Nov 20 '24 17:11 bakkot

@bakkot thanks for the clarification - please just let us know if there's anything needed to help move this one along.

guybedford avatar Jan 23 '25 20:01 guybedford

It's now been four months since this reached stage 4 and there doesn't seem to be any movement still. Could we get an update from the editors when they plan to address this PR please?

  • It's blocking #3391 which is also stage 4
  • I'm not sure about the exact cutoff date but it would be unfortunate if this doesn't make it into the ES 2025 snapshot despite being part of the language now
  • I can't speak for any other small-scale engine implementors but personally I do want to implement both of these features but not before they have been integrated to avoid potential editorial churn

I'm not bringing this up formally at the upcoming plenary but I feel like there is a discussion to be had about requiring editorial sign-off before requesting stage 4 so that PRs can be merged without further thought afterwards.

linusg avatar Feb 13 '25 13:02 linusg

Note that this PR has LGTMs from Michael (https://github.com/tc39/ecma262/pull/3057#pullrequestreview-1480381585), Kevin (https://github.com/tc39/ecma262/pull/3057#pullrequestreview-1473591443) and jmdyck (https://github.com/tc39/ecma262/pull/3057#pullrequestreview-2573923746).

After Michael's and Kevin's LGTMs, there has been the change to delete the part about the "legacy assert keyword", so the PR needs another quick review, but it should be almost ready to land at this point.

nicolo-ribaudo avatar Feb 13 '25 16:02 nicolo-ribaudo

@linusg the cutoff is typically based on when stage 4 was achieved, and not based on when the PR is mergeable.

ljharb avatar Feb 13 '25 17:02 ljharb

That's not what I mean - there will be a snapshot of the entire spec somewhere in the Ecma archives that says "this is ECMAScript 2025". If this PR is not merged by the time that snapshot is finalized import attributes and JSON modules won't be in there, despite having achieved stage 4. I was unable to find out when that happens exactly but I'm told it's around this time of the year.

linusg avatar Feb 13 '25 17:02 linusg

@linusg i'm the one who makes that snapshot, and it's never made until all outstanding stage 4 PRs are merged, so you don't have to worry about it.

ljharb avatar Feb 13 '25 17:02 ljharb

(it's also a living standard, so nobody should be caring about the snapshots, only what's merged into github :-) )

ljharb avatar Feb 13 '25 17:02 ljharb

Absolutely, I don't use these snapshots ever. Still seemed worth pointing out to avoid this situation, thanks for clarifying!

linusg avatar Feb 13 '25 17:02 linusg

I rebased on main and squashed and now esmeta is complaining, I'm checking why.

nicolo-ribaudo avatar Feb 14 '25 23:02 nicolo-ribaudo

[ParamTypeMismatch] Call[16128] argument assignment to second parameter _request_ when Call[16128] function call from Record[SourceTextModuleRecord].GetExportedNames (step 8.b, 19:43-93) to GetImportedModule
- expected: Record[ModuleRequestRecord]
- actual  : String
[ParamTypeMismatch] Call[16163] argument assignment to second parameter _request_ when Call[16163] function call from Record[SourceTextModuleRecord].ResolveExport (step 6.a.ii, 16:44-94) to GetImportedModule
- expected: Record[ModuleRequestRecord]
- actual  : String
[ParamTypeMismatch] Call[16178] argument assignment to second parameter _request_ when Call[16178] function call from Record[SourceTextModuleRecord].ResolveExport (step 9.b, 31:42-92) to GetImportedModule
- expected: Record[ModuleRequestRecord]
- actual  : String
[ParamTypeMismatch] Call[16215] argument assignment to second parameter _request_ when Call[16215] function call from Record[SourceTextModuleRecord].InitializeEnvironment (step 7.a, 13:42-93) to GetImportedModule
- expected: Record[ModuleRequestRecord]
- actual  : String

These are wrong. It thinks ExportEntry's [[ModuleRequest]] is a String, but this PR updates it to be a ModuleRequest Record.

[ParamTypeMismatch] Call[16296] argument assignment to first parameter _left_ when Call[16296] function call from FinishLoadingImportedModule (step 1.a, 3:105-151) to ModuleRequestsEqual
- expected: Record[LoadedModuleRequestRecord | ModuleRequestRecord]
- actual  : Record[{ Module : Record[ModuleRecord | ModuleRecordN], Specifier : String }]

This is also wrong. The actual type is LoadedModuleRequestRecord, declared in the line before.

[ParamTypeMismatch] Call[9455] argument assignment to first parameter _O_ when Call[9455] function call from EvaluateImportCall (step 11.d.iv.1, 28:35-54) to Get
- expected: Record[Object]
- actual  : ESValue
[ParamTypeMismatch] Call[9457] argument assignment to first parameter _O_ when Call[9457] function call from EvaluateImportCall (step 11.d.iv.2, 29:37-56) to Get
- expected: Record[Object]
- actual  : ESValue

These are because esmeta doesn't know that the entries of the list returned by EnumerableOwnProperties are arrays (thus objects).

[ReturnTypeMismatch] Block[9363] return statement in EvaluateImportCall (step 3, 4:36-73)
- expected: Normal[Record[Promise]] | Throw
- actual  : Return | Throw
[ReturnTypeMismatch] Block[9376] return statement in EvaluateImportCall (step 5.a, 7:36-71)
- expected: Normal[Record[Promise]] | Throw
- actual  : Return | Throw

This caught a bug! In case of import(yield), evaluating yield can return, thus EvaluateImportCall's type definition needs to allow Return completions.

I'll edit the PR fixing this last bug, and ignoring the other errors.

nicolo-ribaudo avatar Feb 14 '25 23:02 nicolo-ribaudo