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

Fix several edge cases with context expression state #8972

Merged
merged 3 commits into from Nov 7, 2018

Conversation

danez
Copy link
Member

@danez danez commented Nov 5, 2018

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

I started fixing #8891 and ended in an endless loop of breaking things with the fixes and having to fix that.

I took a lot of fixes from acorn, and reported a bunch of issues to acorn too.

@danez danez added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser labels Nov 5, 2018
nicolo-ribaudo
nicolo-ribaudo previously approved these changes Nov 5, 2018
// because parseIdentifier will remove an item from the expression stack
// if function or class is parsed as identifier (in objects e.g.).
let meta = this.startNode();
this.next();
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment which explains that when this function is called we already know that there is a function token?

if (
(name === "class" || name === "function") &&
(this.state.lastTokEnd !== this.state.lastTokStart + 1 ||
this.input.charCodeAt(this.state.lastTokStart) !== 46)
Copy link
Member

Choose a reason for hiding this comment

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

Could you use the constants from charcodes here?

if (lineBreak.test(this.input.slice(this.state.end))) {
if (
lineBreak.test(
this.input.slice(this.state.end, this.lookahead(true).start),
Copy link
Member

Choose a reason for hiding this comment

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

Does this do a lookahead every time we find var foo? Is it possible to avoid it somehow? 😟

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this and instead used the same fix that acorn did by calling readRegex() in parseExprAtom()

@@ -1359,6 +1379,14 @@ export default class Tokenizer extends LocationParser {
return !this.state.exprAllowed;
}

inGeneratorContext() {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this.state.inGenerator work instead of this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes works. Acorn does have both things too, but with the difference that they also allow running the tokenizer without the parser, that's why they need both inGenerator and inGeneratorContext.

@nicolo-ribaudo nicolo-ribaudo dismissed their stale review November 5, 2018 23:29

I didnìt mean to approve 😅 Good work, though. I started fixing some context issues a while ago but gave up because I couldn't touch anything without breaking something.

@danez danez mentioned this pull request Nov 6, 2018
@babel-bot
Copy link
Collaborator

babel-bot commented Nov 6, 2018

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

@@ -2392,7 +2393,10 @@ export default (superClass: Class<Parser>): Class<Parser> =>
refNeedsArrowPos?: ?Pos,
): N.Expression {
let jsxError = null;
if (tt.jsxTagStart && this.match(tt.jsxTagStart)) {
if (
this.hasPlugin("jsx") &&
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to replace this, as tt.jsxTagStart is always set in our tests, even if jsx plugin not loaded. Yeahh monkeypatching :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that are the most hard to debug test failures 🤦‍♂️

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing error when JSX argument contains object with class field ES spec violation
3 participants