[unstable option] match_arm_blocks

[scampi]

Tracking issue for unstable option: match_arm_blocks

[Iron-E]

With the following configuration I can get rustfmt to produce errors:

control_brace_style = 'AlwaysNextLine'
match_arm_blocks = false

It says:

error[internal]: left behind trailing whitespace

The lines it refers to are these, which went over the specified maximum width before formatting:

MatchWhen::HasAll(required_values) =>

	required_values.len() == 1 && required_values.contains(value),

It seems like it leaves a blank line in-between the arm and the expression that's causing the error. I believe it wants to do this:

MatchWhen::HasAll(required_values) =>
	required_values.len() == 1 && required_values.contains(value),

Changing either setting from the sampled TOML config will fix the issue.

[RalfJung]

Would it be possible to also support Preserve for match_arm_blocks? I think that would be very useful. :)

[calebcartwright]

Would it be possible to also support Preserve for match_arm_blocks? I think that would be very useful. :)

Sounds quite reasonable to me. We added support for the same on closure bodies fairly recently and of course have opt-in support for preservation of leading pipes on arms so feels rational to support the same here.

At first blush it seems like it should be feasible as well, so I'll open a separate issue later with some implementation pointers and considerations for anyone interested in working on it

[calebcartwright]

With the following configuration I can get rustfmt to produce errors:

@Iron-E - apologies I missed your post. Could you do me a favor whenever you get a chance and open a new issue with a complete and minimal snippet to reproduce?

[RalfJung]

Another possibly reasonable value for match_arm_blocks might be Force (Always?), which makes all arms always have a block. I am not sure if this is ever suited "globally", but there are some matches where I think this is better than deciding on a per-arm basis -- so together with #4843 this could be really useful for match formatting.

That said, Force can (hopefully) be emulated with Preserve by just manually putting each arm between braces.

[Iron-E]

With the following configuration I can get rustfmt to produce errors:

@Iron-E - apologies I missed your post. Could you do me a favor whenever you get a chance and open a new issue with a complete and minimal snippet to reproduce?

I opened #4844 with a minimal reproduction 👍

[gilescope]

In general I really like this option set to false but when the line above is already indented due to it being too long it's probably best to add the branckets (or not add the newline):

NodeKeyConfig::Ed25519(sc_network::config::Secret::Input(ref ski))
    if node_key_type == NodeKeyType::Ed25519 && &sk[..] == ski.as_ref() =>
    Ok(()),

versus

NodeKeyConfig::Ed25519(sc_network::config::Secret::Input(ref ski))
    if node_key_type == NodeKeyType::Ed25519 && &sk[..] == ski.as_ref() =>
{
    Ok(())
},

Can you see why I feel a little uncomfortable about match_arm_blocks=false in these situations?

[RalfJung]

Yeah I agree, having braces is strictly better in that case @gilescope.

[Iron-E]

I feel one more indent would look better, while taking up less space:

NodeKeyConfig::Ed25519(sc_network::config::Secret::Input(ref ski))
  if node_key_type == NodeKeyType::Ed25519 && &sk[..] == ski.as_ref() =>
    Ok(()),

[calebcartwright]

In general I really like this option set to false but when the line above is already indented due to it being too long it's probably best to add the branckets (or not add the newline):

This is more a reflection of a bug in the option that didn't consider the scenario. The currently produced formatting is operating under the assumption of the extra line breaks from the braces which is why the body isn't indented from the guard (as per the Style Guide prescriptions).

We do need to change something here but I don't want to get into a situation where the boolean match_arm_blocks needs a bunch of "well, unless" clauses.

Given the fairly narrow use case the option applies to (non-block required body whose first line can't fit on the same line as the operator) I'd be much more inclined to apply the same indent to the body as the line of the => operator to be consistent with how the rule works in all other scenarios, as @Iron-E suggested above

For cases where folks want more flexibility/control over the braces I think the upcoming Preserve variant for the new option that'll replace match_arm_blocks (refs #4896) would be a better approach

[yhm-amber]

In general I really like this option set to false but when the line above is already indented due to it being too long it's probably best to add the branckets (or not add the newline):

NodeKeyConfig::Ed25519(sc_network::config::Secret::Input(ref ski))
    if node_key_type == NodeKeyType::Ed25519 && &sk[..] == ski.as_ref() =>
    Ok(()),

versus

NodeKeyConfig::Ed25519(sc_network::config::Secret::Input(ref ski))
    if node_key_type == NodeKeyType::Ed25519 && &sk[..] == ski.as_ref() =>
{
    Ok(())
},

Can you see why I feel a little uncomfortable about match_arm_blocks=false in these situations?

Agree too.