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.

Sign up for GitHub

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

Jump to bottom

Trailing comma after rest - The final fix #10491

Merged
merged 6 commits into from Oct 8, 2019
Merged

Trailing comma after rest - The final fix #10491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
88e49b6
[parser] Track trailing commas in extras instead of state
nicolo-ribaudo Sep 24, 2019
c1f8ae6
Update existing tests
nicolo-ribaudo Sep 24, 2019
fac4f9f
Update test262 whitelist
nicolo-ribaudo Sep 24, 2019
375584e
Improve error message and location
nicolo-ribaudo Oct 3, 2019
0524164
nit
nicolo-ribaudo Oct 3, 2019
c7b0d4a
Use lookaheadCharCode
nicolo-ribaudo Oct 8, 2019
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
62 changes: 41 additions & 21 deletions packages/babel-parser/src/parser/expression.js
View file
Open in desktop
Expand Up @@ -168,9 +168,6 @@ export default class ExpressionParser extends LValParser {
}
}

const oldCommaAfterSpreadAt = this.state.commaAfterSpreadAt;
this.state.commaAfterSpreadAt = -1;

let failOnShorthandAssign;
if (refShorthandDefaultPos) {
failOnShorthandAssign = false;
Expand Down Expand Up @@ -230,18 +227,13 @@ export default class ExpressionParser extends LValParser {
);
}

if (patternErrorMsg) this.checkCommaAfterRestFromSpread();
this.state.commaAfterSpreadAt = oldCommaAfterSpreadAt;

this.next();
node.right = this.parseMaybeAssign(noIn);
return this.finishNode(node, "AssignmentExpression");
} else if (failOnShorthandAssign && refShorthandDefaultPos.start) {
this.unexpected(refShorthandDefaultPos.start);
}

this.state.commaAfterSpreadAt = oldCommaAfterSpreadAt;

return left;
}

