Weird chain formatting issues

[RReverser]

Rustfmt used to chain function calls in a consistent uniform fashion, e.g.:

fn f() -> Vec<u32> {
    [1, 2, 3]
        .iter()
        .map(|&x| {
            return x * 2;
        })
        .collect()
}

But recently something has changed on nightly version and it started breaking the chain like below:

fn f() -> Vec<u32> {
    [1, 2, 3]
        .iter()
        .map(|&x| {
            return x * 2;
        }).collect()
}

[RReverser]

It looks even more weird when adding another call to the chain:

stable

fn f() -> CustomVec<u32> {
    [1, 2, 3]
        .iter()
        .map(|&x| {
            return x * 2;
        })
        .collect()
        .into()
}

nightly

fn f() -> CustomVec<u32> {
    [1, 2, 3]
        .iter()
        .map(|&x| {
            return x * 2;
        }).collect()
        .into()
}

[crawford]

I've noticed the exact same. My code was previously formatted like this:

Client::new()
    .request(
        Request::get(&req.state().upstream)
            .header(
                header::ACCEPT,
                HeaderValue::from_static(CONTENT_TYPE_GRAPH_V1),
            )
            .body(Body::empty())
            .expect("unable to form request"),
    )
    .from_err::<Error>()
    .and_then(|res| {
        if res.status().is_success() {
            future::ok(res)
        } else {
            future::err(format_err!(
                "failed to fetch upstream graph: {}",
                res.status()
            ))
        }
    })
    .and_then(|res| res.into_body().concat2().from_err::<Error>())
    .and_then(|body| {
        let graph: Graph = serde_json::from_slice(&body)?;
        Ok(HttpResponse::Ok()
            .content_type(CONTENT_TYPE_GRAPH_V1)
            .body(serde_json::to_string(&graph)?))
    })

But new versions of rustfmt prefer the following:

Client::new()
    .request(
        Request::get(&req.state().upstream)
            .header(
                header::ACCEPT,
                HeaderValue::from_static(CONTENT_TYPE_GRAPH_V1),
            ).body(Body::empty())
            .expect("unable to form request"),
    ).from_err::<Error>()
    .and_then(|res| {
        if res.status().is_success() {
            future::ok(res)
        } else {
            future::err(format_err!(
                "failed to fetch upstream graph: {}",
                res.status()
            ))
        }
    }).and_then(|res| res.into_body().concat2().from_err::<Error>())
    .and_then(|body| {
        let graph: Graph = serde_json::from_slice(&body)?;
        Ok(HttpResponse::Ok()
            .content_type(CONTENT_TYPE_GRAPH_V1)
            .body(serde_json::to_string(&graph)?))
    })

I find the latter to be quite difficult to read.

[azriel91]

From an ergonomics point of view, I tend to use a duplicate line shortcut in this kind of code:

something
    .with_function(|| {
        // ...
    })
    .add_stuff(1) // This is where I'd focus the cursor and duplicate line
    .add_stuff(1) // <- and this pops out, so I can change the argument
    // ...

But with the new one

something
    .with_function(|| {
        // ...
    }).add_stuff(1) // This is where I'd focus the cursor and duplicate line
    }).add_stuff(1) // <- but this pops out, which is broken syntax
    // ...

It's true I can go and delete the extra brackets, but it is annoying

[crawford]

I'm seeing another strange artifact that feels similar:

                 .txqptr
                 .read()
                 .dmatxqptr()
-                .bits() << 2
+                .bits()
+                << 2

[scampi]

The chains::join_rewrites method causes the chain to format as shown by @RReverser.

Because there is a closure, identified as a block by https://github.com/rust-lang-nursery/rustfmt/blob/4bbfe0829d0ca4579c20c51100bb43c70f7c8e2d/src/chains.rs#L132, the newline is not added:

https://github.com/rust-lang-nursery/rustfmt/blob/4bbfe0829d0ca4579c20c51100bb43c70f7c8e2d/src/chains.rs#L645-L647

@RReverser Looks like the option indent_style: Visual would kinda give you what you expect.
@nrc maybe the is_block_like method could be changed so that a closure is not recognised as such ? What do you think ?

[RReverser]

@scampi I don't think special-casing closures would be sufficient, logic needs to be modified more substantially (or removed) since blocks in the middle of chain are formatted equally poorly.

[scampi]

Can you expand on the formatting of middle chain blocks ? I see the following at the moment:

• a chained method should start on a newline
• a shift operator shouldn't be on a newline, alone As per Weird chain formatting issues #2932 (comment), that is intentional

[nrc]

I'm seeing another strange artifact that feels similar:

This one is intentional - it was too easy to miss operators which were hidden in the chain.

blocks in the middle of chain are formatted equally poorly

