Bug 147742 – [ES6] Implement ES6 arrow function syntax. Prototype of arrow function should be undefined

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

[GSkachkov]

Arrow function should support following condition:
var af = ()=>{}; 
typeof af.prototype === 'undefined';

[GSkachkov]

Created attachment 259371 [details]
Patch

Implemented feature

[GSkachkov]

Created attachment 259372 [details]
Patch

Upload after rebase

[GSkachkov]

Created attachment 259456 [details]
Patch

Fix build

[Filip Pizlo]

Comment on attachment 259456 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:70
> +    removeDirect(vm, vm.propertyNames->prototype);

This is not necessary, and it's also quite wrong.  It's quite bad to remove properties; it puts the object into a conservative (i.e. slow) mode.  Also, there is no way for the object to have a "prototype" property at this point, since it's added lazily.

[GSkachkov]

Created attachment 259943 [details]
Patch

Fix comments

[GSkachkov]

(In reply to comment #4)
> Comment on attachment 259456 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259456&action=review
> 
> > Source/JavaScriptCore/runtime/JSArrowFunction.cpp:70
> > +    removeDirect(vm, vm.propertyNames->prototype);
> 
> This is not necessary, and it's also quite wrong.  It's quite bad to remove
> properties; it puts the object into a conservative (i.e. slow) mode.  Also,
> there is no way for the object to have a "prototype" property at this point,
> since it's added lazily.

Hmm, it is really unnecessary. I've removed this code. Could you please make review one more time? 

Also something wrong with tests during build, because tests don't pass without my patch.

[Saam Barati]

Comment on attachment 259943 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSFunction.cpp:481
> +            || (propertyName == exec->propertyNames().prototype && !executable->isArrowFunction())

Why the isArrowFunction check here?
If we're an arrow function, don't we want to prevent deletion here?

[GSkachkov]

How I understand this rule:
If an arrow function doesn't have own prototype so it can be initialized by custom js code. And if it possible to init this property so there has to be way to delete it. 
Does it make sense? 
Possible I need add this to task description or ChangeLog

[Saam Barati]

Comment on attachment 259943 [details]
Patch

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

r=me with comment.
If you upload again I'll cq+

> Source/JavaScriptCore/runtime/JSFunction.cpp:361
>      if (propertyName == exec->propertyNames().prototype) {

I think this code is more clear if we just check !executable->isArrowFunction()
here instead of doing it twice below.

[Saam Barati]

(In reply to comment #8)
> How I understand this rule:
> If an arrow function doesn't have own prototype so it can be initialized by
> custom js code. And if it possible to init this property so there has to be
> way to delete it. 
> Does it make sense? 
> Possible I need add this to task description or ChangeLog

Okay. I see. So for normal functions, we're definitely
not allowed to delete "prototype", but for arrow functions,
we start with no prototype, but we're allowed to define
one and delete one?

[GSkachkov]

(In reply to comment #10)
> (In reply to comment #8)
> > How I understand this rule:
> > If an arrow function doesn't have own prototype so it can be initialized by
> > custom js code. And if it possible to init this property so there has to be
> > way to delete it. 
> > Does it make sense? 
> > Possible I need add this to task description or ChangeLog
> 
> Okay. I see. So for normal functions, we're definitely
> not allowed to delete "prototype", but for arrow functions,
> we start with no prototype, but we're allowed to define
> one and delete one?

Yes, that is correct.
I've double checked the es6 spec, and did not find any mention that related to 'prototype' property in arrow function section(14.2), only in "usual" function section(14.1.20 Note 2) http://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions-runtime-semantics-evaluation, so I expect that 'prototype' property of arrow function should behave as ordinary property, in contrast with 'prototype' property of "usual" function that automatically created, can be changed, but can't be deleted.

[GSkachkov]

Created attachment 260532 [details]
Patch

Increate code readability

[Saam Barati]

Comment on attachment 260532 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 260532 [details]
Patch

Clearing flags on attachment: 260532

Committed r189341: <http://trac.webkit.org/changeset/189341>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #14)
> Comment on attachment 260532 [details]
> Patch
> 
> Clearing flags on attachment: 260532
> 
> Committed r189341: <http://trac.webkit.org/changeset/189341>

es6.yaml/es6/arrow_functions_no_prototype_property.js.default 
fails on all JSC tester bot, please check what happened.

Don't you run JSC tests before commiting a patch?

[Alexey Proskuryakov]

Is anyone available to fix this soon? If not, I'm going to roll out the patch.

[Basile Clement]

This patch actually made the test pass, but it is marked as "must fail" in the harness. r189350 <http://trac.webkit.org/changeset/189350> marks the test as "must succeed".

[Alexey Proskuryakov]

Thank you Basile!

[GSkachkov]

Basile Clemen Thank you for the fix. I'll try to avoid such think in future!(Run all tests on the lastest jsc version)