Fix: arrow-body-style fixer for in wrap (fixes #11849)

[anikethsaha]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fixed the fixer for in operator being wrongly fixed. wrapping the whole body when there is an in operator. inIn method will check if the body has in operator or not. wrapping the whole as mentioned in the issue was decided.

Is there anything you'd like reviewers to focus on?

fixes #11849

[yeonjuan]

Thanks for this work. 😀 I left some comments.

[yeonjuan]

@anikethsaha
Sorry for the delay. 😂 I left comments about the code and unexpected behavior.

It seems to add extra parenthesis when it is already wrapped.

// input
for ( a = (b) => { return (c in d) }; ;);

// expected
for ( a = (b) => (c in d); ;);

// result of the auto fix -  PR 
for ( a = (b) => ((c in d)); ;);

[yeonjuan]

@anikethsaha
I tested this PR branch and the fixer seems to generate a syntax error code on these cases.

  code: "for ( a = (b) => { return (1) + c in d }; ;);",
 output: "for ( a = (b) => (1) + c in d; ;);",

I'm not sure what the proper result should be.
maybe wrapping all?

 output: "for ( a = (b) => ((1) + c in d); ;);",

[anikethsaha]

@yeonjuan thanks for pointing.

I think , this check needs to be changed.

I think there will be conflict to solve this 👇

// input
for ( a = (b) => { return (c in d) }; ;);

// expected
for ( a = (b) => (c in d); ;);

// result of the auto fix -  PR 
for ( a = (b) => ((c in d)); ;);

and the one you mentioned in #13228 (review) ?

What I am thinking is to track the opening braces and closing braces, if all opening braces are closed then the next token should be ; .

Not sure though.

Edit:

  code: "for ( a = (b) => { return (1) + c in d }; ;);",
 output: "for ( a = (b) => (1) + c in d; ;);",

or we can keep this ?

[yeonjuan]

@anikethsaha

What I am thinking is to track the opening braces and closing braces, if all opening braces are closed then the next token should be ;

Well, I'm not sure how we can treat it.
Maybe just checks whether the node to fix already wrapped by parens?

or we can keep this ?

IMO, we should not keep it.
Let's hear other's thoughts.

[anikethsaha]

Maybe just checks whether the node to fix already wrapped by parens?

@yeonjuan this is what I am trying to do by checking the leading ( ,

What I can do it to check if they are with ) or not.

but then this case might be conflicting.

for ( a = (b) => { return c in (d+c) }; ;);

do you have any other way to check for wrapped or not.? maybe I am missing something here.

[nzakas]

@yeonjuan looks like there's a question for you on this PR

[mdjermanovic]

There are many cases where those parens wouldn't be necessary:

for (a = b => { return (c in d) } ;;) {}

for (a = b => { return [c in d] } ;;) {}

for (a = b => { return c || (d in e) } ;;) {}

for (a = b => { return c || (d && e in f) } ;;) {}

for (a = (b => { return c in d }) ;;) {}

for (a = [b => { return c in d }] ;;) {}

A similar problem (in in for-loop initializer) with no-extra-parens rule was fixed in #11848, and it handles all these cases, but that's the rule about extra parens, so such complexity to avoid extra parens makes sense there

In my opinion, arrow-body-style should just always add those parens if the arrow function is in a for-loop initializer and there's in inside the arrow function. That combination is an edge case anyway, and those parens will be removed by no-extra-parens if they're unnecessary.

[mdjermanovic]

So, regarding the question, I personally think we shouldn't care much about extra parens in this rule.

On the other hand, it seems that some cases where the parens are necessary are still not covered.

[yeonjuan]

@anikethsaha

do you have any other way to check for wrapped or not.? Maybe I am missing something here.

maybe we can do it by checking whether the whole returned node is wrapped by parenthesis (not only checking the first token of the node) -but not sure cause I didn't try it.

In my opinion, arrow-body-style should just always add those parens if the arrow function is in a for-loop initializer and there's in inside the arrow function. That combination is an edge case anyway, and those parens will be removed by no-extra-parens if they're unnecessary.

But, as the @mdjermanovic 's comment, it might be reasonable not to check extra parens in here strictly. Sorry if you got a confusion.

[anikethsaha]

@mdjermanovic I changed the code to a bit different, It seems working though, Let me know if this needs to be changed if so I will check with the approach you mentioned.

[mdjermanovic]

It still doesn't work well in this case (we should probably add this as a test case):

/*eslint arrow-body-style: ["error", "as-needed"]*/

a in b;

for (var f = () => { return c };;);

auto-fixes to:

/*eslint arrow-body-style: ["error", "as-needed"]*/

a in b;

for (var f = () => (c);;);

This is the order of events for this example:

BinaryExpression[operator='in'] > *:exit (a)
BinaryExpression[operator='in'] > *:exit (b)
BinaryExpression[operator='in']:exit
ArrowFunctionExpression
ArrowFunctionExpression:exit

[anikethsaha]

@mdjermanovic I did add a test case for the mentioned code here https://github.com/eslint/eslint/pull/13228/files#diff-a5a55cb049da483c38e374fd2af60fecR62 , is this what you were referring?

not sure about a in b; .

BinaryExpression[operator='in'] > *:exit (a)
BinaryExpression[operator='in'] > *:exit (b)
BinaryExpression[operator='in']:exit
ArrowFunctionExpression
ArrowFunctionExpression:exit

cool, I will check this

/*eslint arrow-body-style: ["error", "as-needed"]*/

a in b;

for (var f = () => { return c };;);

in this, are these two separate test cases or same ?

[mdjermanovic]

in this, are these two separate test cases or same ?

1 test, something like:

code: "a in b; for (var f = () => { return c };;);"
output: "a in b; for (var f = () => c;;);"

(no parens in output)

[mdjermanovic]

LGTM, thanks!

[kaicataldo]

@yeonjuan Have your concerns been addressed?

[nzakas]

@yeonjuan just checking in to see if you're ready to approve this latest version?

[yeonjuan]

Sorry for the late, It LGTM!