Skip to content
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

Jump to bottom

ternary operator > lambda > ternary fails to parse himself after formatting #2194

Closed
deddu opened this issue Jun 19, 2017 · 10 comments
Closed

ternary operator > lambda > ternary fails to parse himself after formatting #2194

deddu opened this issue Jun 19, 2017 · 10 comments
Labels
lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! scope:dependency Issues that cannot be solved inside Prettier itself, and must be fixed in a dependency type:bug Issues identifying ugly output, or a defect in the program

Comments

@deddu
Copy link

deddu commented Jun 19, 2017 •
edited by vjeux
Loading

The following nested ternary

let icecream = what == "cone"
  ? p => !!p ? `here's your ${p} cone` : `just the empty cone for you`
  : p => `here's your ${p} ${what}`;

gets formatted to

let icecream = what == "cone"
  ? p => (!!p ? `here's your ${p} cone` : `just the empty cone for you`)
  : p => `here's your ${p} ${what}`;

which consequently fails a subsequent parsing with
SyntaxError: Invalid left-hand side in arrow function parameters (2:11);

(using prettier 1.44)
@hawkrives
Copy link
Contributor

online reproduction

Thanks for reporting, @deddu!

@k15a k15a added the type:bug Issues identifying ugly output, or a defect in the program label Jun 19, 2017
@vjeux vjeux added the priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! label Jun 19, 2017
@vjeux
Copy link
Contributor

vjeux commented Jun 19, 2017

@k15a fyi, if prettier generates a piece of code that doesn't parse or produces an output with a different meaning, this should be flagged as high pri. It should not happen, ever :)

@vjeux
Copy link
Contributor

vjeux commented Jun 19, 2017

This looks like an issue with babylon, flow parser handles it fine. cc @hzoo

@vjeux vjeux added the scope:dependency Issues that cannot be solved inside Prettier itself, and must be fixed in a dependency label Jun 19, 2017
@hzoo
Copy link
Contributor

hzoo commented Jun 19, 2017

Looks related to issues regarding babel/babylon#58

@vjeux
Copy link
Contributor

vjeux commented Jun 19, 2017

@hzoo looks like it indeed, and there's already a PR for it! You are on fire :)

@vjeux vjeux mentioned this issue Jun 19, 2017
Merged
2 tasks
hzoo pushed a commit to babel/babylon that referenced this issue Jun 27, 2017
@nicolo-ribaudo @hzoo
Distinguish between ternary's : and arrow fn's return type (#573)
a9a55fb 
* Distinguish between ternary's : and arrow fn's return type

* Correctly parse nested arrow functions inside conditional expressions

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

* Fix also async functions

* Add test from prettier

prettier/prettier#2194

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

* Use state instead of relying on the "noArrowParamsConversion" parameter

* Remove noArrowParamsConversion
@josephfrazier josephfrazier mentioned this issue Jul 4, 2017
Closed
@josephfrazier
Copy link
Collaborator

This was fixed in babel/babylon@a9a55fb, but then reverted in babel/babylon@8829853. babel/babylon#596 has the next iteration of the fix.

@nicolo-ribaudo
Copy link
Contributor

That pr was released in babylon v.7.0.0-beta.27!

@lydell
Copy link
Member

lydell commented Oct 1, 2017 •
edited
Loading

@nicolo-ribaudo Thanks for the heads-up! Would you be interested in making a PR bumping the babylon version and adding a test case? :)

@nicolo-ribaudo
Copy link
Contributor

Sure, I can do that this evening (CET)

@existentialism
Copy link
Collaborator

existentialism commented Oct 2, 2017 •
edited
Loading

I have a PR ready for this, but the latest Babylon version can't be bumped in Prettier b/c we split the TS/Flow types. I have an update ready for this as well but waiting for an upstream change in typescript-eslint-parser to land.

@azz azz added the lang:javascript Issues affecting JS label Oct 4, 2017
@existentialism existentialism mentioned this issue Oct 15, 2017
Merged
@azz azz closed this as completed in #3039 Oct 15, 2017
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! scope:dependency Issues that cannot be solved inside Prettier itself, and must be fixed in a dependency type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@existentialism @vjeux @hawkrives @hzoo @azz @lydell @deddu @josephfrazier @nicolo-ribaudo @k15a