Bug 162530 – [JSC] ES6 Method functions should not have prototype

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

[Caitlin Potter (:caitp)]

ES6 object literal and class literal Methods, which are not GeneratorMethods and not "constructor" (in classes) incorrectly have a "prototype" property.

Example:

```
({ foo() {} }).foo.prototype // produces an Object with an own property "constructor" whose value is the "foo" function. Should be `undefined`
```

- Per https://tc39.github.io/ecma262/#sec-method-definitions-runtime-semantics-propertydefinitionevaluation, the "prototype" property should not be added.

This causes test262 failure for test262/test/language/expressions/object/method-definition/name-prototype-prop.js, and likely others

[Caitlin Potter (:caitp)]

Created attachment 289749 [details]
Do not add prototype to MethodDefinitions

[Build Bot]

Comment on attachment 289749 [details]
Do not add prototype to MethodDefinitions

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

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

[Build Bot]

Created attachment 289752 [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 289749 [details]
Do not add prototype to MethodDefinitions

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

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

[Build Bot]

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

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

[Build Bot]

Comment on attachment 289749 [details]
Do not add prototype to MethodDefinitions

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

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

[Build Bot]

Created attachment 289754 [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

[Caitlin Potter (:caitp)]

Created attachment 289755 [details]
Fix LayoutTests

[Build Bot]

Comment on attachment 289755 [details]
Fix LayoutTests

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

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

[Build Bot]

Created attachment 289757 [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 289755 [details]
Fix LayoutTests

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

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

[Build Bot]

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

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

[Build Bot]

Comment on attachment 289755 [details]
Fix LayoutTests

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

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

[Build Bot]

Created attachment 289759 [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

[Build Bot]

Comment on attachment 289755 [details]
Fix LayoutTests

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

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

[Build Bot]

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

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

[Caitlin Potter (:caitp)]

Created attachment 289761 [details]
Fix LayoutTests

[Saam Barati]

Comment on attachment 289761 [details]
Fix LayoutTests

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

> Source/JavaScriptCore/runtime/JSFunction.cpp:356
> +    bool isMethod = executable->isMethod() && !executable->isClass();

Why the  "!isClass" here?
We distinguish between methods on object literals and methods on classes?

What's the behavior for object literals? Please add tests for that.

[Caitlin Potter (:caitp)]

Comment on attachment 289761 [details]
Fix LayoutTests

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

>> Source/JavaScriptCore/runtime/JSFunction.cpp:356
>> +    bool isMethod = executable->isMethod() && !executable->isClass();
> 
> Why the  "!isClass" here?
> We distinguish between methods on object literals and methods on classes?
> 
> What's the behavior for object literals? Please add tests for that.

'isClass' really means "class constructor", which is a special case (gets a prototype property during MakeConstructor (https://tc39.github.io/ecma262/#sec-makeconstructor) during step 16 of ClassDefinitionEvaluation (https://tc39.github.io/ecma262/#sec-runtime-semantics-classdefinitionevaluation).

I'll add some extra tests either tomorrow or during the week)

[Saam Barati]

(In reply to comment #19)
> Comment on attachment 289761 [details]
> Fix LayoutTests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289761&action=review
> 
> >> Source/JavaScriptCore/runtime/JSFunction.cpp:356
> >> +    bool isMethod = executable->isMethod() && !executable->isClass();
> > 
> > Why the  "!isClass" here?
> > We distinguish between methods on object literals and methods on classes?
> > 
> > What's the behavior for object literals? Please add tests for that.
> 
> 'isClass' really means "class constructor", which is a special case (gets a
> prototype property during MakeConstructor
> (https://tc39.github.io/ecma262/#sec-makeconstructor) during step 16 of
> ClassDefinitionEvaluation
> (https://tc39.github.io/ecma262/#sec-runtime-semantics-
> classdefinitionevaluation).
Oh wow. Let's please rename that method. That's horribly misleading.

> 
> I'll add some extra tests either tomorrow or during the week)
Cool, thanks.

[Caitlin Potter (:caitp)]

Created attachment 289766 [details]
Add extra es6-function-properties test cases

[Caitlin Potter (:caitp)]

(In reply to comment #20)
> (In reply to comment #19)
> > Comment on attachment 289761 [details]
> > Fix LayoutTests
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=289761&action=review
> > 
> > >> Source/JavaScriptCore/runtime/JSFunction.cpp:356
> > >> +    bool isMethod = executable->isMethod() && !executable->isClass();
> > > 
> > > Why the  "!isClass" here?
> > > We distinguish between methods on object literals and methods on classes?
> > > 
> > > What's the behavior for object literals? Please add tests for that.
> > 
> > 'isClass' really means "class constructor", which is a special case (gets a
> > prototype property during MakeConstructor
> > (https://tc39.github.io/ecma262/#sec-makeconstructor) during step 16 of
> > ClassDefinitionEvaluation
> > (https://tc39.github.io/ecma262/#sec-runtime-semantics-
> > classdefinitionevaluation).
> Oh wow. Let's please rename that method. That's horribly misleading.
> 
> > 
> > I'll add some extra tests either tomorrow or during the week)
> Cool, thanks.

Got it done while waiting in the airport instead, I guess :) I'm not positive the tests will pass (laptop is a bit low powered to check this at the moment), but if it doesn't, will obviously fix it up tomorrow or monday.

[Build Bot]

Comment on attachment 289766 [details]
Add extra es6-function-properties test cases

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

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

[Build Bot]

Created attachment 289769 [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 289766 [details]
Add extra es6-function-properties test cases

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

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

[Build Bot]

Created attachment 289770 [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 289766 [details]
Add extra es6-function-properties test cases

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

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

[Build Bot]

Created attachment 289771 [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

[Build Bot]

Comment on attachment 289766 [details]
Add extra es6-function-properties test cases

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

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

[Build Bot]

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

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

[Caitlin Potter (:caitp)]

realized after takeout the generator method expectations were wrong :( sorry bout the spam

[Caitlin Potter (:caitp)]

Huh, I guess WebKit also gets part of https://tc39.github.io/ecma262/#sec-forbidden-extensions wrong, not forbidding caller/arguments for accessors in object literals. I guess I'll fix that in this patch as well.

[Caitlin Potter (:caitp)]

Created attachment 289779 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones

[Caitlin Potter (:caitp)]

Created attachment 289780 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk

[Build Bot]

Comment on attachment 289780 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk

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

New failing tests:
js/arrowfunction-prototype.html
js/caller-property.html
js/mozilla/strict/15.3.5.2.html

[Build Bot]

Created attachment 289781 [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 289780 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk

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

New failing tests:
js/arrowfunction-prototype.html
js/caller-property.html
js/mozilla/strict/15.3.5.2.html

[Build Bot]

Created attachment 289782 [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

[Build Bot]

Comment on attachment 289780 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk

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

New failing tests:
js/arrowfunction-prototype.html
js/caller-property.html
js/mozilla/strict/15.3.5.2.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 289780 [details]
Fix accessors too, and fix that one LayoutTest, but didn't try any other ones --- and rebase on Trunk

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

New failing tests:
js/arrowfunction-prototype.html
js/caller-property.html
js/mozilla/strict/15.3.5.2.html

[Build Bot]

Created attachment 289784 [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

[Caitlin Potter (:caitp)]

Created attachment 289815 [details]
Fix LayoutTests

[Caitlin Potter (:caitp)]

Would it be frowned upon to request EditBugs permissions so that I can obsolete all of the no-longer-relevant EWS failures? :)

[Saam Barati]

(In reply to comment #44)
> Would it be frowned upon to request EditBugs permissions so that I can
> obsolete all of the no-longer-relevant EWS failures? :)

No, that would be great. Have you landed more than 10 patches in WebKit? (I would guess yes.) If so, we should get you committer access too.

[Caitlin Potter (:caitp)]

(In reply to comment #45)
> (In reply to comment #44)
> > Would it be frowned upon to request EditBugs permissions so that I can
> > obsolete all of the no-longer-relevant EWS failures? :)
> 
> No, that would be great. Have you landed more than 10 patches in WebKit? (I
> would guess yes.) If so, we should get you committer access too.

I'm a committer, but I do not yet have EditBugs, heh :)

[Saam Barati]

(In reply to comment #44)
> Would it be frowned upon to request EditBugs permissions so that I can
> obsolete all of the no-longer-relevant EWS failures? :)

Ok I'll try to get you edit bugs privileges tomorrow when more folks are on IRC

[Caitlin Potter (:caitp)]

(In reply to comment #47)
> (In reply to comment #44)
> > Would it be frowned upon to request EditBugs permissions so that I can
> > obsolete all of the no-longer-relevant EWS failures? :)
> 
> Ok I'll try to get you edit bugs privileges tomorrow when more folks are on
> IRC

Hi, has there been any update on this? I can send a mail to webkit-dev to ask for this, but I prefer to avoid that if possible.

[Caitlin Potter (:caitp)]

Okay, cleaned up the bug a bit. It would be fantastic if I could get a look at this

[Caitlin Potter (:caitp)]

Created attachment 290767 [details]
Patch

[Saam Barati]

Comment on attachment 290767 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

Please add some explanatory text here, and maybe a link to the spec.

> Source/JavaScriptCore/runtime/Executable.h:673
> +        ).contains(parseMode()) || isClass();

I'm still in favor of renaming isClass() to isClassConstructor()

> Source/JavaScriptCore/runtime/JSFunction.cpp:342
> +    bool isES6OrStrictMode = function->jsExecutable()->isStrictMode() || function->jsExecutable()->isES6Function();
> +    if (function->isHostOrBuiltinFunction() || !isES6OrStrictMode)

Please change the error message below since this no longer just throws for strict mode. Also, a note on style, I think this condition would be easier to read like so:
if (function->isHostOrbuiltinFunction() || (!function->isStrictMode() && !function->isES6Function()))
    return caller;
... throw error

> Source/JavaScriptCore/runtime/JSFunction.cpp:357
> +    FunctionExecutable* executable = thisObject->jsExecutable();
> +    bool hasPrototype = executable->hasInitialPrototypeProperty();

Nit: I don't think these temporary variables add any value.

[Caitlin Potter (:caitp)]

Created attachment 290925 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 290767 [details]
Patch

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

I think I've addressed this (except for the `isClass() -> isClassConstructor()` difference, for the reason mentioned --- happy to submit a followup to do that if it's important, though)

>> Source/JavaScriptCore/runtime/Executable.h:673
>> +        ).contains(parseMode()) || isClass();
> 
> I'm still in favor of renaming isClass() to isClassConstructor()

Apparently I could use `isClassConstructorFunction()` instead, if you'd prefer that.

But: a class is its constructor in ECMAScript, so I'm not sure the distinction really adds much. I think I'd prefer to make this change in a separate bug where it's the main issue being addressed.

>> Source/JavaScriptCore/runtime/JSFunction.cpp:342
>> +    if (function->isHostOrBuiltinFunction() || !isES6OrStrictMode)
> 
> Please change the error message below since this no longer just throws for strict mode. Also, a note on style, I think this condition would be easier to read like so:
> if (function->isHostOrbuiltinFunction() || (!function->isStrictMode() && !function->isES6Function()))
>     return caller;
> ... throw error

Would you prefer something like "Function.caller cannot be accessed in this context"? I'll try that.

>> Source/JavaScriptCore/runtime/JSFunction.cpp:357
>> +    bool hasPrototype = executable->hasInitialPrototypeProperty();
> 
> Nit: I don't think these temporary variables add any value.

Done

[Geoffrey Garen]

Comment on attachment 290925 [details]
Patch

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

> Source/JavaScriptCore/runtime/Executable.h:665
> +    bool isES6Function() const
> +    {
> +        // Per https://tc39.github.io/ecma262/#sec-forbidden-extensions, special function forms which are forbidden from having legacy
> +        // Annex B properties
> +        return SourceParseModeSet(
> +            SourceParseMode::ArrowFunctionMode,
> +            SourceParseMode::GeneratorBodyMode,
> +            SourceParseMode::GeneratorWrapperFunctionMode,
> +            SourceParseMode::MethodMode,
> +            SourceParseMode::GetterMode,
> +            SourceParseMode::SetterMode).contains(parseMode()) || isClassConstructorFunction();
> +    }

I see a few problems with this function and its name:

(1) Getters and setters are not ES6 features, so it's surprising that getters and setters claim to be ES6 functions.

(2) The comment in the function, which describes its behavior, does not say "returns true for ES6 functions" -- implying that the name is incorrect or at least incomplete.

(3) All of our clients seem to check isStrictMode() || isES6Function(). That implies an impedance mismatch, in which our clients would prefer a different function that built in the whole test for "do I support .arguments and .caller".

I think I would change this function to incorporate the isStrictMode() test and then rename it to "hasRestrictedFunctionProperties" or "hasCallerAndArugmentsProperties". What do you think?

> Source/JavaScriptCore/runtime/Executable.h:666
> +    bool hasInitialPrototypeProperty() const

I would just call this "hasPrototypeProperty" to match "hasRestrictedFunctionProperties" or "hasCallerAndArugmentsProperties".

> Source/JavaScriptCore/runtime/Executable.h:668
> +        // Return `true` if is a Function with an initial "prototype" property

No need for this comment: The function name already says it.

> Source/JavaScriptCore/runtime/JSFunction.cpp:344
> +    bool isES6OrStrictMode = function->jsExecutable()->isStrictMode() || function->jsExecutable()->isES6Function();
> +    if (function->isHostOrBuiltinFunction() || !isES6OrStrictMode)
>          return JSValue::encode(caller);
> -    return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller used to retrieve strict caller")));
> +    return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context")));

getOwnPropertySlot does this for .caller:

        if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function())
            return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);

Under what conditions can we invoke the callerGetter for a strict mode or ES6 function?

[Caitlin Potter (:caitp)]

Comment on attachment 290925 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Executable.h:665
>> +    }
> 
> I see a few problems with this function and its name:
> 
> (1) Getters and setters are not ES6 features, so it's surprising that getters and setters claim to be ES6 functions.
> 
> (2) The comment in the function, which describes its behavior, does not say "returns true for ES6 functions" -- implying that the name is incorrect or at least incomplete.
> 
> (3) All of our clients seem to check isStrictMode() || isES6Function(). That implies an impedance mismatch, in which our clients would prefer a different function that built in the whole test for "do I support .arguments and .caller".
> 
> I think I would change this function to incorporate the isStrictMode() test and then rename it to "hasRestrictedFunctionProperties" or "hasCallerAndArugmentsProperties". What do you think?

"hasRestricted...Properties" is the inverse condition, but sounds reasonable.

While it's true that getters/setters predate ES6, they became Methods in ES6, and this was found to be web compatible (since they are almost never interacted with as function), hence why I think the currentname is "ok".

>> Source/JavaScriptCore/runtime/Executable.h:666
>> +    bool hasInitialPrototypeProperty() const
> 
> I would just call this "hasPrototypeProperty" to match "hasRestrictedFunctionProperties" or "hasCallerAndArugmentsProperties".

SGTM

>> Source/JavaScriptCore/runtime/JSFunction.cpp:344
>> +    return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context")));
> 
> getOwnPropertySlot does this for .caller:
> 
>         if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function())
>             return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
> 
> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function?

The ones in the caller-property LayoutTest, I suppose.

 > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context.

when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with.

[Caitlin Potter (:caitp)]

Comment on attachment 290925 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSFunction.cpp:344
>>> +    return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context")));
>> 
>> getOwnPropertySlot does this for .caller:
>> 
>>         if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function())
>>             return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
>> 
>> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function?
> 
> The ones in the caller-property LayoutTest, I suppose.
> 
>  > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context.
> 
> when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with.

For what it's worth, as far as I can tell, there is nothing in the spec requiring the behaviour here (preventing ES6 callers from using the caller property of a non-strict function). Seems to be a WebKit-ism.

As an example:

```
function f() { return [...g()]; }
function* g() { yield f.caller; }
function y() { return f(); }

y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC
```

[Caitlin Potter (:caitp)]

Comment on attachment 290925 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSFunction.cpp:344
>>>> +    return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context")));
>>> 
>>> getOwnPropertySlot does this for .caller:
>>> 
>>>         if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function())
>>>             return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
>>> 
>>> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function?
>> 
>> The ones in the caller-property LayoutTest, I suppose.
>> 
>>  > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context.
>> 
>> when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with.
> 
> For what it's worth, as far as I can tell, there is nothing in the spec requiring the behaviour here (preventing ES6 callers from using the caller property of a non-strict function). Seems to be a WebKit-ism.
> 
> As an example:
> 
> ```
> function f() { return [...g()]; }
> function* g() { yield f.caller; }
> function y() { return f(); }
> 
> y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC
> ```

Er, sorry, that's a bad example...

```
var f = () => [...g()];
function* g() { yield f.caller; }
function y() { return f(); }

y()
```

is more accurate, and V8 also throws in this case --- but I'm fairly sure this isn't required by anything. I'll double check.

[Caitlin Potter (:caitp)]

Comment on attachment 290925 [details]
Patch

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

>>>>> Source/JavaScriptCore/runtime/JSFunction.cpp:344
>>>>> +    return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context")));
>>>> 
>>>> getOwnPropertySlot does this for .caller:
>>>> 
>>>>         if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function())
>>>>             return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
>>>> 
>>>> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function?
>>> 
>>> The ones in the caller-property LayoutTest, I suppose.
>>> 
>>>  > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context.
>>> 
>>> when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with.
>> 
>> For what it's worth, as far as I can tell, there is nothing in the spec requiring the behaviour here (preventing ES6 callers from using the caller property of a non-strict function). Seems to be a WebKit-ism.
>> 
>> As an example:
>> 
>> ```
>> function f() { return [...g()]; }
>> function* g() { yield f.caller; }
>> function y() { return f(); }
>> 
>> y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC
>> ```
> 
> Er, sorry, that's a bad example...
> 
> ```
> var f = () => [...g()];
> function* g() { yield f.caller; }
> function y() { return f(); }
> 
> y()
> ```
> 
> is more accurate, and V8 also throws in this case --- but I'm fairly sure this isn't required by anything. I'll double check.

After discussing this with others a bit, I've decided to just revert the changes to callerGetter, which correctly follows the restrictions from annex B.

[Caitlin Potter (:caitp)]

Created attachment 291034 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 291034 [details]
Patch

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

some self-review / notes:

> Source/JavaScriptCore/runtime/Executable.h:658
> +        return !isStrictMode() && parseMode() == SourceParseMode::NormalFunctionMode && !isClassConstructorFunction();

Unfortunately, the builtin default constructor has a strange parseMode() (NormalFunctionMode), so the `!isClassConstructorFunction()` is also needed in addition to the parse mode check.

> Source/JavaScriptCore/runtime/JSFunction.cpp:355
> +    if (propertyName == vm.propertyNames->prototype && thisObject->jsExecutable()->hasPrototypeProperty() && !thisObject->jsExecutable()->isClassConstructorFunction()) {

The `!thisObject->jsExecutable()->isClassConstructorFunction()` is needed because the bytecode for the ClassExprNodes emits a DefineOwnProperty op to add the prototype, which fails if the default prototype is added here.

Maybe it's better to just omit the `|| isClass();` in `hasPrototypeProperty()` to keep this simpler. Thoughts?

> Source/JavaScriptCore/runtime/JSFunction.cpp:381
> +        

TODO: fix whitespace before landing

> Source/JavaScriptCore/runtime/JSFunction.cpp:449
> +    if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller) {

cleanup: remove duped branching

> Source/JavaScriptCore/runtime/JSFunction.cpp:476
> +        if (propertyName == exec->propertyNames().caller || propertyName == exec->propertyNames().arguments)

cleanup: simplify this a bit

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:865
> +    return throwVMTypeError(exec, scope, "'arguments', 'callee', and 'caller' cannot be accessed in this context.");

Was asked to update the error message --- I've reverted the change to the error message in callerGetter(), which is still accurate (will only throw when the caller is a strict function). The more generic "property is not accessible" error is updated to be a bit more generic and hopefully not misleading about the language mode of the receiver.

[Saam Barati]

(In reply to comment #56)
> Comment on attachment 290925 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290925&action=review
> 
> >>> Source/JavaScriptCore/runtime/JSFunction.cpp:344
> >>> +    return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context")));
> >> 
> >> getOwnPropertySlot does this for .caller:
> >> 
> >>         if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function())
> >>             return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
> >> 
> >> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function?
> > 
> > The ones in the caller-property LayoutTest, I suppose.
> > 
> >  > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context.
> > 
> > when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with.
> 
> For what it's worth, as far as I can tell, there is nothing in the spec
> requiring the behaviour here (preventing ES6 callers from using the caller
> property of a non-strict function). Seems to be a WebKit-ism.
> 
> As an example:
> 
> ```
> function f() { return [...g()]; }
> function* g() { yield f.caller; }
> function y() { return f(); }
> 
> y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC
> ```
Why is this a bad example? I wouldn't expect this to throw.

[Saam Barati]

Comment on attachment 291034 [details]
Patch

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

LGTM still, but I'll also give some time for Geoff to comment before I r+

>> Source/JavaScriptCore/runtime/Executable.h:658
>> +        return !isStrictMode() && parseMode() == SourceParseMode::NormalFunctionMode && !isClassConstructorFunction();
> 
> Unfortunately, the builtin default constructor has a strange parseMode() (NormalFunctionMode), so the `!isClassConstructorFunction()` is also needed in addition to the parse mode check.

what is the SourceParseMode for the constructor here:
class Foo { constructor() { } }
I'm guessing it's MethodMode. Why not just make the default constructors also have that SourceParseMode?
(This could be done in a follow up if it seems valid to change it.)

>> Source/JavaScriptCore/runtime/JSFunction.cpp:355
>> +    if (propertyName == vm.propertyNames->prototype && thisObject->jsExecutable()->hasPrototypeProperty() && !thisObject->jsExecutable()->isClassConstructorFunction()) {
> 
> The `!thisObject->jsExecutable()->isClassConstructorFunction()` is needed because the bytecode for the ClassExprNodes emits a DefineOwnProperty op to add the prototype, which fails if the default prototype is added here.
> 
> Maybe it's better to just omit the `|| isClass();` in `hasPrototypeProperty()` to keep this simpler. Thoughts?

This seems confusing. Why did this ever work before?

> Source/JavaScriptCore/runtime/JSFunction.cpp:387
> +        if (!thisObject->jsExecutable()->hasCallerAndArgumentsProperties())

Can you ever have caller but not arguments? Or are those property names always either both present or both not present?

>> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:865
>> +    return throwVMTypeError(exec, scope, "'arguments', 'callee', and 'caller' cannot be accessed in this context.");
> 
> Was asked to update the error message --- I've reverted the change to the error message in callerGetter(), which is still accurate (will only throw when the caller is a strict function). The more generic "property is not accessible" error is updated to be a bit more generic and hopefully not misleading about the language mode of the receiver.

This might be worth a bugzilla bug and a FIXME since we could come up with variants of this function which specialize on what "this context" actually is.
Regarding the phrasing "this context", I wonder if it would be better if we enumerated the places where these properties are disallowed. Not sure if that'd bloat the error message too much or not.

[Saam Barati]

Comment on attachment 291034 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:865
>>> +    return throwVMTypeError(exec, scope, "'arguments', 'callee', and 'caller' cannot be accessed in this context.");
>> 
>> Was asked to update the error message --- I've reverted the change to the error message in callerGetter(), which is still accurate (will only throw when the caller is a strict function). The more generic "property is not accessible" error is updated to be a bit more generic and hopefully not misleading about the language mode of the receiver.
> 
> This might be worth a bugzilla bug and a FIXME since we could come up with variants of this function which specialize on what "this context" actually is.
> Regarding the phrasing "this context", I wonder if it would be better if we enumerated the places where these properties are disallowed. Not sure if that'd bloat the error message too much or not.

Actually, scratch that, I think there are too many functions to enumerate in an error message. I'm ok with "this context"

[Caitlin Potter (:caitp)]

Comment on attachment 290925 [details]
Patch

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

>>>>>>> Source/JavaScriptCore/runtime/JSFunction.cpp:344
>>>>>>> +    return JSValue::encode(throwTypeError(exec, scope, ASCIILiteral("Function.caller cannot be accessed in this context")));
>>>>>> 
>>>>>> getOwnPropertySlot does this for .caller:
>>>>>> 
>>>>>>         if (thisObject->jsExecutable()->isStrictMode() || thisObject->jsExecutable()->isES6Function())
>>>>>>             return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
>>>>>> 
>>>>>> Under what conditions can we invoke the callerGetter for a strict mode or ES6 function?
>>>>> 
>>>>> The ones in the caller-property LayoutTest, I suppose.
>>>>> 
>>>>>  > PASS strictCaller(nonStrictCallee) threw exception TypeError: Function.caller cannot be accessed in this context.
>>>>> 
>>>>> when the callee is an accessor (or a different ES6 function) you get the different error from callerGetter, I guess. Theres no real reason for the tdistinct errors afaik, thats just the way it was to begin with.
>>>> 
>>>> For what it's worth, as far as I can tell, there is nothing in the spec requiring the behaviour here (preventing ES6 callers from using the caller property of a non-strict function). Seems to be a WebKit-ism.
>>>> 
>>>> As an example:
>>>> 
>>>> ```
>>>> function f() { return [...g()]; }
>>>> function* g() { yield f.caller; }
>>>> function y() { return f(); }
>>>> 
>>>> y() // returns `[ y ]` in V8 and SpiderMonkey, but throws in JSC
>>>> ```
>>> 
>>> Er, sorry, that's a bad example...
>>> 
>>> ```
>>> var f = () => [...g()];
>>> function* g() { yield f.caller; }
>>> function y() { return f(); }
>>> 
>>> y()
>>> ```
>>> 
>>> is more accurate, and V8 also throws in this case --- but I'm fairly sure this isn't required by anything. I'll double check.
>> 
>> After discussing this with others a bit, I've decided to just revert the changes to callerGetter, which correctly follows the restrictions from annex B.
> 
> Why is this a bad example? I wouldn't expect this to throw.

They're bad examples because the idea was to show "WebKit throws when the caller is an ES6 function", but n'either example really demonstrates that. It does throw when the caller is strict (which y implicitly is when declared in the web console). I misread callerGetter and thought it was looking at the callee's strictness/ES6ness rather than the caller's, and thus wrote a nonsense example. Hence reverting the change relating to this.

While the spec does not say anything about throwing here (could just return null instead), it'sunspecified and good enough, IMO.

[Caitlin Potter (:caitp)]

Created attachment 291109 [details]
also address nit in Executable.h

[Saam Barati]

Comment on attachment 291109 [details]
also address nit in Executable.h

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

r=me
leaving cq? since I have just one question

> Source/JavaScriptCore/runtime/JSFunction.cpp:456
> +        if (!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()) {
> +            bool okay = thisObject->hasProperty(exec, propertyName);
> +            ASSERT_UNUSED(okay, okay);
> +            scope.release();
> +            return Base::put(thisObject, exec, propertyName, value, slot);
> +        }

I know this is touching code that was here before, but I'm not sure why this is correct. Can you elaborate on this? If we don't own a specially handled "caller"/"arguments" property, how can we guarant7ee hasProperty doesn't have side effects? Why not just reify the property here before calling Base::put()?

[Caitlin Potter (:caitp)]

Comment on attachment 291109 [details]
also address nit in Executable.h

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

>> Source/JavaScriptCore/runtime/JSFunction.cpp:456
>> +        }
> 
> I know this is touching code that was here before, but I'm not sure why this is correct. Can you elaborate on this? If we don't own a specially handled "caller"/"arguments" property, how can we guarant7ee hasProperty doesn't have side effects? Why not just reify the property here before calling Base::put()?

I'm not sure what reifying here really entails (or what the `hasProperty` call accomplishes), because the `!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()` indicates that these are not virtual/lazy properties.

AFAIK all that's really needed is the Base::put() call, but I left the `hasProperty()` just in case there's something I'm missing. I think it's safe to remove it and not replace it with anything, though.

[Saam Barati]

(In reply to comment #67)
> Comment on attachment 291109 [details]
> also address nit in Executable.h
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291109&action=review
> 
> >> Source/JavaScriptCore/runtime/JSFunction.cpp:456
> >> +        }
> > 
> > I know this is touching code that was here before, but I'm not sure why this is correct. Can you elaborate on this? If we don't own a specially handled "caller"/"arguments" property, how can we guarant7ee hasProperty doesn't have side effects? Why not just reify the property here before calling Base::put()?
> 
> I'm not sure what reifying here really entails (or what the `hasProperty`
> call accomplishes), because the
> `!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()` indicates
> that these are not virtual/lazy properties.
> 
> AFAIK all that's really needed is the Base::put() call, but I left the
> `hasProperty()` just in case there's something I'm missing. I think it's
> safe to remove it and not replace it with anything, though.

Interesting. I'm never a big fan of having code in the tree where we don't know what it does. Let me try to figure out what's going on here. That said, I don't want this to hold up your patch since this was here before your patch. Can you just land a version of this patch with a FIXME linked to something saying "why is this code necessary" or something similar.

[Caitlin Potter (:caitp)]

Created attachment 291733 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 291109 [details]
also address nit in Executable.h

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

>>>> Source/JavaScriptCore/runtime/JSFunction.cpp:456
>>>> +        }
>>> 
>>> I know this is touching code that was here before, but I'm not sure why this is correct. Can you elaborate on this? If we don't own a specially handled "caller"/"arguments" property, how can we guarant7ee hasProperty doesn't have side effects? Why not just reify the property here before calling Base::put()?
>> 
>> I'm not sure what reifying here really entails (or what the `hasProperty` call accomplishes), because the `!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()` indicates that these are not virtual/lazy properties.
>> 
>> AFAIK all that's really needed is the Base::put() call, but I left the `hasProperty()` just in case there's something I'm missing. I think it's safe to remove it and not replace it with anything, though.
> 
> Interesting. I'm never a big fan of having code in the tree where we don't know what it does. Let me try to figure out what's going on here. That said, I don't want this to hold up your patch since this was here before your patch. Can you just land a version of this patch with a FIXME linked to something saying "why is this code necessary" or something similar.

sgtm

[Caitlin Potter (:caitp)]

I've updated the comment, but I guess I can't CQ. I'd prefer to CQ over dcommit, though.

[Keith Miller]

Comment on attachment 291733 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSFunction.cpp:453
> +            // FIXME: Investigate if the `hasProperty()` call is even needed, as in the `!hasCallerAndArgumentsProperties()` case,
> +            // these properties are not lazy and should not need to be reified.

I think we generally try to have FIXMEs point to bugzillas these days.

[Keith Miller]

(In reply to comment #71)
> I've updated the comment, but I guess I can't CQ. I'd prefer to CQ over
> dcommit, though.

You're a committer so the commit queue should take your patches. Maybe the patch can't have a review? though.

[Saam Barati]

(In reply to comment #71)
> I've updated the comment, but I guess I can't CQ. I'd prefer to CQ over
> dcommit, though.

Usually what I do when using the commit queue after somebody has already reviewed the patch:
1. I fill in the "Reviewed by ..." on each changelog
2. Upload to bugzilla and cq+

[Saam Barati]

(In reply to comment #72)
> Comment on attachment 291733 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291733&action=review
> 
> > Source/JavaScriptCore/runtime/JSFunction.cpp:453
> > +            // FIXME: Investigate if the `hasProperty()` call is even needed, as in the `!hasCallerAndArgumentsProperties()` case,
> > +            // these properties are not lazy and should not need to be reified.
> 
> I think we generally try to have FIXMEs point to bugzillas these days.

Agreed. Can we put a link next to this FIXME?

[Caitlin Potter (:caitp)]

Created attachment 291910 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 291733 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSFunction.cpp:453
>>> +            // these properties are not lazy and should not need to be reified.
>> 
>> I think we generally try to have FIXMEs point to bugzillas these days.
> 
> Agreed. Can we put a link next to this FIXME?

bug link added

[Saam Barati]

Comment on attachment 291910 [details]
Patch

Did you mean to set r? again or did you mean to set cq?

[Caitlin Potter (:caitp)]

just uploaded with webkit-patch and waited for the tree to get fixed up, since RenderThemeEfl.cpp was broken a while ago. I'll cq now

[WebKit Commit Bot]

Comment on attachment 291910 [details]
Patch

Clearing flags on attachment: 291910

Committed r207461: <http://trac.webkit.org/changeset/207461>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.