Configuration option to preserve blank lines at start of block

[ayazhafiz]

Adds the option for

• Functions
• Control flow blocks
• Expression statements (i.e. let x = { ... };)
• Traits
• Trait Impls
• Mods
• Foreign Mods

If I am missing a kind of item, let me know :)

Closes #2868

[ayazhafiz]

(don't let the 900 line diff scare you; ~800 of it is tests)

[calebcartwright]

(don't let the 900 line diff scare you; ~800 of it is tests)

That made me chuckle, as my eyes did pop at first before I saw that note 👀

[calebcartwright]

I know the original issue only referenced start of blocks, but I have a suspicion that a decent size chunk of users that would leverage this config option may also be interested in something similar for the end of blocks.

Do you think an equivalent end-of-block config and behavior would be feasible, and if so would it make sense for those to be two distinct config options or easier to use as one? Based on the explanation in the linked issue it sounds like there's scenarios where folks may want to keep the lines at the beginning but still trim at the end so separate would probably be best for more granular control.

[ayazhafiz]

That makes sense to me, if people want one they may want the other. I definitely think it's feasible (and probably easily doable, but those are famous last words). However I also think it's worth waiting and seeing if there are any requests for such a configuration option, as it's easier to add an option than remove one (from a stability and "how many people are using this?" point of view). What do you think?

[ayazhafiz]

This is totally orthogonal, but it would be interesting to pull GitHub data and see what are the most common rustfmt options are too.

[calebcartwright]

However I also think it's worth waiting and seeing if there are any requests for such a configuration option, as it's easier to add an option than remove one (from a stability and "how many people are using this?" point of view). What do you think?

Agreed, and certainly no need to do anything with end of block lines as part of this PR, I'm betting it'll get requested sooner or later though.

but it would be interesting to pull GitHub data and see what are the most common rustfmt options are too.

Definitely! Funny you should mention this, as just the other day I was thinking about how beneficial it would be to have quantitative data about config option usage. There's some that we know are used pretty extensively in their respective corners (indent_style, hard_tabs, use_small_heuristics, etc.) but there's plenty of others where we don't really know.

[calebcartwright]

Looks pretty good, thank you so much! I've left a few thoughts/questions inline for your review, but since this is adding a new config option I'll defer to @topecongiro for approval and merging

[calebcartwright]

Thanks for the updates! I'll try to get back to this soon, though have also requested a review from @topecongiro for the reasons mentioned earlier in the thread.

One of the things that I can't quite shake is a feeling that there's a little too much going on with advance_to_first_block_item and/or some of the internal logic is leaking out a bit that has to be conditionally interpreted by consumers of the function.

For example, I can see 3 different branches internal to the function that can all result in function returning the exact same thing, an empty string, and the length of the returned string (or more precisely whether or not it's empty) is significant for at least some consumers.

I don't have any specific suggestions/modifications just yet (although maybe wrapping the return string in an Option could help?) and I wouldn't say this is a blocking concern for me; just wanted to share my thinking. Properly (and idempotently) maintaining line spacing is certainly a nontrivial task and you've done a great job here!

[ayazhafiz]

a little too much going on with advance_to_first_block_item

I tried to condense the logic a bit, lmk what you think

[ayazhafiz]

https://github.com/rust-lang/rustfmt/pull/4295/files broke this, so there is probably a better way to handle this using the code there...

[topecongiro]

@ayazhafiz @calebcartwright Thank you forks! The PR LGTM, and additional thanks for writing more tests ❤️

[calebcartwright]

Thanks for the changes @ayazhafiz! I know it was fairly minor but I definitely appreciate the updated advance_to_first_block_item 👍