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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 34 additions & 1 deletion packages/babel-parser/src/parser/expression.js
Expand Up @@ -23,6 +23,7 @@ import * as N from "../types";
import LValParser from "./lval";
import { reservedWords } from "../util/identifier";
import type { Pos, Position } from "../util/location";
import * as charCodes from "charcodes";

export default class ExpressionParser extends LValParser {
// Forward-declaration: defined in statement.js
Expand Down Expand Up @@ -718,6 +719,10 @@ export default class ExpressionParser extends LValParser {
// or `{}`.

parseExprAtom(refShorthandDefaultPos?: ?Pos): N.Expression {
// If a division operator appears in an expression position, the
// tokenizer got confused, and we force it to read a regexp instead.
if (this.state.type === tt.slash) this.readRegexp();

const canBeArrow = this.state.potentialArrowAt === this.state.start;
let node;

Expand Down Expand Up @@ -964,7 +969,16 @@ export default class ExpressionParser extends LValParser {

parseFunctionExpression(): N.FunctionExpression | N.MetaProperty {
const node = this.startNode();
const meta = this.parseIdentifier(true);

// We do not do parseIdentifier here because when parseFunctionExpression
// is called we already know that the current token is a "name" with the value "function"
// This will improve perf a tiny little bit as we do not do validation but more importantly
// here is that parseIdentifier will remove an item from the expression stack
// if "function" or "class" is parsed as identifier (in objects e.g.), which should not happen here.
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?

meta = this.createIdentifier(meta, "function");

if (this.state.inGenerator && this.eat(tt.dot)) {
return this.parseMetaProperty(node, meta, "sent");
}
Expand Down Expand Up @@ -1863,8 +1877,14 @@ export default class ExpressionParser extends LValParser {
parseIdentifier(liberal?: boolean): N.Identifier {
const node = this.startNode();
const name = this.parseIdentifierName(node.start, liberal);

return this.createIdentifier(node, name);
}

createIdentifier(node: N.Identifier, name: string): N.Identifier {
node.name = name;
node.loc.identifierName = name;

return this.finishNode(node, "Identifier");
}

Expand All @@ -1884,6 +1904,19 @@ export default class ExpressionParser extends LValParser {
name = this.state.value;
} else if (this.state.type.keyword) {
name = this.state.type.keyword;

// `class` and `function` keywords push new context into this.context.
// But there is no chance to pop the context if the keyword is consumed
// as an identifier such as a property name.
// If the previous token is a dot, this does not apply because the
// context-managing code already ignored the keyword
if (
(name === "class" || name === "function") &&
(this.state.lastTokEnd !== this.state.lastTokStart + 1 ||
this.input.charCodeAt(this.state.lastTokStart) !== charCodes.dot)
) {
this.state.context.pop();
}
} else {
throw this.unexpected();
}
Expand Down
11 changes: 9 additions & 2 deletions packages/babel-parser/src/plugins/flow.js
Expand Up @@ -6,6 +6,7 @@ import * as N from "../types";
import type { Options } from "../options";
import type { Pos, Position } from "../util/location";
import type State from "../tokenizer/state";
import { types as tc } from "../tokenizer/context";
import * as charCodes from "charcodes";
import { isIteratorStart } from "../util/identifier";

Expand Down Expand Up @@ -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 🤦‍♂️

(this.match(tt.jsxTagStart) || this.isRelational("<"))
) {
const state = this.state.clone();
try {
return super.parseMaybeAssign(
Expand All @@ -2408,7 +2412,10 @@ export default (superClass: Class<Parser>): Class<Parser> =>
// Remove `tc.j_expr` and `tc.j_oTag` from context added
// by parsing `jsxTagStart` to stop the JSX plugin from
// messing with the tokens
this.state.context.length -= 2;
const cLength = this.state.context.length;
if (this.state.context[cLength - 1] === tc.j_oTag) {
this.state.context.length -= 2;
}

jsxError = err;
} else {
Expand Down
16 changes: 15 additions & 1 deletion packages/babel-parser/src/plugins/jsx/index.js
Expand Up @@ -506,6 +506,15 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return this.parseLiteral(this.state.value, "JSXText");
} else if (this.match(tt.jsxTagStart)) {
return this.jsxParseElement();
} else if (
this.isRelational("<") &&
this.state.input.charCodeAt(this.state.pos) !==
charCodes.exclamationMark
) {
// In case we encounter an lt token here it will always be the start of
// jsx as the lt sign is not allowed in places that expect an expression
this.finishToken(tt.jsxTagStart);
return this.jsxParseElement();
} else {
return super.parseExprAtom(refShortHandDefaultPos);
}
Expand Down Expand Up @@ -538,7 +547,12 @@ export default (superClass: Class<Parser>): Class<Parser> =>
}
}

if (code === charCodes.lessThan && this.state.exprAllowed) {
if (
code === charCodes.lessThan &&
this.state.exprAllowed &&
this.state.input.charCodeAt(this.state.pos + 1) !==
charCodes.exclamationMark
) {
++this.state.pos;
return this.finishToken(tt.jsxTagStart);
}
Expand Down
56 changes: 32 additions & 24 deletions packages/babel-parser/src/tokenizer/context.js
Expand Up @@ -12,7 +12,7 @@ export class TokContext {
token: string,
isExpr?: boolean,
preserveSpace?: boolean,
override?: Function, // Takes a Tokenizer as a this-parameter, and returns void.
override?: ?Function, // Takes a Tokenizer as a this-parameter, and returns void.
) {
this.token = token;
this.isExpr = !!isExpr;
Expand All @@ -31,11 +31,12 @@ export const types: {
} = {
braceStatement: new TokContext("{", false),
braceExpression: new TokContext("{", true),
templateQuasi: new TokContext("${", true),
templateQuasi: new TokContext("${", false),
parenStatement: new TokContext("(", false),
parenExpression: new TokContext("(", true),
template: new TokContext("`", true, true, p => p.readTmplToken()),
functionExpression: new TokContext("function", true),
functionStatement: new TokContext("function", false),
};

// Token-specific context update code
Expand All @@ -46,33 +47,26 @@ tt.parenR.updateContext = tt.braceR.updateContext = function() {
return;
}

const out = this.state.context.pop();
if (
out === types.braceStatement &&
this.curContext() === types.functionExpression
) {
this.state.context.pop();
this.state.exprAllowed = false;
} else if (out === types.templateQuasi) {
this.state.exprAllowed = true;
} else {
this.state.exprAllowed = !out.isExpr;
let out = this.state.context.pop();
if (out === types.braceStatement && this.curContext().token === "function") {
out = this.state.context.pop();
}

this.state.exprAllowed = !out.isExpr;
};

tt.name.updateContext = function(prevType) {
if (this.state.value === "of" && this.curContext() === types.parenStatement) {
this.state.exprAllowed = !prevType.beforeExpr;
return;
}

this.state.exprAllowed = false;

if (prevType === tt._let || prevType === tt._const || prevType === tt._var) {
if (lineBreak.test(this.input.slice(this.state.end))) {
this.state.exprAllowed = true;
let allowed = false;
if (prevType !== tt.dot) {
if (
(this.state.value === "of" && !this.state.exprAllowed) ||
(this.state.value === "yield" && this.state.inGenerator)
) {
allowed = true;
}
}
this.state.exprAllowed = allowed;

if (this.state.isIterator) {
this.state.isIterator = false;
}
Expand Down Expand Up @@ -107,8 +101,22 @@ tt.incDec.updateContext = function() {
};

tt._function.updateContext = tt._class.updateContext = function(prevType) {
if (this.state.exprAllowed && !this.braceIsBlock(prevType)) {
if (
prevType.beforeExpr &&
prevType !== tt.semi &&
prevType !== tt._else &&
!(
prevType === tt._return &&
lineBreak.test(this.input.slice(this.state.lastTokEnd, this.state.start))
) &&
!(
(prevType === tt.colon || prevType === tt.braceL) &&
this.curContext() === types.b_stat
)
) {
this.state.context.push(types.functionExpression);
} else {
this.state.context.push(types.functionStatement);
}

this.state.exprAllowed = false;
Expand Down
36 changes: 28 additions & 8 deletions packages/babel-parser/src/tokenizer/index.js
Expand Up @@ -1324,14 +1324,25 @@ export default class Tokenizer extends LocationParser {
}

braceIsBlock(prevType: TokenType): boolean {
if (prevType === tt.colon) {
const parent = this.curContext();
if (parent === ct.braceStatement || parent === ct.braceExpression) {
return !parent.isExpr;
}
const parent = this.curContext();
if (parent === ct.functionExpression || parent === ct.functionStatement) {
return true;
}
if (
prevType === tt.colon &&
(parent === ct.braceStatement || parent === ct.braceExpression)
) {
return !parent.isExpr;
}

if (prevType === tt._return) {
// The check for `tt.name && exprAllowed` detects whether we are
// after a `yield` or `of` construct. See the `updateContext` for
// `tt.name`.
if (
prevType === tt._return ||
prevType === tt._yield ||
(prevType === tt.name && this.state.exprAllowed)
) {
return lineBreak.test(
this.input.slice(this.state.lastTokEnd, this.state.start),
);
Expand All @@ -1341,13 +1352,22 @@ export default class Tokenizer extends LocationParser {
prevType === tt._else ||
prevType === tt.semi ||
prevType === tt.eof ||
prevType === tt.parenR
prevType === tt.parenR ||
prevType === tt.arrow
) {
return true;
}

if (prevType === tt.braceL) {
return this.curContext() === ct.braceStatement;
return parent === ct.braceStatement;
}

if (
prevType === tt._var ||
prevType === tt._let ||
prevType === tt._const
) {
return false;
}

if (prevType === tt.relational) {
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-parser/src/tokenizer/types.js
Expand Up @@ -174,7 +174,7 @@ export const keywords = {
new: new KeywordTokenType("new", { beforeExpr, startsExpr }),
this: new KeywordTokenType("this", { startsExpr }),
super: new KeywordTokenType("super", { startsExpr }),
class: new KeywordTokenType("class"),
class: new KeywordTokenType("class", { startsExpr }),
extends: new KeywordTokenType("extends", { beforeExpr }),
export: new KeywordTokenType("export"),
import: new KeywordTokenType("import", { startsExpr }),
Expand Down
@@ -0,0 +1 @@
for (const {a} of /b/) {}