Treat chains with just expr? specially.

[nrc]

Fixes #1004

[marcusklaas]

Hmmm, this feels somewhat of a bandaid solution. Do we understand why it didn't format properly before?

[nrc]

The double indent makes sense in a normal chain because then there is one level of indent for the chain and one for the match (or closure or whatever). When a chain is only one long, it wouldn't count as a chain and wouldn't go down this code path. expr? is treated like a chain of length 2 so we get the double indent. The solution is to treat expr? like expr + ?, rather than like a chain, which is what this PR does.

[marcusklaas]

Shouldn't we fix that by not formatting the stump of the three with extra indentation? I don't see how this is specific to a ? suffix as opposed to a .foo() suffix.

[marcusklaas]

I just tested this, and rustfmt also produces

fn issue_1004() {
    match *self {
            ty::ImplOrTraitItem::MethodTraitItem(ref i) => write!(f, "{:?}", i),
            ty::ImplOrTraitItem::ConstTraitItem(ref i) => write!(f, "{:?}", i),
            ty::ImplOrTraitItem::TypeTraitItem(ref i) => write!(f, "{:?}", i),
        }
        .foo;

    ty::tls::with(|tcx| {
            let tap = ty::Binder(TraitAndProjections(principal, projections));
            in_binder(f, tcx, &ty::Binder(""), Some(tap))
        })
        .foo;
}

for non-? suffixes.

[marcusklaas]

We should probably look at these two lines: https://github.com/rust-lang-nursery/rustfmt/blob/master/src/chains.rs#L103-L104

Any reason to not just use the passed in context there? I think this is the cause of some other bugs too.

[nrc]

The difference is that non-? chain elements go on the next line, whereas ? stays on the same line at the end. That means if we didn't do the extra indent with a normal chain then either the chain wouldn't be indented, or we would get a weird floating left brace.

[marcusklaas]

There's different indents for the chain base and the rest of the chain. We can kill the extra indentation for the base without affecting the rest of the chain.

[nrc]

I don't think it is a case of killing the indentation for the base vs the rest, but it depends on what comes after the base. If it is just a ?, then we should kill, if it is a function or method call, then we should not (i.e., we should keep the indent).

[marcusklaas]

Hmmmm, I believe I understand what you're saying, but I'm not convinced. Do you believe the example above (the one with the .foo suffixes) is formatted idiomatically?

[nrc]

I do. Although it is not really ideal, I think it is better than the alternative. In reality, such things rarely occur like this, which is why the examples look a bit odd. They're likely to be on the rightside of a let statement.

[marcusklaas]

Hmmm, I don't know man. Apart from being inconsistent:

let x = match y {
    1 => foo(),
    _ => bar(),
};

vs

let x = match y {
        1 => foo(),
        _ => bar(),
    }
    .baz;

I suspect that giving the base extra indentation is actually the cause of bugs #1018 and #1019.

I'll have to look into it a bit more carefully, though.

[nrc]

I can't see why 1018 would be caused by this - this causes a double indent, not a pseudo-visual indent.

I'm somewhat resigned to that inconsistency. I think it is acceptable that a chain formats differently to a single expression though. The trouble is that being consistent looks worse - either the always single indent case where you either get no indentation for the rest of the chain or floating method calls (worse, a closure deeper in the chain would be indented differently from a closure in the chain root) or we always double indent and you get weird floating double indents all the time.

[marcusklaas]

I think unindented chains are quite nice. Didn't we do the following at some point?

1. check whether the base "ends on the left", e.g. block, if-else, match, loop or closure expressions
2. if yes -> don't indent rest of chain
3. if no -> indent rest of chain

We could extend the check in 1 to include function calls with a "left-ending" final argument.

Examples:

let x = match y {
    1 => foo(),
    _ => bar(),
}
.foo();

and

let x = baz(|| {
    foo() + bar()
})
.kaas(|| {
    same_indent!();
});

and

kip_tandori(simple_arg, some_other + arg)
    .bar()
    .baz();

We get the following:

• no floating method calls
• consistent closure indenting in base and chain
• consistent formatting of matches with and without chain suffix
• least amount of rightward drift possible
• no need to make special cases for all-?-suffix-chains

I hope you consider this proposal. I truly believe it to be the best way forward (hence my reluctance merging this PR). I'd gladly make PR implementing this. The changes should be very small anyway.

[nrc]

I don't think we did that in the past, but we could consider it for the future. The issue with no indent is that it makes it hard to scan for new statements. With indent, you can scan the left margin and every unindented bit of code is a new statement. With no indent, you have to look at the end of the lines for semi-colons too (in this case you can look for .s at the start too, but I think that is harder to scan for than whitespace).

I think this example is instructive:

let x = baz(|| {
    foo() + bar()
})
.kaas(|| {
    same_indent!();
});

Is this one block statement or two? You have to read the code to tell. Whereas with indenting, you can tell pretty easily that the let starts a statement.

[marcusklaas]

That's a good point. I hadn't considered that. It is clear to me now that no strategy will be optimal by every metric.

How do we go forward from here? I'd be okay with merging this if we can continue the discussion over what should be the default indentation behaviour in a later PR. I can probably submit something this week.

[nrc]

I would like to merge this, since it seems to be improvement of the current behaviour, then add some other options for other possible behaviours so we can see them in action. We can decide on the default later.

[marcusklaas]

                                                                                  Ok, let's do it! r+ Apologies for the recalcitrance.                                                                                                                                                                                                                                                                                                                                                                                                                   Van: Nick CameronVerzonden: woensdag 1 juni 2016 1:08 PMAan: rust-lang-nursery/rustfmtBeantwoorden: rust-lang-nursery/rustfmtCc: Marcus Klaas de Vries; CommentOnderwerp: Re: [rust-lang-nursery/rustfmt] Treat chains with just expr? specially. (#1016)I would like to merge this, since it seems to be improvement of the current behaviour, then add some other options for other possible behaviours so we can see them in action. We can decide on the default later.

—You are receiving this because you commented.Reply to this email directly, view it on GitHub, or mute the thread.