Bug 152985 – ES6: Classes: Should be allowed to create a static method with name "arguments"

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

[Joseph Pecoraro]

* SUMMARY
ES6: Classes: Should be allowed to create a static method with name "arguments".

jsc> class MyClass { static arguments() { } }
Exception: TypeError: Attempting to change configurable attribute of unconfigurable property.

* NOTES
- Behaves fine in Firefox and Chrome.
- See related bug 144281.

[GSkachkov]

I think should be the same for name 'caller' and getter and setter.
class A { 
 static get caller() { return '';}
}

[GSkachkov]

Created attachment 277870 [details]
Patch

Patch

[Saam Barati]

Comment on attachment 277870 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        class A { static argumetns() { return 'value'; } }

typo: 'argumetns' => 'arguments'

> Source/JavaScriptCore/runtime/JSFunction.cpp:368
> +        if (!(thisObject->jsExecutable()->isClass() && slot.internalMethodType() == PropertySlot::InternalMethodType::GetOwnProperty)) {

Why do we only care to skip this if we're doing a GetOwnProperty access on a class?
We will not skip if we're doing a normal Get or VMInquiry or HasProperty?

> Source/JavaScriptCore/runtime/JSFunction.cpp:399
> +                bool result = Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
> +                if (!result) {
> +                    GetterSetter* errorGetterSetter = thisObject->jsExecutable()->isClassConstructorFunction() || thisObject->jsExecutable()->parseMode() == SourceParseMode::MethodMode
> +                        ? thisObject->globalObject()->throwTypeErrorArgumentsAndCallerGetterSetter(exec->vm())
> +                        : thisObject->globalObject()->throwTypeErrorGetterSetter(exec->vm());
> +
> +                    thisObject->putDirectAccessor(exec, propertyName, errorGetterSetter, DontDelete | DontEnum | Accessor);
> +                    result = Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
> +                    ASSERT(result);
> +                }
> +                return result;

Can we write a helper for the common code?

[GSkachkov]

Created attachment 277905 [details]
Patch

Fix comments

[Joseph Pecoraro]

I see you are adding a new test. You should update the existing test that references this bug:

> LayoutTests/js/script-tests/keyword-method-names.js:79
> +    // FIXME: <https://webkit.org/b/152985> ES6: Classes: Should be allowed to create a static method with name "arguments"
> +    // static arguments() { }

[GSkachkov]

 I hope to upload soon new patch with fixes of your and Saam comments

[GSkachkov]

Created attachment 278348 [details]
Patch

New patch for review

[GSkachkov]

Created attachment 278358 [details]
Patch

Patch with rebase

[GSkachkov]

Created attachment 278360 [details]
Patch

More tests

[Keith Miller]

Comment on attachment 278360 [details]
Patch

I'm a little confused, if I have read the spec correctly (https://tc39.github.io/ecma262/#sec-forbidden-extensions), it looks like "arguments" and "caller" should not be on any non-strict function anyway. It seems like we should just get rid of those properties and make them act like normal for non-strict functions. I'm pretty sure that all class constructors are strict anyway so I think this would entirely fix this issue. Is there something I am missing?

[GSkachkov]

(In reply to comment #10)
> Comment on attachment 278360 [details]
> Patch
> 
> I'm a little confused, if I have read the spec correctly
> (https://tc39.github.io/ecma262/#sec-forbidden-extensions), it looks like
> "arguments" and "caller" should not be on any non-strict function anyway. It
Hmm, I did not find that about non-strict function in this spec. Only about function in strict-mode and arrow function, class declaration and etc.
> seems like we should just get rid of those properties and make them act like
> normal for non-strict functions. I'm pretty sure that all class constructors
> are strict anyway so I think this would entirely fix this issue. Is there
> something I am missing?

Currently my patch does not cover arrow function, GeneratorDeclaration, GeneratorExpression and functions created using the bind. So I'll add try to cover those cases

[GSkachkov]

Created attachment 279291 [details]
Patch

New patch

[Build Bot]

Comment on attachment 279291 [details]
Patch

Attachment 279291 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1344770

New failing tests:
fast/scrolling/ios/scrollTo-at-page-load.html

[Build Bot]

Created attachment 279313 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[GSkachkov]

Created attachment 279579 [details]
Patch

Small fixes

[GSkachkov]

Created attachment 279797 [details]
Patch

Rebase

[Keith Miller]

(In reply to comment #11)
> (In reply to comment #10)
> > Comment on attachment 278360 [details]
> > Patch
> > 
> > I'm a little confused, if I have read the spec correctly
> > (https://tc39.github.io/ecma262/#sec-forbidden-extensions), it looks like
> > "arguments" and "caller" should not be on any non-strict function anyway. It
> Hmm, I did not find that about non-strict function in this spec. Only about
> function in strict-mode and arrow function, class declaration and etc.
> > seems like we should just get rid of those properties and make them act like
> > normal for non-strict functions. I'm pretty sure that all class constructors
> > are strict anyway so I think this would entirely fix this issue. Is there
> > something I am missing?
> 
> Currently my patch does not cover arrow function, GeneratorDeclaration,
> GeneratorExpression and functions created using the bind. So I'll add try to
> cover those cases

I think you might have misunderstood my comment. I think that all strict functions should not have an "arguments" or "caller" property. Instead those properties should be on %FunctionPrototype% and should throw a type error on get or set.

For example,
function foo() {
    "use strict";
    return 1;
}

class bar {}


Object.getOwnPropertyDescriptor(foo, "arguments") // undefined
Object.getOwnPropertyDescriptor(bar, "arguments") // undefined
Object.getOwnPropertyDescriptor(foo, "caller") // undefined
Object.getOwnPropertyDescriptor(bar, "caller") // undefined

Object.getOwnPropertyDescriptor(foo.__proto__, "arguments") // { get: () => { throw new TypeError(), set: () => { throw new TypeError() }, writable: false, enumerable: false }
Object.getOwnPropertyDescriptor(foo.__proto__, "caller") // { get: () => { throw new TypeError(), set: () => { throw new TypeError() }, writable: false, enumerable: false }

Object.getOwnPropertyDescriptor(bar.__proto__, "arguments") // { get: () => { throw new TypeError(), set: () => { throw new TypeError() }, writable: false, enumerable: false }
Object.getOwnPropertyDescriptor(bar.__proto__, "caller") // { get: () => { throw new TypeError(), set: () => { throw new TypeError() }, writable: false, enumerable: false }

Does my example seem correct to you based on the spec?

[GSkachkov]

(In reply to comment #17)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Comment on attachment 278360 [details]
> > > Patch
> > > 
> > > I'm a little confused, if I have read the spec correctly
> > > (https://tc39.github.io/ecma262/#sec-forbidden-extensions), it looks like
> > > "arguments" and "caller" should not be on any non-strict function anyway. It
> > Hmm, I did not find that about non-strict function in this spec. Only about
> > function in strict-mode and arrow function, class declaration and etc.
> > > seems like we should just get rid of those properties and make them act like
> > > normal for non-strict functions. I'm pretty sure that all class constructors
> > > are strict anyway so I think this would entirely fix this issue. Is there
> > > something I am missing?
> > 
> > Currently my patch does not cover arrow function, GeneratorDeclaration,
> > GeneratorExpression and functions created using the bind. So I'll add try to
> > cover those cases
> 
> I think you might have misunderstood my comment. I think that all strict
> functions should not have an "arguments" or "caller" property. Instead those
> properties should be on %FunctionPrototype% and should throw a type error on
> get or set.
> 
> For example,
> function foo() {
>     "use strict";
>     return 1;
> }
> 
> class bar {}
> 
> 
> Object.getOwnPropertyDescriptor(foo, "arguments") // undefined
> Object.getOwnPropertyDescriptor(bar, "arguments") // undefined
> Object.getOwnPropertyDescriptor(foo, "caller") // undefined
> Object.getOwnPropertyDescriptor(bar, "caller") // undefined
> 
> Object.getOwnPropertyDescriptor(foo.__proto__, "arguments") // { get: () =>
> { throw new TypeError(), set: () => { throw new TypeError() }, writable:
> false, enumerable: false }
> Object.getOwnPropertyDescriptor(foo.__proto__, "caller") // { get: () => {
> throw new TypeError(), set: () => { throw new TypeError() }, writable:
> false, enumerable: false }
> 
> Object.getOwnPropertyDescriptor(bar.__proto__, "arguments") // { get: () =>
> { throw new TypeError(), set: () => { throw new TypeError() }, writable:
> false, enumerable: false }
> Object.getOwnPropertyDescriptor(bar.__proto__, "caller") // { get: () => {
> throw new TypeError(), set: () => { throw new TypeError() }, writable:
> false, enumerable: false }
> 
> Does my example seem correct to you based on the spec?

I got it. I think you are right. It should be in this way.

[GSkachkov]

Created attachment 281184 [details]
Patch

Refactored patch

[GSkachkov]

Created attachment 281186 [details]
Patch

Fix build

[Build Bot]

Comment on attachment 281186 [details]
Patch

Attachment 281186 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1496045

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 281189 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281186 [details]
Patch

Attachment 281186 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1496044

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 281190 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281186 [details]
Patch

Attachment 281186 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1496067

New failing tests:
js/kde/function_arguments.html

[Build Bot]

Created attachment 281191 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 281186 [details]
Patch

Attachment 281186 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1496051

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 281193 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 281274 [details]
Patch

Fix webkit tests

[GSkachkov]

Created attachment 281276 [details]
Patch

Made rebase

[Keith Miller]

Comment on attachment 281276 [details]
Patch

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

So I'm a little confused what this patch changed. It would be great if the changelog went into more detail on what the code you changed does. It might also be helpful if you explained why you made the changes you did. Also, is there a reason why caller and arguments are not just normal static, lazy properties? Then you can have JSFunction intercept the lookup of arguments and caller for non-strict functions and it will probably make this code much simpler.

> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:247
> +    if (thisObject->inherits(JSFunction::info())) {
> +        JSFunction* function = jsCast<JSFunction*>(thisObject);

You can just do if (JSFunction* function = jsDynamicCast<JSFunction*>(JSValue::decode(thisValue)))

> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:254
> +        ASSERT(!function->isHostFunction());

Is this code reachable?

[GSkachkov]

(In reply to comment #31)
> Comment on attachment 281276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281276&action=review
> 
> So I'm a little confused what this patch changed. It would be great if the
> changelog went into more detail on what the code you changed does. It might
> also be helpful if you explained why you made the changes you did. Also, is
> there a reason why caller and arguments are not just normal static, lazy
> properties? Then you can have JSFunction intercept the lookup of arguments
> and caller for non-strict functions and it will probably make this code much
> simpler.
OK. I'll try explain my changes in more details in ChangeLog. I think mostly my patch is moving all logic that is related to caller/arguments from JSFunction to FunctionPrototype with small adjustments. The main reason why I moved caller/arguments from JSFunction to FunctionPrototype class, because I decided to have all code that is related to caller/arguments in one place:
So code that handle cases when lookup of arguments/caller of function and arguments/caller of function prototype in one place:
  var foo = function () {};
  var args = foo.arguments; // case 1 
  var proto = foo.__proto__;
  var caller = proto.caller; // case 2 

Do think it is better to split this logic between JSFunction and FunctionPrototype classes? 

> 
> > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:247
> > +    if (thisObject->inherits(JSFunction::info())) {
> > +        JSFunction* function = jsCast<JSFunction*>(thisObject);
> 
> You can just do if (JSFunction* function =
> jsDynamicCast<JSFunction*>(JSValue::decode(thisValue)))
> 
> > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:254
> > +        ASSERT(!function->isHostFunction());
> 
> Is this code reachable?

[GSkachkov]

Created attachment 286891 [details]
Patch

Rebase the patch

[GSkachkov]

Created attachment 286904 [details]
Patch

Rebase the patch #2

[Build Bot]

Comment on attachment 286904 [details]
Patch

Attachment 286904 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1936681

New failing tests:
js/class-method-and-constructor-properties.html
inspector/model/remote-object-get-properties.html
js/basic-strict-mode.html
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 286917 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 286904 [details]
Patch

Attachment 286904 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1936684

New failing tests:
js/class-method-and-constructor-properties.html
inspector/model/remote-object-get-properties.html
js/basic-strict-mode.html
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 286918 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 286904 [details]
Patch

Attachment 286904 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1936663

New failing tests:
js/class-method-and-constructor-properties.html
js/basic-strict-mode.html
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 286920 [details]
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Build Bot]

Comment on attachment 286904 [details]
Patch

Attachment 286904 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1936721

New failing tests:
js/class-method-and-constructor-properties.html
inspector/model/remote-object-get-properties.html
js/basic-strict-mode.html
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 286921 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 286952 [details]
Patch

Fix brocken tests

[Build Bot]

Comment on attachment 286952 [details]
Patch

Attachment 286952 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1939137

New failing tests:
js/class-method-and-constructor-properties.html
inspector/model/remote-object-get-properties.html

[Build Bot]

Created attachment 286955 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 286952 [details]
Patch

Attachment 286952 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1939142

New failing tests:
js/class-method-and-constructor-properties.html
inspector/model/remote-object-get-properties.html

[Build Bot]

Created attachment 286956 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 286952 [details]
Patch

Attachment 286952 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1939140

New failing tests:
js/class-method-and-constructor-properties.html
inspector/model/remote-object-get-properties.html

[Build Bot]

Created attachment 286957 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 286952 [details]
Patch

Attachment 286952 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1939154

New failing tests:
js/class-method-and-constructor-properties.html

[Build Bot]

Created attachment 286958 [details]
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[GSkachkov]

Comment on attachment 286952 [details]
Patch

F

[GSkachkov]

Created attachment 286999 [details]
Patch

Refactoring of the patch

[GSkachkov]

(In reply to comment #31)
> Comment on attachment 281276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281276&action=review
> 
> So I'm a little confused what this patch changed. It would be great if the
> changelog went into more detail on what the code you changed does. It might
> also be helpful if you explained why you made the changes you did. Also, is
> there a reason why caller and arguments are not just normal static, lazy
> properties? Then you can have JSFunction intercept the lookup of arguments
> and caller for non-strict functions and it will probably make this code much
> simpler.
> 
I've made refactoring of the Patch as you suggested, so it allow to decrease size of changes

[Build Bot]

Comment on attachment 286999 [details]
Patch

Attachment 286999 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1941656

New failing tests:
js/class-method-and-constructor-properties.html

[Build Bot]

Created attachment 287017 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 286999 [details]
Patch

Attachment 286999 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1941659

New failing tests:
js/class-method-and-constructor-properties.html

[Build Bot]

Created attachment 287018 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Keith Miller]

Comment on attachment 286999 [details]
Patch

Looks like EWS is angry that you deleted the class-method-and-constructor-properties.js file. Also, can you explain why "arguments" and "caller" are on Function prototype, I'm not sure why I follow that they need to be there?

[Build Bot]

Comment on attachment 286999 [details]
Patch

Attachment 286999 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1941670

New failing tests:
js/class-method-and-constructor-properties.html

[Build Bot]

Created attachment 287020 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Build Bot]

Comment on attachment 286999 [details]
Patch

Attachment 286999 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1942205

New failing tests:
js/class-method-and-constructor-properties.html

[Build Bot]

Created attachment 287042 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 287129 [details]
Patch

[GSkachkov]

(In reply to comment #59)
> Comment on attachment 286999 [details]
> Patch
> 
> Looks like EWS is angry that you deleted the
> class-method-and-constructor-properties.js file. Also, can you explain why
> "arguments" and "caller" are on Function prototype, I'm not sure why I
> follow that they need to be there?

I move this to the Function prototype to allow execute following code:
var f = function ()  {}
f.__proto__.caller; //Should raise type error

I've checked in Chrome and FireFox:
1) Chrome returns TypeError: 'caller' and 'arguments' are restricted function properties and cannot be accessed in this context
2) FireFox return just null.

[GSkachkov]

Also there are two tests that covered this case 
https://github.com/WebKit/webkit/blob/master/JSTests/test262/test/built-ins/Function/prototype/restricted-property-arguments.js and https://github.com/WebKit/webkit/blob/master/JSTests/test262/test/built-ins/Function/prototype/restricted-property-caller.js

[Keith Miller]

Comment on attachment 287129 [details]
Patch

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

I think this is getting close but I still think there are a few things that need to be changed.

> Source/JavaScriptCore/ChangeLog:62
> +        * runtime/Executable.h:
> +        * runtime/FunctionPrototype.cpp:
> +        (JSC::FunctionPrototype::defineOwnProperty):
> +        (JSC::FunctionPrototype::getThrowTypeErrorOwnPropertySlot):
> +        (JSC::FunctionPrototype::getOwnPropertySlot):
> +        (JSC::FunctionPrototype::getOwnNonIndexPropertyNames):
> +        (JSC::functionProtoFuncBind): Deleted.
> +        * runtime/FunctionPrototype.h:
> +        * runtime/JSFunction.cpp:
> +        (JSC::JSFunction::getOwnPropertySlot):
> +        (JSC::JSFunction::getOwnNonIndexPropertyNames):
> +        (JSC::JSFunction::put):
> +        (JSC::JSFunction::deleteProperty):
> +        (JSC::JSFunction::defineOwnProperty):
> +        * runtime/JSFunction.h:
> +        * runtime/JSObject.cpp:
> +        (JSC::ordinarySetSlow):
> +        (JSC::JSObject::getOwnPropertyDescriptor):
> +        * runtime/PropertySlot.h:
> +        (JSC::PropertySlot::PropertySlot):
> +        (JSC::PropertySlot::setIsClassMethod):
> +        (JSC::PropertySlot::isClassMethod):
> +        (JSC::PropertySlot::isTaintedByOpaqueObject): Deleted.

Delete.

> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:184
> +    if (!object->inherits(JSFunction::info()))
> +        return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException);

I don't think you need this check. object should always be FunctionPrototype*.

> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:204
> +    JSFunction* thisObject = jsCast<JSFunction*>(object);
> +    if (thisObject->isHostOrBuiltinFunction())
> +        return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException);
> +
> +    if (propertyName == exec->propertyNames().arguments) {
> +        if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isArrowFunction()) {
> +            PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry);
> +            if (!Base::getOwnPropertySlot(thisObject, exec, propertyName, slot))
> +                thisObject->putDirectAccessor(exec, propertyName, thisObject->globalObject()->throwTypeErrorArgumentsCalleeAndCallerGetterSetter(), DontDelete | DontEnum | Accessor);
> +            return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException);
> +        }
> +    } else if (propertyName == exec->propertyNames().caller) {
> +        if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isArrowFunction()) {
> +            PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry);
> +            if (!Base::getOwnPropertySlot(thisObject, exec, propertyName, slot))
> +                thisObject->putDirectAccessor(exec, propertyName, thisObject->globalObject()->throwTypeErrorArgumentsCalleeAndCallerGetterSetter(), DontDelete | DontEnum | Accessor);
> +            return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException);
> +        }
> +    }

