Tracking issue for rustfmt

[tcharding]

Please not: There is not strong consensus that we should enable rustfmt

This is a tracking issue for pursuing the idea and helping reach consensus.

Previous discussion on rustfmt:

• The original issue: Files are not formatted with latest rustfmt #172
• Could we get a contribution/style guideline and Makefile? #685
• Rust Style Conventions #692

The main blockers to getting consensus

1. Showing that the results of rustfmt'd code are acceptable to folks [1]
2. Working out how to manage the massive changes in a safe reviewable manner

This is likely an organization wide change because it will painful for devs to have to configure their environment differently for different repos in the org?

[1] implies agreeing on a configuration of rustfmt that is acceptable to folks.

[tcharding]

Usage or non-usage is perversely invasive both upstream and downstream of rust-bitcoin lib. In my experience its a real pain to work on two repos one with rustfmt and one without. But if everything is the same you just get used to it. So this change has a non trivial effect on peoples daily work life. We don't know all the crates that use rust-bitcoin that do use rustfmt but we know that the following do not. Perhaps we should get some buy in from these projects?

• electrs
• rust-lightning
• rust-elements

[elichai]

I think we should probably come up with rustfmt configurations that change the least amount of code, and from there we can slowly modify configurations back to default(or non-default depending on maintainer's preferences) PR by PR.

[apoelstra]

@elichai there are no rustfmt configurations that don't cause massive code-diffs, and in ways that upset me.

You can't turn off the agressive code reflowing, because rustfmt works by parsing then reserializing an AST (although you can "trick" it by ending lines with // so it has some ability to preserve formatting..). And in particular you can't prevent it from ruining hand-formatted byte arraws.

[elichai]

And in particular you can't prevent it from ruining hand-formatted byte arraws.

Yeah, for those I always use #[rustfmt(skip)] even for my new projects :/

[apoelstra]

ISTR that this annotation doesn't work on 1.29, and you need to use a much uglier one. I guess with the MSRV bump that won't be true anymore?

I wouldn't mind sticking #[rusfmt(skip)] in lots of places.

[Kixunil]

because it will painful for devs to have to configure their environment differently for different repos in the org?

rustfmt.toml should fix this but I can see it can still break the flow if you forget that a project doesn't use rustfmt. Maybe if we don't use it we could write invalid syntax ("We don't use rustfmt") to that file to force rustfmt to fail instead of accidentally changing things?

[TheBlueMatt]

Personally I do not think rustfmt is something that should reasonably be applied to a codebase until at least some substantial readability improvements are made. Probably at a minimum rust-lang/rustfmt#4306 and rust-lang/rustfmt#4146

[Kixunil]

Hmm, I didn't notice the cases you mentioned in rust-bitcoin specifically. But yes, there's a bunch of things that annoy me about rustfmt.

[tcharding]

I totally agree that some things rustfmt does are ugly and annoying but there are some benefits. The question is, I suppose, do the benefits outweigh the costs?

Benefits

• rustfmt creates a uniform formatting
  • Uniform code is easier to read (I define 'easier' as quicker and producing less cognitive load)
  • Uniform code is easier to see bugs because strange things jump out at you
• Saves reviewer time/effort because the tools catch the mistakes not the reviewers

Costs

• Some of the formatting is ugly/not-acceptable
• Leads to much usage of rustfmt(skip)
• Bigger diffs and code churn (e.g. adding/removing one variable changes a bunch of lines

[TheBlueMatt]

Id actually argue rustfmt, in its current state, increases my cognitive load and makes me much less likely to spot bugs - the comically vertical layout of rustfmt pushes critical context further away from the code being reviewed, often outside of the diff context displayed in review tools. I couldn't care less about ugly code, I only care about reviewability.

[tcharding]

Compelling point.

[sanket1729]

IMO, One of the main benefits of rustfmt is that it is newcomer friendly. Based on personal experience, it is much easier to review code that is formatted by rustfmt than it is review code written by a new rust dev.

I don't know if @apoelstra remembers this was the reason that rust-miniscript has rustfmt rust-bitcoin/rust-miniscript#18. As a new rust dev with less than 15 days of writing code in rust, I found it really useful to worry about the code formatting and leave it to rustfmt. It took some time to adjust to rust-bitcoin and many of my PRs still have formatting-related comments. I think #685 also shares a similar concern. Although only a few PRs come from rust newcomers, I am personally willing to accept the cons of rustfmt to get a wider contributor ecosystem.

Leads to much usage of rustfmt(skip)

I also get annoyed by rustfmt when it changes one line to multiple lines for adding an extra arg to a function. If it gets annoying to read code for some PR, I think having a single rustfmt_skip for code impl block, or completely skipping it for the file is also acceptable.

[TheBlueMatt]

many of my PRs still have formatting-related comments

I have a feeling many post-rustfmt PRs will still have formatting comments, except now its doubly annoying because its "please exempt this code and make it readable" instead of having a chance of doing it reasonably the first time :(

[tcharding]

I agree totally that growing the contributor base is top priority but we should optimize for reviewers/maintainers IMO because their time is the most scarce.

[TheBlueMatt]

Note that I haven't had a ton of time to do real review on rust-bitcoin projects in some time, so certainly don't take anything I say too seriously, its just my takes that at least apply to repos where I do have the time to do a lot of review. If people who have more rust-bitcoin time than I feel otherwise, so be it!

[Kixunil]

I agree with @TheBlueMatt code style is not causing too much cognitive load. I also don't remember seeing any specifically ugly code by any contributor. For me personally, when contributing to a new project the biggest deciding factor is "can I understand the code and the problem well enough?" Formatting is totally non-issue.

What does increase my reviewer cognitive load is seeing things like slice[42] instead of slice.get(42).ok_or(Error)?, panics in supposedly unreachable branches and mathematical operators instead of checked_*. But this has nothing to do with formatting.

[casey]

Just wanted to add my 2 sats in favor of using rustfmt. Every code formatter I've ever used has had issues, but they've all still been better than having to manually format code. Additionally, I, like most Rust programmers, have my editor set up to auto format code, and remembering to toggle this off when working on projects that don't use rustfmt is a hassle.

[dpc]

@casey

In my text editor I have a auto-formatting on write set conditional on the presence of rustfmt.toml. This way I only auto-format when it seems like it is desired. Depending on your code editor it might be possible.

[casey]

In my text editor I have a auto-formatting on write set conditional on the presence of rustfmt.toml. This way I only auto-format when it seems like it is desired.

A large fraction of projects use rustfmt but don't have a rustfmt.toml file, since they just use the default formatting, so this would produce a lot of false negatives.

[dpc]

since they just use the default formatting, so this would produce a lot of false negatives.

Yeah, it happens sometimes. When the file is not there, you will just not get auto-formatting so it is not the end of the world, and you can run cargo fmt --all once to fix it.

Submit a PR with an empty file, explain the situation.

[casey]

Submit a PR with an empty file, explain the situation.

I think the opposite approach is probably better. More projects use rustfmt than don't, so asking projects that don't to go out of there way misplaces the burden.

A better approach would be to add a format = false option to rustfmt.toml, and then projects that don't want to use rustfmt, which are the minority, don't place any configuration burden on contributors.

[dpc]

A better approach would be to add a format = false option to rustfmt.toml, and then projects that don't want to use rustfmt, which are the minority, don't place any configuration burden on contributors.

Agreed, but that would would first have to be implemented, and my trick personally work for me very well.

[dpc]

Hey, look at this: https://rust-lang.github.io/rustfmt/?version=v1.4.38&search=#disable_all_formatting

So I guess rust-bitcoin could (at least for now) make rustfmt do nothing?

[casey]

Hey, look at this: https://rust-lang.github.io/rustfmt/?version=v1.4.38&search=#disable_all_formatting

So I guess rust-bitcoin could (at least for now) make rustfmt do nothing?

Oooo, I didn't know about that! That would be awesome.

[apoelstra]

ACK putting a rustfmt.toml with disable_all_formatting. This should help people whose editors are configured to rustfmt-mangle formatting on save.

I will also put in my two cents, which is that rustfmt is frequently significantly worse than manually formatting code.

[tcharding]

This is a massive step forward! PRs created for

• bitcoin_hashes: Add a disabled rustfmt.toml bitcoin_hashes#141
• rust-bitcoinconsensus: Add a disabled rustfmt.toml rust-bitcoinconsensus#39
• rust-secp256k1: Add a disabled rustfmt.toml rust-secp256k1#383
• rust-bitcoin: Add a disabled rustfmt.toml #803

Note, rust-bech32 already enforces rustfmt, with default config, on CI.

[tcharding]

FTR its #[rustfmt::skip]

[tcharding]

For those interested in pursuing (or resisting) usage of rustfmt in this crate please follow along with rustfmt PRs in rust-miniscript. rustfmt is already used there and I am attempting to find a configuration that is acceptable to folks that may then be acceptable here: rust-bitcoin/rust-miniscript#374

In parallel I have attempted to come up with a minimal diff to introduce rustfmt here: #959

Thanks

[tcharding]

#959 was closed but contains much discussion, flagging it here. #1040 is the current attempt to push this forward.

[tcharding]

Closing this, rustfmt has been introduced. We still ignore all directories in src but everything else is formatted. I'll un-ignore them as I think reviewers can tolerate the pain of reviewing.