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.

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

[Fix] Add avoid parens config for Printer #1257

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

phoenisx
Copy link

Fixes #1191, and possibly #1248

petergok added a commit to TempoLabsAI/recast that referenced this pull request May 17, 2023
@GianlucaGuarini
Copy link

GianlucaGuarini commented May 30, 2023

This Patch could help us to upgrade recast in our @riotjs/compiler.
@eventualbuddha have you had any chance to check this? How can we help you further?

@techLeadGuy
Copy link

This will also fix an issue in jscodeshift, namely facebook/jscodeshift#566. How can we push this PR ?

@eventualbuddha
Copy link
Collaborator

A quick read of this makes me concerned that enabling this flag will, in some cases, cause printing of invalid or incorrect code. Unless needsParens doesn't actually mean need in any case?

@techLeadGuy
Copy link

@eventualbuddha I think you are right, for example when you go from 1 argument in an arrow fct to no/multiple arguments, in which case you want the final code to have auto-added parens even in the case this flag would be set to true.

I guess a better solution would be to just fix the FPp.needsParens method for the cases where parens can be bypassed (if the input code did not have them in the first place). For the await case inside conditional expression, I think it's because of this line.

Unless I'm missing something, I'll be as easy as removing L451 from the printer. If you agree with this approach, I'm happy to create a PR for it.

@phoenisx
Copy link
Author

phoenisx commented Aug 4, 2023

That's a very good find.
Just curious, apart from arrow-function params are there any other syntax where adding parens is required after a code change?

What I am trying to understand here is why adding parens is a generic behaviour rather than being specific to such special cases.

@techLeadGuy
Copy link

@phoenisx Just by looking at recast's printer logic (this line), there's the case where you add type definitions for params that would also need to enforce parens.

const myFunc = name => console.log(name);
// becoming
const myFunc = (name: string) => console.log(name);

My feeling is that, since for the most part, the parens logic is handled appropriately ( keeping existing parens in place, only adding parens when the transformation requires for syntax validity and not adding extra parens when not required ), our specific issues are more exceptions than rules.

@michaelfaith
Copy link

michaelfaith commented Aug 4, 2023

My feeling is that, since for the most part, the parens logic is handled appropriately ( keeping existing parens in place, only adding parens when the transformation requires for syntax validity and not adding extra parens when not required ), our specific issues are more exceptions than rules.

Or the logic to detect when parens are required isn't entirely sound. I.e. getting incorrectly triggered by jsx elements: #1248 Reading through the printer code, did you see anything that stood out that might cause that behavior?

@techLeadGuy
Copy link

@michaelfaith Unfortunately not. Tried yesterday to fiddle around with recast locally, hoping to find an easy fix for all these parens related issues but there are a lot more corner cases than I originally expected and a lot of tests for them, which is great, but time consuming to properly change. Someone with a better understanding of all these cases could perhaps be a lot more efficient than myself, but as of right now, don't know when I will have the time to try to fix these again.

@topwood
Copy link

topwood commented Aug 14, 2023

need this config too,pls fix this soon

@KernelDeimos
Copy link

KernelDeimos commented Jul 9, 2024

August of last year... oof

edit: and reasonably so, after about 40 minutes I realized there is no good solution to this right now. The parser (babel at least; the default parser is useless to me because it's wayyy behind the spec) is lossy and does not preserve information about the parens unless you pass tokens: true

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

Successfully merging this pull request may close these issues.

recast.print modifies original source adding brackets around some statements
7 participants