I think this could just be FunctionPrototype* thisObject = jsCast<FunctionPrototype*>(object);

You would need to add a reifyLazyPropertyIfNeeded function to FunctionPrototype that handles arguments and length like:

void FunctionPrototype::reifyLazyPropertyIfNeeded(exec, propertyName) {
    if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller)
        // Do reification of arguments/caller to type error getter/setter if needed.
    Base::reifyLazyPropertyIfNeeded(exec, propertyName);
}

> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:231
> +    if (propertyName == exec->propertyNames().caller || propertyName == exec->propertyNames().arguments) {
> +        // For non-host functions, don't let these properties by deleted - except by DefineOwnProperty.
> +        if (slot.thisValue().inherits(JSFunction::info())) {
> +            JSFunction* thisObject = jsDynamicCast<JSFunction*>(slot.thisValue());
> +            if (!thisObject->isHostOrBuiltinFunction())
> +                return getThrowTypeErrorOwnPropertySlot(object, exec, propertyName, slot);
> +        } else if (slot.thisValue().inherits(FunctionPrototype::info()))
> +            return getThrowTypeErrorOwnPropertySlot(object, exec, propertyName, slot);
> +    }

This could just be (if you do my change above): 

FunctionPrototype* thisObject = jsCast<FunctionPrototype*>(object);
thisObject->reifyLazyPropertyIfNeeded(exec, propertyName);

> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:246
> +        VM& vm = exec->vm();
> +        // Make sure prototype has been reified.
> +        PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry);
> +        thisObject->methodTable(vm)->getOwnPropertySlot(thisObject, exec, vm.propertyNames->prototype, slot);
> +        
> +        propertyNames.add(vm.propertyNames->arguments);
> +        propertyNames.add(vm.propertyNames->caller);

Ditto.

> Source/JavaScriptCore/runtime/JSFunction.cpp:402
> +        if (!thisObject->jsExecutable()->isStrictMode()) {
> +            propertyNames.add(vm.propertyNames->arguments);
> +            propertyNames.add(vm.propertyNames->caller);
> +        }

You might need to change this to work with my proposal for reification.

> Source/JavaScriptCore/runtime/JSObject.cpp:517
> +        // We do not update the value of caller & arguments properties in function

Why do we not allow the user to set arguments on a strict function?

> Source/JavaScriptCore/runtime/JSObject.cpp:518
> +        if (object->inherits(JSFunction::info()))

I think we usually just do this with jsDynamicCast<JSFunction>(object)

> Source/JavaScriptCore/runtime/JSObject.cpp:2858
> +    if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller) {
> +        if (auto thisObject = jsDynamicCast<JSFunction*>(this)) {
> +            if (!thisObject->isHostFunctionNonInline() && thisObject->jsExecutable()->isClass() && !thisObject->jsExecutable()->isClassConstructorFunction())
> +                slot.setIsClassMethod();
> +        }
> +    }

