Our parser doesn't properly parse default parameter expressions in a class method

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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);
});

[Caio Lima]

Created attachment 280072 [details]
Patch

[Caio Lima]

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.

[Saam Barati]

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

[Saam Barati]

(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

[Caio Lima]

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! =)).

[Caio Lima]

Created attachment 280076 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[GSkachkov]

(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

[Build Bot]

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

[Build Bot]

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

[Caio Lima]

Created attachment 280110 [details]
Patch

[Caio Lima]

(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.

[Saam Barati]

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

[Caio Lima]

(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.

[Caio Lima]

Created attachment 280222 [details]
Patch

[Saam Barati]

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.

[Caio Lima]

Created attachment 280566 [details]
Patch

[Caio Lima]

Hi Saam. Do I need do something to get this patch applied to your code tree?

[Saam Barati]

(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.

[WebKit Commit Bot]

Comment on attachment 280566 [details]
Patch

Clearing flags on attachment: 280566

Committed r202074: <http://trac.webkit.org/changeset/202074>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.