Expand Down Expand Up @@ -680,14 +672,12 @@ export default class ExpressionParser extends LValParser {
let node = this.startNodeAt(startPos, startLoc);
node.callee = base;

const oldCommaAfterSpreadAt = this.state.commaAfterSpreadAt;
this.state.commaAfterSpreadAt = -1;

node.arguments = this.parseCallExpressionArguments(
tt.parenR,
state.maybeAsyncArrow,
base.type === "Import",
base.type !== "Super",
node,
);
if (!state.optionalChainMember) {
this.finishCallExpression(node);
Expand All @@ -698,8 +688,6 @@ export default class ExpressionParser extends LValParser {
if (state.maybeAsyncArrow && this.shouldParseAsyncArrow()) {
state.stop = true;

this.checkCommaAfterRestFromSpread();

node = this.parseAsyncArrowFromCallExpression(
this.startNodeAt(startPos, startLoc),
node,
Expand Down Expand Up @@ -743,7 +731,6 @@ export default class ExpressionParser extends LValParser {
}

this.state.maybeInArrowParameters = oldMaybeInArrowParameters;
this.state.commaAfterSpreadAt = oldCommaAfterSpreadAt;

return node;
} else if (this.match(tt.backQuote)) {
Expand Down Expand Up @@ -825,6 +812,7 @@ export default class ExpressionParser extends LValParser {
possibleAsyncArrow: boolean,
dynamicImport?: boolean,
allowPlaceholder?: boolean,
nodeForExtra?: ?N.Node,
): $ReadOnlyArray<?N.Expression> {
const elts = [];
let innerParenStart;
Expand All @@ -837,13 +825,21 @@ export default class ExpressionParser extends LValParser {
first = false;
} else {
this.expect(tt.comma);
if (this.eat(close)) {
if (this.match(close)) {
if (dynamicImport) {
this.raise(
this.state.lastTokStart,
"Trailing comma is disallowed inside import(...) arguments",
);
}
if (nodeForExtra) {
this.addExtra(
nodeForExtra,
"trailingComma",
this.state.lastTokStart,
);
}
this.next();
break;
}
}
Expand Down Expand Up @@ -883,7 +879,12 @@ export default class ExpressionParser extends LValParser {
call: N.CallExpression,
): N.ArrowFunctionExpression {
this.expect(tt.arrow);
this.parseArrowExpression(node, call.arguments, true);
this.parseArrowExpression(
node,
call.arguments,
true,
call.extra?.trailingComma,
);
return node;
}

Expand Down Expand Up @@ -1046,6 +1047,7 @@ export default class ExpressionParser extends LValParser {
tt.bracketR,
true,
refShorthandDefaultPos,
node,
);
if (!this.state.maybeInArrowParameters) {
// This could be an array pattern:
Expand Down Expand Up @@ -1289,7 +1291,7 @@ export default class ExpressionParser extends LValParser {
),
);

this.checkCommaAfterRest();
this.checkCommaAfterRest(charCodes.rightParenthesis);

break;
} else {
Expand Down Expand Up @@ -1504,7 +1506,11 @@ export default class ExpressionParser extends LValParser {
first = false;
} else {
this.expect(tt.comma);
if (this.eat(tt.braceR)) break;
if (this.match(tt.braceR)) {
this.addExtra(node, "trailingComma", this.state.lastTokStart);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we instead record the traillingCommaAttachedNodeStart? So we can always throw at the node.

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'd prefer to make trailingComma a boolean, and then throw at props[props.length - 1].start

this.next();
break;
}
}

const prop = this.parseObjectMember(isPattern, refShorthandDefaultPos);
Expand Down Expand Up @@ -1572,7 +1578,7 @@ export default class ExpressionParser extends LValParser {
this.next();
// Don't use parseRestBinding() as we only allow Identifier here.
prop.argument = this.parseIdentifier();
this.checkCommaAfterRest();
this.checkCommaAfterRest(charCodes.rightCurlyBrace);
return this.finishNode(prop, "RestElement");
}

Expand Down Expand Up @@ -1856,6 +1862,7 @@ export default class ExpressionParser extends LValParser {
node: N.ArrowFunctionExpression,
params: ?(N.Expression[]),
isAsync: boolean,
trailingCommaPos: ?number,
): N.ArrowFunctionExpression {
this.scope.enter(functionFlags(isAsync, false) | SCOPE_ARROW);
this.initFunction(node, isAsync);
Expand All @@ -1867,7 +1874,7 @@ export default class ExpressionParser extends LValParser {
this.state.yieldPos = -1;
this.state.awaitPos = -1;

if (params) this.setArrowFunctionParameters(node, params);
if (params) this.setArrowFunctionParameters(node, params, trailingCommaPos);
this.parseFunctionBody(node, true);

this.scope.exit();
Expand All @@ -1881,11 +1888,13 @@ export default class ExpressionParser extends LValParser {
setArrowFunctionParameters(
node: N.ArrowFunctionExpression,
params: N.Expression[],
trailingCommaPos: ?number,
): void {
node.params = this.toAssignableList(
params,
true,
"arrow function parameters",
trailingCommaPos,
);
}

Expand Down Expand Up @@ -2012,6 +2021,7 @@ export default class ExpressionParser extends LValParser {
close: TokenType,
allowEmpty?: boolean,
refShorthandDefaultPos?: ?Pos,
nodeForExtra?: ?N.Node,
): $ReadOnlyArray<?N.Expression> {
const elts = [];
let first = true;
Expand All @@ -2021,7 +2031,17 @@ export default class ExpressionParser extends LValParser {
first = false;
} else {
this.expect(tt.comma);
if (this.eat(close)) break;
if (this.match(close)) {
if (nodeForExtra) {
this.addExtra(
nodeForExtra,
"trailingComma",
this.state.lastTokStart,
);
}
this.next();
break;
}
}

elts.push(this.parseExprListItem(allowEmpty, refShorthandDefaultPos));
Expand Down
54 changes: 38 additions & 16 deletions packages/babel-parser/src/parser/lval.js
View file
Open in desktop
@@ -1,5 +1,6 @@
// @flow

import * as charCodes from "charcodes";
import { types as tt, type TokenType } from "../tokenizer/types";
import type {
TSParameterProperty,
Expand Down Expand Up @@ -60,6 +61,14 @@ export default class LValParser extends NodeUtils {
const prop = node.properties[i];
const isLast = i === last;
this.toAssignableObjectExpressionProp(prop, isBinding, isLast);

if (
isLast &&
prop.type === "RestElement" &&
node.extra?.trailingComma
) {
this.raiseRestNotLast(node.extra.trailingComma);
Copy link
Contributor

Choose a reason for hiding this comment

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

By raising error at comma position we are not really consistent with where this error is throw in other situation, i.e. in toAssignableObjectExpressionProp.

babel/packages/babel-parser/src/parser/lval.js

Lines 132 to 134 in 11fa246

} else if (prop.type === "SpreadElement" && !isLast) {
this.raiseRestNotLast(prop.start);
} else {

Hence we could throw at prop.start.

}
}
break;

Expand All @@ -78,7 +87,12 @@ export default class LValParser extends NodeUtils {

case "ArrayExpression":
node.type = "ArrayPattern";
this.toAssignableList(node.elements, isBinding, contextDescription);
this.toAssignableList(
node.elements,
isBinding,
contextDescription,
node.extra?.trailingComma,
);
break;

case "AssignmentExpression":
Expand Down Expand Up @@ -142,6 +156,7 @@ export default class LValParser extends NodeUtils {
exprList: Expression[],
isBinding: ?boolean,
contextDescription: string,
trailingCommaPos?: ?number,
): $ReadOnlyArray<Pattern> {
let end = exprList.length;
if (end) {
Expand All @@ -160,6 +175,11 @@ export default class LValParser extends NodeUtils {
) {
this.unexpected(arg.start);
}

if (trailingCommaPos) {
this.raiseTrailingCommaAfterRest(trailingCommaPos);
}

--end;
}
}
Expand Down Expand Up @@ -213,11 +233,6 @@ export default class LValParser extends NodeUtils {
undefined,
refNeedsArrowPos,
);

if (this.state.commaAfterSpreadAt === -1 && this.match(tt.comma)) {
this.state.commaAfterSpreadAt = this.state.start;
}

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

Expand All @@ -234,7 +249,11 @@ export default class LValParser extends NodeUtils {
case tt.bracketL: {
const node = this.startNode();
this.next();
node.elements = this.parseBindingList(tt.bracketR, true);
node.elements = this.parseBindingList(
tt.bracketR,
charCodes.rightSquareBracket,
true,
);
return this.finishNode(node, "ArrayPattern");
}

Expand All @@ -247,6 +266,7 @@ export default class LValParser extends NodeUtils {

parseBindingList(
close: TokenType,
closeCharCode: $Values<typeof charCodes>,
allowEmpty?: boolean,
allowModifiers?: boolean,
): $ReadOnlyArray<Pattern | TSParameterProperty> {
Expand All @@ -265,7 +285,7 @@ export default class LValParser extends NodeUtils {
break;
} else if (this.match(tt.ellipsis)) {
elts.push(this.parseAssignableListItemTypes(this.parseRestBinding()));
this.checkCommaAfterRest();
this.checkCommaAfterRest(closeCharCode);
this.expect(close);
break;
} else {
Expand Down Expand Up @@ -455,19 +475,21 @@ export default class LValParser extends NodeUtils {
}
}

checkCommaAfterRest(): void {
checkCommaAfterRest(close: $Values<typeof charCodes>): void {
if (this.match(tt.comma)) {
this.raiseRestNotLast(this.state.start);
}
}

checkCommaAfterRestFromSpread(): void {
if (this.state.commaAfterSpreadAt > -1) {
this.raiseRestNotLast(this.state.commaAfterSpreadAt);
if (this.lookaheadCharCode() === close) {
this.raiseTrailingCommaAfterRest(this.state.start);
} else {
this.raiseRestNotLast(this.state.start);
}
}
}

raiseRestNotLast(pos: number) {
this.raise(pos, `Rest element must be last element`);
}

raiseTrailingCommaAfterRest(pos: number) {
this.raise(pos, `Unexpected trailing comma after rest element`);
}
}
1 change: 1 addition & 0 deletions packages/babel-parser/src/parser/statement.js
View file
Open in desktop
Expand Up @@ -1105,6 +1105,7 @@ export default class StatementParser extends ExpressionParser {
this.expect(tt.parenL);
node.params = this.parseBindingList(
tt.parenR,
charCodes.rightParenthesis,
/* allowEmpty */ false,
allowModifiers,
);
Expand Down
11 changes: 10 additions & 1 deletion packages/babel-parser/src/plugins/flow.js
View file
Open in desktop
Expand Up @@ -1798,6 +1798,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
((node.params: any): N.Expression[]),
true,
"arrow function parameters",
node.extra?.trailingComma,
);
// Enter scope, as checkParams defines bindings
this.scope.enter(functionFlags(false, false) | SCOPE_ARROW);
Expand All @@ -1820,6 +1821,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
((node.params: any): N.Expression[]),
true,
"arrow function parameters",
node.extra?.trailingComma,
);
}
return [arrows, []];
Expand All @@ -1831,6 +1833,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
((node.params: any): N.Expression[]),
true,
"arrow function parameters",
node.extra?.trailingComma,
);
return true;
} catch (err) {
Expand Down Expand Up @@ -2005,14 +2008,20 @@ export default (superClass: Class<Parser>): Class<Parser> =>
exprList: N.Expression[],
isBinding: ?boolean,
contextDescription: string,
trailingCommaPos?: ?number,
): $ReadOnlyArray<N.Pattern> {
for (let i = 0; i < exprList.length; i++) {
const expr = exprList[i];
if (expr && expr.type === "TypeCastExpression") {
exprList[i] = this.typeCastToParameter(expr);
}
}
return super.toAssignableList(exprList, isBinding, contextDescription);
return super.toAssignableList(
exprList,
isBinding,
contextDescription,
trailingCommaPos,
);
}

// this is a list of nodes, from something like a call expression, we need to filter the
Expand Down