field and method chains

[nrc]

e.g., a.foo.bar(x).baz()

[nrc]

(I'm going to try something different and specify this across multiple comments, hopefully to make things easier to comment on).

Simple cases

If the whole thing fits on one line, do that.

E.g.,

x.foo.bar.baz(foo);
x.foo(bar, baz)?.qux(foo, bar).baz?;

If it does not, split across multiple lines, one line for each element of the chain. Block indent, start lines with dots, newline after the chain root. ? never gets its own line

E.g.,

x
    .foo
    .bar;

a_long_name?
    .foo(bar, baz)?
    .bar(qux, foo, bar)?;

[joshtriplett]

@nrc That seems right for simple cases. Most complex cases would involve breaking one of the individual components across lines.

[solson]

I am fond of breaking only the last method call without breaking the earlier methods to save vertical space when only the last method call is wide. However, I'm not really sure how to codify the rule.

Examples: 1, 2, 3

Example 1, to save you a click:

let global_value = self.globals.get_mut(&id)
    .expect("global should have been cached (static)");

A more naive rule would give me this less desirable result:

let global_value = self
    .globals
    .get_mut(&id)
    .expect("global should have been cached (static)");

[solson]

After looking at it for a bit, I don't entirely mind what I called the naive rule, but it's not what I write naturally and it reduces the amount of code I can fit on my screen (I consider it valuable to be able to see more at once without scrolling back and forth).

[nrc]

Multiple line components

If any component is multi-line, then that forces the multiline form from the above comment. Sub-expressions should be further indented (i.e., as if the left margin were one block indent). E.g.,:

asdfasdf
    .foo
    .bar(
        argument1,
        argument2,
        argument3,
    ) // Note newline
    .baz;

asdfasdf
    .foo
    .bar(if foo {
        bar
    } else {
        qux
    }) // Note newline
    .baz;

Last element exception

The last element in the chain may go multiline, without forcing the rest of the chain to be multiline, e.g.,

foo.bar.baz(
    argument1,
    argument2,
);

[nrc]

I prefer to avoid the special case in #66 (comment), I don't mind the extra vertical space in the example you give.

[joshtriplett]

@nrc I'd suggest phrasing the general rule as "If any component other than the last is multi-line, ..."

[joshtriplett]

For the sake of considering alternatives (without necessarily advocating this style), I've also seen code that doesn't add the newline after the ) of an internal multiline component:

asdfasdf
    .foo
    .bar(
        argument1,
        argument2,
        argument3,
    ).baz(
        bazargument,
    );

I've seen cases where this improves code, and cases where it doesn't; I don't know that I could articulate a general rule for when it makes sense.

[joshtriplett]

@nrc I would advocate a slightly different rule. You can break a component other than the last across multiple lines, but you must break every component after that one as well. You can, however, keep all the components before that first break on the same line. So, for instance:

x = foo.short().short2(arg)
    .long_name(argument)
    .short()
    .longer_name(
        argument1,
        argument2,
    )
    .still_longer_name();

Notice that the .short() after .long_name(argument) must go on a separate line, even if it fits on the previous one. However, the short calls before that can all go on one line.

[solson]

@joshtriplett Nice, that's a pretty understandable rule. I could go either way about having .short().short2(arg) on the first line in that case, but I would feel more strongly if they were fields.

That is, I like this:

let x = foo.field.subfield
    .long_name(argument)
    .short()
    .longer_name(
        argument1,
        argument2,
    );

And I dislike this:

let x = foo
    .field
    .subfield
    .long_name(argument)
    .short()
    .longer_name(
        argument1,
        argument2,
    );

It feels excessive to have a line just for a field access, and I'm comfortable treating short method calls the same way to keep the rule simple. Not to mention it deals with the previous case I mentioned the way I like.

[joshtriplett]

@solson As long as the rule doesn't allow .field.subfield or .short().short2(arg) on one line except on the first line, that seems fine to me.

[casey]

I don't like keeping the first line on a single line if later lines are broken. So I'd rather see:

x = foo
    .short()
    .short2(arg)
    .long_name(argument)
    .short()
    .longer_name(
        argument1,
        argument2,
    )
    .still_longer_name();

The ability to easily scan down the chain of calls is preferable to me than saving the initial few lines.

It's not something that I feel strongly enough to argue about whether or not it should be the default formatting, but it would be nice if there was an option to configure it.

[blankenshipz]

I agree with @casey it's much easier for me to read the text if I don't have to do both horizontal and vertical scanning.

for this reason - in general my preference is that once a switch is made in a block of text (V->H, H->V) that the new form is completely adopted.

[killercup]

