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

Disallow escape sequences in contextual keywords #9618

Merged
merged 7 commits into from
Mar 6, 2019
Merged

Disallow escape sequences in contextual keywords #9618

merged 7 commits into from
Mar 6, 2019

Conversation

danez
Copy link
Member

@danez danez commented Feb 28, 2019

Q                       A
Fixed Issues?
Patch: Bug Fix? y
Major: Breaking Change? n
Minor: New Feature? n
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Resolves two problems:

  • Disallow (async)(a) => {}
  • Disallow escape sequences in contextual keywords like async, as, of, get, set, static, ...

The cases which were parsing successfully with escape sequences (although shouldn't) were: async in almost all cases. get & set in classes.

Also added tests for a lot of cases that were already correctly working.

@danez danez added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser labels Feb 28, 2019
@@ -704,11 +712,11 @@ export default class ExpressionParser extends LValParser {

atPossibleAsync(base: N.Expression): boolean {
return (
!this.state.containsEsc &&
Copy link
Member Author

@danez danez Feb 28, 2019

Choose a reason for hiding this comment

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

containsEsc only says if the current token has escape sequences. Here it already references the token after async which is not what we want, hence the slice check.

node.arguments = this.parseCallExpressionArguments(
tt.parenR,
possibleAsync,
);
Copy link
Member Author

@danez danez Feb 28, 2019

Choose a reason for hiding this comment

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

I removed this, as I don't see a what an OptionalCallExpression has anything todo with the async keyword. No tests failed or changed. I guess this was copied wrong.

@@ -525,12 +525,21 @@ export default class ExpressionParser extends LValParser {
startLoc: Position,
noCalls?: ?boolean,
): N.Expression {
const maybeAsyncArrow = this.atPossibleAsync(base);
Copy link
Member Author

@danez danez Feb 28, 2019

Choose a reason for hiding this comment

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

moving it from line 624 to here, basically prevents (async)(a) => {}.

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 1, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10376/

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10356/

if (possibleAsync && this.shouldParseAsyncArrow()) {
if (
maybeAsyncArrow &&
!this.canInsertSemicolon() &&
Copy link
Member Author

Choose a reason for hiding this comment

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

!this.canInsertSemicolon() prevents

async (x) 
=> {}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could move the check inside shouldParseAsyncArrow?
Also note that both flow and TS allow this code:

async (f)
: t => { }

but disallow

async (f) : t
=> { }

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I added testcases and moved it into shouldParseAsyncArrow

@danez danez added the PR: Needs Review A pull request awaiting more approvals label Mar 1, 2019
@nicolo-ribaudo nicolo-ribaudo self-requested a review March 5, 2019 05:44
@danez danez merged commit 2999900 into babel:master Mar 6, 2019
@danez danez deleted the invalid-escape branch March 6, 2019 01:20
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories PR: Needs Review A pull request awaiting more approvals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants