rustfmt should avoid rightwards drifting big blocks of code

[nagisa]

Currently rustfmt formats from

            let mut arms = variants.iter().enumerate().map(|(i, &(ident, v_span, ref summary))| {
                let i_expr = cx.expr_usize(v_span, i);
                let pat = cx.pat_lit(v_span, i_expr);

                let path = cx.path(v_span, vec![substr.type_ident, ident]);
                let thing = rand_thing(cx, v_span, path, summary, |cx, sp| rand_call(cx, sp));
                cx.arm(v_span, vec!( pat ), thing)
            }).collect::<Vec<ast::Arm> >();

to

            let mut arms = variants.iter()
                                   .enumerate()
                                   .map(|(i, &(ident, v_span, ref summary))| {
                                       let i_expr = cx.expr_usize(v_span, i);
                                       let pat = cx.pat_lit(v_span, i_expr);

                                       let path = cx.path(v_span, vec![substr.type_ident, ident]);
                                       let thing = rand_thing(cx,
                                                              v_span,
                                                              path,
                                                              summary,
                                                              |cx, sp| rand_call(cx, sp));
                                       cx.arm(v_span, vec!(pat), thing)
                                   })
                                   .collect::<Vec<ast::Arm>>();

which is much worse than the original.

[nrc]

I kind of like the new version - it still fits and I find aligning the methods in the chain more readable.

However, there should be an option for block indenting of methods of a chain (rather than visual indenting) which should avoid the huge chunk of whitespace. I also thought we could format like the first example because there is enough space and the argument to map is a closure - that is new work that just landed. @marcusklaas - do you know what is going on?

[marcusklaas]

I think rustfmt would pick the former strategy if it wasn't for the collect call.

Edit: confirmed

[nrc]

Ah, that's interesting. I wonder if we could we fix that (as an option, as I said I quite like the new version).

[nagisa]

Note that the issue title mentions “big” blocks. I find current style better when the block of code is 1-to-3ish lines of code too, but it gets exponentially worse as the LOC of the block increases.

[nrc]

Hmm, good point. @arichto has also pointed out some formatting choices which make sense for small blocks but not large ones. I guess we're going to have to build in this concept some how, although the idea gives me the fear.

[nrc]

I guess it is just a cut-off at n lines, but it still seems scary some how.

[oli-obk]

how about moving to

            let mut arms = variants
                .iter()
                .enumerate()
                .map(|(i, &(ident, v_span, ref summary))| {
                    let i_expr = cx.expr_usize(v_span, i);
                    let pat = cx.pat_lit(v_span, i_expr);

                    let path = cx.path(v_span, vec![substr.type_ident, ident]);
                    let thing = rand_thing(cx,
                                           v_span,
                                           path,
                                           summary,
                                           |cx, sp| rand_call(cx, sp));
                    cx.arm(v_span, vec!(pat), thing)
                })
                .collect::<Vec<ast::Arm>>();

for large blocks?

[solson]

In the second code block in the original post, there is indentation (the closure body) past an alignment (the . for the method calls). If the method call . isn't on a multiple-of-4 column, then hitting TAB in vim (and I think other editors) will insert less than 4 spaces to reach the next multiple-of-4 column.

For example, in that code sample, hitting tab on the line after let pat = cx.pat_lit(v_span, i_expr); inserts just 1 space. This can make editing in these blocks frustrating.

Plus, the alignment forced rand_thing(cx, v_span, path, summary, |cx, sp| rand_call(cx, sp)); far enough to the right that it had to go vertical. It seems like it's easier for an automated tool to keep from doing anything drastic like that if it sticks to a style more like @nagisa's original. Maybe that should be the default, and the rightwards alignment style should be an optional flag.

[alexcrichton]

I've run across this many many times when working with rustfmt, and I agree with @nagisa's original post that the first block of code is much more readable. We've often had a problem with rightward-drift of code in Rust in the past and we have tried to tailor idioms (like RAII) around preventing that. I personally prefer to have style wherever possible that avoids rightward drift as much as possible.

[killercup]

This came up recently when I ran rustfmt on Miri: rust-lang/miri#6 (comment)

[regexident]

This right-drift behaviour has been a major annoyance of mine, when working with rustfmt.

I've found passing an in-place created PDS to functions to be a nice pattern for keeping function signatures clean and extensible and even allow for some kind of faux default arguments. It however clashes with rustfmt on every single use.

This one is fine and remains as it is when fed through rustfmt:

let input = Input {
    foo: 42,
};
some_fn_with_struct_and_closure(input, |foo| {
    println!("foo: {:?}", foo);
});

