rustfmt formats different arms of the same match inconsistently

[RalfJung]

The big rustc format made some code I care about much less readable. Here's an example:

-            ref mut local @ LocalValue::Live(Operand::Immediate(_)) |
-            ref mut local @ LocalValue::Uninitialized => {
-                Ok(Ok(local))
-            }
+            ref mut local @ LocalValue::Live(Operand::Immediate(_))
+            | ref mut local @ LocalValue::Uninitialized => Ok(Ok(local)),

The actual code running here for this branch got moved onto the same line as the last line of a pattern. That makes it quite hard to visually identify this code -- compared to the old version of the code, the visual separation of pattern and code got lost.

[RalfJung]

More generally, I disagree with the way rustfmt eagerly turns

pat =>
    very_long_expr,
pat =>
    short_expr

into

pat =>
    very_long_expr,
pat => short_expr

and it also turns

pat => {
  stmt;
  expr
}
pat => {
  expr
}

into

pat => {
  stmt;
  expr
}
pat => expr
// or expr might be on its own line if it is long enough

In a long match, I think it is a good idea to consistently either have pattern and expression on the same line, or on different lines, for all arms in that match. The visual similarities make it much easier to parse this code. But unfortunately, running rustfmt on such manually formatted code turns something symmetric into an inconsistent chaos. One really bad example is this: before -> after. (Yes, the write! arguments were not formatted properly before, and I am totally fine with those being formatted like rustfmt does. This is about the match arms, not the write! arguments.)

IMO, rustfmt should honor my choice to start expr in a new line, and never remove that newline. That would make it maintain good manual formatting, unlike now where it actively makes code less consistently formatted.

Alternatively, if you want to enforce some rule, my proposal would be to only ever have expr on the same line as pat if we can do this for every branch of the match (so all branches have just one pattern, and the expression is short enough). That would ensure consistency.

[RalfJung]

