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

Added parenthesis around await expression in if statement #566

Open
techLeadGuy opened this issue Aug 2, 2023 · 4 comments
Open

Added parenthesis around await expression in if statement #566

techLeadGuy opened this issue Aug 2, 2023 · 4 comments

Comments

@techLeadGuy
Copy link

techLeadGuy commented Aug 2, 2023

Abstract

When parsing a file to AST and back (no transformation), the unary expressions containing await inside if statements get wrapped in parenthesis.

Details

I've stumbled upon the following situation that looks like a bug while developing some codemods for a large Typescript codebase:

// Transformer
export default function transform(fileInfo: FileInfo, api: API) {
    const j = api.jscodeshift;
    return j(fileInfo.source).toSource();
}

// Input code
if(!await a()) {
  // do stuff
}

if(a && await b()) {
 
}

// Expected output: identical code as the input

// Actual output:
if(!(await a())) {
  // do stuff
}

if(a && (await b())) {
	
}

AST Explorer repro link: https://astexplorer.net/#/gist/4f8d333d868a270bd53098d4f283e8fa/963bb4d108676986c24b5f5174e8ef36a98b14ea

It's worthy of mentioning that the code is valid and the functionality is not affected by this change but, as I'm applying the transform to a large codebase (23k files), I see a lot of files changed in Git not due to my desired transformation, but due to paranthesis being added in if statements, which makes the review process a lot harder.

Environment

jscodeshift: 0.15.0

  • babel: 7.22.9
  • babylon: 7.22.7
  • flow: 0.213.1
  • recast: 0.23.3

node 16.20.0
npm 8.19.4
typescript 4.7.4 ( using "ts" parser )
OS: Microsoft Windows 10 Enterprise
The comand is being ran as npm script, with the following value: jscodeshift -t src/transform.ts [path_to_source_directory] --parser=ts --extensions=ts

Note: setting recast version to 0.20.4 in package.json resolutions did not solve the issue

Related issues

While digging for an explanation, I found these issues which seem to be the opposite of mine ( missing parenthesis ):
#442
#420

And they got fixed as part of this PR, which I'm suspecting introduced the problem:
benjamn/recast#923

Later update: The issue seems to be caused by recast, as somebody stumbled upon a very similar problem: benjamn/recast#1191. Still, if there is a way this can be fixed independently of recast, my heart & soul will have a special place for the developer of that fix.

Thank you for taking the time to consider this!

@Daniel15
Copy link
Member

Daniel15 commented Aug 2, 2023

I do think this has to be changed in recast. @ElonVolo Have we thought about forking recast to roll in changes like this one without having to wait for upstream to merge them?

I personally use parentheses in these cases since it makes the statement less ambiguous :)

@techLeadGuy
Copy link
Author

@Daniel15 I also do prefer parentheses for this cases and we'll probably going to use them if no fix can be applied shortly.
I believe I found the culprit in recast and offered to try to fix it, if the authors agree.

It's just that the transformation alters the input in ways we aren't expecting which is a bit scary, especially when applied to a large codebase.

@ElonVolo
Copy link
Collaborator

ElonVolo commented Aug 3, 2023 via email

@techLeadGuy
Copy link
Author

@ElonVolo (coughs awkwardly while looking down) Well, actually.... evcodeshift has the same issue.

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

No branches or pull requests

3 participants