Block indent large closures and field/method chains

[nrc]

Closes #690
Closes #439
cc #566 (not fixed, but an improvement).

This probably needs some polishing, but I'd like to land it and then weed out the bugs. I'm sure there are edge cases for chains that are now worse. We can probably apply the 'big closure' in other places, though I'm not sure where.

r? @marcusklaas

[marcusklaas]

I can't say that I'm a big fan of this new strategy. In my mind, we shouldn't do block indenting when aligned indentation fits on just as many lines. When it doesn't, why don't we put the entire expression on the next line? We'll get close to the amount of space as with block indentation and the chain will still be easy to follow..

[marcusklaas]

The as_ref should go to the next line as well here.

[nrc]

Why? I was aiming for an ideal of

foo_qux.bar
    .baz

[marcusklaas]

I think the second link (bar) is more easily overlooked with that style. With the one link per line style, it's clearer to find each subsequent link and your eye doesn't need to make the jump.

Pathological example:

very_long_function(with.loads().of_arguments, arg2, arg3, sub_funk(test)).foo
    .bar(xs)

vs

very_long_function(with.loads().of_arguments, arg2, arg3, sub_funk(test))
    .foo
    .bar(xs)

[nrc]

Hmm, that's a good counter-example. Should probably look at some more code examples (I think the heuristic for leaving code on multiple lines if it was originally gets in the way of getting a good idea of the trade-off because it means examples with short lines don't get put on a single line). I was thinking of tweaking or dumping that heuristic in any case...

[marcusklaas]

Ok, so with a little more nuance: I think the "compactness" heuristic above would more adequately solve the issues for assignments, but it won't when the chain is part of another expressions, e.g. an argument in a function call. In that case, I think we should

1. try to format everything on 1 line
2. compare the number of lines in aligned formatting and tabbed formatting and pick the one with the lowest count, with ties going to alignment
3. when we do tabbed formatting all links should go on a new line

This would be a compromise between the current strategy and the one you've implemented.

I also see this addresses the alignment of closure bodies. I'm not terribly excited about this either, but from the discussions in the issue tracker it has become clear that this addresses a much encountered issue. I could live with these closure changes.

[marcusklaas]

This cut-off seems fairly arbitrary. Can we consider a "try-both-and-take-the-more-compact-one" heuristic?

[nrc]

That doesn't really work. Block formatting will always be more compact because it means more horizontal space. The heuristic is making the subjective decision between a small closure where it's easier to be inline with the current indentation (i.e., it's not worth the effort for the eye to move left) vs a big closure where the large amount of wasted whitespace becomes offensive (and it's worth moving the eye left because there's enough of a closure to justify it).

[nrc]

Examples:

fn main() {
    // small closure
    let some_variable = some_function(foo_bar_baz,
                                      |x| {
                                          let a = dsfgsdfgsdfg;
                                          let b = dsfasdfasdf;
                                          sdfsdf(a, b)
                                      });

    // big closure
    let some_variable = some_function(foo_bar_baz,
                                      |x| {
        debug!("pretty printing source code {:?}", s);
        let sess = annotation.sess();
        let ast_map = annotation.ast_map().expect("--unpretty missing HIR map");
        let mut pp_state =
            pprust_hir::State::new_from_input(sess.codemap(),
                                              sess.diagnostic(),
                                              src_name.to_string(),
                                              &mut rdr,
                                              box out,
                                              annotation.pp_ann(),
                                              true,
                                              Some(ast_map.krate()));
        for node_id in uii.all_matching_node_ids(ast_map) {
            let node = ast_map.get(node_id);
            pp_state.print_node(&node)?;
            pp::space(&mut pp_state.s)?;
            let path = annotation.node_path(node_id)
                                 .expect("--unpretty missing node paths");
            pp_state.synth_comment(path)?;
            pp::hardbreak(&mut pp_state.s)?;
        }
        pp::eof(&mut pp_state.s)
    });
}

[marcusklaas]

Well, block formatting is always at least as compact. In case of a tie, one could opt for alignment.

Your argument makes sense though. Reading large blocks of code at very deep indentation isn't optimal I agree.

[nrc]

So, my reasoning for going with block indenting for chains is as follows:

• all things being equal I still prefer visual indenting. But,
• if we block indent closures, then visual indentation of chains looks awful, and I don't see a way to solve that. I think this is common enough that we can't ignore it.
• there are a few other situations where visual indenting leads to bad rightwards drift
• therefore we must use block indenting some of the time
• I'd rather use it all of the time, since I don't feel it looks a lot worse, I think the consistency is worth some detriment to the aesthetics.

As for not putting a new line before the first child in the chain, it just seemed to look OK and save us some space. See examples here, I think no option is great, so we may as well choose the most compact.

I think tabbed will always be more compact then visual, no? So there doesn't seem to be any point to step 2. Step 1 falls out in the wash (From my understanding of the chain code, we always try to do one line if we can). So the only change left is 3, put the newline in. I'm amenable to that if you like - I think no newline is marginally better, but don't feel strongly.

[marcusklaas]

Good points. Some thoughts:

• I agree that block indented closures look terrible in visually indented parents. However, I would rather move the entire expression to block level than just the second argument. This to me looks jarring:

let looooooooooooooooooooooooooooooong_name = some_function_call(yada, yada).foo()
    .bar;

• if we're being consistent: will we opt out of visual indentation of function call arguments later? When large closures are placed in them, the same issue arises.
• what exactly is the issue with rightward drift? Is it aesthetically unsatisfying to have code be indented deeply, or is compactness (number of lines) an issue? I've always understood it to be the latter, but from the discussion I'm not sure any more.

I guess my focus is not so much on visual vs block indentation, but more about aligning the links (the dots, if you will). Block indentation is okay in my book if we can make sure that it's easy to keep track of all the seperate links.

[nrc]

How would you solve the example?

let looooooooooooooooooooooooooooooong_name = some_function_call(yada, yada)
    .foo()
    .bar;
// or
let looooooooooooooooooooooooooooooong_name =
    some_function_call(yada, yada)
        .foo()
        .bar;

(assuming some tight framing)

I don't think function args have the same problem. Partly because by convention, closures are nearly always passed last. Partly because the problem with chains is that a closure appears as an arg to a chain item (2 levels deep) vs with functions where they are one level deep

The problem with rightward drift for me is that we get into situations where we have really bunched up code because our budget gets down to 20 or 30 chars. The compactness is secondary (for me) and aesthetics are pretty subjective, I don't mind the aesthetics of rightward drift, but some do.

[marcusklaas]

100% (provided the chain itself wouldn't fit on one line)

let looooooooooooooooooooooooooooooong_name =
    some_function_call(yada, yada)
        .foo()
        .bar;

I fully share your concern with rightward drift. The reason I've been pushing for these compactness tests is that they're good indicators of bunched up code. If something fits in as many lines visually indented as it does with block indentation, its inner code is almost surely formatted identically.

[nrc]

@marcusklaas I removed the last commit, so we no longer move the first child in the chain on to the first line. This gives us

let looooooooooooooooooooooooooooooong_name = some_function_call(yada, yada)
    .foo()
    .bar;

I'd be happy to leave the rest for a follow-up, i.e., getting to:

let looooooooooooooooooooooooooooooong_name =
    some_function_call(yada, yada)
        .foo()
        .bar;

In particular, since this will add an extra line, which I'm not sure if it is really necessary.

[marcusklaas]

I'm still not convinced this is ideal, but I guess it's an improvement overall. I may do a pull request with tweaks at some point.

Last request for this PR: could we reuse the old chain tests with a visual indent option?

r+ with the extra tests