Bug 216437

(In reply to HyeockJin Kim from comment #0) > When using the bracket key in destructuring assignment, rest ignores the > bracket key. > > If the issue hasn't been fixed yet, can I try this issue? This is a great find, thank you! The bug is still there as of r266924, a patch is very welcome. In case you need assistance, there is a #jsc channel on Slack: https://webkit.org/getting-started/#staying-in-touch.

Created attachment 409160 [details] Patch

If the computed property is not a string value, the property in the excludedList was not recognized because the type was different.

Comment on attachment 409160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409160&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5237 > + generator.emitToString(propertyString.get(), propertyName.get()); I don't think we can call ToString here as the `propertyName` might have a symbol, rising a TypeError. https://tc39.es/ecma262/#sec-object-initializer-runtime-semantics-evaluation for ComputedPropertyName calls ToPropertyKey.

Also, could you please run `Tools/Scripts/test262-runner --release --save`? There might be new test(s) passing. If there are none, it would be great to submit a PR to https://github.com/tc39/test262 so other runtimes would get fixed if needed.

Comment on attachment 409160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409160&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5237 >> + generator.emitToString(propertyString.get(), propertyName.get()); > > I don't think we can call ToString here as the `propertyName` might have a symbol, rising a TypeError. > https://tc39.es/ecma262/#sec-object-initializer-runtime-semantics-evaluation for ComputedPropertyName calls ToPropertyKey. We should avoid emitting this if we know that this is not necessary. For example, if target.propertyExpression is nullptr, we know what is specified in propertyName.

Comment on attachment 409160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409160&action=review >>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5237 >>> + generator.emitToString(propertyString.get(), propertyName.get()); >> >> I don't think we can call ToString here as the `propertyName` might have a symbol, rising a TypeError. >> https://tc39.es/ecma262/#sec-object-initializer-runtime-semantics-evaluation for ComputedPropertyName calls ToPropertyKey. > > We should avoid emitting this if we know that this is not necessary. For example, if target.propertyExpression is nullptr, we know what is specified in propertyName. Yeah, so let's call `emitToPropertyKey`. And avoid emitting this if propertyName is known as value that does not need `ToPropertyKey`.

I think this is related. https://github.com/tc39/test262/issues/2089

Created attachment 409198 [details] Patch

(In reply to Alexey Shvayka from comment #5) > Also, could you please run `Tools/Scripts/test262-runner --release --save`? > There might be new test(s) passing. > If there are none, it would be great to submit a PR to > https://github.com/tc39/test262 so other runtimes would get fixed if needed. JSTests/test262/expectations.yaml has been modified by executing the command. Is this correct? If not, is there any reference for me on how to send a PR to https://github.com/tc39/test262?

(In reply to HyeockJin Kim from comment #10) > JSTests/test262/expectations.yaml has been modified by executing the command. > Is this correct? --save caused the modification, yet intl402/Collator failures are unrelated. Please revert JSTests/test262/expectations.yaml change. > If not, is there any reference for me on how to send a PR to > https://github.com/tc39/test262? You need to clone tc39/test262/src/dstr-assignment/obj-rest-computed-property.case (it is a template), make relevant modifications (see https://github.com/tc39/test262/issues/2089 for examples), and re-generate .js tests (like tc39/test262/test/language/statements/for-of/dstr/obj-rest-computed-property.js) via Python script (see https://github.com/tc39/test262/blob/main/CONTRIBUTING.md#procedurally-generated-tests).

Comment on attachment 409198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409198&action=review > Source/JavaScriptCore/ChangeLog:3 > + Add the property to be excluded from the rest operation of the object as string This is a bit hard to follow IMO. What about "Coerce computed property before adding to |excludedList|"? > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5233 > if (!target.propertyExpression) Maybe we should flip this negation? > JSTests/stress/object-rest-deconstruct.js:205 > +// Destructuring non-string computed property Could you please add a Symbol() test (that would fail with previous patch)?

Comment on attachment 409198 [details] Patch Need to upload a new patch that applies cleanly, ideally also with the changes that Alexey requested.

<rdar://problem/69215687>

Comment on attachment 409198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409198&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5234 > propertyName = generator.emitLoad(nullptr, target.propertyName); Let's check what the target.propertyExpression is. If it is number constant, then we need to generate JSString from that here. If it is not JSString, then we need to have generator.emitToPropertyKey. But if it is JSString, we do not need it. And I think this special handling is very important since in major cases, it is JSString or Number. So we can simply avoid ToPropertyName call for most cases.

Comment on attachment 409198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409198&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5234 >> propertyName = generator.emitLoad(nullptr, target.propertyName); > > Let's check what the target.propertyExpression is. If it is number constant, then we need to generate JSString from that here. > If it is not JSString, then we need to have generator.emitToPropertyKey. But if it is JSString, we do not need it. > And I think this special handling is very important since in major cases, it is JSString or Number. So we can simply avoid ToPropertyName call for most cases. Ah, no. The above should work since target.propertyName is `const Identifer&`. So it is always producing JSStirng. Ignore my comment above.

(In reply to Alexey Shvayka from comment #12) > > Source/JavaScriptCore/ChangeLog:3 > > + Add the property to be excluded from the rest operation of the object as string > > This is a bit hard to follow IMO. > What about "Coerce computed property before adding to |excludedList|"? > I change title. > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5233 > > if (!target.propertyExpression) > > Maybe we should flip this negation? > Segmentation fault occurs when do negation this condition. >>> a = 1 1 >>> ({[a]:b, ...c} = {[a]: 2, 2:3}) [1] 73132 segmentation fault ./Tools/Scripts/run-jsc --jsc-only > > JSTests/stress/object-rest-deconstruct.js:205 > > +// Destructuring non-string computed property > > Could you please add a Symbol() test (that would fail with previous patch)? I didn't understand what the `Symbol() test` means. I'm sorry, can you explain a little more?

(In reply to Alexey Shvayka from comment #11) > (In reply to HyeockJin Kim from comment #10) > You need to clone > tc39/test262/src/dstr-assignment/obj-rest-computed-property.case (it is a > template), make relevant modifications (see > https://github.com/tc39/test262/issues/2089 for examples), and re-generate > .js tests (like > tc39/test262/test/language/statements/for-of/dstr/obj-rest-computed-property. > js) via Python script (see > https://github.com/tc39/test262/blob/main/CONTRIBUTING.md#procedurally- > generated-tests). I did PR, is it correct? https://github.com/tc39/test262/pull/2799

Comment on attachment 409198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409198&action=review Can you rebase the patch to make it applied to the tip of tree? :) The other part looks good. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:5233 >> if (!target.propertyExpression) > > Maybe we should flip this negation? I think "negation" here means, if (target.propertyExpression) generator.emitToPropertyKey(propertyName.get(), propertyName.get()); else propertyName = generator.emitLoad(nullptr, target.propertyName); > JSTests/test262/expectations.yaml:1557 > +test/intl402/Collator/ignore-invalid-unicode-ext-values.js: > + default: 'Test262Error: Locale ko is affected by key co; value standard. Expected SameValue(«ko», «ko-KR») to be true' > + strict mode: 'Test262Error: Locale ko is affected by key co; value standard. Expected SameValue(«ko», «ko-KR») to be true' > test/intl402/Collator/missing-unicode-ext-value-defaults-to-true.js: > - default: "Test262Error: \"kn-true\" is returned in locale, but shouldn't be. Expected SameValue(«7», «-1») to be true" > - strict mode: "Test262Error: \"kn-true\" is returned in locale, but shouldn't be. Expected SameValue(«7», «-1») to be true" > + default: "Test262Error: \"kn-true\" is returned in locale, but shouldn't be. Expected SameValue(«4», «-1») to be true" > + strict mode: "Test262Error: \"kn-true\" is returned in locale, but shouldn't be. Expected SameValue(«4», «-1») to be true" > test/intl402/DateTimeFormat/constructor-options-order-fractionalSecondDigits.js: I think this is due to ICU version. So let's exclude this. I think test262 does not cover this case.

Created attachment 409229 [details] Patch

Created attachment 409230 [details] Patch

(In reply to Yusuke Suzuki from comment #19) > Comment on attachment 409198 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409198&action=review > > Can you rebase the patch to make it applied to the tip of tree? :) The other > part looks good. I think it's all fixed. :)

(In reply to Alexey Shvayka from comment #11) > You need to clone > tc39/test262/src/dstr-assignment/obj-rest-computed-property.case (it is a > template), make relevant modifications (see > https://github.com/tc39/test262/issues/2089 for examples), and re-generate > .js tests (like > tc39/test262/test/language/statements/for-of/dstr/obj-rest-computed-property. > js) via Python script (see > https://github.com/tc39/test262/blob/main/CONTRIBUTING.md#procedurally- > generated-tests). I sent a PR and it was merged. :) https://github.com/tc39/test262/pull/2799

Comment on attachment 409230 [details] Patch r=me

Committed r267440: <https://trac.webkit.org/changeset/267440> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409230 [details].