Fixed comment formatting issue between assignment operator and rhs

[whizsid]

Fixes #4427 .

I tried to use combine_strs_with_missing_comments in rewrite_assign_rhs_with. But it is a huge change and already fixed it in some places using boiler plates. So I didn't take that approach.

[calebcartwright]

Thanks so much for the PR @whizsid! I've only had a chance to do a quick glance through this, will try to set aside some time in the next couple days to go through in detail.

One initial thought I had is that I still think it would be good if we could have a more generic/reusable approach for handling assignments that have comments between the lhs and rhs. The existing util functions are pretty clear that they expect lhs to (inclusively) end with the assignment operator, but it feels like we keep solving these situations by peeking into the rhs and attaching the comments between the assignment op and the rhs to the lhs because these util functions are dropping those comments. This has almost certainly resulted in some duplicated efforts throughout the codebase.

What if we created a new function (something along the lines of rewrite_assign_rhs_with_comments) that was basically the same as rewrite_assign_rhs_with but would take some additional params, like the "between" span (the one between the assignment op and the expr lo), perhaps the RHS tactics, and anything else it needed so that it could actually be responsible to incorporating these comments.

The logic conditions would be basically the same as what I gathered you were doing here, though do keep in mind that it's preferable if rustfmt can preserve comment-associated line breaks so

const B: usize =   
/*    Some constant */
3;

would ideally be formatted as

const B: usize =   
    /*    Some constant */
    3;

while

const C :   usize   
      = /* foo */5;

would be:

const C: usize = /* foo */ 5;

(this is where combine_strs_with_missing_comments really shines!)

Think we should be able to achieve that pretty easily solely within this new function by:

• check if there's a comment (and if so newlines) within the "between" snippet, and if so block indent the shape (note there's a contains_comment helper function that should be used for this)
• get the rhs via the same rewrite_assign_rhs_expr process just like rewrite_assign_rhs_with currently does
• if there were no comments then can return the same Some(lhs + rhs)
• otherwise return combine_strs_with_missing_comments with a trimming of the start of rhs (since combine_strs_with_missing_comments will take care of this)

Then the only other change needed to fix the static item bug would be to switch the call from rewrite_assign_rhs to this new rewrite_assign_rhs_with_comments function (or whatever this hypothetical function would be named), and pass it the span between the = and expr lo.

What do you think?

[whizsid]

@calebcartwright It is a really good idea to implement a new function. I faced a problem when implementing it. choose_rhs is always prepending whitespace. I have trimmed the start of rhs. But it is not an idiomatic way.

[calebcartwright]

choose_rhs is always prepending whitespace. I have trimmed the start of rhs. But it is not an idiomatic way.

Could you elaborate on what you mean? There's quite a lot of trimming done within rustfmt, both start and end, in various places to ensure proper formatting and alignment, so I'd push back against calling the usage of a trim function as non-idiomatic.

That being said, ideally we wouldn't prepend the leading space on the rhs unnecessarily in the first place only to peel it off later (though again there's quite a few instances of this type of workflow within rustfmt). It'd take quite a bit of additional refactoring to be able to achieve that goal though, so I'd prefer to defer that to future PRs.

[calebcartwright]

Awesome, thank you @whizsid. The approach definitely seems like it'll work and the PR is in pretty good shape! Couple minor inline changes requested along with a few additional test scenarios, but should be good to merge after those are addressed

[whizsid]

@calebcartwright I have changed the code as you guided. Can you review it again?

[calebcartwright]

Awesome, thanks so much!

[karyon]

Looks like this was backported in #4632.