Skip to content

Remove rustfmt formatting requirement from project#2127

Closed
stevenroose wants to merge 4 commits intorust-bitcoin:masterfrom
stevenroose:bye-bye-rustfmt-go-burn-in-hell
Closed

Remove rustfmt formatting requirement from project#2127
stevenroose wants to merge 4 commits intorust-bitcoin:masterfrom
stevenroose:bye-bye-rustfmt-go-burn-in-hell

Conversation

@stevenroose
Copy link
Copy Markdown
Collaborator

@stevenroose stevenroose commented Oct 17, 2023

There are enough voices of discontent with rustfmt throughout the Rust ecosystem. Personally, reading rustfmt-formatted Rust code makes me unhappy and not want to read the code anymore at all. rustfmt generally sucks at formatting code in a sensible way, has absolutely no respect for the developer's judgement on readability, nor for vertical space. For me, it almost always makes code less easy to read and manage. Not even talking about adding nightly Rust to our required development stack.

Anyway, just my two bits. Had to throw them in here. I think formatting matters. It's an important tool to make code understandable, glancable and manageable.

Not wanting to repeat,

@tcharding
Copy link
Copy Markdown
Member

Awesome, during review, we can all get in the habit of writing "please fix formatting here to be some suitable layout, although we choose to define suitable as however each individual conrtibutor thinks the code should be layed out, oh you did that already - your layout sucks, oh I said that last week on an almost-identical change (thanks Steven)"

Copy link
Copy Markdown
Contributor

@junderw junderw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are enough voices of discontent with rustfmt throughout the Rust ecosystem.

I am actually curious to see examples of this if you have any.

I think formatting matters.

Then replace it with another tool that checks for your formatting... otherwise this is just increasing the cognitive load of everyone who reviews code for this repository and opens up each PR for endless bike-shedding.

Concept NACK.

@junderw
Copy link
Copy Markdown
Contributor

junderw commented Oct 17, 2023

adding nightly Rust to our required development stack.

Which is why I think plain old vanilla rustfmt without any customization is the best route to go.

But that's my 2 sats.

@tcharding
Copy link
Copy Markdown
Member

Perhaps we can split this problem into two parts

  1. Requiring devs to have nightly is a plain rude: rustfmt without nightly toolchain #2128
  2. rustfmt sucks but its the least sucks thing we have (this PR)

@dpc
Copy link
Copy Markdown
Contributor

dpc commented Oct 17, 2023

FWIW. I think code formatted with rustfmt is beautiful and I stare at it for hours sometimes, admiring it in it's effortless perfection.

@junderw
Copy link
Copy Markdown
Contributor

junderw commented Oct 17, 2023

FWIW. I think code formatted with rustfmt is beautiful and I stare at it for hours sometimes, admiring it in it's effortless perfection.

Me too.

Just kidding. I have literally never looked at any code output by rustfmt and thought "wow that's ugly and or hard to read."

As long as there's a consistent format and people agree upon it, and CI can check PRs for compliance, I don't care what it is.

Some people argue that this "agree upon it" should be community wide. Some say it should be project wide...

I think project wide is fine, but having a set of rules that is checked in CI is important to take away a large source of bike shedding on each issue.

Copy link
Copy Markdown
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we should not format the code anymore? or we should use the cargo fmt --all without nightly?

@stevenroose
Copy link
Copy Markdown
Collaborator Author

Awesome, during review, we can all get in the habit of writing "please fix formatting here to be some suitable layout, although we choose to define suitable as however each individual conrtibutor thinks the code should be layed out, oh you did that already - your layout sucks, oh I said that last week on an almost-identical change (thanks Steven)"

Yeah this definitely seems to be a huge problem in other projects that don't use code formatters.. Like bitcoin core or rust-lightning..

And I'm obviously not against having some code formatting rules, like tabs/spaces, organizing of use statements, newlines around functions and if-blocks, basic things like that. Things that are actually important to be consistent and are generally just opinionated but can't really make a big difference in readability. If there is a tool that checks/fixes those things, great. That's what gofmt does for Go. But rustfmt isn't a format fixer, it's a canonical formatter, which will not just fix a few basic rules, it will entirely disregard everything you wrote and output it in a canonical format.

