Against braces always demanding rightward drift

[burdges]

There were several Rust RFCs for strange constructs combining else, loops, and match in non-traditional ways. As stated, these mostly looked questionable, but often they existed to fight extra { } and the accompanying rightward drift, which incurs costs.

Instead, I'd suggest fmt should not demand that each { } always induce rightward drift because sometimes non-drifting { } is more expressive.

In particular, there was a discussion of loop match in rust-lang/rfcs#1712 but actually the logical approach is simply to make this stylistically correct :

loop { match ... {
    ...
} }

It's way less mental overhead to see a } } than to wonder what some new language construct means. And our } } is clearer than the usual two lines containing lone } when human eyes cannot keep the columns straight over an extended distance.

A similar if weaker argument could be made for } } over else match. A for else example is

if cs.is_empty() {
    ...
} else { for c in cs {
    ...
} }

In these cases, the fmt rule could simply be that several { on the same line pair with the corresponding several } on the same line. It's worth considering ) as well though, like

Wrapper( Thing {
    ...
} )

I suppose } } works better with block indent than with visual indent. It works better with the closing } } living on a clean line too, than when they are all closed together, as commonly happens with visually indented function calls. In particular the naively generalization of the rule I just stated to ( ) sounds too strong for many people's taste.

There are nice variations where { { .. } } indents the .. by say six spaces too, but this sounds hard for people's editors.

Apologies if this was discussed elsewhere but I was initially pointed to #34 where I felt this might be a derail.

[joshtriplett]

I do sometimes write code like this:

something(|value| {
    ...
})

something.func(|value| {
    ...
})

something.func(simple_arg, |value| {
    ...
})

I'd like to consider that case carefully when we standardize function call formatting, and see if we can write a formatting rule that rustfmt can follow. It'd likely look something like "if the last argument to a function includes a braced block, such as a closure, with no arguments after the block, and the start of the block occurs on the same line as the start of the function call, end the block and the function call on the same line with })".

A similar premise might apply for a function call whose last argument consists of a function call, placing the )) on the same line; however, that can more easily lead to ambiguity, and I feel less confident that we can specify a rule that will always produce results we want.

However, for blocks, I find it quite confusing to combine two constructs with a pair of braces on the same line. If one or more of those RFCs standardize constructs like else match expr { or similar, then I don't see anything wrong with formatting that in the obvious way; however, in the absence of such a construct, I find it confusing to have else { match expr { on one line and }} or } } on a later line. I would rather see the extra couple of newlines and the extra level of indentation, to make it easier to follow at a glance.

I don't find that that causes excessive rightward drift, because (by design) I try to avoid nesting such constructs particularly deep.

[steveklabnik]

I would find this change to be wasting our "strangeness budget" on something that I don't personally find important enough to waste it on.

[joshtriplett]

@steveklabnik I like your choice of phrasing there; it fits quite well.

[burdges]

I'm not saying any else { \n for construct should be converted into this. I'm saying the "strangeness cost" of some requested constructs, like for .. else, exceeds anything you might do with braces, so this should be considered a better option.

If you think those messy constructs can be avoided no matter what choices fmt makes, then I'm unlikely to convince you. :) Yet, we've seen a surprising number of RFCs in that direction, so I wanted to point out that potentially being relaxed about } } that pair with a { .. { might help. At least this is something that could be foisted on the next for .. else RFC's alternatives section.

As noted in that thread, else match might a slightly different situation because if is kinda a special case of match already, and it does not create all the chaos with borrowing of things like for .. else.

I'd agree of course that } } is normally inferior to say

    } // for
}

As an aside, an option to automatically add notes like // for on closing } for loop bodies over a certain size might be useful.

[louy2]

If one or more of those RFCs standardize constructs like else match expr { or similar, then I don't see anything wrong with formatting that in the obvious way; however, in the absence of such a construct, I find it confusing to have else { match expr { on one line and }} or } } on a later line. I would rather see the extra couple of newlines and the extra level of indentation, to make it easier to follow at a glance.

Why do you consider the latter more confusing than the former? IMO {} is a very clearly defined boundary of block, so it is semantically clearer to have them. Cognition-wise, matching a starting line of double/triple/quadruple/etc { with an ending line of corresponding number of closing } isn't very different from the case with single {}. You don't need to count the braces because the compiler makes sure that they match. It is recognized as a block with a header that lists all its properties and a closing line.

There is somewhat a consensus in rust-lang/rfcs#1712 that such reduction of right-drift is desirable. But I think it is not worth it to change semantic for cosmetic. rustfmt, meanwhile, is exactly for cosmetic.

If long distance is of concern, then how about:

else { loop { for i in iter { match expr {
} } } } // match for loop else

[joshtriplett]

Cognition-wise, matching a starting line of double/triple/quadruple/etc { with an ending line of corresponding number of closing } isn't very different from the case with single {}. You don't need to count the braces because the compiler makes sure that they match.

I do if I'm typing them. Even with editor assistance for matching up braces, I still find that awkward to track.

But braces aside, I find the lack of corresponding indentation confusing, because I use indentation (not braces) to keep track of blocks. The compiler uses braces; I rely on indentation. I match up levels of indentation with corresponding constructs; if one level of indentation corresponds to two or more constructs, I find that confusing.

On top of that, rustfmt would have to have criteria for when to "merge" blocks like that, and unlike a human, rustfmt wouldn't have information like "do these blocks semantically relate to each other in a way that would make sense to merge?".

else { loop { for i in iter { match expr {
} } } } // match for loop else

Those lines seem like a case study in "things a style guide should prevent".

[nrc]

I do not think there is a lot of support for this idea (sorry). I propose we close and am putting this into FCP (I agree that there are some special cases, e.g., #50 (comment), where we should do something like this, but that we should do so on a case by case basis).

[burdges]

I'm fine to close. We've made it a month or so without any new for .. else RFCs. ;)

[TedDriggs]

I just ran into something similar to this:

pub fn move_outcome<'a, A: Adjudicate>(context: &'a ResolverContext<'a>,
                                       resolver: &'a mut ResolverState<'a, A>,
                                       order: &'a MappedMainOrder<'a>)
                                       -> Option<Attack<'a>> {
    use order::MainCommand::*;
    match order.command {
        Move(..) if !path_exists(context, resolver, order) => Some(Attack::NoPath),
        Move(ref dst) => {
            Some({

                let dst_occupant =
                    context.orders.iter().find(|occ| occ.region.province() == dst.province());
                let supports = support::find_successful_for(context, resolver, order);
                match dst_occupant {
                    None => Attack::AgainstVacant(supports),
                    Some(ref occ) => {
                        // In the case that the occupier is leaving, this is a follow-in
                        if is_move_and_dest_is_not(occ, order.region) &&
                           resolver.resolve(context, occ) == OrderState::Succeeds {
                            Attack::FollowingIn(supports)
                        } else if occ.nation == order.nation {
                            Attack::FriendlyFire
                        } else {
                            Attack::AgainstOccupied(supports)
                        }
                    }
                }
            })
        }
        // non-move commands don't generate a move outcome
        Hold | Support(..) | Convoy(..) => None,
    }
}

On the second branch of the match, I originally wrote Move(ref dst) => Some({ /*several lines*/ }), and then had rustfmt change it to what you see above. Personal opinion, but I don't think the shape of the expression inside the Some() should change the placement of the start of the Some( expression.