Stage 3 reviews

@jridgewell

This proposal reached conditional consensus for Stage 3 during the March 2023 TC39 meeting.

The consensus is conditional on #133 being merged, and on Stage 3 reviews. This issue tracks the review status.

@jridgewell (review: Stage 3 reviews #137 (comment), pr: Fixes for jridgewell's review #139)
@ljharb (review: Stage 3 reviews #137 (comment) and Stage 3 reviews #137 (comment), pr: ljharb's review #141)
@tc39/ecma262-editors (Normative: Add Import Attributes ecma262#3057)

EvaluateImportCall
- I can't find EnumerableOwnPropertyNames anywhere, did you mean EnumerableOwnProperties?
- "Sort attributes by the code point order of…", I don't see this phrase for sorting anywhere else in the spec so I'm not sure if it's valid.
AllImportAttributesSupported
- "For each Record attribute of attributes" should list the Record properties Record { [[Key]], [[Value]] }, or use "For each ImportAttribute Record…"
Static Semantics: ModuleRequests
- I don't see a pattern like "Let specifier be SV of the ModuleSpecifier contained in FromClause" anywhere else. Is this valid?
ModuleRequest and ImportAttribute Records
- We use "same [[Attributes]]" all over (for equality in ([[Specificer]], [[Attributes]]) paris), but we don't define its equality when using "same", only when using AttributesEqual.
HostLoadImportedModule
- The editorial note "There are three possible ways to handle multiple imports of the same module with different valid attributes:" is biased towards the Race semantics (which IMHO is the wrong choice) because the other semantics choices have caveats. Let's either add a caveat that Race will behave incorrectly in some scenarios, or remove the caveats from the other choices.
AttributesEqual
- Nit: I would split the looping If statement to do an early return if [[Key]] matches but [[Value]] doesn't.
Static Semantics: WithClauseToAttributes
- "Let key be StringValue of AttributeKey.", StringValue isn't defined on AttributeKey: StringLiteral.
- Ditto for "[[Value]]: StringValue of StringLiteral"

@jridgewell

Thanks @jridgewell! I opened #139 for your review.

The only point I'm not sure how to solve is "Sort attributes by the code point order of…". There is already a step in the spec that sorts a list (step 6 of ModuleNamespaceCreate), and it does so by referring to %Array.prototype.sort%. However, that strategy doesn't work in this case because we are not sorting a list of values, but a list of records with an order based on one of their fields. I could write a separate sorting AO, but it feels overkill for the use case.

"code point order" is kind of a tricky concept here, because strings in JS are lists of UTF16 code units, not of code points - so, for example, does ﬀ (the code point '\uFB00', represented by the single code unit '\uFB00') come before '😀' (the code point '\u1F600', represented by the UTF16 two code units '\uD83D\uDE00')?

The simplest thing is probably something like

Sort attributes according to the lexicographic order of their [[Key]] attributes, treating the value of each such attribute as a sequence of UTF-16 code unit values.

Thank you!

Initial review:

Is https://tc39.es/proposal-import-attributes/#note-HostLoadImportedModule-referrer-Realm-Record an addition? or is it just green because it's a note.
HostGetSupportedImportAttributes should s/same contents/same contents in the same order.

Is https://tc39.es/proposal-import-attributes/#note-HostLoadImportedModule-referrer-Realm-Record an addition? or is it just green because it's a note.

It's green just because it's a note. Except for that one, all the other notes are new.

EDIT: I pushed 2b161db to mark new non-editorial notes.

HostGetSupportedImportAttributes should s/same contents/same contents in the same order.

That list is effectively used as a Set (it's only used at step 2.a of https://tc39.es/proposal-import-attributes/#sec-AllImportAttributesSupported), so I don't think that the stricter requirement would have any effect.

true, that's a fair point. however, it means that it's idempotent, whereas currently it's not.

I merged #133, the spec text is now ready for review!

In EvaluateImportCall step 11.e.ii, you get the keys, and then get and validate the values. Is there a reason to prefer that over getting the entries, and then validating the values? The observable difference ofc would be whether all the Gets were done before validation, or whether the result of validation determined how many Gets were done. It seems nice to me to unconditionally get every property, and then validate them.
A minor bikeshed: can AttributesEqual be named ImportAttributesEqual just to ensure no confusion?
why are we supporting with {}? i assume for consistency with import {} and export {}andconst {} = obj` etc?

In HostLoadImportedModule, Note 2: is this really something we can't specify? Have we documented some example cases that indicate that more than one of these three approaches might be appropriate?

@ljharb

Thanks @ljharb for the review, sorry it took this long to tackle it :) I opened #141.

HostGetSupportedImportAttributes should s/same contents/same contents in the same order.

✔️

In EvaluateImportCall step 11.e.ii, you get the keys, and then get and validate the values. Is there a reason to prefer that over getting the entries, and then validating the values? The observable difference ofc would be whether all the Gets were done before validation, or whether the result of validation determined how many Gets were done. It seems nice to me to unconditionally get every property, and then validate them.

✔️ Done! I agree with making the behavior as much internally consistent as possible. I tried thinking about other parts in the spec where we might be doing something similar, but this is probably the first time we validate a "dictionary" object with unknown keys.

A minor bikeshed: can AttributesEqual be named ImportAttributesEqual just to ensure no confusion?

✔️

why are we supporting with {}? i assume for consistency with import {} and export {}andconst {} = obj` etc?

Yes, there has been some discussion in #94 (and you already wrote there that you didn't particularly like it :P)

tl;dr:

for consistency with export {} / import {} (const {} is actually different, since it asserts that the value is not nullish)

it makes it less annoying to comment out assertions:

import json from "./mod" with {
  // TODO: This is temporarily a JS file
  // type: "json"
};

In HostLoadImportedModule, Note 2: is this really something we can't specify? Have we documented some example cases that indicate that more than one of these three approaches might be appropriate?

The previous "import assertions" spec in practice mandated "reject" (even if it happened when loading and not when parsing). The spec as is written right now does "clone", since "reject" ended up not being integratable in HTML. Unfortunately it's not really possible to mandate "clone but not reject/race", since once they are different entries in the imports cache they have separate behaviors. Also, one import passing with one attribute doesn't imply that also another import will pass with a different attribute.

Tbh I would just not include that note in ECMA-262 — it was originally there because the author wanted to express "we are specifying assertions, but a future proposal might introduce something else that has a different behavior". Now that the proposal has been changed, that note doesn't have it's original utility.

@tc39/ecma262-editors Do you prefer if I open a PR to ecma262 to help reviewing this?

@nicolo-ribaudo

@nicolo-ribaudo If that's not too burdensome for you, yes, that is my preferred way to review proposals.

I opened tc39/ecma262#3057 to facilitate reviewing this proposal as an actual diff on top of ECMA-262.

Thanks everyone for the reviews!

Navigation Menu

Repository navigation

Stage 3 reviews #137

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Stage 3 reviews #137

Description

Activity

jridgewell commented on Mar 31, 2023

nicolo-ribaudo commented on Apr 3, 2023

bakkot commented on Apr 3, 2023

nicolo-ribaudo commented on Apr 3, 2023

ljharb commented on Apr 4, 2023

nicolo-ribaudo commented on Apr 6, 2023

ljharb commented on Apr 6, 2023

nicolo-ribaudo commented on Apr 21, 2023

ljharb commented on Apr 21, 2023

ljharb commented on Apr 21, 2023

nicolo-ribaudo commented on May 3, 2023

nicolo-ribaudo commented on May 4, 2023

michaelficarra commented on May 4, 2023

nicolo-ribaudo commented on May 8, 2023

nicolo-ribaudo commented on Jun 16, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions