New config options for match arm block wrapping

[ashvin021]

This PR adds a new config option for controlling block wrapping in match arms as below:

pub enum MatchArmWrapping {
    /// Follow the Style Guide Prescription
    Default,
    /// Same as Default, except don't block wrap match arms when the opening line of its body
    /// can't fit on the same line as the `=>`.
    FitFirstLine,
    /// Always block wrap match arms
    Always,
    /// Preserve the block wrapping on match arms
    Preserve,
    /// Same as Default, except wrap the match arm if the entire body cannot fit on the same line
    /// as the `=>`.
    FitEntireBody,
}

This also soft deprecates match_arm_blocks, and remaps it to variants of the new config option.

Resolves #4896

[calebcartwright]

Thanks for this. I've updated the target branch to reflect some things that have happened in this repo since you created your fork.

If you could rebase the topic branch of your fork on the master branch in this repo that'd be helpful

You likely already know how to do that, but just in case something like the below should work

git remote add upstream https://github.com/rust-lang/rustfmt
git fetch upstream
git rebase upstream/master
git push --force

[ashvin021]

Done :) Hope this works, let me know if there's anything else to be done.

[ashvin021]

Sorry about the confusion, I think it's all in sync now. Turns out I'd mistakenly pulled an old version of upstream/master into the feature branch while working on it earlier lol.

[calebcartwright]

Sorry about the confusion, I think it's all in sync now. Turns out I'd mistakenly pulled an old version of upstream/master into the feature branch while working on it earlier lol.

No worries, glad it's all sorted now. It'll probably be a little while before I can give this a thorough review, so thanks in advance for your patience!

[calebcartwright]

I'm still digesting this one as I haven't had a chance to go through in detail, but I love what I'm seeing with the test cases! The empty block body/empty tuple for an arm is an interesting scenario I hadn't really considered before but it will be an important one :thinking:

[calebcartwright]

@RalfJung - not urgent but would love to get your thoughts on an aspect of this given some of your past feedback and requests for more control over how match expressions are formatted (this particular new option being focused on if/when/how arms get block wrapped)

The specific piece I'm wondering about is how to best handle empty arm bodies, including when the input source contains both implicit or explicit empty tuples.

It becomes relevant for some of the variants of this new option, including the 'Always' variant being initially added as well as the potential future variant that would be based on the match more holistically (e.g. if any arm needs to be wrapped then wrap all and vice versa)

For example, using the following snippet as an example, how much flexibility do we think folks would need over the first arm in such scenarios?

// always block wrap or wrap consistently when needed
match x {
    Foo::Bar => (),
    Foo::Baz => {
        // abcdefg
        do_stuff()
    }
    Foo::Qux => other_stuff(),
}

We could go rather verbose, e.g.:

// always block wrap or wrap consistently
match x {
    Foo::Bar => {
        ()
    }
    Foo::Baz => {
        // abcdefg
        do_stuff()
    }
    Foo::Qux => {
        other_stuff()
    }
}

Or we could treat empty arm bodies differently/separately and support leaving them as-is even when wrapping/unwrapping behavior is needed on other arms:

match x {
    Foo::Bar => (),
    Foo::Baz => {
        // abcdefg
        do_stuff()
    }
    Foo::Qux => {
        other_stuff()
    }
}

Or we could fan out the variants of our config option to support both if we think there's sufficient desire for both flavors. (I realize we could also convert between explicit and implicit as well, but leaving that out for now as we're likely to introduce a separate config option that allows users to control that behavior and we'll need to make sure this config option plays nice with that one)

[RalfJung]

Hm, that's a good question. FWIW I have not seen such empty bodies in code I work on. I would definitely prefer to write this as {} rather than () though. So my initial gut feeling is to prefer

match x {
    Foo::Bar => {},
    Foo::Baz => {
        // abcdefg
        do_stuff()
    }
    Foo::Qux => {
        other_stuff()
    }
}

That is consistent with saying there are braces on each match arm. My only concern is that there is a slight risk of missing that the first arm does nothing though, since on first glance this looks fair similar to

