Bug 157872 - Our parser doesn't properly parse default parameter expressions in a class method
Summary: Our parser doesn't properly parse default parameter expressions in a class me...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-18 16:29 PDT by Saam Barati
Modified: 2016-06-14 17:43 PDT (History)
14 users (show)

See Also:


Attachments
Patch (1.92 KB, patch)
2016-05-29 22:43 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (5.56 KB, patch)
2016-05-30 00:51 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (977.07 KB, application/zip)
2016-05-30 01:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (690.01 KB, application/zip)
2016-05-30 01:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.44 MB, application/zip)
2016-05-30 01:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (961.43 KB, application/zip)
2016-05-30 03:27 PDT, Build Bot
no flags Details
Patch (7.62 KB, patch)
2016-05-30 14:05 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (10.04 KB, patch)
2016-06-01 01:23 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (10.06 KB, patch)
2016-06-05 16:48 PDT, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-05-18 16:29:00 PDT
This is a syntax error:
it shouldn't be:

test(function() {
    class C {
        constructor() { this._x = 45; }
        get foo() { return this._x;}
    }
    class D extends C {
        x(x = ()=>super.foo) {
            return x();
        }
    }
    assert((new D).x() === 45);
});
Comment 1 Caio Lima 2016-05-29 22:43:22 PDT
Created attachment 280072 [details]
Patch
Comment 2 Caio Lima 2016-05-29 23:08:02 PDT
Hello guys, It is a RFC.

I created a test case based on the Saam's sample:

class C {
  constructor() { this._x = 45; }
  get foo() { return this._x;}
}

class D extends C {

  x(y = () => super.foo) {
    return y();
  }
}

if ((new D).x() === 45) {
  print("passed");
} else {
  print("failed");
}

I tested it in V8 and SpiderMonkey and they parsed and executed correctly. So, I decided to investigate the problem.

I tested the following workaround to find the reason of failing

class D extends C {

  x(y) {
    y ||= () => super.foo;
    return y();
  }
}

And it worked. I noticed that the failing point was the following test present in Parser.cpp::parseMemberExpression():

if (!m_lexer->isReparsingFunction()) {
                SuperBinding functionSuperBinding = !functionScope->isArrowFunction() && !closestOrdinaryFunctionScope->isEvalContext()
                    ? functionScope->expectedSuperBinding()
                    : closestOrdinaryFunctionScope->expectedSuperBinding();
                semanticFailIfTrue(functionSuperBinding == SuperBinding::NotNeeded, "super is not valid in this context");
            }

In the first test case, the "closestOrdinaryFunctionScope->expectedSuperBinding()" returned "SuperBinding::NotNeeded" and in the second case the return was "SuperBinding::Needed". I thought it was strange, since the "closestOrdinaryFunctionScope" in both cases should be in the same configuration. I debugged the parsing phase and noticed that "closestOrdinaryFunctionScope->m_expectedSuperBinding" was not being set before "parseFunctionParameters" in "Parser<LexerType>::parseFunctionInfo" (Parser.cpp).

In my patch proposal, I am setting "functionScope->setExpectedSuperBinding(expectedSuperBinding)" before call "parseFunctionParameters" in Parser.cpp:2019 as a proof of concept, however, I think it should be placed in the first 13 lines of the "Parser<LexerType>::parseFunctionInfo" function, since there are others "parseFunctionParameters" above the current modification. What do you think?

Also, I would like to know where I should can create a test case for this case.
Comment 3 Saam Barati 2016-05-29 23:08:10 PDT
Comment on attachment 280072 [details]
Patch

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

Looks good to me, but this patch needs tests before we can land it
I'd add parser tests. We have a file that is for these tests:
LayoutTests/js/parser-syntax-... (I forget the actual file name)
Also, please add some stress tests. I believe I added a fixme in a test
with this bug number. I think if you grep for this bug number you will
find it.

> Source/JavaScriptCore/parser/Parser.cpp:2074
> +        functionScope->setExpectedSuperBinding(expectedSuperBinding);

