Skip to content

Fail speculative parsing of arrow functions when their parameter initialisers are missing a = #18417

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

Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Sep 12, 2017

Fail speculative parsing of arrow function expressions whenever it has a parameter with an initialiser that is missing '='. Ordinarily this is allowed for better error recovery in the language service, but for speculative parsing, the errors can compound. When the initialiser is an error, and when the '=>' is missing (which is also allowed), what is putatively an arrow function may actually be something else.

For example, (a / 8) + function () { } is currently parsed as if someone had intended to write

(a = /8)+function()/) => { } but they forgot the = of the
initialiser, the => of the lambda, forgot to close the regular
expression, and mistakenly inserted a newline right after the regular
expression.

I need to test whether this causes problems when using the language service, but I do not think that it will.

Fixes #18158
Fixes #18406
Fixes #18306

Fail speculative parsing of arrow function expressions whenever it has a
parameter with an initialiser that is missing '='. Ordinarily this is
allowed for better error recovery in the language service, but for
speculative parsing, the errors can compound. When the initialiser is an
error, and when the '=>' is missing (which is also allowed), what is
putatively an arrow function may actually be something else.

For example, `(a / 8) + function ()
{ }` is currently parsed as if someone had intended to write

`(a = /8)+function()/) => { }` but they forgot the `=` of the
initialiser, the `=>` of the lambda, forgot to close the regular
expression, and mistakenly inserted a newline right after the regular
expression.
Makes another test case pass that was taking exponential time to parse,
because now it notices that the = is not present and doesn't even try to
parse the initialiser expression.
@sandersn sandersn changed the title Fail spec parsing lambdas on parameter missing a = Fail speculative parsing of arrow functions when their parameter initialisers are missing a = Sep 12, 2017
@sandersn
Copy link
Member Author

The editor experience seems OK to me, but admittedly it's hard to judge when trying it with preconceptions.

if (inParameter && requireEqualsToken) {
// = is required when speculatively parsing arrow function parameters,
// so return a fake initializer as a signal that the equals token was missing
const result = createNode(SyntaxKind.Identifier, scanner.getStartPos()) as Identifier;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use createMissingNode, just in case, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case the node slips through into the real parse, right? That incurs some overhead, and I was actually thinking of going the other way; just creating a sentinel Identifier node with no content and just use === to identify it later.

Hmmm...I don't know, I'll try the missing node I guess. Better safe than sorry, and the overhead of this parsing pales next to the rest of speculative arrow parsing, which happens every time we see a parenthesis.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 14, 2017

We will need to port this to release-2.5 as well.

@sandersn sandersn merged commit b934c8b into master Sep 14, 2017
@sandersn sandersn deleted the fail-spec-lambda-parsing-on-parameter-initialiser-missing-= branch September 14, 2017 20:06
@sandersn
Copy link
Member Author

It is now in release-2.5 too.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants