Bug 155568 - Method names should not appear in the lexical scope of the method's body.
Summary: Method names should not appear in the lexical scope of the method's body.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-16 16:52 PDT by Mark Lam
Modified: 2016-03-17 08:00 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (22.84 KB, patch)
2016-03-16 17:56 PDT, Mark Lam
saam: review+
Details | Formatted Diff | Diff
x86_64 benchmark results. (69.04 KB, text/plain)
2016-03-16 17:58 PDT, Mark Lam
no flags Details
patch for landing w/ Saam's feedback addressed. (33.56 KB, patch)
2016-03-16 23:19 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing (take 2): with fixed LayoutTests/ChangeLog. (33.29 KB, patch)
2016-03-16 23:22 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing (take 3). (33.29 KB, patch)
2016-03-16 23:27 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-03-16 16:52:07 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-03-16 16:53:01 PDT
<rdar://problem/25206150>
Comment 2 Mark Lam 2016-03-16 17:56:26 PDT
Created attachment 274248 [details]
proposed patch.
Comment 3 Mark Lam 2016-03-16 17:58:13 PDT
Created attachment 274249 [details]
x86_64 benchmark results.
Comment 4 Mark Lam 2016-03-16 17:59:09 PDT
Perf is neutral with this patch.  The 2 "definitely slower"s are just due to noise.  They do not reproduce on re-runs.
Comment 5 Saam Barati 2016-03-16 19:33:15 PDT
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.
Comment 6 Mark Lam 2016-03-16 20:35:18 PDT
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.
Comment 7 Mark Lam 2016-03-16 23:19:43 PDT
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.
Comment 8 Mark Lam 2016-03-16 23:22:19 PDT
Created attachment 274266 [details]
patch for landing (take 2): with fixed LayoutTests/ChangeLog.
Comment 9 Mark Lam 2016-03-16 23:25:51 PDT
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/.
Comment 10 Mark Lam 2016-03-16 23:27:20 PDT
Created attachment 274267 [details]
patch for landing (take 3).
Comment 11 Mark Lam 2016-03-17 08:00:02 PDT
Landed in r198332: <http://trac.webkit.org/r198332>.