I agree with #66 (comment) and #66 (comment) but I also like to go one step further in my own code:

let result_with_long_name: Option<SomeType> =
    source // ← on a new line, makes parsing the chain as one block easy; would like to see this at least if there are ≥40 chars (incl. indentation) in front of the `=`
    .field
    .map(Thingy::do_stuff)?
    .lorem(|x| if x.is_good() { x } else { Thingy::default() })
    .ipsum(|thingy| {
        thingy // ← using a one-letter binding looks bad here, which will remind you to make your code readable ;)
            .foo() // ← new line
            .parse()
            .unwrap_or(42)
    })
    .dolor(
        sit,
        amet,
        <Consetetur as Sadipscing>::elitr(), // ← trailing commas EVERYWHERE
    )
    ; // ← not necessarily on a  new line but reduces diff churn

Just wanted to let you know my preference, as I usually write long chains :)

[nrc]

I also agree with @casey - I prefer not to allow a dense first line. I think there is a huge advantage to being able to scan down a chain once one is being used.

[nrc]

I would like to propose a tweak to my simple rules. If the first item width is <= the tab width, then the second item may go on the same line. E.g.,

x.second_item
    .third_item

first_item
    .second_item
    .third_item

firs.second_item
    .third item

After implementing, I found the 'orphan's formed by my initial rule to be annoying.

[nrc]

Complex chains

If a chain fits on one line, but is not short (see #47), then it should be split across multiple lines.

[nrc]

Try operator

The try operater (? should always go on the same line as its operand, e.g.,

first_thing?
    .second_thing()???
    .third_thing?

or in the single line case:

first_thing?.second_thing()???.third_thing?

[nrc]

An edge case

If the parent is multi-line and ends with ) (or whatever) then the first child floats. E.f.,

    given(
        arg1,
        arg2,
    )
        .running(waltz)
        .foo

We could allow the first child only on to the same line as ) (this is inconsistent with not allowing this for other children in the chain, but note that in those cases it is just about saving space, here there is a bad floating indent):

    given(
        arg1,
        arg2,
    ).running(waltz)
        .foo

We could double indent the parent element (this is visually a bit inconsistent because how a call is formatted depends on what comes next:

    given(
            arg1,
            arg2,
        )
        .running(waltz)
        .foo

Also note that now arg1 is floating.

[killercup]

Alternatively, we could not increase the indentation level for the chain at all if the parent ends with a closing bracket:

given(
    weirdly_long_arg1,
    arg2,
)
.running(waltz)
.foo

We might differentiate that depending of the context of the chain. The example above is free-standing, but if it were in a binding, I'd try to put it in its own visual block, indented by one level:

let result_thingy =
    given(
        arg1,
        arg2,
    )
    .running(waltz)
    .foo;

[joshtriplett]

@nrc I would favor the approach of writing ).nextcall( . The outdent for the ) provides a good delimiter.

[ivandardi]

I want to leave my opinion here.

I was working on a really long chain and I came up with a style that I like:

pub fn number_of_conflicting_queens(&self) -> usize {
    let same_line: usize = self.board
        .iter()
        .enumerate()
        .take(self.len() - 1)
        .map(|(i, queen)| self.board
            .iter()
            .skip(i + 1)
            .filter(|&other| queen == other)
            .count())
        .sum();
    let diagonals: usize = self.board
        .iter()
        .enumerate()
        .map(|(cola, rowa)| self.board
            .iter()
            .enumerate()
            .skip(cola + 1)
            .filter(|&(colb, rowb)| difference(*rowa, *rowb) == difference(cola, colb))
            .count())
        .sum();
    return same_line + diagonals;
}

Basically in closures, the root object says in the same line as the closure parameters, and every method is indented by one. That gives it a sense of "I'm chaining on something else here" while only increasing indentation by 1. It looks more aesthetically pleasing than

pub fn number_of_conflicting_queens(&self) -> usize {
    let same_line: usize = self.board
        .iter()
        .enumerate()
        .take(self.len() - 1)
        .map(|(i, queen)| {
             self.board
                 .iter()
                 .skip(i + 1)
                 .filter(|&other| queen == other)
                 .count()
         })
        .sum();
    let diagonals: usize = self.board
        .iter()
        .enumerate()
        .map(|(cola, rowa)| {
            self.board
                .iter()
                .enumerate()
                .skip(cola + 1)
                .filter(|&(colb, rowb)| difference(*rowa, *rowb) == difference(cola, colb))
                .count()
        })
        .sum();
    return same_line + diagonals;
}

Here each chain increases indentation by 2, which is really ugly and space wasting. The only "benefit" is that it makes the object being chained more obvious, but that's not a problem in the previous suggestion, I don't think.