@stevenroose
Copy link
Copy Markdown
Collaborator Author

Awesome, during review, we can all get in the habit of writing "please fix formatting here to be some suitable layout, although we choose to define suitable as however each individual conrtibutor thinks the code should be layed out, oh you did that already - your layout sucks, oh I said that last week on an almost-identical change (thanks Steven)"

Also it doesn't seem like the current formatting rules were added because this ever was a problem, but because one random dude (who's not even a contributor) wondered why we were not doing it. And then Maxim adding a formatting section to our CONTRIBUTING.md without any motivation given. (Funnily enough he "based" his CONTRIBUTING.md on the rust-lightning one that never had a formatting section.)

@stevenroose
Copy link
Copy Markdown
Collaborator Author

There are enough voices of discontent with rustfmt throughout the Rust ecosystem.

I am actually curious to see examples of this if you have any.

This is the most recent one I came across and it made me decide to finally open this MR.

adding nightly Rust to our required development stack.

Which is why I think plain old vanilla rustfmt without any customization is the best route to go.

But that's my 2 sats.

I'd rather go the Maxim way and fork the project in that case.

@stevenroose
Copy link
Copy Markdown
Collaborator Author

FWIW. I think code formatted with rustfmt is beautiful and I stare at it for hours sometimes, admiring it in it's effortless perfection.

Sure, write your code that way. Probably you won't because it would be insane to hit enter so many times as a human. But sure if you like it that way, write it like a normal human and then apply a formatter to your own code. Unfortunately I don't think there's a tool that can do that.

@yancyribbens
Copy link
Copy Markdown
Contributor

If there is a tool that checks/fixes those things, great. That's what gofmt does for Go. But rustfmt isn't a format fixer, it's a canonical formatter, which will not just fix a few basic rules, it will entirely disregard everything you wrote and output it in a canonical format.

Git can actually fix formatting and white space https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration. See the section "Formatting and Whitespace".

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Oct 17, 2023

We're not gonna accept this because there are too many strong views on the other side, but FWIW I totally agree wih @stevenroose.

A bit of history though:

  • It was not just "one random non-contributor" asking why we weren't using rustfmt that made us use it. We had a parade of people, including actual contributors, who would complain that they needed to override their IDE to not auto-format the project, and why were we being different, etc.
  • We did also have a good number of format-only changes and iteration on PRs where we were just asking people to fix formatting. (There were also regular contributors who would pretty-consistently submit poorly formatted code. I will not name them :).)
  • So there was a strong push for rustfmt which lasted many years.
  • We had very strong opposition to introducing rustfmt to begin with, including from me and BlueMatt. If I thought this PR had a chance of moving forward I'd be willing to go dig through the history and find examples.
  • The opposition was (a) we didn't want a "format the world" PR (which we eventually did, on the theory that if we were gonna do it we should do it as early in rust-bitcoin's history as possible), and (b) rustfmt is ugly as hell and overriding it is somehow just as ugly.

Eventually we moved to a scheme where we would format individual modules that nobody cared about using rustfmt, and we still had liberal #[cfg_attr(rustfmt, rustfmt_skip)] everywhere. Changing those to #[rustfmt::skip] was one of many instances where rustfmt itself forced us to do shitty format-only PRs because it was changing. I seem to recall "let's just drop rustfmt" movements showing up each time.

This got mired because rustfmt's output was so bad that we couldn't format any more modules, until somebody found out that the nightly rustfmt lets you disable a lot more stuff. This got us from "impossibly bad" to "tolerably bad" so we could move forward. At the time, all developers except for @stevenroose were willing to download and run daily-replaced binary blobs from Mozilla Amazon on our own machines, and since we weren't forcing users to do this, it seemed okay.

@junderw

Just kidding. I have literally never looked at any code output by rustfmt and thought "wow that's ugly and or hard to read."

