Rustfmt: Support preserving block wrapping of closure bodies

[workingjubilee]

This is actually from your own repo!
https://github.com/rust-lang/rustfmt/blob/1eb8764e4043d80ad75f8bbb59ba3e6ce2d95975/src/formatting/utils.rs#L156-L157

It took me a good long moment to realize this is a closure and not a... typo, frankly, given that it also uses the fallthrough pipes for a bool. I normally don't have an issue with rustfmt occasionally making weird choices but this is actually "maybe I should stop using rustfmt" level of concerning since this makes code harder to read, which I know is not desired. | | might be more consistently legible? But there's many different ways to solve this. However, all of them will be eliminated by rustfmt, as it stands, and I don't see any options that offer an escape hatch.

• rustfmt version: 1.4.20 nightly or 2.0.0-rc2 nightly
• From: rustup or cargo make install

[calebcartwright]

but this is actually "maybe I should stop using rustfmt" level of concerning since this makes code harder to read

Sorry to hear that. The formatting emitted by rustfmt is governed by the Rust Style Guide which is established via RFCs. The formatting requirements for closures can be found here.

Notably, there's not supposed to be any spaces when there's no params (|| is correct, | | would conflict), just like a standard function: fn foo() not fn foo( ). If you'd really like to change how FnDecl portion of a closure is formatted when there's no parameters then you'll need to follow the RFC process in the Style Guide repo. If and when the Style Guide is modified, we'll update rustfmt accordingly.

Developers have the option of utilizing blocks around the closure body if they prefer, so the snippet you referenced could optionally be written as below, and rustfmt would format it with the block wrapping maintained

    let explicit_conversion_preserves_semantics = || {
        !is_mod || (is_mod && attrs.map_or(true, |a| a.is_empty()))
    };

FWIW, I don't find the current code with the block-less closure body to be challenging to grok, although I wrote the code in this function so admittedly not an unbiased observer :smile:

[workingjubilee]

Granted, I only found it puzzling because of the unique case of a combination with pipes, but it's very peculiar since I was literally looking at it to evaluate the bool and went "hm, what's this? a bool that starts with a pair of pipes? surely that's incorrect?"
...and found out when Rust complained that no, it needs that to be callable elsewhere.

Injecting a block is actually something I tried, experimentally, but rustfmt continually stripped the block no matter what permutation I used (I didn't exhaust all possibility space, but I tried the obvious variations on wrapping it). So that is a bug, if you thought that's how it should work and it doesn't.

[calebcartwright]

Injecting a block is actually something I tried, experimentally, but rustfmt continually stripped the block no matter what permutation I used (I didn't exhaust all possibility space, but I tried the obvious variations on wrapping it). So that is a bug, if you thought that's how it should work and it doesn't.

Interesting, I was checking locally with v1.4.17 but will check on v1.4.20 later

[workingjubilee]

I made probably most of my attempts to wrap the closure body with 2.0.0-rc2 (by "I didn't exhaust all possibility space", I don't mean I didn't try!), but I know I tried at least once, possibly twice or more, with v1.4.20 and it didn't work out.

[calebcartwright]

No worries, user error on my end as I forgot to remove the semi after the copy/paste resulting in the body containing a statement and being formatted differently. So I was wrong anyway, the default behavior is indeed correct as the body has no statements and the expression can fit on one line, thus doesn't need to be wrapped in a block.

As such there's no bug here, and the formatting is expected/aligned with the Style Guide. However, I'd be open to considering a new config option that would support the preservation of the block wrapping around closure bodies in cases where the developer has added them, if you (or anyone else) would be interested in implementing such an option.

[workingjubilee]

I disagree slightly that a style guide that produces unexpected results is not a bug, but I grant that I would have to carry any such lamentation to your upstream dependency, as it were. :^)

I will definitely consider implementing this as closures in general seem to need some attention to how they are formatted in rustfmt. Also, and at this point I am mostly expanding on the thought for possible future reference, an author can force the closure to be formatted with a block anyways using a comment (tried with 1.4.20):

    let explicit_conversion_preserves_semantics = || {
        // ha.
        !is_mod || (is_mod && attrs.map_or(true, |a| a.is_empty()))
    };

Which is consistent with the style guide but Seems Like A Hack, as it were.

[calebcartwright]

I will definitely consider implementing this

Great thanks!

closures in general seem to need some attention to how they are formatted in rustfmt

That's a little too vague for a substantive response, but we're always happy for improvements! if you do end up with additional changes you'd like to propose, i'd suggest cross referencing with the style guide to check for any conflicts, as well as the typical suggestion to break things into separate and small-as-possible PRs

I disagree slightly that a style guide that produces unexpected results is not a bug, but I grant that I would have to carry any such lamentation to your upstream dependency, as it were. :^)

A bug in rustfmt would be default formatting that diverges from the style guide, idempotency failures, etc. Whether or not one likes an aspect of the style guide or prefers/expects a different result doesn't make it a bug.

The formatting produced by rustfmt here is in fact expected because the expected result is dictated by the style guide. If the bug determination was based on the subjective preferences of the individual, then rustfmt would be nothing but bugs because for each and every emitted formatting there'd always be someone who'd disagree :smile:

[workingjubilee]

Oh, I was just observing that many of the open issues seem somehow closure-related.

I only (half-jokingly) suggested it was a bug because _you_ expected it to work differently, mind!

[calebcartwright]

I only (half-jokingly) suggested it was a bug because you expected it to work differently, mind!

Ah okay gotcha. Yup, that was just my bad running a quick test without fully updating the snippet. That all important semicolon makes a major difference!

Oh, I was just observing that many of the open issues seem somehow closure-related.

SGTM :+1: I was thinking the other day we could probably use a new a-closure label as I'd noticed the same

[workingjubilee]

SGTM +1 I was thinking the other day we could probably use a new a-closure label as I'd noticed the same

It would be lovely if that was a thing, actually! Even back-tagging closed issues would help... er, that's not a request for you to go through all 2000 mind, but history of previous issues, even their resolution-states, is useful to troubleshooting issues so that they stay down. I can search for "closure" in the issues but it doesn't indicate which ones are actually About closures.