tc39 / proposal-import-attributes

Proposal for syntax to import ES modules with assertions
https://tc39.es/proposal-import-attributes/
Apache License 2.0
569 stars 32 forks source link

Consider restricting attribute keys syntax to identifiers and strings again #145

Closed nicolo-ribaudo closed 11 months ago

nicolo-ribaudo commented 1 year ago

BigInt attribute keys (i.e. import "x" with { 10n: "val" }) need to be converted to strings while parsing, to check for duplicate keys. In SpiderMonkey this would require allocating the bigint at parse time, which is something that does not currently happen (the object { 10n: x } is parsed as { [10n]: x }).

The motivation for allowing number&bigint literals in import attribute keys is for symmetry with object literals.

mgaudet commented 1 year ago

The other thing that I find sort of strange here is that the argument is that we're trying to be symmetric with the object literal version for dynamic import, but the syntax ends up still being a subset of the object literal syntax. For example,

let x = "type"
import "foo" with { [x]: "js"} 

won't parse -- IMO having a tighter simpler rule for the WithClause makes more sense than trying to align it with the dynamic import case.

nicolo-ribaudo commented 1 year ago

It looks like JSC currently allocates bigints while parsing, but they would like to stop doing so: https://github.com/WebKit/WebKit/blob/4b27f17f5ade44bc02f5d8707c8140a762bb9253/Source/JavaScriptCore/parser/ParserArena.cpp#L98-L112

V8 allocates bigints while parsing without problems

nicolo-ribaudo commented 1 year ago

Also cc @tc39/ecma262-editors, since the keys syntax was relaxed due to feedback from the stage 3 review 🙂

michaelficarra commented 1 year ago

I don't understand the problem. BigInts are only allocated when someone uses them as keys.

mgaudet commented 1 year ago

The spec requires that duplicate assertion keys are detected and reported as syntax errors; but that means that we need to be able to report that import "f.js" with { 1_000n: "value", 1000n: "value" } are the same. In the arbitrary case of a BigInt, we don't want our parser to have to have a full understanding of BigInt semantics, so we would have to allocate a BigInt.

However, as a design principle we are not allocating GC things during parse; this would be an instance of us having to do so.

mgaudet commented 1 year ago

(for regular numeric values we have specialized machinery to handle this, but since a bigint is arbitrary precision, we would need the full arbitrary precision comparison)

ljharb commented 1 year ago

Forgive me if i don't understand some nuance of parsers, but if it's successfully parsed as a bigint, you can't just remove the _s and compare literal source text to see if it's the same?

michaelficarra commented 1 year ago

@ljharb No, the BigInt grammar is not otherwise canonicalising. 0xFFn === 255n

ljharb commented 1 year ago

ah, i forgot about hex literals, fair point

michaelficarra commented 1 year ago

Regardless, though, I am skeptical of the claim that a parser must instantiate an actual JS BigInt to be able to detect duplicate keys. You don't need to allocate an actual BigInt to compute BigInt::toString.

edit: And this cost, even if excessive, is only paid when the program actually uses this combination of features (import assertions with bigint keys), which will be exceptionally rare.

bakkot commented 1 year ago

It's definitely possible in principle. Seems like an awful lot of work to ask of implementations for something which isn't ever actually going to be used, though.

bojavou commented 1 year ago

Just implemented an attributes parser, with numeric literal support. I was surprised it was there. Just some thoughts:

I don't have strong feelings. Following this so my parser can stay aligned with whatever happens.