Bug 144281 - keywords ("super", "delete", etc) should be valid method names
Summary: keywords ("super", "delete", etc) should be valid method names
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
: 153296 (view as bug list)
Depends on:
Blocks: 140491
  Show dependency treegraph
 
Reported: 2015-04-27 15:13 PDT by Erik Arvidsson
Modified: 2016-01-20 21:32 PST (History)
9 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (18.74 KB, patch)
2016-01-11 14:35 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (18.88 KB, patch)
2016-01-11 17:42 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2015-04-27 15:13:13 PDT
http://trac.webkit.org/browser/trunk/LayoutTests/js/script-tests/class-syntax-super.js#L40

shouldThrow('x = class extends Base { constructor() { super(); } super() {} }', '"SyntaxError: \'super\' keyword unexpected here"');

If you reformat this:

x = class extends Base {
  constructor() {
    super();
  }
  super() {
  }
}

It should be clear that all this does is define a method named super. There is on restriction on method names like that. Method names can be any keyword.
Comment 1 Ryosuke Niwa 2015-04-27 15:28:50 PDT

*** This bug has been marked as a duplicate of bug 144282 ***
Comment 2 Ryosuke Niwa 2015-05-01 18:48:44 PDT
Oh wait, this is a separate bug.
Comment 3 Joseph Pecoraro 2016-01-11 11:40:10 PST
I'll take this. I have a simple fix and we ran into this in the inspector recently.
Comment 4 Joseph Pecoraro 2016-01-11 14:35:25 PST
Created attachment 268714 [details]
[PATCH] Proposed Fix

This catches another bug (bug 152985) and fixes a few issues with keyword method names.
Comment 5 Ryosuke Niwa 2016-01-11 17:24:29 PST
Comment on attachment 268714 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/parser/Parser.cpp:2158
> +                restoreSavePoint(savePoint);
> +                isStaticMethod = false;

Instead of setting a boolean flag back, can we move the condition above into the if condition,
initialize isStaticMethod to false, and then set it true when we don't match open paren?

That would make this code easier to read without the comment.

> Source/JavaScriptCore/parser/Parser.cpp:2184
> +            if (!isGenerator && (match(IDENT) || match(STRING) || match(DOUBLE) || match(INTEGER) || match(OPENBRACKET) || m_token.m_type & KeywordTokenFlag)) {

Why don't we use isIdentifierOrKeyword instead?
Comment 6 Joseph Pecoraro 2016-01-11 17:41:10 PST
Comment on attachment 268714 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/parser/Parser.cpp:2184
>> +            if (!isGenerator && (match(IDENT) || match(STRING) || match(DOUBLE) || match(INTEGER) || match(OPENBRACKET) || m_token.m_type & KeywordTokenFlag)) {
> 
> Why don't we use isIdentifierOrKeyword instead?

Awesome! I didn't see this existed. Here we can use matchIdentifierOrKeyword()!
Comment 7 Joseph Pecoraro 2016-01-11 17:42:35 PST
Created attachment 268734 [details]
[PATCH] Proposed Fix
Comment 8 Ryosuke Niwa 2016-01-11 18:13:32 PST
Comment on attachment 268734 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/parser/Parser.cpp:2203
> +            if (m_token.m_type & KeywordTokenFlag)
> +                goto namedKeyword;

It's annoying that we have to use goto here but I can't think of a better way.
Comment 9 Joseph Pecoraro 2016-01-11 18:57:59 PST
Comment on attachment 268734 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/parser/Parser.cpp:2203
>> +                goto namedKeyword;
> 
> It's annoying that we have to use goto here but I can't think of a better way.

Yeah. `goto` matched the parseProperty handling of keywords in object literals, so I just matched that and used a goto here. We could convert to a bunch of if/else statements, but I'm not sure that would be any clearer.
Comment 10 WebKit Commit Bot 2016-01-11 19:02:23 PST
Comment on attachment 268734 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 268734

Committed r194881: <http://trac.webkit.org/changeset/194881>
Comment 11 WebKit Commit Bot 2016-01-11 19:02:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Devin Rousso 2016-01-20 21:32:14 PST
*** Bug 153296 has been marked as a duplicate of this bug. ***