Use rewrite_assign_rhs_with_comments by ControlFlow rewrite_pat_expr

[davidBar-On]

Change ControlFlow rewrite_pat_expr() to use rewrite_assign_rhs_with_comments() (subset of proposed changes in PR #4578).

[calebcartwright]

Beautiful thank you! I know you've already got a couple PRs in flight, though if you're interested, one of the outstanding items is backporting the addition of rewrite_assign_rhs_with_comments (introduced to master branch via #4440) to the latest 1.x branch (rustfmt-1.4.31 at the moment), and then additionally refactoring the code (on both master and 1.x branch) to more fully take advantage

[davidBar-On]

if you're interested, one of the outstanding items is backporting the addition of rewrite_assign_rhs_with_comments (introduced to master branch via #4440) to the latest 1.x branch (rustfmt-1.4.31 at the moment), and then additionally refactoring the code (on both master and 1.x branch) to more fully take advantage

I am interested. Will try to do that after submitting change to an issue I am now evaluating.

[davidBar-On]

... one of the outstanding items is backporting the addition of rewrite_assign_rhs_with_comments (introduced to master branch via #4440) to the latest 1.x branch (rustfmt-1.4.31 at the moment)

Submitted PR #4632 with the backporting to 1.x the changes to add rewrite_assign_rhs_with_comments and its use.

... and then additionally refactoring the code (on both master and 1.x branch) to more fully take advantage

I am not sure what type of refactoring you are expecting. Do you have an example of code that you think may be refactored? Should I try to suggest additional comments handling refactoring like the addition of rewrite_assign_rhs_with_comments?

[calebcartwright]

I am not sure what type of refactoring you are expecting.

#4440 has a more complete picture, but the shortish version:

We've historically had a lot of largely duplicative code throughout the codebase that's attempting to handle comments between lhs and rhs by trying to force various rewrite_assign_rhs* functions to work. In addition to the duplicative efforts, there's also been a lot of bugs stemming from this code because various scenarios weren't covered, and then when a bug was reported and fixed it would only apply in the one place (many of the other duplicated sites often had the same bug)

rewrite_assign_rhs_with_comments was a new utility function to provide a single, reusable implementation as the first step at resolving these issues. However, most of the existing duplicative code has not been updated to use the new rewrite_assign_rhs_with_comments, and is still using the various other rewrite_assign_rhs*. This PR is one such case where the old rewrite_assign_rhs* usage could be refactored to take advantage of rewrite_assign_rhs_with_comments.

I don't know offhand where every single case is in the current codebase where there's an opportunity to refactor and simplify by switching to use rewrite_assign_rhs_with_comments instead. I'd guess there's some formatting for expressions, items, and maybe types as well, but would suggest searching for rewrite_assign_rhs... to find the current places, and evaluate whether it would be possible and beneficial to update the code to use rewrite_assign_rhs_with_comments instead, just like you did in this PR!

[davidBar-On]

suggest searching for rewrite_assign_rhs... to find the current places, and evaluate whether it would be possible and beneficial to update the code to use rewrite_assign_rhs_with_comments instead

The search is relevant to the use of rewrite_assign_rhs() and rewrite_assign_rhs_with(). I didn't find any other place where these functions are used with handling comments without using common functions. In all the cases I found the comments between lhs and rhs are not handled.

Probably in most cases, calling a rewrite_assign_rhs_with_comments instead of rewrite_assign_rhs... function can be done quite easily. The main issue is to add sufficient test cases. Therefore, prioritization is needed, including whether it is desired to do the change at all. Also, can one PR be used for several of these cases, with separate test file for each?

Following are the cases I found:

1. rewrite_assignment()
2. format_variant()
3. format_trait()
4. format_trait_alias()
5. rewrite_type()
6. rewrite_struct_field()
7. rewrite_opaque_impl_type()
8. Static ast::ForeignItem
9. Macro format_lazy_static()
10. ast::WherePredicate::BoundPredicate
11. ast::WherePredicate::EqPredicate

[calebcartwright]

Therefore, prioritization is needed

This overall effort is really just refactoring/cleanup of code, so there's not really any intra-prioritization relative to refactoring one case against another.

Also, can one PR be used for several of these cases, with separate test file for each?

Smaller PRs are always preferrable over large ones, so multiple small PRs will just about always be better than one (or fewer) larger ones.

including whether it is desired to do the change at all

Sorry, but it's not possible to answer that question in the abstract and I don't have the bandwidth to go through each line item (otherwise I'd just make the code changes myself 😄). If you're interested in looking into this, then I'd suggest two basic considerations to use when determining if refactoring a given case would be worthwhile:

• Does it improve the code (e.g. does it simplify/cleanup the code, and/or reduce duplication, etc.)
• Does it fix or improve comment handling between the lhs and rhs

[davidBar-On]

Does it improve the code (e.g. does it simplify/cleanup the code, and/or reduce duplication, etc.)
Does it fix or improve comment handling between the lhs and rhs

In all cases I identified there will be no code improvement, since in all just doesn't handle comments, but comments handling will be added.

Smaller PRs are always preferable over large ones,

I will try to make the change to rewrite_assign_rhs_with_comments in one or two cases that I understand well enough (e.g. not Traits) and see how it works.

[ytmimi]

Looks like these changes were pulled into the 1.x branch via #4632