Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Meriyah does not properly follow ECMA standards on function declaration and calling. #182

Closed
LumenUI opened this issue Feb 28, 2021 · 16 comments · Fixed by #184
Closed

Meriyah does not properly follow ECMA standards on function declaration and calling. #182

LumenUI opened this issue Feb 28, 2021 · 16 comments · Fixed by #184
Assignees

Comments

@LumenUI
Copy link

LumenUI commented Feb 28, 2021

When experimenting with Meriyah, I discovered the parser properly registers the following invalid function declarations and calls:

function 10() {
   return 10;
}

10();
function "string"() {
   return "string"();
}

"string"();

Meriyah also struggles when calling functions. It will assume that the name of the function being called is an identifier, and not a literal.

@3cp
Copy link
Member

3cp commented Feb 28, 2021

Probably missed check in

export function validateFunctionName(

@3cp 3cp self-assigned this Mar 2, 2021
@3cp
Copy link
Member

3cp commented Mar 4, 2021

@KFlash what's this line doing?

validateFunctionName(parser, context | ((context & 0b0000000000000000000_1100_00000000) << 11), parser.token);

It says (context & (Context.Strict | Context.Module)) << 11, the shifting 11 bits to the left which moves the strict and module context to InYieldContext and InAwaitContext. I have no idea why it does that.

@KFlash
Copy link
Contributor

KFlash commented Mar 4, 2021

validates func names and remove module and strict check

@3cp
Copy link
Member

3cp commented Mar 4, 2021

Why shift 11 bits?

@3cp
Copy link
Member

3cp commented Mar 4, 2021

I think I missed the leading context | ... but I still don't understand the logic.

@KFlash
Copy link
Contributor

KFlash commented Mar 4, 2021

then rewrite it. I guess that logic broke at some point where token.ts was updated with support for newer AST nodes

@3cp
Copy link
Member

3cp commented Mar 4, 2021

That logic was not the reason of this bug. I am just curious about the line which I still don't understand.

@KFlash
Copy link
Contributor

KFlash commented Mar 4, 2021

The logic is - I think - is that await and yield has a value around 21? Then you shift to left so you can unset this masks. Doing so will let you validate cases that should be valid even in async or yield context

@3cp
Copy link
Member

3cp commented Mar 5, 2021

There are lots of missed rejection on CallExpression, for example:

["a"]();
var a = {a:1}();
null();
true();
false();

It's bit hard to clear up all of them, which leads me to think maybe our Flags.DisallowCall is not well fitted for its purpose.

I fixed all the above, but surely there are more (mainly in parsePrimaryExpression).

@3cp
Copy link
Member

3cp commented Mar 5, 2021

@KFlash some interesting finding. In real JS parser (nodejs), null(), true(), false() are not parser error, but runtime error.

function a() { null(); }
a(); // error throws on this line, not previous line

I guess we don't need to reject the 3 in parsing.

@3cp
Copy link
Member

3cp commented Mar 5, 2021

My lord "a"() is runtime error too.

function a() { "a"() }

This parses with no issue in browser or nodejs.

@KFlash I guess we don't change the rejections of CallExpression at all?

@3cp
Copy link
Member

3cp commented Mar 5, 2021

1() is runtime error too.

@3cp
Copy link
Member

3cp commented Mar 5, 2021

Other parsers like acorn and TS do not reject them.

@3cp 3cp closed this as completed in #184 Mar 5, 2021
@LumenUI
Copy link
Author

LumenUI commented Mar 7, 2021

Apologies.

@3cp
Copy link
Member

3cp commented Mar 7, 2021

@LumenUI nothing to be sorry for. It was a valid point, and very subtle and surprising behavior, which clarified some edge cases.

In fact, meriyah's behavior is still not same as native JS parser. []() and ({}()) are parsing time error (means even when wrapped inside function) but meriyah did not pick them up. I will open another issue to clean up that.

@3cp
Copy link
Member

3cp commented Mar 8, 2021

NVM, my mistake, []() and ({}()) is runtime error too :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants