Bug 147742

Created attachment 259371 [details] Patch Implemented feature

Created attachment 259372 [details] Patch Upload after rebase

Created attachment 259456 [details] Patch Fix build

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.

Created attachment 259943 [details] Patch Fix comments

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

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?

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

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.

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

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

Created attachment 260532 [details] Patch Increate code readability

Comment on attachment 260532 [details] Patch LGTM

Comment on attachment 260532 [details] Patch Clearing flags on attachment: 260532 Committed r189341: <http://trac.webkit.org/changeset/189341>

All reviewed patches have been landed. Closing bug.

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

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

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

Thank you Basile!

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