Bug 216437 – Coerce computed property before adding to |excludedList|

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[HyeockJin Kim]

version: 13.1.2(15609.3.5.1.3)

chrome:
> ({[1]: c, ...b} = {1: 4})
{1: 4}
> b
{}
 
Firefox:
> ({[1]: c, ...b} = {1: 4})
Object { 1: 4 }

> b
Object {  }

Safari:
> ({1: c, ...b} = {1: 4})
< {1: 4}
> b
< {}

> ({[1]: c, ...b} = {1: 4})
< {1: 4}
> b
< {1: 4}

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?

[Alexey Shvayka]

(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.

[HyeockJin Kim]

Created attachment 409160 [details]
Patch

[HyeockJin Kim]

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

[Alexey Shvayka]

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.

[Alexey Shvayka]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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`.

[Yusuke Suzuki]

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

[HyeockJin Kim]

Created attachment 409198 [details]
Patch

[HyeockJin Kim]

(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?

[Alexey Shvayka]

(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).

[Alexey Shvayka]

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)?

[Darin Adler]

Comment on attachment 409198 [details]
Patch

Need to upload a new patch that applies cleanly, ideally also with the changes that Alexey requested.

[Radar WebKit Bug Importer]

<rdar://problem/69215687>

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[HyeockJin Kim]

(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?

[HyeockJin Kim]

(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

[Yusuke Suzuki]

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.

[HyeockJin Kim]

Created attachment 409229 [details]
Patch

[HyeockJin Kim]

Created attachment 409230 [details]
Patch

[HyeockJin Kim]

(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. :)

[HyeockJin Kim]

(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

[Yusuke Suzuki]

Comment on attachment 409230 [details]
Patch

r=me

[EWS]

Committed r267440: <https://trac.webkit.org/changeset/267440>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409230 [details].