Allow empty arrow bodies by wcjohnson · Pull Request #9 · lightscript/babylon-lightscript

wcjohnson · 2017-04-06T19:37:06Z

rattrayalex

I think this looks good... how do you feel about it?

rattrayalex · 2017-04-06T19:40:24Z

  this.addExtra(node, "curly", false);
-  if (!node.body.length) this.unexpected(node.start, "Expected an Indent or Statement");
+  if (!isArrowBody && !node.body.length)
+    this.unexpected(node.start, "Expected an Indent or Statement");


nit: same line or curly please (surprised this isn't in the eslint rules... hmm)

rattrayalex · 2017-04-06T19:42:10Z

 // c/p parseBlock

-pp.parseWhiteBlock = function (allowDirectives?, isIfExpression?) {
+pp.parseWhiteBlock = function (allowDirectives?, isIfExpression?, isArrowBody?) {


I don't love adding an extra param here, but I can't think of a cleaner way...

I think it's necessary due to the way this is used throughout the code. I first tried removing the empty block check entirely, but then the parser allows a lot of malformed if: constructs that it shouldn't. So we need to know if we're in an arrow body or not.

Yeah. Maybe allowEmptyBody as the param name instead? not really impt

rattrayalex · 2017-04-06T19:44:09Z

+        "body": {
+          "type": "BlockStatement",
+          "start": 5,
+          "end": 7,


hmm, odd that the blockstatement has this positioning... looks like it's capturing the arrow... is that happening for other arrow function blocks?

@rattrayalex Yes:

https://github.com/wcjohnson/babylon-lightscript/blob/a27269694673bbe26aed026fcdbc23ecc64ca94f/test/fixtures/lightscript/named-arrow-functions/skinny-white/expected.json

parseWhiteBlock is calling startNode before eat(tt.Arrow), so the arrow's position is mapped into the resulting whiteblock node.

Not sure if that is the intended behavior or not, ultimately, but changing it should probably be out of scope for this PR.

Makes sense, thanks for checking. Fine to leave for now.

rattrayalex · 2017-04-06T20:03:56Z

Cool, I think this looks good... if you're down to change the param name, I'll merge once you've done that + squashed!

3 squashed commits: - Allow empty arrow bodies - Delint - Change param name

wcjohnson · 2017-04-06T20:35:48Z

Squashed

rattrayalex · 2017-04-06T20:51:45Z

 // c/p parseBlock

-pp.parseWhiteBlock = function (allowDirectives?, isIfExpression?) {
+pp.parseWhiteBlock = function (allowDirectives?, isIfExpression?, allowEmptyBody?) {


sorry I didn't catch this before, but instead of doing the param, we can detect by whether there's an arrow in the code itself:

let allowEmptyBody; // must start with colon or arrow if (isIfExpression) { 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)) { allowEmptyBody = true; if (!this.isLineBreak()) {

Thoughts?

rattrayalex · 2017-04-06T20:53:21Z

+    "sourceType": "script",
+    "body": [
+      {
+        "type": "NamedArrowDeclaration",


Sorry I didn't think of this before, but we should probably have a test for non-named arrows, eg:

=>

perhaps plus the slightly more sane

f((x) =>)

These don't work right, basically because parseWhiteBlock calls parseMaybeAssign here, which expects a non empty expression. I intend to fix this, but odds are you won't like it much.

wcjohnson#4

@rattrayalex

Actually not as bad as I thought:
wcjohnson@997428e

Let me know if you want this plucked

Added some comments there, I think... not sure if they went through

@rattrayalex Saw your comments. Plucked the changeset to this branch, will test and fix here.

wcjohnson · 2017-04-07T17:12:50Z

Incidentally, I have compiler fixtures for all this stuff (all passing) at wcjohnson/babel-plugin-lightscript@1d464e9

- Update `parseWhiteBlock` to only call `parseMaybeAssign` if it seems like the start of an expression. - Added fixtures

wcjohnson · 2017-04-07T22:27:56Z

@rattrayalex

Changing the arrow token to startsExpr worked great, except it broke one Flow fixture in a way that's kind of strange. The fixture break was expected because the startsExpr in the token stream changed from false to true, but nonetheless the result is a little strange.

The parse tree being output looks correct to me, including the type annotation data, but the structure of the fixture got changed. See: e7ff094

The return type annotation node got moved before the function body node. Is this because lsc parses flow types in a different order than babylon?
All the token information got dropped from the regenerated fixture even though it was present in the original, no idea why that happened.

Thoughts?

rattrayalex · 2017-04-07T22:44:32Z

The ordering of the nodes doesn't matter, and tokens aren't added to new fixtures these days, so that doesn't matter either. Neither does the identifierName thing.

What was the incompatible part of the diff? I'm surprised it overwrote it...

wcjohnson · 2017-04-07T22:46:12Z

If both those things are expected, then I see no problems with the new fixture.

rattrayalex · 2017-04-07T22:50:16Z

I try pretty hard not to modify existing stuff unless it's needed, especially tests. What error is thrown if it's left as it was on master?

wcjohnson · 2017-04-07T22:51:01Z

It's expecting startExpr to be false when it's true...

  index › flow/type annotations/99
  Error: flow/type-annotations/99/actual.js: false !== true (startsExpr/type/7/tokens)
    runTest (test/utils/runFixtureTests.js:164:13)
    Test.fn (test/utils/runFixtureTests.js:25:22)

  index › flow/type annotations/99
  Error: /Users/wcj/dev/lightscript-devkit/packages/babylon-lightscript/test/fixtures/flow/type-annotations/99/actual.js: false !== true (startsExpr/type/7/tokens)
    runTest (test/utils/runFixtureTests.js:164:13)
    Test.fn (test/utils/runFixtureTests.js:107:20)

rattrayalex · 2017-04-08T00:23:34Z

Ah, gotcha. Let's just delete the tokens from that test then. I'd rather do it another way but this is fine.

wcjohnson · 2017-04-09T18:09:27Z

Restored the old fixture and just removed the token stream; everything passing.

rattrayalex · 2017-04-09T19:04:53Z

  dot: new TokenType("."),
  question: new TokenType("?", { beforeExpr }),
-  arrow: new TokenType("=>", { beforeExpr }),
+  arrow: new TokenType("=>", { beforeExpr, startsExpr }),


ideally this would only happen when this.hasPlugin("lightscript"), but I don't think there's an easy way to do that yet. So this is fine for now.

Yes, unfortunately this is all declarative code, running outside the context of a particular this or parser configuration. Would require major changes to the way babylon initializes itself.

rattrayalex · 2017-04-09T19:07:15Z

Alright, merging – thanks for this!

This reverts commit a7d60a7.

rattrayalex reviewed Apr 6, 2017

View reviewed changes

rattrayalex approved these changes Apr 6, 2017

View reviewed changes

Allow empty arrow bodies

287eddf

3 squashed commits: - Allow empty arrow bodies - Delint - Change param name

wcjohnson force-pushed the upstream-empty-arrows branch from ab2bddb to 287eddf Compare April 6, 2017 20:34

rattrayalex reviewed Apr 6, 2017

View reviewed changes

Remove allowEmptyBody param.

6e634cc

wcjohnson added 2 commits April 7, 2017 18:01

Allow empty arrow expressions

2f0d9f0

- Update `parseWhiteBlock` to only call `parseMaybeAssign` if it seems like the start of an expression. - Added fixtures

Add startsExpr to Arrow token.

e7ff094

Restore flow fixture; remove tokens

a014c6a

rattrayalex reviewed Apr 9, 2017

View reviewed changes

rattrayalex merged commit a7d60a7 into lightscript:lightscript Apr 9, 2017

wcjohnson deleted the upstream-empty-arrows branch April 12, 2017 00:53

rattrayalex added a commit that referenced this pull request May 15, 2017

Revert "Allow empty arrow bodies (#9)"

cf6d3eb

This reverts commit a7d60a7.

rattrayalex added a commit that referenced this pull request May 15, 2017

Revert "Allow empty arrow bodies (#9)"

c69be04

This reverts commit a7d60a7.

Conversation

wcjohnson commented Apr 6, 2017

Uh oh!

rattrayalex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wcjohnson Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rattrayalex commented Apr 6, 2017

Uh oh!

wcjohnson commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wcjohnson Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wcjohnson commented Apr 7, 2017

Uh oh!

wcjohnson commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rattrayalex commented Apr 7, 2017

Uh oh!

wcjohnson commented Apr 7, 2017

Uh oh!

rattrayalex commented Apr 7, 2017

Uh oh!

wcjohnson commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rattrayalex commented Apr 8, 2017

Uh oh!

wcjohnson commented Apr 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rattrayalex commented Apr 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wcjohnson Apr 6, 2017 •

edited

Loading

wcjohnson Apr 6, 2017 •

edited

Loading

wcjohnson commented Apr 7, 2017 •

edited

Loading

wcjohnson commented Apr 7, 2017 •

edited

Loading