match x {
    Foo::Bar |
    Foo::Baz => {
        // abcdefg
        do_stuff()
    }
    Foo::Qux => {
        other_stuff()
    }
}

That could be avoided with

match x {
    Foo::Bar => {
    },
    Foo::Baz => {
        // abcdefg
        do_stuff()
    }
    Foo::Qux => {
        other_stuff()
    }
}

but that looks really awkward to my eyes...

[calebcartwright]

Exactly, and I'm a bit torn as well. I'd reach for => {} for an empty arm, though I could see how some would want something more verbose for more complete visual symmetry

My only concern is that there is a slight risk of missing that the first arm does nothing though, since on first glance this looks fair similar to

I may be forgetting some new-ish options we've added for pipes on arms, but I don't think we could end up with that exact case that with a line ending with a pipe so I think that scenario (with assumed forced wrapping of the pattern and an extra arm added) would be more like:

match x {
    Foo::Xyz => {}
    Foo::Bar
    | Foo::Baz => {
        // abcdefg
        do_stuff()
    }
    Foo::Qux => {
        other_stuff()
    }
}

Does that alleviate the concern at all or do you still think the empty body arm is too visually similar to the first line of the next arm?

[RalfJung]

Does that alleviate the concern at all or do you still think the empty body arm is too visually similar to the first line of the next arm?

That alternative looks generally less visually pleasing to me because the variants Foo::Bar and Foo::Baz are not aligned, even though they are entirely symmetric.

I have used the following in some matches to mitigate this problem, but that's also not very pretty:

match x {
    Foo::Xyz => {}
    | Foo::Bar
    | Foo::Baz => {
        // abcdefg
        do_stuff()
    }
    Foo::Qux => {
        other_stuff()
    }
}

And it still leaves an asymmetry between cases, even if within each arm things are symmetric now.

But you are right that it helps with the issue I brought up previously.

[calebcartwright]

Gotcha, so with the combination of this new option plus match_arm_leading_pipes you could get rustfmt to emit the formatting in your snippet.

My inclination at this point is to just leave empty bodies alone {} or (), whichever the user wrote. I don't think perfection is a realistic goal, but I think this will help move things forward in a substantive way, and the combination of the two options provides some aid in the event there's an empty arm preceding a line wrapped multi pattern arms

If someone comes to us and explicitly asks for one of the more verbose bodies then we'll cross that bridge at that time, and do the bikeshed game with the variant names

[RalfJung]

I'm not concerned about () (I wouldn't write it and would ask for it to be changed to {} in reviews) and agree for {}.

[calebcartwright]

@ashvin021 I realize this has been sitting in limbo for a long time, but are you by chance still interested in working on this? No worries if not, we can grab the commits from your branch and pull them into a different one to both leverage what you've already done and ensure you get credit for it as well.

[ashvin021]

Yes I'm still interested, I'll make these changes soon as I get the chance. So current consensus is to leave empty / tuple bodies alone even for the Always variant, yes?

Also I wanted to ask thoughts on the naming for FitFirstLine and FitEntireBody. I was unsure since the first is "don't wrap if the first line can't fit" and the second is "wrap if the entire body can't fit", which might be confusing. Perhaps we could rename FitFirstLine to something like FitLongFirstLine or AllowLongFirstLine to make this clearer?

[calebcartwright]

So current consensus is to leave empty / tuple bodies alone even for the Always variant, yes?

Still some naming to consider, but yes for the initial variant. I'm thinking we'll want to have variants like AlwaysExceptEmpty and then some potential future Always that has to solve for the empty body question above

Also I wanted to ask thoughts on the naming for FitFirstLine and FitEntireBody. I was unsure since the first is "don't wrap if the first line can't fit" and the second is "wrap if the entire body can't fit", which might be confusing. Perhaps we could rename FitFirstLine to something like FitLongFirstLine or AllowLongFirstLine to make this clearer?

Hmm, isn't the first one more of a special case scenario for single-line bodies that can't fit? I.e. if the body is something like a multiline call, won't it still get block wrapped with FitFirstLine? If so then perhaps a tweak of one of your suggestions: AllowLongSingleLine?