What does this do? It doesn't look like you use the isClassMethod bit anywhere?

[Keith Miller]

(In reply to comment #66)
> Also there are two tests that covered this case 
> https://github.com/WebKit/webkit/blob/master/JSTests/test262/test/built-ins/
> Function/prototype/restricted-property-arguments.js and
> https://github.com/WebKit/webkit/blob/master/JSTests/test262/test/built-ins/
> Function/prototype/restricted-property-caller.js

It would be great if you could also update the expected results for these tests in JSTests/test262.yaml too.

[GSkachkov]

Created attachment 287871 [details]
Patch

Fix comments&refactoring

[GSkachkov]

Comment on attachment 287129 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:62
>> +        (JSC::PropertySlot::isTaintedByOpaqueObject): Deleted.
> 
> Delete.

Done

>> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:184
>> +        return Base::defineOwnProperty(object, exec, propertyName, descriptor, throwException);
> 
> I don't think you need this check. object should always be FunctionPrototype*.

Remove whole method :-)

>> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:204
>> +    }
> 
> I think this could just be FunctionPrototype* thisObject = jsCast<FunctionPrototype*>(object);
> 
> You would need to add a reifyLazyPropertyIfNeeded function to FunctionPrototype that handles arguments and length like:
> 
> void FunctionPrototype::reifyLazyPropertyIfNeeded(exec, propertyName) {
>     if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller)
>         // Do reification of arguments/caller to type error getter/setter if needed.
>     Base::reifyLazyPropertyIfNeeded(exec, propertyName);
> }

