Do not put if on the same line as match arm

[topecongiro]

cc rust-lang/style-team#61, #1791.

Please feel free to keep this open and umerged if necessary.

[topecongiro]

@nrc Do you have any thoughts on this PR?

[nrc]

Hmm, I'm not sure, and I don't think we really decided on the style team. @rust-lang-nursery/style what do you think? (I found it useful to look at the second commit to get a feel for the change).

[joshtriplett]

Honestly, both approaches seem fine to me. I don't have a problem with the previous formatting, and in shorter cases it looks better to me, while in longer cases it doesn't.

I think this ties back into some of the discussions about when to fold things onto the same line, regarding issues like prefixes and suffixes, and ease of scanning.

[ghost]

For me, the previous formatting is in some cases definitely nicer and more concise. The new formatting is obviously more verbose and induces unfortunate rightward drift, but at the same time I find it to be a net win when it comes to readability and writeability.

Readability

The following, in my opinion, is quite unreadable. I've been living with this style for quite a while now, and still haven't been able to get accustomed to it.

match part {
    NumberPart::OpenParen => if c != '(' {
        return IssueClassification::Bad(issue);
    } else {
        part = NumberPart::Pound;
    },
    NumberPart::Pound => if c == '#' {
        part = NumberPart::Number;
    },
    NumberPart::Number => if c >= '0' && c <= '9' {
        part = NumberPart::CloseParen;
    } else {
        return IssueClassification::Bad(issue);
    },
    NumberPart::CloseParen => {}
}

When quickly skimming over the code, my mind just can't decide which lines are pattern-matching, and which lines are else if/else blocks. Even this example is not that bad because it doesn't have too much code, but when pattern-matching over the current state in a complex state machine, it easily gets out of hand.

On the other hand, simpler cases (e.g. those with a single if without else) are perfectly readable, though:

match lorem {
    Lorem::Ipsum => if ipsum {
        println!("dolor");
    },
    Lorem::Dolor => println!("amet"),
}

This looks better than the newly proposed style (concise and no rightward drift!). However, I think it is still a net loss because it impairs writeability. Hear me out.

Writeability

Suppose that we want to print the value of ipsum before the if:

match lorem {
    Lorem::Ipsum => {
        debug!("Matched on the Ipsum case. ipsum = {}", ipsum);
        if ipsum {
            println!("dolor");
        }
    },
    Lorem::Dolor => println!("amet"),
}

This is a lot of manual work to add just a single debug! statement, not to mention the sudden visual change. You have to manually add braces, make sure you don't mess up the comma (the closing if-brace had a trailing coma before, but now it doesn't!), and reindent the code (either manually or by hitting Ctrl+S or whatever to make rustfmt kick in).

However, if we had started with what this pull request is proposing, adding a simple debug statement would be a trivial visual change and a trivial change when it comes to typing code:

match lorem {
    Lorem::Ipsum => {
        if ipsum {
            println!("dolor");
        }
    }
    Lorem::Dolor => println!("amet"),
}

Ergonomics and formatting

With all the exciting ergonomics improvements that are coming to Rust this year, I die a little inside when a simple formatting rule adds unnecessary friction to quick debugging and prototyping. :)

While it's clearly visible how this pull request affects readability of code, please also consider the angle of writeability. Rustfmt is an important tool in making code in Rust easier to write, and I think this pull request will push it a little bit further in that direction.

[topecongiro]

I rebased and resolved conflicts. As @stjepang nicely stated in the comment, I think the benefit outweighs the verbosity here.

[nrc]

Hmm, I'm still not convinced by the arguments in favour of this change. In particular the change from a non-block to a block match arm happens all the time with lots of different expressions, and this just tackles one kind of those expression, and not one which is particularly common, so I don't put much weight on that.

So this comes down to whether we find the compressed version or brace-happy version more readable. Here, I agree with @stepancheg I think this change makes large blocks better, and small ones worse. I personally come down weakly on the side of not making the change, but that is pure subjective opinion. So, let's and this for now and see how we feel about it in practice (I often feel better once I'm familiar with a new style, and/or others may like it less).

[ghost]

@nrc

In particular the change from a non-block to a block match arm happens all the time with lots of different expressions, and this just tackles one kind of those expression, and not one which is particularly common, so I don't put much weight on that.

It's definitely subjective - hard to say why, but the match { pattern => if condition { combination is the only one that I personally find uncomfortable in practice. It could just be the codebases I work with, dunno (it's crossbeam-epoch in particular).

I feel I might have come off somewhat strongly opinionated on this particular formatting style, so apologies if I'm getting tiresome. Just to fully clarify, I'd be okay with some kind of compromise if you decide to revert this style in the end. But if there is one thing I'm really strongly against, it would be this:

match foo {
    pattern => if cond {
        // ...
    } else {
        // ...
    }
}

Everything else is tolerable. Even this example would look okay without the else block, as far as I'm concerned.

Thanks for giving this style a shot!