[nrc]

Summary for FCP (more details in various comments above):

• If the whole chain fits on one line, do that.
• If it does not, split across multiple lines, one line for each element of the chain. Block indent, start lines with dots
• If the first item width is <= the tab width, then the second item may go on the same line
• The try operater (?) should always go on the same line as its operand
• If any component other than the last is multi-line, then that forces the multiline form above. Sub-expressions should be further indented (i.e., as if the left margin were one block indent)
• The trailing elementa in the chain may go multiline, without forcing the prefix to be multiline
• If a chain fits on one line, but is not short (see Define 'short' #47), then it should be split across multiple lines
• If the parent is multi-line and ends with ) (or whatever), then the first child would float, then the first child only is allowed on the same line as the parent's )

[nrc]

I'm having second thoughts about the last line/multiline exception, e.g., this change:

-        self.pre_comment
-            .as_ref()
-            .map_or(false, |comment| comment.starts_with("//"))
+        self.pre_comment.as_ref().map_or(
+            false,
+            |comment| comment.starts_with("//"),
+        )

that seems to make things worse. When reading this code, the original is easy to scan the important things by scanning the newlines, whereas in the formatted version, the important stuff is all bunched together and less important arguments are easy to scan.

[robinst]

I've noticed what I think is the same problem:

let mybaz = foo.bar().baz().expect("error getting baz");

Turned into:

let mybaz = foo.bar().baz().expect(
    "error getting baz"
);

Makes the expect message stand out a bit too much IMO.

[joshtriplett]

@nrc I think no matter which option we pick, we'll end up with cases where it produces a result we don't like; I can think of cases where doing it the other way around would produce a poor result as well. However, I do agree that the case you posted works better with the whole map_or call on one line.

Would it require too much search in rustfmt to say something like "if breaking the method chain across lines would leave the whole call on one line, do that, otherwise break the call and keep the method chain on one line if that's not too wide, otherwise break both"?

Also, would dropping the rule of "If a chain fits on one line, but is not short (see #47), then it should be split across multiple lines" make your example format better?

[nrc]

Would it require too much search in rustfmt to say something like "if breaking the method chain across lines would leave the whole call on one line, do that, otherwise break the call and keep the method chain on one line if that's not too wide, otherwise break both"?

I think this would be ok

Also, would dropping the rule of "If a chain fits on one line, but is not short (see #47), then it should be split across multiple lines" make your example format better?

I don't think it helps this example. In general it would mean it affects fewer cases.

[matklad]

If the parent is multi-line and ends with ) (or whatever), then the first child would float, then the first child only is allowed on the same line as the parent's )

Could you clarify the the first child only bit? Looks like this will cascade through all of the children

foo(
  ...
).map(|| { // edge case, start on the line with `)`
  ...
}).filter(|| { // should we start with on the same line here as well?
  ...
}).flat_map(|| { // and here?

})

[RobinMcCorkell]

I've encountered some issues with the "edge-case" of a multi-line parent followed by one or more children, as referenced by @matklad above:

foo(
    ....
).map_err(|err| ......)
    .and_then(|val| ...)
    ....

// contrast with

foo(
    ....
).map_err(|err| ........
    .and_then(|val| ...)
    ....

Notice how the second block misses the final parenthesis of the .map_err(), so .and_then() applies to something inside the closure, yet in the first block .and_then() is a chained method. I much prefer the alternative syntax suggested here, floating the first child after a newline:

foo(
    ....
)
    .map_err(|err| ......)
    .and_then(|val| ...)
    ....

[topecongiro]

I found the following pattern to be common when writing a method chain in rust:

items.iter().map(|item| {
    // ...
    item.foo().bar()
}).collect()

that is, iter().map(|| {..}).collect() with map() occupying multiple lines.
Currently, rustfmt formarts the above chain to this:

items.iter()
    .map(|item| {
        // ...
        item.foo().bar()
    })
    .collect()

which, IMHO, is worse than the former.

Is it reasonable to capture the above pattern and use the former style for it?

[jplatte]

which, IMHO, is worse than the former.

Can you try to explain why? I actually find the second form easier to read.

[ivandardi]

I'd usually format it like this:

items
    .iter()
    .map(|item| {
        // ...
        item.foo().bar()
    })
    .collect()

All chained methods are aligned so that I can have a visual cue about what chained methods are being applied to an object all in one go.

[mathstuf]

I can see some benefit of the former. It doesn't have rightward drift and it isn't too fat vertically. However, if once there are two calls in the chain that have function calls, it gets uglier much quicker IMO.