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
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"); ```
Created attachment 389029 [details] Patch
Created attachment 390368 [details] Patch
Wow, interesting that every engine missed this! Looks like this is the test262 PR? https://github.com/tc39/test262/pull/2457
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
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.
Created attachment 390419 [details] Patch
(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?
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?
(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.
Created attachment 390507 [details] Patch
Comment on attachment 390507 [details] Patch Thank you very much for the review!
Comment on attachment 390507 [details] Patch Clearing flags on attachment: 390507 Committed r256541: <https://trac.webkit.org/changeset/256541>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59436664>