Removed whole method

>> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:231
>> +    }
> 
> This could just be (if you do my change above): 
> 
> FunctionPrototype* thisObject = jsCast<FunctionPrototype*>(object);
> thisObject->reifyLazyPropertyIfNeeded(exec, propertyName);

Removed whole method

>> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:246
>> +        propertyNames.add(vm.propertyNames->caller);
> 
> Ditto.

Removed whole method

>> Source/JavaScriptCore/runtime/JSFunction.cpp:402
>> +        }
> 
> You might need to change this to work with my proposal for reification.

I've just added small check, to add this method for function not in Strict mode

>> Source/JavaScriptCore/runtime/JSObject.cpp:517
>> +        // We do not update the value of caller & arguments properties in function
> 
> Why do we not allow the user to set arguments on a strict function?

Removed whole change

>> Source/JavaScriptCore/runtime/JSObject.cpp:2858
>> +    }
> 
> What does this do? It doesn't look like you use the isClassMethod bit anywhere?

Yes, you are right. It is not used anymore. Removed

[Keith Miller]

Comment on attachment 287871 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSFunction.cpp:404
> +        if (!thisObject->jsExecutable()->isStrictMode()) {

Don't you want !thisObject->jsExecutable()->isStrictMode() && !thisObject->jsExecutable()->isES6Function()?

Also, does this work with Function.prototype when the arguments property has been deleted? i.e.

delete Function.prototype.arguments;
for (name in Function.prototype) {
   if (name !== "arguments")
       throw new Error("bad name");
}

[GSkachkov]

Created attachment 288449 [details]
Patch

Small fix & rebase

[GSkachkov]

Comment on attachment 287871 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSFunction.cpp:404
>> +        if (!thisObject->jsExecutable()->isStrictMode()) {
> 
> Don't you want !thisObject->jsExecutable()->isStrictMode() && !thisObject->jsExecutable()->isES6Function()?
> 
> Also, does this work with Function.prototype when the arguments property has been deleted? i.e.
> 
> delete Function.prototype.arguments;
> for (name in Function.prototype) {
>    if (name !== "arguments")
>        throw new Error("bad name");
> }

Yeah, I've add !thisObject->jsExecutable()->isES6Function() condition to the new patch. 

In  case of 'delete Function.prototype.arguments;' we delete it from FunctionPrototyp(FunctionPrototype.cpp) where it added with property DontEnum, so it will not appear during  for..in operation before and after delete operation.

[Keith Miller]

(In reply to comment #73)
> Comment on attachment 287871 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287871&action=review
> 
> >> Source/JavaScriptCore/runtime/JSFunction.cpp:404
> >> +        if (!thisObject->jsExecutable()->isStrictMode()) {
> > 
> > Don't you want !thisObject->jsExecutable()->isStrictMode() && !thisObject->jsExecutable()->isES6Function()?
> > 
> > Also, does this work with Function.prototype when the arguments property has been deleted? i.e.
> > 
> > delete Function.prototype.arguments;
> > for (name in Function.prototype) {
> >    if (name !== "arguments")
> >        throw new Error("bad name");
> > }
> 
> Yeah, I've add !thisObject->jsExecutable()->isES6Function() condition to the
> new patch. 
> 
> In  case of 'delete Function.prototype.arguments;' we delete it from
> FunctionPrototyp(FunctionPrototype.cpp) where it added with property
> DontEnum, so it will not appear during  for..in operation before and after
> delete operation.

Ah, I was thinking that FunctionPrototype subclassed JSFunction but I see that it actually subclasses InternalFunction. Then, you're right, it should work for Function.prototype.

[GSkachkov]

Created attachment 288454 [details]
Patch

Fix build

[Build Bot]

Comment on attachment 288454 [details]
Patch

Attachment 288454 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2044561

New failing tests:
js/es6-function-properties.html

[Build Bot]

Created attachment 288469 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288454 [details]
Patch

Attachment 288454 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2044582

New failing tests:
js/es6-function-properties.html

[Build Bot]

Created attachment 288471 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288454 [details]
Patch

Attachment 288454 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2044600

New failing tests:
js/es6-function-properties.html

[Build Bot]

Comment on attachment 288454 [details]
Patch

Attachment 288454 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2044581

New failing tests:
js/es6-function-properties.html

[Build Bot]

Created attachment 288474 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288454 [details]
Patch

Attachment 288454 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2045124

New failing tests:
js/es6-function-properties.html

[Build Bot]

Created attachment 288480 [details]
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[GSkachkov]

Created attachment 288483 [details]
Patch

Use debug intead of print in Layout tests

[Keith Miller]

Comment on attachment 288483 [details]
Patch

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

r=me if you fix the typo. Thanks, for powering through this patch! :D

> Source/JavaScriptCore/ChangeLog:15
> +        To implement this patch âcallerâ and âargumentsâ were put to the FunctionPrototype

should be "caller" and "arguments".

[GSkachkov]

All reviewed patches have been landed.  Closing bug.
Committed r205856: <https://trac.webkit.org/changeset/205856>