Bug 155568 – Method names should not appear in the lexical scope of the method's body.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Consider this scenario:

    var f = "foo";
    var result = ({
        f() {
            return f; // f should be the string "foo", not this method f.
        }
    }).f();
    result === "foo"; // Should be true.

The reason this is not current working is because the parser does not yet distinguish between FunctionExpressions and MethodDefinitions.  The ES6 spec explicitly distinguishes between the 2, and we should do the same.

[Radar WebKit Bug Importer]

<rdar://problem/25206150>

[Mark Lam]

Created attachment 274248 [details]
proposed patch.

[Mark Lam]

Created attachment 274249 [details]
x86_64 benchmark results.

[Mark Lam]

Perf is neutral with this patch.  The 2 "definitely slower"s are just due to noise.  They do not reproduce on re-runs.

[Saam Barati]

Comment on attachment 274248 [details]
proposed patch.

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

r=me with comments

> Source/JavaScriptCore/parser/ParserModes.h:45
> +enum FunctionMode { FunctionExpression, FunctionDeclaration, MethodDefinition };

If you feel inspired, it'd be nice to make this an enum class.

> Source/JavaScriptCore/tests/es6.yaml:808
> -  cmd: runES6 :fail
> +  cmd: runES6 :normal

Do we already have our own tests that cover this as well?
It'd be nice to have our own tests that cover more than what this covers.
I.e, object literal syntax, class syntax, and other variations.
get/set, etc.

[Mark Lam]

Thanks for the review.

(In reply to comment #5)
> > Source/JavaScriptCore/parser/ParserModes.h:45
> > +enum FunctionMode { FunctionExpression, FunctionDeclaration, MethodDefinition };
> 
> If you feel inspired, it'd be nice to make this an enum class.

Sounds good, but let's do this in a separate patch.  I'll follow up after this patch lands.

> > Source/JavaScriptCore/tests/es6.yaml:808
> > -  cmd: runES6 :fail
> > +  cmd: runES6 :normal
> 
> Do we already have our own tests that cover this as well?
> It'd be nice to have our own tests that cover more than what this covers.
> I.e, object literal syntax, class syntax, and other variations.
> get/set, etc.

I'll look into this now before landing.

[Mark Lam]

Created attachment 274265 [details]
patch for landing w/ Saam's feedback addressed.

In this updated patch, I added the tests to exercise all variants of methods as requested by Saam.

In the process, I also noticed a minor bug in the shouldBe() function in LayoutTests/js/script-tests/function-toString-vs-name.js.  I went ahead and fixed that as well.

[Mark Lam]

Created attachment 274266 [details]
patch for landing (take 2): with fixed LayoutTests/ChangeLog.

[Mark Lam]

Comment on attachment 274266 [details]
patch for landing (take 2): with fixed LayoutTests/ChangeLog.

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

> Source/JavaScriptCore/ChangeLog:23
> +        have a FunctionMode of MethodDefinition (instead of FunctionExpression).

typo: /have/to have/.

> Source/JavaScriptCore/ChangeLog:25
> +        should be in its scope or not.  It already returns false for all function's

typo: /all function's/any function/.

[Mark Lam]

Created attachment 274267 [details]
patch for landing (take 3).

[Mark Lam]

Landed in r198332: <http://trac.webkit.org/r198332>.