Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Parses flow type cast expression with parenthesis in call expression correctly #446

Closed
wants to merge 5 commits into from

Conversation

chikara-chan
Copy link

Q A
Bug fix? yes
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets #413
License MIT

By the way, now the async arrow expression is not parsed from call expression any more. Instead, check if async is a keyword or function name there. Then decides to parse call expression arguments or function params.

@codecov
Copy link

codecov bot commented Apr 2, 2017

Codecov Report

Merging #446 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   98.25%   98.41%   +0.15%     
==========================================
  Files          20       20              
  Lines        3504     3526      +22     
  Branches      927      934       +7     
==========================================
+ Hits         3443     3470      +27     
+ Misses         22       18       -4     
+ Partials       39       38       -1
Flag Coverage Δ
#babel 81.99% <76.08%> (-0.03%) ⬇️
#babylon 97.16% <100%> (+0.16%) ⬆️
Impacted Files Coverage Δ
src/tokenizer/types.js 100% <ø> (ø) ⬆️
src/parser/expression.js 97.45% <100%> (+0.07%) ⬆️
src/plugins/flow.js 98.79% <100%> (+0.6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d18221...5564a0e. Read the comment docs.

Copy link
Member

@danez danez left a comment

Choose a reason for hiding this comment

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

Do you think this could be fixed without changing the parsing to rely on the lookahead?

@@ -298,22 +323,23 @@ pp.parseSubscripts = function (base, startPos, startLoc, noCalls) {
this.expect(tt.bracketR);
base = this.finishNode(node, "MemberExpression");
} else if (!noCalls && this.match(tt.parenL)) {
const possibleAsync = this.state.potentialArrowAt === base.start && base.type === "Identifier" && base.name === "async" && !this.canInsertSemicolon();
const old = this.state.clone(true);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little bit worried about this and doing it whenever we encounter a parenL. clone() is very expensive and we have never done lookaheads of this extend before (besides one token with this.lookahead()).

@wcjohnson
Copy link

It is almost certainly possible to fix this without using parser lookahead. See wcjohnson@322842a for an outline of an approach.

Basic outline:

  • parse from within parenLists as TypeCastish
  • only convert to TypeCastExpression from within parseParenAndDistinguishExpression
  • lvalue conversion converts TypeCastish into the correct annotations
  • if TypeCastish is seen anywhere else, raise an unexpected type cast error

@danez
Copy link
Member

danez commented Nov 1, 2017

We moved babylon back into the monorepo at https://github.com/babel/babel/tree/master/packages/babylon.

Unfortunately pull requests cannot be migrated between repositories automatically. If this PR is still valid please reopen it there.
Thanks for your effort.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants