Fixed trailing comment being moved when comma is inserted

[vallentin]

trafficstars

Fixes #4654

The trailing comma being inserted no longer causes a subsequent comment to be moved up.

This change does not affect the trailing_comma configuration.

---

This change broke issue-3532.rs (#3532). I have modified the test case, as I thought that was appropriate.

In short, the test asserted that the comment was moved up. However, with multiple match arms, that is inconsistent formatted, as it would only do so for the last match arm. This change also fixes that, and thus I modified the test case as well.

Unformatted	Before	Now

---

Note if comment_end == ")" && !leave_last { was added, to not break foo_one_post() in issue-3198.rs (#3198) as otherwise it would add a newline along with a trailing comma.

[calebcartwright]

Thanks for the report and opening a corresponding PR!

This change broke issue-3532.rs (#3532). I have modified the test case, as I thought that was appropriate.

Will hopefully have some time over the next couple days to review the changes in more detail, though this is one minor thing we'll want to change. Could you move your enhanced version of this test case into a new separate file and leave the original source file as-is? (understand the target/output formatting for that source would be adjusted by these changes)

It'll be great to extend the test cases, but we like to keep the issue-mapped source/input files identical to what was provided in the respective issue as a general practice.

[vallentin]

@calebcartwright Thanks for the response. I'm in no hurry, so review whenever you or anybody else has time! 😄

leave the original source file

I'll move the extended version into a new file. Any ideas for a name?

Just one thing I need clarified. Do I still modify target/issue-3532.rs? As otherwise that test case would break.

https://github.com/rust-lang/rustfmt/blob/79c36960af85fa52ae4dc60ec1f83ec7f9b2733f/tests/target/issue-3532.rs#L1-L6

Because given source/issue-3532.rs as is:

https://github.com/rust-lang/rustfmt/blob/79c36960af85fa52ae4dc60ec1f83ec7f9b2733f/tests/source/issue-3532.rs#L1-L7

Then this PR changes, such that it formats into:

fn foo(a: T) {
    match a {
        1 => {}
        0 => {}
        // _ => panic!("doesn't format!"),
    }
}

[calebcartwright]

Just one thing I need clarified. Do I still modify target/issue-3532.rs? As otherwise that test case would break.

Yup, that's perfectly alright, just want to keep the source matching the issue report. Generally speaking we wouldn't expect to see the target formatting for older issues updated either, though there's certainly exceptions for that in cases where there may be a separate bug and/or subsequent formatting improvements as seems to be the case here.

I'll move the extended version into a new file. Any ideas for a name?

That doesn't matter too much. Would be alright to stick it in the new file for this latest 4654 issue, or even just plug it in the match catch-all file in https://github.com/rust-lang/rustfmt/blob/master/tests/source/match.rs

[vallentin]

I have rebased and restored source/issue-3532.rs and only fixed the affected part of target/issue-3532.rs.

I added the "extended match version" to the new test case I added.

https://github.com/rust-lang/rustfmt/blob/1d599915808ff381240d870a4eabe9a2ff79f1bd/tests/source/issue-4654.rs#L37-L44

[calebcartwright]

Note if comment_end == ")" && !leave_last { was added, to not break foo_one_post() in issue-3198.rs (#3198) as otherwise it would add a newline along with a trailing comma.

Thanks for including this note. I did a quick pass through the changes and was rather confused by this one. Something about this feels off and I'm not convinced just yet we'd want to proceed with this odd side effect handling. Perhaps some of the changes made originally to deal with #3198 need to be adjusted to account for the changes here?