I'm not going to accuse you of gaslighting, but this is an insane point of view ;). Right now there are 59 instances in this codebase of rustfmt::skip and there would be far more if I were to actually nit the rustfmt formatting every time I hated it. @stevenroose BTW if you want to make a statement, one approach may be to start nitting formatting in rustfmt-formatted PRs and see if that changes peoples' minds about how much rustfmt "reduces the cognitive load" vs just making it so irritating to deal with formatting that nobody bothers anymore.

Then replace it with another tool that checks for your formatting...

There is no other tool in the Rust ecosystem. In a recent thread somebody found one that was somehow even worse than rustfmt in terms of mangling code in impossible-to-override ways.

Somehow this does not even crack the top 5 problems with the Rust ecosystem.

@TheBlueMatt
Copy link
Copy Markdown
Member

TheBlueMatt commented Oct 17, 2023

While I strongly agree rustfmt absolutely sucks, I think rust-lightning, too, is gonna go rustfmt. Having review comments around formatting is just too much of a roadblock to contributors to remain practical. We haven't figured out exactly what we'll do yet, and I personally refuse to run rustfmt binary blobs on my system in my general workflow (or use any rustup garbage except in rare cases). Sadly this means my workflow for contributing to rustfmt projects is to write code, then use CI to fix formatting. Is there no way to RUSTC_BOOTSTRAP on stable rustfmt here?

@apoelstra
Copy link
Copy Markdown
Member

Is there no way to RUSTC_BOOTSTRAP on stable rustfmt here?

What does this mean?

If you just mean "use stable rustfmt", then personally I do think there's a path forward where we delete each line individually from our rustfmt.toml, adding #[rustfmt::skip] to every line of code that gets wrecked. I'm confident we could get, say, 80% of the way there, and then we could have a debate about a much smaller set of nightly-only options.

@TheBlueMatt
Copy link
Copy Markdown
Member

I meant is there a way to "use nightly rustfmt" on a stable rustfmt binary (similar to what you can do with some rustc/cargo features with RUSTC_BOOTSTRAP).

@dpc
Copy link
Copy Markdown
Contributor

dpc commented Oct 17, 2023

I meant is there a way to "use nightly rustfmt" on a stable rustfmt binary (similar to what you can do with some rustc/cargo features with RUSTC_BOOTSTRAP).

Yes. All you need is rustfmt in your PATH to be a nightly one. Dev shell in Fedimint does it automatically for everyone.

BTW.

We have twice as many Rust LoC in Fedimnt, contributors coming and going and of various level of experience. Stuff being ripped apart, rewritten furiously all the time. No one ever even mentions formatting. No one. For all intents and purposes formatting is a solved problem and we don't even think about it.

Same in majority of Rust Open Source projects I contributed to. The ones where I remember that wasn't the case it was always some disagreeable opinionated author with strong opinions from previous work with C and C++.

I wonder if a single person complaining about rustfmt formatting isn't a previous C/C++ developer. Please let me know if that's the case - it's just a theory, but I think that they generally overfit their neural network staring at C++ where every & and * can be deadly.

@apoelstra
Copy link
Copy Markdown
Member

Yes. All you need is rustfmt in your PATH to be a nightly one. Dev shell in Fedimint does it automatically for everyone.

@dpc the goal is that you don't need a nightly rustfmt to be installed at all. Ideally you can just tell a stable rustfmt "pretend you're a nightly".

I wonder if a single person complaining about rustfmt formatting isn't a previous C/C++ developer. Please let me know if that's the case - it's just a theory,

Lol. I'm curious about this as well. (For my part, yes, I am a previous C and C++ developer.)

@junderw
Copy link
Copy Markdown
Contributor

junderw commented Oct 17, 2023

I meant is there a way to "use nightly rustfmt" on a stable rustfmt binary (similar to what you can do with some rustc/cargo features with RUSTC_BOOTSTRAP).

It requires the nightly (or dev) release channel. It detects it with the CFG_RELEASE_CHANNEL env var.

https://github.com/rust-lang/rustfmt/blob/547577fa5d309d90292ca3a58fef1bf0d9325cc0/src/bin/main.rs#L202-L204

@TheBlueMatt
Copy link
Copy Markdown
Member

It requires the nightly (or dev) release channel. It detects it with the CFG_RELEASE_CHANNEL env var.

