fmt: set force_multiline_blocks=true

[RalfJung]

This is an experiment, I am not yet sure if I like it... but it does prevent rustfmt from putting stuff after the => in a match (unless the entire arm fits there), which IMO is a big plus. What do others think?
(I also tried setting match_arm_blocks back to its default of true, but that adds too many braces for my taste.)

Btw, @calebcartwright is the interaction of match_arm_blocks = false and force_multiline_blocks = true as can be seen here expected? I think I like it, but it it is not at all what I expected from the docs which describe force_multiline_blocks = true as "Force multiline closure and match arm bodies to be wrapped in a block" -- but here that is not the effect it has, there are no new blocks being added.

[RalfJung]

FWIW, this kind of "having code on the same line as the => is actually fine IMO... having a nested match is very difefrent from a big method call here, in my view. Oh well, I still view this diff as a net positive.^^

[oli-obk]

I don't like anything in this diff, but I also don't care much. So, fine by me

[RalfJung]

So you're okay with code like this?

        _ => show_error(format!(
            "`cargo-miri` called without first argument; please only invoke this binary through `cargo miri`"
        )),

I think this is quite bad, spreading the method call over multiple lines but sharing the first line with the pattern makes this code very hard to read IMO.

[oli-obk]

I'm perfectly fine with it. Even outside of match arms, doing it for regular function call statements is fine by me. We do it for struct, array and tuple constructors. function calls are not special to me.

[oli-obk]

makes this code very hard to read IMO.

I may just have gotten used to it. it may be hard to read, but not to me

[RalfJung]

I'm perfectly fine with it. Even outside of match arms, doing it for regular function call statements is fine by me. We do it for struct, array and tuple constructors. function calls are not special to me.

I am specifically talking about match arms. This quickly leads to code like

Long(Pattern, That { is, not_entirely: trivial }) => something.foo().call(
   a,
   b
)

where (in particular in the context of a large match statement) I find it exceedingly hard to separate the pattern from what happens when the pattern occurs. Outside match arms there is no such ambiguity so there is no problem.

But I guess one can get used to this...

[oli-obk]

to me => followed by a newline is just really surprising. I have never seen this before in any Rust code. Can we instead generate a block?

[RalfJung]

to me => followed by a newline is just really surprising. I have never seen this before in any Rust code.

FWIW we already have something like that in the codebase:

miri/src/shims/intrinsics.rs

Lines 489 to 490 in b061307

	"atomic_max_relaxed" =>
	this.atomic_op(args, dest, AtomicOp::Max, AtomicRwOp::Relaxed)?,

I'd much prefer if all those atomic intrinsics were consistently written like this, instead of some having the function call on the same line because their name happens to be a bit shorter. This inconsistency in large matches which lots of entirely symmetric arms that are formatted differently is by far my biggest gripe with rustfmt. But that's a separate issue...

Can we instead generate a block?

We can if we also set match_arm_blocks = true (which is its default value). But that adds a lot more blocks than I like. I pushed it as a new commit so you can see what it looks like.

[RalfJung]

That CI failure is interesting... I ran formatting on the entire codebase before psuhing. Is it possible that rustfmt is non-idempotent?
EDIT: Yes it is: rust-lang/rustfmt#4897

[oli-obk]

I see... I am warming up to this PR (before the last commit). Let's go with it

[RalfJung]

To be clear, I don't want to force this, I can also live with the status quo (though I think I'd prefer applying both commits over keeping the status quo -- and prefer just the first commit over that).

[oli-obk]

No I really mean it. It was just very surprising at first, but I don't have any argument against it.

[RalfJung]

All right, thanks! Landing just the first commit then.

@bors r+

[bors]

📌 Commit cffa1d3 has been approved by RalfJung

[bors]

⌛ Testing commit cffa1d3 with merge a4a9a36...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing a4a9a36 to master...

[calebcartwright]

Btw, @calebcartwright is the interaction of match_arm_blocks = false and force_multiline_blocks = true as can be seen here expected? I think I like it, but it it is not at all what I expected from the docs which describe force_multiline_blocks = true as "Force multiline closure and match arm bodies to be wrapped in a block" -- but here that is not the effect it has, there are no new blocks being added.

To be honest I'm not sure how much consideration went into how those two opts would work together. I was somewhat taken aback initially by the resultant formatting, though after looking at it for a bit realize that it's really the only possible formatting with the configuration combo since they'd otherwise be in a mutually exclusive conflict.

I think this probably warrants some more documentation updates to clarify that force_multiline_blocks will block indent the body of a closure/match arm if the formatted body contains multiple lines, and the body will also be wrapped in braces unless otherwise configured (i.e. match_arm_blocks)