while this one (which I'd prefer to remain untouched):

some_fn_with_struct_and_closure(Input {
    foo: 42,
}, |foo| {
    println!("foo: {:?}", foo);
});

gets turned into this:

some_fn_with_struct_and_closure(Input {
                                    foo: 42,
                                },
                                |foo| {
                                    println!("foo: {:?}", foo);
                                });

😢

[nrc]

I've been experimenting with this. It is relatively easy to get a naive fix:

    let mut arms = variants.iter()
                           .enumerate()
                           .map(|(i, &(ident, v_span, ref summary))| {
        let i_expr = cx.expr_usize(v_span, i);
        let pat = cx.pat_lit(v_span, i_expr);

        let path = cx.path(v_span, vec![substr.type_ident, ident]);
        let thing = rand_thing(cx, v_span, path, summary, |cx, sp| rand_call(cx, sp));
        cx.arm(v_span, vec![pat], thing)
    })
                           .collect::<Vec<ast::Arm>>();

The obvious problem here is the orphan method call (cc #566 and probably some other issues too). Not sure what a good solution to that is. One idea (that would fix #566 too) if to always block indent method/fields on new lines. I don't think this looks as good (I've learned to really like the aligned .s) but it does solve the floater problem.

    let mut arms = variants.iter()
        .enumerate()
        .map(|(i, &(ident, v_span, ref summary))| {
            let i_expr = cx.expr_usize(v_span, i);
            let pat = cx.pat_lit(v_span, i_expr);

            let path = cx.path(v_span, vec![substr.type_ident, ident]);
            let thing = rand_thing(cx, v_span, path, summary, |cx, sp| rand_call(cx, sp));
            cx.arm(v_span, vec![pat], thing)
        })
        .collect::<Vec<ast::Arm>>();

Anyone got any better ideas?

[solson]

@nrc I think block indent (your second example) looks better, but more importantly, it also seems to cause fewer problems for rustfmt, fairly consistently (as this comment about a similar problem mentioned, alignment is not as scalable). On top of that, it's an easier rule for a text editor to implement.

Aligned method calls should be an optional feature for those who are willing to risk more problematic formatting. I know this is against your personal preferences, but the defaults should be designed so rustfmt's results are acceptable in the highest number of cases

[retep998]

I'm strongly in favor of using block indenting by default. In my experience alignment indenting has always simply caused problems for very little gain. Block indenting already serves the purpose of making it easy to see where things continue onto the next line, while alignment indenting just makes editing more difficult, causes noisy diffs, and results in horrible rightward drift that can induce even more newlines needed leading to pathological situations.

[killercup]

In some cases, I really like the look of the aligned indenting… but in most cases it just feels wrong (too far right, creating empty space on the left over several lines, etc).

Thus, I too favor block indenting. @nrc's second code block looks pretty much like my preferred style (though I did move the variants.iter() into the block and combined some lines).

[regexident]

… but in most cases it just feels wrong (too far right, creating empty space on the left over several lines, etc)

The main issue I have with dot-aligned indenting (in addition to wasted space) is that it forces my line of sight to wander in curvy lined all across the screen instead of just an almost straight line parallel to the left window edge.

The second version (@nrc's last comment) makes scanning a code file so much quicker.

[strega-nil]

Block indent is just less complicated, and less likely to go wrong. Therefore, I think we should be using block formatting.

[strega-nil]

One thing I do do, which is very different from the vast majority of rust code, is to use 2-space indents; it fights against the rightward drift of rust quite well.

[oli-obk]

is to use 2-space indents; it fights against the rightward drift of rust quite well.

rightwards drift is not in the number of spaces, but in the number of indentations.

[regexident]

[…] use 2-space indents; it fights against the rightward drift of rust quite well.

Use ultra-short identifiers; it fights against the rightward drift of rust quite well. 😈 😇

Joking aside, formatting should behave adequately regardless of indentation size, imho.

[solson]

2-space indent does reduce rightwards drift and doesn't obfuscate the code like shortening identifiers, but I can imagine it being unpopular in the Rust community. (It's arguable that 2-space is narrow enough to make it harder to scan.)

[retep998]

Use ultra-short identifiers; it fights against the rightward drift of rust quite well. 😈 😇

Not really an option when I'm working with Windows API identifiers that take up an entire line on their own 😛

[strega-nil]

@tsion I thought it would be harder to scan, but it turns out it's actually not. shrug

@oli-obk it does fight rightward drift, with multiple matches. I keep myself to 80 columns, as well, so it's nice.

[oli-obk]

What I meant is that readability goes down with every extra indentation. If you just want to reduce horizontal space, you can reduce the font size. What we actually want is to reduce the size of jumps from one level to another, so we have a "feeling" for where we are in the code wrt nesting.

[strega-nil]

@oli-obk Kinda, but smaller indentation does allow for longer lines of real code.