Wait, so does that mean I can do export CFG_RELEASE_CHANNEL=nightly and do rustfmt with a release binary? Is there any difference in the actual compiled code?

@junderw
Copy link
Copy Markdown
Contributor

junderw commented Oct 17, 2023

export CFG_RELEASE_CHANNEL=nightly

I tried it, but I couldn't get it to work...

@junderw
Copy link
Copy Markdown
Contributor

junderw commented Oct 17, 2023

I'm not going to accuse you of gaslighting, but this is an insane point of view ;).

I know this was tongue in cheek, but just in case someone thought I was being superfluous, I'm being 100% serious.

Code formatting just really isn't that important to me. I can read it just fine.

But then, of course, I don't come from a C/C++ background... I come from a higher level language background and all the projects I've worked on so far always had formatters as a given.

@tcharding
Copy link
Copy Markdown
Member

I wonder if a single person complaining about rustfmt formatting isn't a previous C/C++ developer. Please let me know if that's the case - it's just a theory,

Lol. I'm curious about this as well. (For my part, yes, I am a previous C and C++ developer.)

FWIW I came from C, found out about code formatters in Go then worked in Rust. In the Linux kernel the "solution" to this problem is what made me be so opinionated about using a formatter

  • Every subsystem has its own formatting style rules that you have to learn when you switch subsystems
  • [At least one subsystem] maintainer[s] will not take code style changes even if this is "bring this code inline with the rest of the style in this file" because they think it is code churn.
  • There are policy documents (and flame wars) outlining coding style (tabs vs spaces)
  • I routinely had to fix formatting issues in my patch sets as well as mention it during review (although this was mostly in staging which has newer devs)

So basically its just a big mess and as long as the formating tool is bearable I think its a big win. FTR I routinely change my code (variable/paramater names, static strings etc.) so that it formats in a not-so-ugly way, this is a fail and I don't think one should ever need to do this. It is just possible with ones own code, it is impossible to PR with "change variable name so rustfmt does not destroy this")

@dpc
Copy link
Copy Markdown
Contributor

dpc commented Oct 17, 2023

xyz-formatted Rust code makes me unhappy and not want to read the code anymore at all.

Can be said about any code. "Rust code formatted by apoelstra makes me unhappy and not want to read the code anymore at all". 🤣 "Too dense, hard to read, goes too much horizontally, so when working with split screen, I have to use a scroll bar and can't see everything, especially painful in split column diff view."

Before automatic and enforced formatting has become popular, every job, every team, every project was another set of pointless, arbitrary rules that 90% at the time dislike and thus constantly argue about, devolving into petty power struggles for nerds.

Nowadays I'm happy people can hate on how rustfmt formats the code, and not on how I did/want it.

@tcharding
Copy link
Copy Markdown
Member

Ok, this PR has successfully derailed my morning and I hazard a guess its done the same to a few others. All I've gotten out of this is:

  • Steven hates rustfmt, still, and is never going to change.
  • Matt is resigned to using it in LDK.
  • Andrew is resigned to using it here.
  • Jonathon is comfortable with the current state of things.
  • Tobin hates this discussion, already spent 18 months trying to resolve this, and wishes the whole thing would go away.
  • Dawid is a well capable of some funny flaming, and is fine with the current rustfmt set up

(Purposely not using mentions so as not to waste even more of peoples time.)

I'm closing this as a total waste of everyones day.

@tcharding tcharding closed this Oct 18, 2023
@yancyribbens
Copy link
Copy Markdown
Contributor

I'm closing this as a total waste of everyones day.

I don't think this discussion has been a total waste and it feels premature to close this even if these changes will not be accepted as is. Using rust fmt nightly can be an annoyance to peoples (rather opinionated) workflow. This project is different from most others in the desire to reduce third party dependencies, and arbitrary rustfmt binaries is such a dependency. Closing this PR and "wishes the whole thing would go away" seems silly. Although the branch title of this PR seems silly also and not very serious.

@apoelstra
Copy link
Copy Markdown
Member

Closing this PR and "wishes the whole thing would go away" seems silly.

