It's incredibly annoying to make a small code change, which to a human is perfectly well formatted and readable, only to have the PR rejected by a rustfmt check. And not because the code is wrong in any way, but because some fuzzy logic crossed some threshold, and now rustfmt wants the code formatted in a different way.
It takes a long time for the CI to run and fail on a triviality, and a tiny PR turns into a back-and-forth.
Rustc needs nightly rustfmt, and nightlies are flaky. Rustfmt in today's nightly fails with 'internal error: entered unreachable code'.
Unlike gofmt, which is a gentle little helper, rustfmt is incredibly harsh, nitpicky, and totally uncompromising even about the most trivial and irrelevant details of formatting.
gofmt is deterministic and idempotent. rustfmt is too, but goes way beyond that to making formatting and AST relationship bijective, and forcibly stripping code from all human judgement to the point of requiring unnecessary or even absurd changes.
Please don't require rustfmt. It's an incredibly inflexible tool that unnecessarily rejects good code.
I totally get where you're coming from. I'm torn between two different things:
Ensure PRs are always rustfmt-clean before merging
I'm so excited I just finished working on this PR! The tests are all passing, so I'm going to push and see if CI agrees. GAAAAH RUSTFMT!!!11@$
What I'd really love is a bot that detects you didn't run rustfmt and then runs it for you. Here's some discussion about supporting that w\ GitHub Actions:
I, for one, am happy that rustfmt is used. I can't remember the formatting style that rustc prefers (it differs from my own and from my $dayjob). I'm happy to let rustfmt take care of that for me.
There might be ways to improve rustfmt or the way it's integrated into rustc. But I'd rather discuss how the situation can be improved rather than discuss removing rustfmt.
rustfmt does a number of things I don't like either (I've filed issues for some). I, like you, also strongly prefer how gofmt works.
I went a long time without adopting rustfmt. But people kept submitting PRs with rustfmt applied on the code, which would contain irrelevant changes that had to be backed out. Then folks had to disable their rustfmt-on-save editor hooks. It was a giant pain. So that combined with my mild-dislike-but-also-appreciative attitude toward rustfmt made me switch.
I gate PRs on rustfmt in CI because the alternative is much worse. You wind up merging PRs without rustfmt applied, which then means subsequent changes to those files will contain more irrelevant formatting changes. This clutters history at an undesirable frequency. (Some amount is unavoidable, since rustfmt itself changes how it formats code very occasionally.)
It might be a good idea to build a gofmt-style code formatter before suggesting folks to reject rustfmt.
I believe that with the parser/syntax tree of rust-analyzer building such a formatter wouldn't be prohibitively complicated. I would maybe estimate it as two weeks of my time (with all the usual software estimate caveats applied). There is a stale effort to do this thing here (see also this isse). I don't have a bandwidth right now to mentor this work, but the issue & PR already contains a ton of design notes, so, if someone just submits a 4k lines (another ballpark estimate) PR with reasonable API, sound-ish intenral design and good test coverage, I'd be happy to maintain it
rustfmt is deterministic and idempotent. I didn't mean to say otherwise.
The need to be deterministic was given as justification for rustfmt's ruthless reformatting of all code, but I'm trying to say that a code formatter can be deterministic and not be unrelenting like rustfmt. Rustfmt changes more than it has to, and it's stricter than it has to.
Specifically, gofmt never changes a multi-line version of code to a single-line version. That is still a deterministic and idempotent behavior (same source gives same output, repeated application doesn't change code). But because it takes into account author's intention, and only corrects execution (such as spacing), it's gentler and non-disruptive. It's helpful — it guesses how I wanted code formatted, and makes it neat. gofmt is cooperative.
OTOH rustfmt purges all formatting information and replaces it with its mechanic heuristic. It will end up compacting multi-line constructs into long single lines whenever that fits its line-length settings. And the rustfmt --check can't even allow both valid options at the same time. If the code is just one character above or below a limit, it's has to be reformatted. No room for gray areas, no room for human judgement.
As part of the CI for the time crate, I have rustfmt enabled, as well as clippy. At least with GitHub Actions, it it very clear what is failing, as I have those two separate from the build & testing. It would be obvious if it was only the formatting causing a build to fail.
Personally, I don't see a big issue with it. It only has to be done in a single commit before the PR is merged. Could rustfmt be better? Of course — that's why a ton of people (including myself) require nightly. However, until there's a better alternative, using rustfmt is the only sensible option to me. I prefer my codebase to be tidy, and rustfmt is the only thing that can systematically demonstrate that.
Or as I have suggested before while bors is merging it can run rustfmt on the results. PRs do not need to be formatted, master is always formated. (does not help with other project.)
This is my biggest concern and why I never adopted the tool for any project. Too often it is more sensible to group some parameters for functions and struct members but still give others new lines. Too many arrays where the layout of elements is more readable if manually adjusted (e.g. bytes in a particular packet). The larger the code base, the more likely it becomes that you will prefer short-heuristic in some structs but still keep the readability benefit in others. And I could go on with line width flexibility, etc. In all the cases there is no 'right' answer based on static analysis but instead it's based on domain knowledge and human input.
To stick another voice on it, you want ./x.py fmt. This will use the pinned, in-tree version of rustfmt, which is both a) guaranteed to work (as CI is gated on it working) and b) won't disagree with ./x.py tidy, like a local version of rustfmt might.
When working in the rustc tree, the current state is that you have to use ./x.py test (which always runs tidy, even when narrowed) rather than cargo test. Even if you can/do get away with cargo-driven tests rather than bootstrap-driven tests, when in the rustc tree you should get into the habit of using ./x.py tidy.
As for making things better:
Enforced-rustfmt has been a net-positive for the rustc tree, because it eliminated human formatting nits and let the repository with ages of history follow a consistent formatting style.
But I'd agree that making a tidy fail not immediately halt the rest of CI fail would also be a positive. Rather than "fix tidy errors and try again," we could actually point out a useful error (or pass), with an additional note of "fix tidy before we allow merge".
The problem is that this requires some sort of "yellow" state for the PR CI. For smaller repos on GitHub Actions, this is typically done by checking style in one job and tests in another. For a repository at rustc's scale, however, splitting jobs arbitrarily just for partial CI feedback can result in a large impact on server time (thus cost), so I don't know how tenable that would be with the current CI architecture.
Yes there are all sorts of ways to avoid forgetting to rustfmt before committing, BUT...
rustfmt is work a robot can do, so instead of giving people instructions, and failing their PRs if they don't follow them, a robot could automatically apply the fixes instead.
How hard do you think it would be to create a bot that did this? I imagine it would be well-received in the actions-rs GitHub org (though only one member is public).
I have wanted to have each commit be separately reformatted (if possible; if syntax-level errors occur then we can't, but that should be rare) by bors or so during merges.
However, this adds a level of complexity and potential for problems in debugging, since the diff you see in GitHub and locally isn't the final diff. I also personally have rustfmt in my editor on every save, which makes me never forget formatting.
I agree though that we can, and should, do better here - at least providing tooling that formats PRs when asked to. I think triagebot would be a good place for that, though I suspect it would be a fairly large addition and we'd want to be quite careful with how best to implement it.
For now, I think we should strive to make x.py fmt more discoverable (or x.py test --bless). I would appreciate PRs improving any docs or error messages from folks who were lost in this thread.
For what it's worth, a fmt failure when running ./x.py test (with any arguments that don't supress the fmt check) is quite clear. I don't recall the exact error, but when fmt fails (locally, at least), you get an error that does say something along the lines of "fmt check failed; run `./x.py fmt`.".
Forgive me for reading between the lines, but I believe the disconnect is that @kornel has been using cargo directly rather than go through bootstrap (reason for claim: talking about cargo +nightly fmt sometimes failing and inability to use cargo +stable fmt).
I don't know the exact status of how supported using a nightly toolchain cargo rather than bootstrap to test things in the rustc tree is, but I think that's where the most friction is. If we can somehow make cargo +nightly test in a directory also do the other non-cargo tests that ./x.py test covers (e.g. tidy, rustfmt) for the directory being tested, that would be a large improvement. (And it'd properly allow more "hack with cargo, final test through bootstrap" workflows to work in more cases.)
TL;DR either we should say directly using cargo isn't supported in the rustc tree and push people harder to use x.py, or we should say using cargo to test a single component is supported and push to make ./x.py test libfoo and cargo test -p foo execute the same tests (including tidy).
