Skip to content

rustfmt formats different arms of the same match inconsistently #3995

Open
@RalfJung

Description

@RalfJung
Member

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.

Activity

RalfJung

RalfJung commented on Jan 3, 2020

@RalfJung
MemberAuthor

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.

changed the title [-]Don't put code into the line after `=>` after a multi-line pattern[/-] [+]rustfmt puts too much code into the line after `=>`[/+] on Jan 3, 2020
RalfJung

RalfJung commented on Jan 3, 2020

@RalfJung
MemberAuthor

(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

RalfJung commented on Jan 3, 2020

@RalfJung
MemberAuthor

(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

RalfJung commented on Jan 3, 2020

@RalfJung
MemberAuthor

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

varkor commented on Jan 4, 2020

@varkor
Member

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

RalfJung commented on Jan 8, 2020

@RalfJung
MemberAuthor

@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

RalfJung commented on Jan 8, 2020

@RalfJung
MemberAuthor

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.

changed the title [-]rustfmt puts too much code into the line after `=>`[/-] [+]rustfmt formats different arms of the same match inconsistently[/+] on Jan 8, 2020
RalfJung

RalfJung commented on Jan 8, 2020

@RalfJung
MemberAuthor

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.

16 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @RalfJung@stephanemagnenat@TheDudeFromCI@varkor@calebcartwright

        Issue actions

          rustfmt formats different arms of the same match inconsistently · Issue #3995 · rust-lang/rustfmt