Bug 206888 – [ESNext][BigInt] We don't support BigInt literal as PropertyName

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

According to spec, BigInt is a valid PropertyName (https://tc39.es/ecma262/#prod-PropertyName) and it should be evaluated using ToString (https://tc39.es/ecma262/#sec-object-initializer-runtime-semantics-evaluation). Given this, this script should be evaluated without error:

```
// Object Literal

let o = { 1n: "foo" };
assert(a[1n] === "foo");
assert(a[1] === "foo");
assert(a["1"] === "foo");

// MethodDeclaration

o = { 1n() => "bar" };
assert(a[1n]() === "bar");
assert(a[1]() === "bar");
assert(a["1"]() === "bar");

class C {
  1n() { return "baz"; }
}

let c = new C();
assert(c.[1n]() === "baz");
assert(c.[1]() === "baz");
assert(c.["1"] === "baz");

// Destructuring

let {1n: a} = {1n: "foo"};
assert(a === "foo");

```

Original report from https://bugzilla.mozilla.org/show_bug.cgi?id=1605835

[Caio Lima]

Oops, previous script is not correct. Correct version is:

```
// Object Literal

let o = { 1n: "foo" };
assert.sameValue(o[1n], "foo");
assert.sameValue(o[1], "foo");
assert.sameValue(o["1"], "foo");

// MethodDeclaration

o = { 1n() { return "bar"; } };
assert.sameValue(o[1n](), "bar");
assert.sameValue(o[1](), "bar");
assert.sameValue(o["1"](), "bar");

class C {
  1n() { return "baz"; }
}

let c = new C();
assert.sameValue(c[1n](), "baz");
assert.sameValue(c[1](), "baz");
assert.sameValue(c["1"](), "baz");

// Destructuring

let {1n: a} = {1n: "foo"};
assert.sameValue(a, "foo");
```

[Caio Lima]

Created attachment 389029 [details]
Patch

[Caio Lima]

Created attachment 390368 [details]
Patch

[Ross Kirsling]

Wow, interesting that every engine missed this!
Looks like this is the test262 PR? https://github.com/tc39/test262/pull/2457

[Ross Kirsling]

Comment on attachment 390368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390368&action=review

r=me with comments.

Seems like Mozilla's tests also cover binary/octal/hex literals:
https://searchfox.org/mozilla-central/source/js/src/tests/non262/BigInt/property-name.js
Do you think it's worth adding tests for these (maybe not necessary since they're all just BIGINT tokens)?

> Source/JavaScriptCore/parser/Parser.cpp:4177
> +        InferName inferName = ident && *ident == m_vm.propertyNames->underscoreProto ? InferName::Disallowed : InferName::Allowed;

bigIntString can't be underscoreProto though, can it? Should this be an ASSERT?

> JSTests/ChangeLog:8
> +        * stress/big-int-as-proeprty-name.js: Added.

Nit: spelling in filename :p

[Caio Lima]

Comment on attachment 390368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390368&action=review

I don't think it hurts to add tests with binary, octals and hexdecimals. I'll add those as well.

>> Source/JavaScriptCore/parser/Parser.cpp:4177
>> +        InferName inferName = ident && *ident == m_vm.propertyNames->underscoreProto ? InferName::Disallowed : InferName::Allowed;
> 
> bigIntString can't be underscoreProto though, can it? Should this be an ASSERT?

This should not be here! I think I'll put InferName::Allowed directly on createProperty.

>> JSTests/ChangeLog:8
>> +        * stress/big-int-as-proeprty-name.js: Added.
> 
> Nit: spelling in filename :p

Oops.

[Caio Lima]

Created attachment 390419 [details]
Patch

[Caio Lima]

(In reply to Ross Kirsling from comment #5)
> Comment on attachment 390368 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390368&action=review
> 
> r=me with comments.
> 
> Seems like Mozilla's tests also cover binary/octal/hex literals:
> https://searchfox.org/mozilla-central/source/js/src/tests/non262/BigInt/
> property-name.js
> Do you think it's worth adding tests for these (maybe not necessary since
> they're all just BIGINT tokens)?

It turns out that those cases found a bug on implementation! Thank you for pointing this out. I updated a new version of the patch to handle cases when we use non-decimal literals. Do you mind take another look?

[Ross Kirsling]

Comment on attachment 390419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390419&action=review

Hooray for tests finding bugs!

> Source/JavaScriptCore/parser/ParserArena.cpp:90
> +    // FIXME: We are allocating a JSBigInt just to be able to use
> +    // JSBigInt::tryGetString when radix is not 10.
> +    // This creates some GC pressure, but since these identifiers
> +    // will only be created when BigInt literal is used as a property name,
> +    // it wont be much problematic, given such cases are very rare.

Should this FIXME have a bug?

[Caio Lima]

(In reply to Ross Kirsling from comment #9)
> Comment on attachment 390419 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390419&action=review
> 
> Hooray for tests finding bugs!
> 
> > Source/JavaScriptCore/parser/ParserArena.cpp:90
> > +    // FIXME: We are allocating a JSBigInt just to be able to use
> > +    // JSBigInt::tryGetString when radix is not 10.
> > +    // This creates some GC pressure, but since these identifiers
> > +    // will only be created when BigInt literal is used as a property name,
> > +    // it wont be much problematic, given such cases are very rare.
> 
> Should this FIXME have a bug?

Yes. I was waiting until get an r+ before creating it. I created https://bugs.webkit.org/show_bug.cgi?id=207627.

[Caio Lima]

Created attachment 390507 [details]
Patch

[Caio Lima]

Comment on attachment 390507 [details]
Patch

Thank you very much for the review!

[WebKit Commit Bot]

Comment on attachment 390507 [details]
Patch

Clearing flags on attachment: 390507

Committed r256541: <https://trac.webkit.org/changeset/256541>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59436664>