Summary: | [ESNext][BigInt] We don't support BigInt literal as PropertyName | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Lima <ticaiolima> | ||||||||||
Component: | JavaScriptCore | Assignee: | Caio Lima <ticaiolima> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rmorisset, ross.kirsling, saam, tzagallo, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Caio Lima
2020-01-28 09:52:43 PST
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. |