Improve formatting of pattern alternatives

[RalfJung]

I found no mention in the spec of how to format pattern alternatives like A | B | C => expr, so I have to assume that what rustfmt does there is some ad-hoc decision made during implementation. I think that formatting should be given some thought. I am not sure where to start with that, so I figured I'd open an issue. ;)

Some of the problems that we currently have:

• With long lists of patterns (that do not fit on a single line), we end up with code like this:

Pat1
| Pat2
| Pat3
| Pat4 => ...

Notice how the patterns start at different vertical positions, destroying symmetry. This is unlike how every other "list-like thing" is formatted, which all are properly vertically aligned. Also see #3973.

• With long matches, rustfmt can end up generating code 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)
            }

Notice how the two lists of alternatives are formatted very differently. This should be done in a more consistent way. Just like #3995 is about formatting the match arms themselves consistent across a match, I think rustfmt should also format lists of pattern alternatives consistent across a match.

Things get even more complicated when considering match guards... but one step after the other, I'd say. ;)

[topecongiro]

With long lists of patterns (that do not fit on a single line), we end up with code like this:...

This was discussed in rust-lang/style-team#34 and ended up with a conclusion that rustfmt puts | in front.

With long matches, rustfmt can end up generating code like this:...

I agree.

[RalfJung]

This was discussed in rust-lang/style-team#34 and ended up with a conclusion that rustfmt puts | in front.

Thanks for the link! I don't see much discussion there though (in terms of pros/cons), just the original proposal of putting them after the pattern and one statement that they prefer the front. (There was some follow-up discussion about binary operators, but | in patterns is not really the same beast so I am not sure what to make of that.)

In #1129, a good argument was brought for something with trailing pipes that looks bad:

fn foo() {
        match *self {
            NotInRegistry(..) |
            FailedToDownload(..) |
            TestFail(..) |
            BuildFail(..) => None,
            FailedToDownload(_, ref e) => Some(&**e),
        }
}

However, I'd say the failure here is to put the code on the line of the =>. So the better version would be:

fn foo() {
        match *self {
            NotInRegistry(..) |
            FailedToDownload(..) |
            TestFail(..) |
            BuildFail(..) =>
                None,
            FailedToDownload(_, ref e) =>
                Some(&**e),
        }
}

This again makes it visually clear what's a pattern and what is not. In contrast, the way this gets formatted today is

fn foo() {
        match *self {
            NotInRegistry(..)
            | FailedToDownload(..)
            | TestFail(..)
            | BuildFail(..) => None,
            FailedToDownload(_, ref e) => Some(&**e),
        }
}

This is visually much less clear, as the code and the pattern isn't clearly separated. The only hint is the fact that the last line doesn't start with a |.

Also there is an alternative today that might not have existed yet when the discussion took place:

fn foo() {
        match *self {
            | NotInRegistry(..)
            | FailedToDownload(..)
            | TestFail(..)
            | BuildFail(..) => None,
            FailedToDownload(_, ref e) => Some(&**e),
        }
}

This achieves "all patterns that go together start in the same column", but now if the last branch also has two alternatives, it's again hard to tell that there are multiple cases.

My personal favorite is pipe-after-pattern-and-newline-after-=>.

[RalfJung]

Oh, I missed the discussion starting at rust-lang/style-team#34 (comment). The main argument is the one from #1129 though, from what I can see, so I maintain that that is better solved by adding a newline after =>.

[topecongiro]

Thank you for the detailed explanation!

My personal favorite is pipe-after-pattern-and-newline-after-=>.

IMHO whether to put pipes before or after patterns is a matter of preference, and hence we are unlikely to change it unless there are any serious readability concerns, sorry.

For arm bodies, I would add a block to each arm body rather than just putting them on their own line, but other than that, I agree with your idea.

fn foo() {
        match *self {
            NotInRegistry(..)
            | FailedToDownload(..)
            | TestFail(..)
            | BuildFail(..) => {
                None
            }
            FailedToDownload(_, ref e) => {
                Some(&**e)
            }
        }
}

[RalfJung]

IMHO whether to put pipes before or after patterns is a matter of preference, and hence we are unlikely to change it unless there are any serious readability concerns, sorry.

I can see that. It is a shame though that it should be impossible to consistently have all patterns start on the same column. Maybe rustfmt could preserve a leading pipe before the first pattern if it exists (Cc #3973)?

Having the pipe at the end results in the nice situation that all patterns start on the same column, and there's always a symbol after the pattern in each line (| or =>). It's very symmetric. But I probably would need a time machine to make that point before tho ecosystem was already converted to pipes-before-patterns... :/

For arm bodies, I would add a block to each arm body rather than just putting them on their own line, but other than that, I agree with your idea.

I think blocks for one-line arms that don't need them are just a waste of space... but we already have match_arm_blocks = false to control exactly that. ;)

[GoldsteinE]

I’ve encountered this case:

pub fn foo(s: &str) {
    match s {
        id @ ("foo" | "bar" | "bazbazbaz" | "qwex" | "lol" | "ttt" | "roflorlfo" | "hiiii"
        | "uwu" | "owo") => println!("lol: {id}"),
        _ => panic!(),
    }
}

which IMHO looks kind of weird. Generally when rustfmt inserts line breaks in the middle of an expression, the next line is indented to show that it’s a continuation. In this case it’s not.