I don't think closing this PR is "wishing this would all go away" (though Tobin did say that this is how he feels :)).

I agree that the discussion has been useful (at least, to collect a ton of spread-out discussion in one place) but I also think it's pretty-much played out.

@stevenroose
Copy link
Copy Markdown
Collaborator Author

A bit of history though:

Thanks @apoelstra.

including actual contributors, who would complain that they needed to override their IDE to not auto-format the project, and why were we being different, etc

If you have a workflow where disabling a tool like rustfmt for a certain project is a non-trivial task, I would seriously doubt the quality of the code these people contribute. Seriously it's like complaining a project uses spaces because your editor only does tabs.

At the time, all developers except for @stevenroose were willing to download and run daily-replaced binary blobs from Mozilla Amazon on our own machines, and since we weren't forcing users to do this, it seemed okay.

It's not even that I'm unwilling. rustup doesn't work on Guix, so I would have to manually build rustc versions from Guix's latest to stable, then build rustc nightly and then build rustfmt with it.

BTW if you want to make a statement, one approach may be to start nitting formatting in rustfmt-formatted PRs and see if that changes peoples' minds about how much rustfmt "reduces the cognitive load" vs just making it so irritating to deal with formatting that nobody bothers anymore.

As it currently stands, even during my sabbatical most MRs here get merged before I have a chance to look at them, so I won't want to try that. (Though commenting on already-merged MR might be a statement against the short review times people are allowed here 🤔 ) I think I already make a statement by distancing myself in my own MRs from the formatting changes introduced by rustfmt.

I wonder if a single person complaining about rustfmt formatting isn't a previous C/C++ developer. Please let me know if that's the case - it's just a theory, but I think that they generally overfit their neural network staring at C++ where every & and * can be deadly.

I'm confused which single person you're talking about? This thread alone has several people complaining about it..

I personally barely wrote any C or C++ in my life. I do agree that Rust's type system slightly reduces the importance of reviewability and readability of code, but I would argue that the potential impact of serious issues in a lib like rust-bitcoin would make that more of a priority compared to other projects where the absolute worst that can happen is "the program doesn't work".

Code formatting just really isn't that important to me. I can read it just fine.

@junderw maybe you have a ton of vertical lines? I have a lot as well myself, 72 or something, but I've had colleagues with worse eye-sight that only had less than 50 or 40 vertical lines and like that you can very often not get the function you're working on in your screen.

I would actually recommend anyone that thinks rustfmt is great to try develop using only 40 lines of vertical space and see how easy you find it to keep function context while writing code. Especially people with mice or that don't use something with efficient key bindings like vim have to do a lot of hand-switching in that case. (I personally have non of these problems, but I have worked with people that have complained about these. I feel like the people that struggle with this wouldn't easily speak out about this as they risk being "that one guy that wants to get rid of rustfmt" in their own project.)

FWIW I came from C, found out about code formatters in Go then worked in Rust. In the Linux kernel the "solution" to this problem is what made me be so opinionated about using a formatter

I am not going to cite each individual section of your comment, but wow, I know many developers aren't your average Joe, but I can't help but be amazed with the level of OCD if so many of them can't stand a piece of code have a slightly different style than the next one.

Ok, this PR has successfully derailed my morning and I hazard a guess its done the same to a few others. All I've gotten out of this is:

I think it's kinda unfortunate to close this conversation after only a single day as it looks that there are quite some people unhappy with rustfmt in general and even those that are strongly in favor of keeping rustfmt don't really seem to like the way it reformats their code, but seem to just accept any standardized format over actually well-formatted code.

@tcharding
Copy link
Copy Markdown
Member

