Distinguish between ternary's : and arrow fn's return type #573

nicolo-ribaudo · 2017-06-10T21:58:32Z

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

I took this approach:

After ?, try to parse an assignment expression (consequent)
If the function threw:
a. Disallow arrow functions in the first position
a. Parse the assignment expression again (consequent)
Else, it there isn't :
a. Find every typed arrow function in that expression
b. If there are more than one, trow an "Ambiguity" error ¹
c. Mark the arrow function so that it is parsed as ParenthesizedExpression : ArrowFunctionExpression
d. Parse the assignment expression again (consequent)
Parse :
Parse an assignment expression (alternate)

~~~[1] I don't know if the "check if there are more than one functions" condition is ok, I have to think about it~~~
This approach doesn't work sometimes, like with a ? (b) : c => (d => e) : f => g;. I think I have to rewrite 3.b and 3.c

codecov · 2017-06-10T22:10:06Z

Codecov Report

Merging #573 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #573      +/-   ##
==========================================
+ Coverage   98.14%   98.18%   +0.03%     
==========================================
  Files          22       22              
  Lines        3663     3737      +74     
  Branches     1024     1038      +14     
==========================================
+ Hits         3595     3669      +74     
  Misses         25       25              
  Partials       43       43

Flag	Coverage Δ
#babel	`80.3% <71.42%> (+0.64%)`	⬆️
#babylon	`96.89% <100%> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
src/plugins/jsx/index.js	`97.7% <100%> (ø)`	⬆️
src/parser/expression.js	`96.9% <100%> (+0.01%)`	⬆️
src/tokenizer/state.js	`100% <100%> (ø)`	⬆️
src/plugins/estree.js	`99.19% <100%> (ø)`	⬆️
src/plugins/flow.js	`98.51% <100%> (+0.11%)`	⬆️

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 dc87d99...4d93bd7. Read the comment docs.

Defer the conversion of arrow function parameters to assignable nodes so that it is possible to use the (invalid) ast to get the exact position of the (wrong) arrow functions.

nicolo-ribaudo · 2017-06-12T13:02:38Z

I don't know if the "check if there are more than one functions" condition is ok, I have to think about it

I'm not 100% sure, but I couldn't find any example for which this check is wrong


- [x]  `a ? async (b) : c => d`

nicolo-ribaudo

I don't like having changed the main parser (adding noArrowParamsConversion — checkFunctionLVals was created just to avoid duplicationg code in flow.js) to fix a bug in a plugin, but I couldn't find a better method.

nicolo-ribaudo · 2017-06-12T13:29:11Z

test/fixtures/flow/regression/issue-58-ambiguous/actual.js

+// This can be parsed in two ways:
+// a ? b : (c => ((d): e => f))
+// a ? ((b): c => d) : (e => f)
+a ? (b) : c => (d) : e => f;


Here throwing is the most future-compatible behavior. Probably we should ask to the flow team how to handle this cases.

vjeux · 2017-06-19T16:53:04Z

Would be awesome if you could add prettier/prettier#2194 (comment) as a test case. Thanks so much for fixing a bug before it was even discovered in prettier!

prettier/prettier#2194

nicolo-ribaudo · 2017-06-20T19:16:27Z

@vjeux Test added 👍

@anyoneFromTheBabelTeam Do you prefer that I resolve the merge conflicts by rebasing or by merging master into this branch?

vjeux · 2017-06-20T19:16:49Z

@nicolo-ribaudo awesome, thanks a ton! I can't wait for it to ship!

hzoo · 2017-06-21T15:56:01Z

@nicolo-ribaudo rebase although if it's a simple change with 1 author we just squash merge it anyway

vjeux · 2017-06-21T21:22:11Z

Fyi, this bug was also found by a Facebook employee during a conversion. So it's at least impacting two people :)

existentialism

I struggled with the same "wish we didn't have to add yet-another-param in the core parser code" for this a while back, but I think we can just improve our param-sanity later :)

nicolo-ribaudo · 2017-06-24T07:14:23Z

@existentialism I'm trying to remove that parameter, but I can't guarantee I will succeed: this PR could be merged with that ugly parameter and I'll open another one when it will be ready

nicolo-ribaudo · 2017-06-25T22:07:37Z