(Moved to separate issue: #4004)

original comment

Ah, and I just realized that yet another formatting choice of rustfmt that I strongly dislike is also related: rustfmt will turn

pat =>
    fun(
        a,
        b,
        c,
        d,
    )

into

pat => fun(
    a,
    b,
    c,
    d,
)

I think this is a total no-go. In a long match, like for example this, that means that some of the indented lines are statements but some are just function arguments. When skimming that match, it is very easy to miss the fact that there is a function call after the =>, and then nothing makes any sense.

We also don't generate code like this

if foo { fun(
    a,
    b,
    c,
    d,
)}

so why is it okay to do the equivalent things for match arms?

[RalfJung]

(Moved to separate issue: #4005)

original comment

Another way in which rustfmt introduces inconsistency into long match is by putting multiple patterns on the same line, so we end up with matches like this:

        match ty.kind {
            // Types without identity.
            ty::Bool
            | ty::Char
            | ty::Int(_)
            | ty::Uint(_)
            | ty::Float(_)
            | ty::Str
            | ty::Array(_, _)
            | ty::Slice(_)
            | ty::RawPtr(_)
            | ty::Ref(_, _, _)
            | ty::FnPtr(_)
            | ty::Never
            | ty::Tuple(_)
            | ty::Dynamic(_, _) => self.pretty_print_type(ty),

            // Placeholders (all printed as `_` to uniformize them).
            ty::Param(_) | ty::Bound(..) | ty::Placeholder(_) | ty::Infer(_) | ty::Error => {
                write!(self, "_")?;
                Ok(self)
            }

Before rustfmt ran, this was neatly formatted with one pattern per line and never mixing patterns and code on the same line. Now it's all over the place. Should I open a separate bug for this?

[RalfJung]

Here's an example diff of what rustfmt does where I disagree with every single change it is making: rust-lang/rust@ff5b102. These all introduce inconsistencies in match formatting.

And here's how I manually formatted a long match, I had to disable rustfmt for that as there seems to be no way to make rustfmt not destroy the formatting: rust-lang/rust@e44f3e7

[varkor]

Regarding the third comment, isn't this incorrect behaviour as per the specification?

I was under the impression that

pat =>
    fun(
        a,
        b,
        c,
        d,
    )

should be reformatted into

pat => {
    fun(
        a,
        b,
        c,
        d,
    )
}

as it's not a single-line expression.

[RalfJung]

@varkor good catch, I don't see any justification for putting the first line of a multi-line expression onto the line of the =>. I moved that to a separate issue: #4004.

[RalfJung]

Turns out this was already a concern back when the formatting got implemented:

we should consider wrapping either all arms in blocks or none to improve consistency within a match. Looking at the current results, I think this makes sense. It's not implemented in this PR, but I think we should have an option for this at least.

AFAIK, no such option exists yet. And indeed I agree entirely. I'd maybe add one thing: even when none of the branches are wrapped, there's still the choice of putting the expression on the same line as the =>, or into a new line. The simplest solution would be to always put it on a new line -- I think that would overall be an improvement today, though some matches would suffer. If we go with having two styles of non-wrapped match arms, then that, too, should be consistent within a match: either all arms should use the line of the =>, or none of them should.

[RalfJung]

I moved formatting of pattern alternatives to a separate issue: #4005. So this one is now just about making the body of the various match arms of the same match more consistent.

[RalfJung]

Here's another example of what I consider bad match formatting:

                        let kind = match rvalue {
                            Rvalue::Ref(_, borrow_kind, _)
                                if borrow_kind.allows_two_phase_borrow() =>
                            {
                                RetagKind::TwoPhase
                            }
                            Rvalue::AddressOf(..) => RetagKind::Raw,
                            _ => RetagKind::Default,
                        };

Notice how all match arms are just an enum constructor, but one of them gets curly braces forcibly added around it.

[scottmcm]

if we can do this for every branch of the match

👍

This is my core complaint with rustfmt, I think. It seems to regularly lose consistency between items in sequences, be that array literals, function arguments, match arms, whatever. I'd definitely like to see it make consistent choices across them, rather than the ones that today seem to be made in isolation.

[stephanemagnenat]

Any update on that? Looking at the close issue above, it seems that the question is more at the level of the style guide rather than in the implementation of rustfmt, but nonetheless as a user I find it very annoying that different arms of match get formatted differently: this breaks symmetry and leads to less readable code IMHO.

[tgross35]

Just to add a differing viewpoint - I actually prefer in many cases that rustfmt does what it currently does re: making some statements on one line vs. making all multiline. For things like Display impls on larger enums, it often seems like there are only one or two long/multiline blocks, and the rest fit on one. If having a single multiline block forced all to go multiline, vertical space of the match statement is effectively tripled.

https://github.com/pluots/zspell/blob/71ae77932f2cc67f593d846a26a9ffd7cf6d0412/crates/zspell/src/affix.rs#L276-L367 for example, I agree with what rustfmt does. 61 matches fit on 91 lines this way, but forcing them to braced multiline would make the total double to 183 lines. (could have been smaller if I used use Enum::*, but that's beside the point).

My personal rule would be that the default should be whichever matches 50% of the statements; if a majority fit on one line, make that the default and have others be an exception. If a majority take >1 line, then make them all be that way. Now of course that can't actually be a rule (code churn...) But I don't think that 1 or 2 multiline expressions should change entire large match statements (I guess that would cause churn too).

Haven't seen this discussed really though, so maybe it's not in the question anymore. Just wanted to add some 2¢.

[TheDudeFromCI]

This would be a nice feature. Match blocks have always been the one feature about rust formatting that always felt off.

[RalfJung]

I recently had a situation where I changed an enum variant and that changed formatting of all the other enum variants as well. Does that mean rustfmt already has the ability to take other variants/arms into account when formatting? I am not sure if the behavior I saw was a bug or precedent for doing something similar with match arms as well.