This repository has been archived by the owner. It is now read-only.

Fix Flow return types on arrow functions #124

Merged
merged 3 commits into from Sep 15, 2016

Conversation

Projects
None yet
3 participants
@danharper
Copy link
Member

danharper commented Sep 12, 2016

Two commits.

First fixes: babel/babel-eslint#348 which can be summed up as this should be valid, but isn't:

const x = (foo: string)
: number => {}

https://flowtype.org/try/#0MYewdgzgLgBAHjAvDAFAMxCAXDaAnASzAHMBKAKBzAFcBbAIwFM8kA+GAbxj0amrzAwATDAC+5IA

Second fixes a bug where Babel won't error when there's a line break before the => when there's a type annotation (basically this existing test, but with a return type in the mix):

(): string
=> {}

https://flowtype.org/try/#0BQSgXABAzgLgTgSwHYHMBQBeAfBA3gXyA

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 12, 2016

Current coverage is 94.93% (diff: 88.88%)

No coverage report found for master at 015035c.

Powered by Codecov. Last update 015035c...774bf48

@@ -613,6 +613,10 @@ pp.parseParenAndDistinguishExpression = function (startPos, startLoc, canBeArrow
return val;
};

pp.shouldParseArrow = function () {
return !this.canInsertSemicolon();

This comment has been minimized.

@danez

danez Sep 12, 2016

Member

This seems to be a tab here?

This comment has been minimized.

@danharper

danharper Sep 12, 2016

Author Member

Oops, sorry! (I'm one of those people lol). Lint didn't pick it up. I've pushed a change.

@@ -1162,6 +1162,7 @@ export default function (instance) {
let state = this.state.clone();
try {
let returnType = this.flowParseTypeAnnotation();
if (this.canInsertSemicolon()) this.unexpected();

This comment has been minimized.

@danez

danez Sep 12, 2016

Member

Can we make this maybe a nice error message here? Flow says "Illegal newline before arrow", which is already better than "unexpected token".

This comment has been minimized.

@danharper

danharper Sep 12, 2016

Author Member

I don't think so, because any syntax errors are immediately caught at the end of parseArrow: https://github.com/danharper/babylon/blob/774bf486fe2f117393dcf5ee14aa2c050ae40dd7/src/plugins/flow.js#L1169-L1175

(which is also why the raise error happens at 1:1 and not at the actual position)

I'm not 100% sure on why it works like this, though.

(it's for regression tests in babel/babel@caecebf)

This comment has been minimized.

@danez

danez Sep 12, 2016

Member

Argh, true.

This try catch is there because when we encounter a colon at this spot we are not yet sure about a.) if it is an valid arrow at all b.) if the colon might be from a ternary operator (something like this i think a ? b: c => {}). So if a SyntaxError is thrown while parsing the return type we reset the state to what it was before we started parsing the type and let the regular parsing function of babylon handle it.

This comment has been minimized.

@danharper

danharper Sep 12, 2016

Author Member

oh I see, thanks for the explanation 👍

This comment has been minimized.

@danharper

danharper Sep 12, 2016

Author Member

Thinking: we could just throw a different error, catch it, then throw with a nicer message instead:

 if (this.match(tt.colon)) {
   let state = this.state.clone();
   try {
     let returnType = this.flowParseTypeAnnotation();
-    if (this.canInsertSemicolon()) this.unexpected();
+    if (this.canInsertSemicolon()) throw new Error("IS_SEMICOLON_ERR");
     if (!this.match(tt.arrow)) this.unexpected();
     // assign after it is clear it is an arrow
     node.returnType = returnType;
   } catch (err) {
     if (err instanceof SyntaxError) {
       this.state = state;
+    } else if (err.message === "IS_SEMICOLON_ERR") {
+      this.unexpected(null, "Illegal newline before arrow");
     } else {
       throw err;
     }
   }
 }

It's not great, but it produces a much nicer error message, in the correct location too.

Thoughts?

This comment has been minimized.

@danez

danez Sep 13, 2016

Member

Not entirely sure if that has any side effects on this try/catch logic.
So maybe we jsut leave it for now, and later on have a look how we can improve the message.

@danez

This comment has been minimized.

Copy link
Member

danez commented Sep 12, 2016

Looks good. In general it would be nice if different fixes could go to different PRs as it is then easier to understand which change is for which fix, but in this small case it does not really matter.

Thanks for all your contributions.

@danharper

This comment has been minimized.

Copy link
Member Author

danharper commented Sep 12, 2016

Sure, I'll make sure to split them in the future! :)

@danez

danez approved these changes Sep 15, 2016

@danez danez merged commit dc30366 into babel:master Sep 15, 2016

1 of 3 checks passed

codecov/patch 88.88% of diff hit (target 94.93%)
Details
codecov/project 94.93% (-1.84%) compared to bc6aa62
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.