@existentialism I managed to remove that parameter! (And magically solved the merge conflicts while doing that 😎)

8541f2c (Remove noArrowParamsConversion) is just noise, the only meaningful changes are in 74ef24e.

existentialism · 2017-06-27T02:36:38Z

@nicolo-ribaudo nice! @hzoo and I were discussing using state for this vs a param a while back too!

Can also use a set for state.noArrowParamsConversionAt (to avoid lots of potential indexOf checks)?

But for now, LGTM 👍

nicolo-ribaudo · 2017-06-27T07:18:55Z

Can also use a set for state.noArrowParamsConversionAt (to avoid lots of potential indexOf checks)?

I searched in the babylon source code and it doesn't seem to use any ES6 function: wouldn't using Set reduce babylon's compatibility?
Another option to replace indexOf might be to use a function which traverses state.noArrowParamsConversionAt from the end and returns early if there is a number which is less than the searched one (sice state.noArrowParamsConversionAt is sorted)

Anyway, I think indexOf is fine because the array will never contain many items (unless there are lots of nested ?: and () =>), so its performance cost should be negligible.

EDIT: It looks that if the array is short, Array#indexOf is faster than Set#has (at least on Chrome & Firefox on Ubuntu 17):

number of items	Array#indexOf (ops/sec)	Set#has (ops/sec)	jsperf
0	402.000.000	41.000.000	https://jsperf.com/indexof-vs-set-empty/1
3	112.000.000	16.000.000	https://jsperf.com/indexof-vs-set-short/1
1000	248.000	3.600.000	https://jsperf.com/indexof-vs-set-long

hzoo · 2017-06-27T19:36:43Z

The initial purpose of Babel 7 was just to drop Node 0.10/0.12. We are using Set in other parts of Babel already and it helped us to drop babel-runtime as dep (Symbol as well), so that's not an idea. But yeah if those specific cases won't have a lot of items then we don't have to change it.

hzoo · 2017-06-27T20:02:44Z

Actually was getting a lot of errors locally for flow npm run flow so going to revert for now and release with what we have. Can you make another PR to fix? I'm not sure why the flow tests are optional (maybe since the infra setup is still broken) https://travis-ci.org/babel/babylon/jobs/246895914

)" This reverts commit a9a55fb.

nicolo-ribaudo · 2017-06-28T08:04:33Z

Errors fixed in #596

nicolo-ribaudo · 2017-06-28T08:25:11Z

I'm not sure why the flow tests are optional

Only the flow test suite is optional, not the type checks. Actually it looks like they pass on travis-ci:

https://travis-ci.org/babel/babylon/jobs/246895911

The command "if [ "$JOB" = "test" ]; then yarn test-only; fi" exited with 0.
11.33s$ if [ "$JOB" = "lint" ]; then yarn run lint && yarn run flow; fi
yarn run v0.24.6
$ eslint src bin
yarn run v0.24.6
$ flow
Launching Flow server for /home/travis/build/babel/babylon
Spawned flow server (pid=2240)
Logs will go to /tmp/flow/zShomezStraviszSbuildzSbabelzSbabylon.log
No errors!

hzoo · 2017-06-28T10:44:58Z

Oh ok that was confusing then

Distinguish between ternary's : and arrow fn's return type

eaef8f8

nicolo-ribaudo changed the title ~~Distinguish between ternary's : and arrow fn's return type~~ [WIP] Distinguish between ternary's : and arrow fn's return type Jun 10, 2017

nicolo-ribaudo added 2 commits June 12, 2017 12:00

Correctly parse nested arrow functions inside conditional expressions

9d6e62e

Defer the conversion of arrow function parameters to assignable nodes so that it is possible to use the (invalid) ast to get the exact position of the (wrong) arrow functions.

Check params of arrow fns w/ type params or w/o return type

1db908a

nicolo-ribaudo changed the title ~~[WIP] Distinguish between ternary's : and arrow fn's return type~~ Distinguish between ternary's : and arrow fn's return type Jun 12, 2017

nicolo-ribaudo commented Jun 12, 2017

View reviewed changes

nicolo-ribaudo mentioned this pull request Jun 12, 2017

