Remove extra whitespace after match arrow

[mujpao]

Fixes #4844

When using control_brace_style = 'AlwaysNextLine' and match_arm_blocks = false, rustfmt would add extra whitespace if the match body didn't fit on one line.

This adds a check for this case.

[ytmimi]

Thanks for the PR!

From my initial look at the proposed changes, things look good to me! From the linked issue, I understand that this bug only occurs when control_brace_style = 'AlwaysNextLine' is used with match_arm_blocks = false. I do wonder if it's worthwhile to test different combinations of control_brace_style and match_arm_blocks, but I realize that's probably outside the scope of the issue you're solving here.

I do want to call out that there seems to be an open issue (#4896) and PR (#4924) that would move to deprecate match_arm_blocks, but there hasn't been progress on the PR since Sep 08, 2021.

[mujpao]

I could add some more tests. Even if match_arm_blocks gets deprecated, the tests could still be relevant, right?

I was just looking through the issues, and I realized that #4844 is probably a duplicate of #4003.

Also, I noticed that rustfmt will preserve a newline between match arms, like this (without any options):

fn main() {
    let fooooooo = "100000000000000000000000";
    let _bar = match fooooooo {
        "100000000000000000000000" => {
            fooooooo.len() == 1 && fooooooo.contains("222222222222222222")
        }

        _ => unreachable!("Should not happen"),
    };
}

Is this what's supposed to happen?

[ytmimi]

I could add some more tests. Even if match_arm_blocks gets deprecated, the tests could still be relevant, right?

I'm personally a fan of having more tests, so I'd encourage you to add more, but as I mentioned, I wouldn't want you to have to test things that aren't related to the issue.

I was just looking through the issues, and I realized that 4844 is probably a duplicate of 4003.

Good catch on noticing the duplicate issue!

Also, I noticed that rustfmt will preserve a newline between match arms, like this (without any options): Is this what's supposed to happen?

That's a really good question. I'm not as familiar with the match arm formatting, but I'd assume that there shouldn't be a newline between match arms. @calebcartwright, do you have a better idea on whether this is the intended behavior or not?

[mujpao]

When adding more tests, should I force push or add another commit?

[calebcartwright]

Is this what's supposed to happen? I'd assume that there shouldn't be a newline between match arms. @calebcartwright, do you have a better idea on whether this is the intended behavior or not?

tbh I wouldn't have expected it to preserve a newline between arms in cases where there was one or more there before. however, the relevant section in the Style Guide doesn't provide any explicit guidance so this is definitely a case where we want to stick with the status quo to avoid introducing any "breaking" formatting changes

[calebcartwright]

When adding more tests, should I force push or add another commit?

Great question, thanks for asking! In this specific case, it's up to you.

In general, I strongly dislike merge commits so outside the subtree syncs (which require merge commits), all PRs are merged with a rebase or squash strategy.With a rebase strategy, that requires the commits be well structured, an admittedly vague/abstract definition :smile: It's fine for a PR to have multiple commits provided they are logically structured (e.g. not a 7 commit PR for a 5 line diff), and that structure can always be discussed in the context of a specific PR. And I can always do a squash merge in cases where I feel like the PR commits can be condensed into a single one.