Formating of `match` should consider consistency across match arms

[RalfJung]

The current style guide for match has some rather unfortunate consequences. rustfmt will turn

pat =>
    very_long_expr,
pat =>
    short_expr

into

pat =>
    very_long_expr,
pat => short_expr

or turn

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

into

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

This can make a match that is entirely symmetric (every pattern and arm is of the same shape) look non-symmetric, making code harder to read since the reader cannot make use of the symmetry. Putting expr on the same line as pat can also make it easy to miss that there is any expression there at all.

A practical example of this looks as follows:

            LocalValue::Dead => {
                // Lots of code here.
            }
            ref mut local @ LocalValue::Live(Operand::Immediate(_))
            | ref mut local @ LocalValue::Uninitialized => Ok(Ok(local)),
            LocalValue::SomeOtherVariant => {
                // Lots of code here.
            }

It is hard to even see the code for the final arm since it got put on the same line as the pattern, making it look like the two large code blocks are the only actual code here.

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

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

[nielsle]

Hehe. I asked about this in 2017 (But I don't have strong opinions)

#34 (comment)

[kpreid]

A couple of thoughts. First, note that adding braces or line breaks for consistency might also harm readability. Consider:

match expr {
    Expr::Add(l, r) => eval(l)? + eval(r)?,
    Expr::Sub(l, r) => eval(l)? - eval(r)?,
    Expr::Mul(l, r) => eval(l)? * eval(r)?,
    Expr::Div(l, r) => eval(l)? / eval(r)?,
    // ...continue for more similarly trivial variants...
    Expr::Call(f, args) => {
        let f = eval(f)?;
        let args = args.iter().map(eval).collect()?;
        match call_function(f, args) {
            ...
        }
    }
}

I would say that this code is good as is and the short arms should not be made bigger just for visual similarity with Expr::Call; that would remove the tabularity of the short arms and make them 2 times longer (with indented form) or 3 times longer (with braces).

---

Second, in the code sample from above,

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

it seems to me that this arm is poorly formatted even in isolation, and the expression would be better put on a separate line to make it more distinguishable for the pattern. I don’t know what would make a good algorithmic rule to fix it, though. Limiting the maximum width by which a pattern can “overhang” the following =>?

[RalfJung]

A couple of thoughts. First, note that adding braces or line breaks for consistency might also harm readability.

Yeah, agreed. I think generally the proper solution here is for rustfmt to better respect the choices of the human programmer: if an arm has curly braces, don't remove them. If an arm is put onto its own line (but not within braces), do not move it up right after the => even if that would fit. That way I can better ensure consistency myself.

I don’t know what would make a good algorithmic rule to fix it, though.

If the pattern takes more than one line, I think the arm itself should always be on a separate line:

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

Personally I'd also force this to be enclosed in braces.

[harpocrates]

Note that _enforcing _symmetry might mean that small changes to one arm in the match might trigger re-formatting that would affect the entire match. That's an undesirable property from the perspective of maintaining a good git blame. (This is in a way quite similar to wanting => to line up - it is aesthetic but means that touching code in one arm might trigger a reformat of the entire match).

Just preserving the code style that was written (ex. leave the braces if added) doesn't have this shortcoming, though it takes away from rustfmt helping "normalize" code into a single format.

[traviscross]

We talked about this today in the style call, and here's where we landed, after working through a number of examples:

• We don't want to have a ripple effect, so we don't want to conditionally format one arm based on how another arm is formatted.
• We don't want to preserve whether the user used braces or not; it's useful for rustfmt to be able to add or remove braces.
• We might want to always break a match arm body onto a new line if the pattern spans multiple lines (and thus always use braces for such a match arm, given the default setting of match_arm_blocks). We will need to evaluate this to make sure it doesn't have an adverse impact on codebases.
• We might want to say that whenever we wrap the { to a new line, we always put the => on the same line as the { rather than hanging it on the end of the previous line. In other words, "never break between the => and the {".
• We might want to break the => { onto a new line if the pattern spans multiple lines and has indented continuation lines (because it has an if guard).

Unrelated, but discovered while experimenting with rustfmt:

• We should never break between the ( and ) of a nullary function call, or of a unit value ().

That still has some "might"s in it, so we'll think those over and review any feedback here, and then we'll propose this for FCP.

[calebcartwright]

I wasn't able to attend the meeting today, but I'd wanted to add a few thoughts on the topic:

The existing style guide text does already prescribe a functional "preserve braces if present" but only in one very narrow case https://doc.rust-lang.org/nightly/style-guide/expressions.html#match

If the right-hand side consists of multiple statements, or has line comments, or the start of the line does not fit on the same line as the left-hand side, use a block. Do not flatten a right-hand side block containing a single macro call because its expanded form could contain a trailing semicolon.

Additionally, while there are some formatting elements for which the style guide doesn't have any explicit prescriptions, I think whether match arm bodies should have braces is too significant and common of a consideration to be elided from the style guide entirely. The style guide is also intended for human consumption including covering the coding bootstrap case (i.e. how they should write new code that doesn't yet exist) as well as for formatters to ensure existing code adheres to an expected style. As such I don't think the style guide could completely punt on the question of when/if to use braces and have the default be "let the developer do what they want with braces and don't change it"

Formatters can certainly add optional support to preserve braces (as rustfmt has already done for leading pipes on match arms), but I think the guide needs to continue to have explicit prescriptions on if/when to use braces, be that the current prescriptions or some new enhanced version of them.

[RalfJung]

We don't want to preserve whether the user used braces or not; it's useful for rustfmt to be able to add or remove braces.

I think with this decision, match formatting will remain poor. There's no way to have all of "no ripple effect" and "rustfmt strictly enforces normalization" and "consistent formatting for similar-looking match arms".

I agree it is useful for rustfmt to be able to add or remove braces, but it is not useful enough to justify the poor formatting of symmetric match blocks that rustfmt produces today, and I think the formatting you propose will do little to improve that situation.

I think the guide needs to continue to have explicit prescriptions on if/when to use braces, be that the current prescriptions or some new enhanced version of them.

The style guide need not be fully algorithmic though, right? So it could say something like, use braces if ..., or if that serves to format a sequence of very similar match arms in a consistent way.

[calebcartwright]

The style guide need not be fully algorithmic though, right? So it could say something like, use braces if ..

Not sure I understand what you mean by algorithmic in this particular context as the guide is essentially a prose form of many "if this then that" rules. It could certainly prescribe the behavior you seek via some "if any arm body must have braces then use braces on all arms".

My point was simply that in my view the guide should not defer the question of braces to the programmer via some simple "never remove any existing braces" type of rule

[traviscross]

• We might want to always break a match arm body onto a new line if the pattern spans multiple lines (and thus always use braces for such a match arm, given the default setting of match_arm_blocks). We will need to evaluate this to make sure it doesn't have an adverse impact on codebases.

In reviewing a PR, I found an example of where I think one could debate about whether the result of this is an improvement. It's here:

fn has_implicit_borrow(Adjustment { kind, .. }: &Adjustment<'_>) -> Option<Mutability> {
    match kind {
        &Adjust::Deref(Some(OverloadedDeref { mutbl, .. })) => Some(mutbl),
        &Adjust::Borrow(AutoBorrow::Ref(mutbl)) => Some(mutbl.into()),
        Adjust::NeverToAny
        | Adjust::Pointer(..)
        | Adjust::ReborrowPin(..)
        | Adjust::Deref(None)
        | Adjust::Borrow(AutoBorrow::RawPtr(..)) => None,
    }
}

With the rule, we'd see instead:

fn has_implicit_borrow(Adjustment { kind, .. }: &Adjustment<'_>) -> Option<Mutability> {
    match kind {
        &Adjust::Deref(Some(OverloadedDeref { mutbl, .. })) => Some(mutbl),
        &Adjust::Borrow(AutoBorrow::Ref(mutbl)) => Some(mutbl.into()),
        Adjust::NeverToAny
        | Adjust::Pointer(..)
        | Adjust::ReborrowPin(..)
        | Adjust::Deref(None)
        | Adjust::Borrow(AutoBorrow::RawPtr(..)) => {
            None
        }
    }
}

That still seems OK to me, but I could see why opinions would differ there.

[traviscross]

There's another bit of inconsistency in that PR that is interesting, apropos of this. The PR author had written:

fn peel_place_mappers<'tcx>(mut expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
    loop {
        match expr.kind {
            ExprKind::Index(base, _idx, _) => {
                expr = &base;
            }
            ExprKind::Field(e, _) => expr = &e,
            _ => break expr,
        }
    }
}

Presumably the author meant to write:

fn peel_place_mappers<'tcx>(mut expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
    loop {
        match expr.kind {
            ExprKind::Index(base, _idx, _) => expr = &base,
            ExprKind::Field(e, _) => expr = &e,
            _ => break expr,
        }
    }
}

But without manually removing that ;, that doesn't happen, so it has to be caught by humans in review, or else we end up with needless inconsistency across match arms.

[RalfJung]

In reviewing a PR, I found an example of where I think one could debate about whether the result of this is an improvement. It's here:

When patterns are so dominant that it's easy to miss the code, I would actually prefer this:

fn has_implicit_borrow(Adjustment { kind, .. }: &Adjustment<'_>) -> Option<Mutability> {
    match kind {
        &Adjust::Deref(Some(OverloadedDeref { mutbl, .. })) => {
            Some(mutbl)
        }
        &Adjust::Borrow(AutoBorrow::Ref(mutbl)) => {
            Some(mutbl.into())
        }
        Adjust::NeverToAny
        | Adjust::Pointer(..)
        | Adjust::ReborrowPin(..)
        | Adjust::Deref(None)
        | Adjust::Borrow(AutoBorrow::RawPtr(..)) => {
            None
        }
    }
}

Now you can easily tell that there's 3 cases, and what the code is for each of them. Both the versions you showed make that non-trivial to see.

At the very least, I'd like rustfmt to not remove any of these braces if I manually chose to format the match like this.

[joshtriplett]

We re-discussed this in today's style meeting, and confirmed that we want to maintain the policy of not having ripple effects like this, as well as continuing to have the default style determine whether to use braces or not. So, in response to the original request, of consistency across match arms:

@rfcbot close

That said, we absolutely think that some of the given examples do not have optimal formatting, so we're going to open a separate issue to improve the formatting of match arms in a future edition, based on the summary @traviscross posted.