Wrong AST for conditional that looks like arrow facebook/flow#2006

Closed

Fix also async functions

4d93bd7

nicolo-ribaudo added 2 commits June 20, 2017 21:08

Add test from prettier

b43240c

prettier/prettier#2194

Don't check arrow params if they are valid at the first attemp

2f60733

hzoo added the Tag: Bug Fix label Jun 21, 2017

existentialism approved these changes Jun 23, 2017

View reviewed changes

nicolo-ribaudo added 2 commits June 25, 2017 23:51

Use state instead of relying on the "noArrowParamsConversion" parameter

74ef24e

Remove noArrowParamsConversion

8541f2c

existentialism assigned hzoo and danez Jun 27, 2017

hzoo merged commit a9a55fb into babel:master Jun 27, 2017

hzoo added a commit that referenced this pull request Jun 27, 2017

Revert "Distinguish between ternary's : and arrow fn's return type (#573

8829853

)" This reverts commit a9a55fb.

nicolo-ribaudo mentioned this pull request Jun 28, 2017

Distinguish between ternary's : and arrow fn's return type #596

Merged

nicolo-ribaudo mentioned this pull request Nov 6, 2019

Parse arrows with params annotations in conditional expressions babel/babel#10669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distinguish between ternary's : and arrow fn's return type #573

Distinguish between ternary's : and arrow fn's return type #573

nicolo-ribaudo commented Jun 10, 2017 •

edited

Loading

codecov bot commented Jun 10, 2017 •

edited

Loading

nicolo-ribaudo commented Jun 12, 2017 •

edited

Loading

nicolo-ribaudo left a comment

nicolo-ribaudo Jun 12, 2017

vjeux commented Jun 19, 2017

nicolo-ribaudo commented Jun 20, 2017 •

edited

Loading

vjeux commented Jun 20, 2017

hzoo commented Jun 21, 2017

vjeux commented Jun 21, 2017

existentialism left a comment

nicolo-ribaudo commented Jun 24, 2017 •

edited

Loading

nicolo-ribaudo commented Jun 25, 2017 •

edited

Loading

existentialism commented Jun 27, 2017 •

edited

Loading

nicolo-ribaudo commented Jun 27, 2017 •

edited

Loading

hzoo commented Jun 27, 2017

hzoo commented Jun 27, 2017

nicolo-ribaudo commented Jun 28, 2017

nicolo-ribaudo commented Jun 28, 2017

hzoo commented Jun 28, 2017

Distinguish between ternary's : and arrow fn's return type #573

Distinguish between ternary's : and arrow fn's return type #573

Conversation

nicolo-ribaudo commented Jun 10, 2017 • edited Loading

codecov bot commented Jun 10, 2017 • edited Loading

Codecov Report

nicolo-ribaudo commented Jun 12, 2017 • edited Loading

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

nicolo-ribaudo Jun 12, 2017

Choose a reason for hiding this comment

vjeux commented Jun 19, 2017

nicolo-ribaudo commented Jun 20, 2017 • edited Loading

vjeux commented Jun 20, 2017

hzoo commented Jun 21, 2017

vjeux commented Jun 21, 2017

existentialism left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Jun 24, 2017 • edited Loading

nicolo-ribaudo commented Jun 25, 2017 • edited Loading

existentialism commented Jun 27, 2017 • edited Loading

nicolo-ribaudo commented Jun 27, 2017 • edited Loading

hzoo commented Jun 27, 2017

hzoo commented Jun 27, 2017

nicolo-ribaudo commented Jun 28, 2017

nicolo-ribaudo commented Jun 28, 2017

hzoo commented Jun 28, 2017

nicolo-ribaudo commented Jun 10, 2017 •

edited

Loading

codecov bot commented Jun 10, 2017 •

edited

Loading

nicolo-ribaudo commented Jun 12, 2017 •

edited

Loading

nicolo-ribaudo commented Jun 20, 2017 •

edited

Loading

nicolo-ribaudo commented Jun 24, 2017 •

edited

Loading

nicolo-ribaudo commented Jun 25, 2017 •

edited

Loading

existentialism commented Jun 27, 2017 •

edited

Loading

nicolo-ribaudo commented Jun 27, 2017 •

edited

Loading