Can't this just be moved up to when we create the function scope?
Then the other place where we set this below can be deleted
Comment 4 Saam Barati 2016-05-29 23:10:32 PDT
(In reply to comment #2)
> Hello guys, It is a RFC.
> 
> I created a test case based on the Saam's sample:
> 
> class C {
>   constructor() { this._x = 45; }
>   get foo() { return this._x;}
> }
> 
> class D extends C {
> 
>   x(y = () => super.foo) {
>     return y();
>   }
> }
> 
> if ((new D).x() === 45) {
>   print("passed");
> } else {
>   print("failed");
> }
> 
> I tested it in V8 and SpiderMonkey and they parsed and executed correctly.
> So, I decided to investigate the problem.
> 
> I tested the following workaround to find the reason of failing
> 
> class D extends C {
> 
>   x(y) {
>     y ||= () => super.foo;
>     return y();
>   }
> }
> 
> And it worked. I noticed that the failing point was the following test
> present in Parser.cpp::parseMemberExpression():
> 
> if (!m_lexer->isReparsingFunction()) {
>                 SuperBinding functionSuperBinding =
> !functionScope->isArrowFunction() &&
> !closestOrdinaryFunctionScope->isEvalContext()
>                     ? functionScope->expectedSuperBinding()
>                     : closestOrdinaryFunctionScope->expectedSuperBinding();
>                 semanticFailIfTrue(functionSuperBinding ==
> SuperBinding::NotNeeded, "super is not valid in this context");
>             }
> 
> In the first test case, the
> "closestOrdinaryFunctionScope->expectedSuperBinding()" returned
> "SuperBinding::NotNeeded" and in the second case the return was
> "SuperBinding::Needed". I thought it was strange, since the
> "closestOrdinaryFunctionScope" in both cases should be in the same
> configuration. I debugged the parsing phase and noticed that
> "closestOrdinaryFunctionScope->m_expectedSuperBinding" was not being set
> before "parseFunctionParameters" in "Parser<LexerType>::parseFunctionInfo"
> (Parser.cpp).
> 
> In my patch proposal, I am setting
> "functionScope->setExpectedSuperBinding(expectedSuperBinding)" before call
> "parseFunctionParameters" in Parser.cpp:2019 as a proof of concept, however,
> I think it should be placed in the first 13 lines of the
> "Parser<LexerType>::parseFunctionInfo" function, since there are others
> "parseFunctionParameters" above the current modification. What do you think?
> 
> Also, I would like to know where I should can create a test case for this
> case.
I saw this after I posted my comment, but I believe my above comment answers your questions. The stress test I was speaking about it inside
...Source/JavaScriptCore/tests/stress/...  I don't remember the exact file but there is a FIXME with this bug number
Comment 5 Caio Lima 2016-05-29 23:39:11 PDT
Hi Saam.
Is there any way to run an specific stress test suite? I executed run-javascriptcore-tests 2 hours ago and it is still executing. I would like to run the tests that I changed to check if they are correct.

The test files are:
LayoutTests/js/script-tests/parser-syntax-check.js
Source/JavaScriptCore/tests/stress/arrow-functions-as-default-parameter-values.js

Thank you for the fast answer (faster than my comment! =)).
Comment 6 Caio Lima 2016-05-30 00:51:45 PDT
Created attachment 280076 [details]
Patch
Comment 7 Build Bot 2016-05-30 01:41:09 PDT
Comment on attachment 280076 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
Comment 8 Build Bot 2016-05-30 01:41:12 PDT
Created attachment 280083 [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
Comment 9 Build Bot 2016-05-30 01:46:53 PDT
Comment on attachment 280076 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
Comment 10 Build Bot 2016-05-30 01:46:56 PDT
Created attachment 280084 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 11 Build Bot 2016-05-30 01:49:32 PDT
Comment on attachment 280076 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
Comment 12 Build Bot 2016-05-30 01:49:36 PDT
Created attachment 280085 [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
Comment 13 GSkachkov 2016-05-30 03:21:27 PDT
(In reply to comment #5)
> Hi Saam.
> Is there any way to run an specific stress test suite? I executed
> run-javascriptcore-tests 2 hours ago and it is still executing. I would like
> to run the tests that I changed to check if they are correct.
> 
> The test files are:
> LayoutTests/js/script-tests/parser-syntax-check.js
> Source/JavaScriptCore/tests/stress/arrow-functions-as-default-parameter-
> values.js
> 
> Thank you for the fast answer (faster than my comment! =)).

Cool!

There is parameter --filter where you can add regex condition to filter tests.
I'm using following command:

Tools/Scripts/run-javascriptcore-tests  --no-testapi --jsc-stress --filter='parser-syntax-check'  

Also for layout tests you need to modify file with expected result 
In current patch is LayoutTests/js/parser-syntax-check-expected.txt
Comment 14 Build Bot 2016-05-30 03:27:17 PDT
Comment on attachment 280076 [details]
Patch

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

New failing tests:
js/parser-syntax-check.html
Comment 15 Build Bot 2016-05-30 03:27:21 PDT
Created attachment 280088 [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
Comment 16 Caio Lima 2016-05-30 14:05:06 PDT
Created attachment 280110 [details]
Patch
Comment 17 Caio Lima 2016-05-30 15:19:30 PDT
(In reply to comment #13)
> (In reply to comment #5)
> > Hi Saam.
> > Is there any way to run an specific stress test suite? I executed
> > run-javascriptcore-tests 2 hours ago and it is still executing. I would like
> > to run the tests that I changed to check if they are correct.
> > 
> > The test files are:
> > LayoutTests/js/script-tests/parser-syntax-check.js
> > Source/JavaScriptCore/tests/stress/arrow-functions-as-default-parameter-
> > values.js
> > 
> > Thank you for the fast answer (faster than my comment! =)).
> 
> Cool!
> 
> There is parameter --filter where you can add regex condition to filter
> tests.
> I'm using following command:
> 
> Tools/Scripts/run-javascriptcore-tests  --no-testapi --jsc-stress
> --filter='parser-syntax-check'  
> 
> Also for layout tests you need to modify file with expected result 
> In current patch is LayoutTests/js/parser-syntax-check-expected.txt

Thanks GSkachkov! It worked well.
Comment 18 Saam Barati 2016-05-31 12:17:31 PDT
Comment on attachment 280110 [details]
Patch

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

r=me wth comments

> Source/JavaScriptCore/parser/Parser.cpp:1929
> +    functionScope->setExpectedSuperBinding(expectedSuperBinding);

Can you remove the duplicate call below to setExpectedSuperKind(...)?

Also, I wonder if we should be calling setConstructKind up here too. I need to think about it more.

> Source/JavaScriptCore/tests/stress/arrow-functions-as-default-parameter-values.js:162
> +    class D extends C {
> +        x(y = (y = () => super.foo) => {return y()}) {
> +            return y();
> +        }
> +    }

Can you also add a test where we use super inside an arrow function inside a derived constructor's default parameter expression?

> LayoutTests/js/script-tests/parser-syntax-check.js:746
> +valid("class C { constructor() { this._x = 45; } get foo() { return this._x;} } class D extends C { x(y = (y = () => super.foo) => {return y()}) { return y(); } }")

Ditto for a constructor's syntax here
Comment 19 Caio Lima 2016-06-01 00:38:20 PDT
(In reply to comment #18)
> Comment on attachment 280110 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280110&action=review
> 
> r=me wth comments
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1929
> > +    functionScope->setExpectedSuperBinding(expectedSuperBinding);
> 
> Can you remove the duplicate call below to setExpectedSuperKind(...)?

Sure. Actually I can just remove the call on line :1951 (inside loadCachedFunction), since the "expectedSuperBinding" can change "if (m_defaultConstructorKind != ConstructorKind::None)".

> Also, I wonder if we should be calling setConstructKind up here too. I need
> to think about it more.

Actually, I think so. I tested the following sample and got SyntaxError:

class C {
    constructor() {
        this._x = 45;
    }

    get foo() {
        return this._x;
    }
}

class D extends C {

    constructor(x = () => super()) {
        x();
    }

    x() {
        return super.foo; 
    }
}

if ((new D).x() === 45) {
    print("passed");
} else {
    print("failed");
}

The result in v8 and Spider monkey was correct. I set it in the beginning of function and things worked properly.

> > Source/JavaScriptCore/tests/stress/arrow-functions-as-default-parameter-values.js:162
> > +    class D extends C {
> > +        x(y = (y = () => super.foo) => {return y()}) {
> > +            return y();
> > +        }
> > +    }
> 
> Can you also add a test where we use super inside an arrow function inside a
> derived constructor's default parameter expression?

> > LayoutTests/js/script-tests/parser-syntax-check.js:746
> > +valid("class C { constructor() { this._x = 45; } get foo() { return this._x;} } class D extends C { x(y = (y = () => super.foo) => {return y()}) { return y(); } }")
> 
> Ditto for a constructor's syntax here

No problem.
Comment 20 Caio Lima 2016-06-01 01:23:09 PDT
Created attachment 280222 [details]
Patch
Comment 21 Saam Barati 2016-06-05 16:04:02 PDT
Comment on attachment 280222 [details]
Patch

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

r=me

> Source/JavaScriptCore/tests/stress/arrow-functions-as-default-parameter-values.js:200
> +    class D extends C {
> +
> +      constructor(x = () => super()) {
> +        x();
> +      }
> +
> +      x() {
> +        return super.foo; 
> +      }
> +    }

Style Nit: This should be 4 space indent.
Comment 22 Caio Lima 2016-06-05 16:48:47 PDT
Created attachment 280566 [details]
Patch
Comment 23 Caio Lima 2016-06-14 15:08:46 PDT
Hi Saam. Do I need do something to get this patch applied to your code tree?
Comment 24 Saam Barati 2016-06-14 17:40:44 PDT
(In reply to comment #23)
> Hi Saam. Do I need do something to get this patch applied to your code tree?

Usually the person posting the patch (you in this case)
will see cq? to indicate that it needs to be added to the
commit queue. I'm going to set cq+ now.
Comment 25 WebKit Commit Bot 2016-06-14 17:43:48 PDT
Comment on attachment 280566 [details]
Patch

Clearing flags on attachment: 280566

Committed r202074: <http://trac.webkit.org/changeset/202074>
Comment 26 WebKit Commit Bot 2016-06-14 17:43:54 PDT
All reviewed patches have been landed.  Closing bug.