Improvements to match formatting

[traviscross]

As originally discussed and described in #196 (comment), we want to do the following:

• We 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 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 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).

cc @rust-lang/style

[traviscross]

@rfcbot fcp merge

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @calebcartwright
• @joshtriplett
• @traviscross

Concerns:

• open-question resolved by Improvements to match formatting #211 (comment)
• the-caleb-example (Improvements to match formatting #211 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @traviscross, I wasn't able to add the final-comment-period label, please do so.

[calebcartwright]

Can we specify the exact change we're proposing?

i.e. current style guide rules where there's a break between the => and opening brace is:

match x {
    a_very_long_pattern | another_pattern
        if expr =>
    {
        ...
    }
}

so are we saying:

// 1.
match x {
    a_very_long_pattern | another_pattern
        if expr
    => {
        ...
    }
}

// 2.
match x {
    a_very_long_pattern | another_pattern
        if expr => {
        ...
    }
}

and if the latter, then what's the prescription for the case of the guard expression being long/multilined?

[calebcartwright]

@rfcbot concern open-question

[joshtriplett]

@calebcartwright The proposal is 1, not 2.

[calebcartwright]

We 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).

And based on this, the proposal also includes changing this:

match foo {
    foo => bar,
    a_very_long_pattern
    | another_pattern
    | yet_another_pattern
    | a_fourth_pattern => {
        ...
    }
}

to the below?

match foo {
    foo => bar,
    a_very_long_pattern
    | another_pattern
    | yet_another_pattern
    | a_fourth_pattern
    => {
        ...
    }
}

[traviscross]

No, I don't think we said that. There are no indented continuation lines there.

[calebcartwright]

Thank you both, and should note I'm just trying to ensure I understand the specifics (not nitpicking) as we will need to incorporate those specifics into the style guide as a next step.

The "pattern spans multiple lines" phrasing in the third bullet of the proposal was a bit unclear, as it seems we want to apply the change on the example I used in #211 (comment) even though this is a case where the pattern itself doesn't span multiple lines.

Does the "indentend continuation lines" phrasing mean to cover any and all situations where there's at least one indented line in the guard, or only where there's an indent before the if?

E.g. with this input:

fn main() {
    match x {
        Token::Dimension {
            value, ref unit, ..
        } if foo(
                "long long long long long",
                "long looooonnnngggggg",
                "foo bar baz",
            ) => {
            //  ...
        }
    }
}

Would the expectation be:

fn main() {
    // 1.
    match x {
        Token::Dimension {
            value, ref unit, ..
        } if foo(
            "long long long long long",
            "long looooonnnngggggg",
            "foo bar baz",
        ) => {
            //  ...
        }
    }


    // 2.
    match x {
        Token::Dimension {
            value, ref unit, ..
        } if foo(
            "long long long long long",
            "long looooonnnngggggg",
            "foo bar baz",
        )
        => {
            //  ...
        }
    }
}

(i.e. where the pattern is multilined and has a guard that is able to start on the same line as the closing token of the pattern, but where the guard itself has to be multilined and indented)

[traviscross]

Yes, good question. I had similarly been thinking of the effect of guard patterns when we do those.

The language I had used, in our discussion was as follows. Let's say we have:

fn main() {
    if let x = get_thing(1)
        && x > 0 // <-- Marked line.
    {
        ..
    }
}

What I would say is that the marked line has consumed that indentation level. By that I mean that its use of it prevents us from doing this:

fn main() {
    if let x = get_thing(1)
        && x > 0 {
        println!("Hello world"); // <--- Bad.
    }
}

And so what we had meant to say above, in context, I think, is that when the pattern has consumed the next indentation level in that way, such that if we had to break and write a block that we'd need some separation there, that we should just go ahead and break. That is, we didn't want:

fn main() {
    match 0u8 {
        00000000000000000000000000000000000000000000000000042
        | 00000000000000000000000000000000000000000000041
            if 1 > 0 => (),
        _ => (),
    };
}

But looking at it now, I'm not sure rustfmt will ever actually do anything like that, because we indeed format that as:

fn main() {
    match 0u8 {
        00000000000000000000000000000000000000000000000000042
        | 00000000000000000000000000000000000000000000041
            if 1 > 0 =>
        {
            ()
        }
        _ => (),
    };
}

So maybe that bullet was really just a kind of restatement of the two before it, in that we want to format that case as:

fn main() {
    match 0u8 {
        00000000000000000000000000000000000000000000000000042
        | 00000000000000000000000000000000000000000000041
            if 1 > 0
        => {
            ()
        }
        _ => (),
    };
}

To your question specifically, I think we'd also want this formatting:

fn main() {
    match 0u8 {
        00000000000000000000000000000000000000000000000000042
        | 00000000000000000000000000000000000000000000041
            if f(
                000000000000000000000000000000000001,
                00000000000000000000002,
            ) == 1
        => {
            ()
        }
        _ => (),
    };
}

Maybe @joshtriplett remembers other details.

[calebcartwright]

This was discussed at the t-style meeting last week with some additional review and clarification provided.
With that clarity I'll now resolve the concern so that this can advance.

@rfcbot resolve open-question

However, I've spent some time processing this but still can't convince myself to check my box to explicitly endorse this change as I don't view these pending changes as objectively better in all cases.

For example, this prior formatting:

fn main() {
    match x {
        Token::Dimension {
            value, ref unit, ..
        } => foo,
        _ => bar,
    }
}

would change to require braces around the body of the first arm because the pattern has to be formatted across multiple lines:

fn main() {
    match x {
        Token::Dimension {
            value, ref unit, ..
        } => {
            foo
        }
        _ => bar,
    }
}

and I find this to be less readable based on how I visually scan arms (I just find I more quickly grok the logic and flow of arms with short/succinct bodies when they don't have braces and the associated line breaks)

By the same token, I'm not fully supportive of the change to break before the => operator because I find the appearance of the => at the arm starting indentation level to be visually jarring (this admittedly may just be an unconscious status quo bias and something to which I'd eventually get accustomed though). There are other contexts where existing formatting around edge cases results in an opening brace appearing on its own, and the current/prior behavior for this edge case on match arms felt "familiar"

Just my .02 and why I don't fully support these changes, even though I recognize the majority of the team wants to move ahead with them