This was deliberate, but maybe not optimal. I don't remember all the motivations - at the least I thought always putting a newline after } was using too much vertical space and }.foo was easy enough to scan for. I think there might also have been an orphan issue under certain indentation conditions, but I might be recalling incorrectly. I think it should be easy to experiment by commenting out the if line in @scampi 's comment.

[RReverser]

putting a newline after } was using too much vertical space and }.foo was easy enough to scan for

I rather meant equal issues with function calls which take e.g. block expression and not closure as an argument - they're formatted just like in the original example in the issue, so I don't think special casing closures is going to cut it.

[RReverser]

Oh, it happens not only with closures and block expressions but also structs:

fn f() -> CustomVec<u32> {
    [1, 2, 3]
        .iter()
        .some_method(S {
            x: 10,
            y: 20,
            z: 30,
        }).collect()
        .into()
}

Moreover, it seems that this bug is now happening on stable too :(

[scampi]

I have commented the if mentioned in #2932 (comment) and below are some observations about the diff I got when running cargo test.

The idea would be to better understand what needs fixing and a basis for further discussion.

Observations

• Commenting the condition resolved the issue described (see diff1 and diff2). The case shown in Weird chain formatting issues #2932 (comment) should also be fixed with that change. The diffs diff3, diff4 and diff5 depict how it would like in other cases of chained methods.
• diff6 and diff7 go against a motivation for removing the newline as explained in Weird chain formatting issues #2932 (comment)
• diff8 is a regression with regards to the orphan issue.
• diff9 shows the impact on ordinals (accessing a tuple value)

In addition to the ergonomics argument, my personal feeling is that removing the newline in cases of method chains (i.e., diff1..5) gives a wavy look to the code which hinders readability.

Removing the newline in cases of a block expression (i.e., diff6..7) does give a more compact look, while not hindering readability I think.

The lone ordinal as shown in diff9 may surprise too.

Examples

Method Chain

diff1

  some_fuuuuuuuuunction()
      .method_call_a(aaaaa, bbbbb, |c| {
          let x = c;
          x
-     }).method_call_b(aaaaa, bbbbb, |c| {
+     })
+     .method_call_b(aaaaa, bbbbb, |c| {
          let x = c;
          x
      });

diff2

  aaaaaaaaaaaaaaaa
      .map(|x| {
          x += 1;
          x
-      }).filter(some_mod::some_filter)
+      })
+      .filter(some_mod::some_filter)

diff3

  match inner_meta_item.node {
      ast::MetaItemKind::List(..) => rewrite_path(
          context,
          PathContext::Type,
          None,
          &inner_meta_item.ident,
          shape,
-     ).map_or(false, |s| s.len() + path.len() + 2 <= shape.width),
+     )
+     .map_or(false, |s| s.len() + path.len() + 2 <= shape.width),
      _ => false,
  }

diff4

  args.iter()
      .filter(|arg| {
          arg.to_expr()
              .map(|e| match e.node {
                  ast::ExprKind::Closure(..) => true,
                  _ => false,
-             }).unwrap_or(false)
-     }).count()
+             })
+             .unwrap_or(false)
+     })
+     .count()
      > 1

diff5

  {
      match x {
          PushParam => {
              // params are 1-indexed
              stack.push(
                  mparams[match cur.to_digit(10) {
                              Some(d) => d as usize - 1,
                              None => return Err("bad param number".to_owned()),
-                         }].clone(),
+                         }]
+                 .clone(),
              );
          }
      }
  }

Block Expressions

diff6

  let x = Foo {
     field1: val1,
     field2: val2,
- }.method_call()
+ }
+ .method_call()
  .method_call();

diff7

  let y = if cond {
      val1
  } else {
      val2
- }.method_call();
+ }
+ .method_call();

Miscellaneous

diff8

Regression of #2415

  let base_url = (|| {
      // stuff

      Ok((|| {
          // stuff
          Some(value.to_string())
-     })().ok_or("")?)
- })().unwrap_or_else(|_: Box<::std::error::Error>| String::from(""));
+     })()
+     .ok_or("")?)
+ })()
+ .unwrap_or_else(|_: Box<::std::error::Error>| String::from(""));

diff9

  .fold(
      (String::new(), true),
      |(mut s, need_indent), (kind, ref l)| {
          if !l.is_empty() && need_indent {
              s += &indent_str;
          }
          (s + l + "\n", !kind.is_string() || l.ends_with('\\'))
      },
- ).0;
+ )
+ .0;

[max-sixty]

A rule that would cause a line break in 1,2,4,5 (though not 3) and not 6,7,8 (borrowed from python's black):

If you need to line break for one method, line break for all sibling methods in that chain.

7 is a canonical case where you wouldn't wrap

[mitsuhiko]

This just landed on stable rust and it just created some really ugly files. A good example is what it did to our clap app definition: https://github.com/getsentry/semaphore/blob/474a29f39b4c90c5f1588fa9f469fe05be1c10be/src/cliapp.rs