Skip to content

Oneline whiteblock parsing fixes #27

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

Merged
merged 1 commit into from
May 16, 2017
Merged

Oneline whiteblock parsing fixes #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/parser/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,18 @@ pp.isLineBreak = function () {
pp.canInsertSemicolon = function () {
return this.match(tt.eof) ||
this.match(tt.braceR) ||
this.isLineBreak();
this.isLineBreak() ||
(this.hasPlugin("lightscript") && (
// LSC oneline statement ASI cases
// Allow if x: throw y else: throw z
this.match(tt._else) ||
this.match(tt._elif) ||
// Allow (-> throw new Error)()
this.match(tt.parenR) ||
// Technically it is legal to insert a ; after a ;.
// Allows -> throw new Error; f()
this.state.tokens[this.state.tokens.length - 1].type === tt.semi
Copy link

Choose a reason for hiding this comment

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

hmm... this hints at something being wrong elsewhere...

Copy link
Author

Choose a reason for hiding this comment

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

The ; after ; thing is the result of going down a pretty deep rabbit hole. In JS, (x) => y; z is legal (compiles to a Body of two ExpressionStatements) and I wanted to maintain consistency.

So that would mean (x) => throw new Error; z should compile to a Body of two ExpressionStatements as well. (In Coffeescript, that actually compiles to one function with two statements in the body)

Ultimately the thing that was preventing that was the absence of this line of code here, so I added it, and it had the result I wanted without breaking JS, so I went with it. Not sure about better options.

Copy link

Choose a reason for hiding this comment

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

This really looks wrong to me. None of these things should be here; they belong elsewhere at a call site or something.

Copy link

Choose a reason for hiding this comment

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

I think this should be able to go away once you rebase on master, does that sound right? (guessing you just haven't gotten to that yet)

Copy link
Author

@wcjohnson wcjohnson May 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's not clear to me how that would happen. How is (-> throw new Error)() going to compile? It calls pp.semicolon() after parsing the throw, but there isn't a semicolon, EOL, or brace there...

Unless I misunderstand the ASI portion of the parser in some fundamental way, I believe this change is necessary.

Copy link

Choose a reason for hiding this comment

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

ah, gotcha. Wasn't clear to me earlier that that was the problem.

Do you know off the top of your head if it's just throw or if this problem exists across many desirably legal statements?

One possibility would be to modify parseThrowStatement to be "softer" than just this.semicolon().

Another would be to parse throw as an expression generally, which I think could make sense as a looser lightscript semantic (would just need to ensure it's sometimes wrapped in an iife in compiler).

The last thing on my mind here is that the cases you've added here do indeed make sense in this context and quite probably others – and I'm thinking about whether they should be enabled conditionally and at what layer of abstraction.

Just want to be thoughtful about this. I'm hopeful to be able to play around with it myself later today, and there's a good chance I'll end up in solid agreement that the current implementation is the best path forward.

));
};

// TODO
Expand Down
83 changes: 50 additions & 33 deletions src/plugins/lightscript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,40 +134,41 @@ pp.parseObjectComprehension = function(node) {
return this.finishNode(node, "ObjectComprehension");
};

// c/p parseBlock
pp.parseInlineWhiteBlock = function(node) {
if (this.state.type.startsExpr) return this.parseMaybeAssign();
// oneline statement case
node.body = [this.parseStatement(true)];
node.directives = [];
this.addExtra(node, "curly", false);
return this.finishNode(node, "BlockStatement");
};

pp.parseMultilineWhiteBlock = function(node, indentLevel) {
this.parseBlockBody(node, false, false, indentLevel);
if (!node.body.length) {
this.unexpected(node.start, "Expected an Indent or Statement");
}

this.addExtra(node, "curly", false);
return this.finishNode(node, "BlockStatement");
};

pp.parseWhiteBlock = function (allowDirectives?, isExpression?) {
pp.parseWhiteBlock = function (isExpression?) {
const node = this.startNode(), indentLevel = this.state.indentLevel;

// must start with colon or arrow
if (isExpression) {
this.expect(tt.colon);
if (!this.isLineBreak()) return this.parseMaybeAssign();
} else if (this.eat(tt.colon)) {
if (!this.isLineBreak()) return this.parseStatement(false);
} else if (this.eat(tt.arrow)) {
if (!this.isLineBreak()) {
if (this.match(tt.braceL)) {
// restart node at brace start instead of arrow start
const node = this.startNode();
this.next();
this.parseBlockBody(node, allowDirectives, false, tt.braceR);
this.addExtra(node, "curly", true);
return this.finishNode(node, "BlockStatement");
} else {
return this.parseMaybeAssign();
}
if (!this.eat(tt.colon)) this.unexpected(null, "Whitespace Block must start with a colon or arrow");

// Oneline whiteblock
if (!this.isLineBreak()) {
if (isExpression) {
return this.parseInlineWhiteBlock(node);
} else {
return this.parseStatement(false);
}
} else {
this.unexpected(null, "Whitespace Block must start with a colon or arrow");
}

// never parse directives if curly braces aren't used (TODO: document)
this.parseBlockBody(node, false, false, indentLevel);
this.addExtra(node, "curly", false);
if (!node.body.length) this.unexpected(node.start, "Expected an Indent or Statement");

return this.finishNode(node, "BlockStatement");
// TODO: document the fact that directives aren't parsed
return this.parseMultilineWhiteBlock(node, indentLevel);
};

pp.expectCommaOrLineBreak = function () {
Expand Down Expand Up @@ -301,7 +302,23 @@ pp.parseArrowFunctionBody = function (node) {
this.state.labels = [];
this.state.inFunction = true;

node.body = this.parseWhiteBlock(true);
const indentLevel = this.state.indentLevel;
const nodeAtArrow = this.startNode();
Copy link

Choose a reason for hiding this comment

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

ah, cool, thanks – that is clearer

this.expect(tt.arrow);
if (!this.isLineBreak()) {
if (this.match(tt.braceL)) {
// restart node at brace start instead of arrow start
node.body = this.startNode();
this.next();
this.parseBlockBody(node.body, true, false, tt.braceR);
this.addExtra(node.body, "curly", true);
node.body = this.finishNode(node.body, "BlockStatement");
} else {
node.body = this.parseInlineWhiteBlock(nodeAtArrow);
}
} else {
node.body = this.parseMultilineWhiteBlock(nodeAtArrow, indentLevel);
}

if (node.body.type !== "BlockStatement") {
node.expression = true;
Expand Down Expand Up @@ -376,7 +393,7 @@ pp.parseIf = function (node, isExpression) {
} else if (!isColon) {
node.consequent = this.parseMaybeAssign();
} else {
node.consequent = this.parseWhiteBlock(false, true);
node.consequent = this.parseWhiteBlock(true);
}
} else {
node.consequent = this.parseStatement(false);
Expand Down Expand Up @@ -425,7 +442,7 @@ pp.parseIfAlternate = function (node, isExpression, ifIsWhiteBlock, ifIndentLeve
} else if (!this.match(tt.colon)) {
return this.parseMaybeAssign();
} else {
return this.parseWhiteBlock(false, true);
return this.parseWhiteBlock(true);
}
}

Expand Down Expand Up @@ -613,9 +630,9 @@ export default function (instance) {
// whitespace following a colon

instance.extend("parseBlock", function (inner) {
return function (allowDirectives) {
return function () {
if (this.match(tt.colon)) {
return this.parseWhiteBlock(allowDirectives);
return this.parseWhiteBlock();
}
const block = inner.apply(this, arguments);
this.addExtra(block, "curly", true);
Expand Down
4 changes: 2 additions & 2 deletions src/tokenizer/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export const types = {
doubleColon: new TokenType("::", { beforeExpr }),
dot: new TokenType("."),
question: new TokenType("?", { beforeExpr }),
arrow: new TokenType("=>", { beforeExpr }),
arrow: new TokenType("=>", { beforeExpr, startsExpr }),
template: new TokenType("template"),
ellipsis: new TokenType("...", { beforeExpr }),
backQuote: new TokenType("`", { startsExpr }),
Expand Down Expand Up @@ -152,7 +152,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 }),
Copy link

Choose a reason for hiding this comment

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

This should be in a separate PR (seems like something that should go upstream? would first want to understand why it wasn't here before...)

Copy link

Choose a reason for hiding this comment

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

eh, this is fine in here. I think it makes sense

"extends": new KeywordTokenType("extends", { beforeExpr }),
"export": new KeywordTokenType("export"),
"import": new KeywordTokenType("import", { startsExpr }),
Expand Down
Loading