Re-opening since my closing seems to have bothered people (I've unsubscribed from notifications on this PR :)

@tcharding tcharding reopened this Oct 20, 2023
@dpc
Copy link
Copy Markdown
Contributor

dpc commented Oct 20, 2023

It's not even that I'm unwilling. rustup doesn't work on Guix, so I would have to manually build rustc versions from Guix's latest to stable, then build rustc nightly and then build rustfmt with it.

There's a price to pay for deviating from mainstream, and you can only pile so many deviations before it falls apart. FOSS community is good at accommodating preferences and experimentation, but ultimately if Guix doesn't have a solution to dealing with rather basic and common requirement, than it's not a practical OS to use. I would expect Guix to be able to roughly do the same thing as Nix.

A dev system is for writing code that will be reviewed anyway and recompiled multiple times. Bootstrapping from 15 bytes base, or whatever was the latest record, is cool and maybe even useful in some use cases, but not very important for working on the code.

And there are also VMs. Qubes users run everything in a VM and they seem happy.

I would actually recommend anyone that thinks rustfmt is great to try develop using only 40 lines of vertical space and see how easy you find it to keep function context while writing code.

I'm using fish, tmux, helix in a 40 line terminal for about everything. That's how my terminal looks ATM. Sometimes, when I feel extra grandpa, I up the font size on time, and it goes to 36 lines.

image

And that's a primitive setup. Kids nowadays have GUIs, folding for code and comments, minimaps and probably conveniences that I don't even know about. Seems like anyone should be able to figure out something to make it workable.

I can't help but be amazed with the level of OCD if so many of them can't stand a piece of code have a slightly different style than the next one

People have preferences. Some, especially after working mostly by themselves or in tiny groups (which was not uncommon before the Internet collaboration became a norm) for decades will have preferences much stronger than

makes me unhappy and not want to read the code anymore at all

@apoelstra
Copy link
Copy Markdown
Member

If you have a workflow where disabling a tool like rustfmt for a certain project is a non-trivial task, I would seriously doubt the quality of the code these people contribute. Seriously it's like complaining a project uses spaces because your editor only does tabs.

It's just an extra inconvenience. I'm sure everyone is able to do this but they'll probably have to do a bit of digging to remember how they set up rustfmt in the first place. Tabs vs spaces is similar but in some languages there is no universal standard so this is a pretty accessible and standard thing to have to change.

I'm confused which single person you're talking about? This thread alone has several people complaining about it..

I also had a bit of trouble parsing @dpc's sentence. I believe he meant: if anybody here complaining about rustfmt isn't a C or C++ developer, please raise your hand. It sounds like that's you.

apoelstra added a commit that referenced this pull request Oct 22, 2023
d391ada ci: nightly rustfmt PR scheduled/manual (Einherjar)

Pull request description:

  ## Summary

  - Removes nightly `rustfmt` checks from CI and `CONTRIBUTING.md`. Superseds #2127
  - Adds automated nightly `rustfmt` PR creations manual and every Sunday 00:00 UTC. Closes #2128. Closes #1712.

  ## Details

  The new `.github/workflows/rustfmt.yml` action does the following:

  1. Checkouts the repo
  1. Install the nightly Rust toolchain with `rustfmt` component
  1. Run `cargo +nightly fmt` which format all the codebase with the nightly Rust toolchain writing the changes
  1. Add the current date in `YYYY-MM-DD` format as an GH action ENV var
  1. If changes are detected, create a PR with the following details:
      - Title: Automated nightly rustfmt (`DATE`)
      - Body: Automated nightly `rustfmt` changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
      - Commit message: `DATE` automated rustfmt nightly
      - label: `rustfmt`

  I did a test run (with `workflow_dispatch` and some minor changes to trigger the PR) in my fork.
  Check how the PR looks in https://github.com/realeinherjar/rust-bitcoin/pull/5
  Of course, all of these things can be changed.
  They are all suggestions...

  ## Security Disclaimer

  We are only introducing a new action, which is quite battle-checked, [`peter-evans/create-pull-request`](https://github.com/peter-evans/create-pull-request) with ~2k stars and proper maintained.

ACKs for top commit:
  apoelstra:
    ACK d391ada
  tcharding:
    ACK d391ada

Tree-SHA512: c8728d03467913005fc92fefd4150a8041a495b0ec08b3a3397a05f381a59ef904e8afe6182509fd295ad1b23875b43206a2f1238a253542da54420e4805ab6f
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Oct 26, 2023

Should this be closed since #2135 was merged?

@tcharding
Copy link
Copy Markdown
Member

We resolved this already with the weekly fmt bot.

@tcharding tcharding closed this Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants