Running rustfmt on rust-lang/rust and other rust-lang repositories

[josh]

Agreed; that’s why I said “amending/rebasing”.

[nikomatsakis]

I don’t think that having tidy perform the check would be that much of a headache. Regular contributors would quickly adjust and have their local environment apply formatting on save or however they prefer. It might occasionally catch them out, but this happens already with tidy (at least for me) when I don’t notice a line got too long every once in a while.

I’m coming around to this POV. It certainly seems no worse than tidy today – and probably significantly better, since you can fix it automatically, and you can configure your editor to do so on your behalf.

[nikomatsakis]

As an even better alternative, however: GitHub now has the ability to submit feedback on a pull request that includes changes the author of the pull request can click to incorporate into their commit.

That would definitely be neat.

[dhardy]

A possible alternative is “format-just-before-commit” rather than “format on save”. It would be accomplished by using git hooks, which in turn can be easily installed by means of a script or some kind of binary.

While initially in favour of this idea, it has a number of drawbacks. Aside from relying on all users to have a compatible version of rustfmt available, it implies that a tool should go in and change your sources after you ran your tests and before making a checkpoint. If rustfmt or your computer barfed at this point it wouldn’t be fun. It might also be problematic during a git rebase.

[petrochenkov]

We can make an experiment and enforce rustfmt on a single crate for a start.
Something that’s actively developed right now - rustc_mir or perhaps librustc.
Thus way the process can be tested without affecting all the codebase and fighting all the accompanying infrastructural issues at scale.

[jjpe]

Not necessarily. Nothing prevents a script from running a test suite in a pre-commit git hook e.g. after running rustfmt, as hooks can run arbitrary code.

[Eh2406]

We can make an experiment and enforce rustfmt on a single crate for a start. Something that’s actively developed right now - rustc_mir or perhaps librustc . Thus way the process can be tested without affecting all the codebase and fighting all the accompanying infrastructural issues at scale.

Historical note: meny of the rust org projects have experimented with this in the past year. Cargo decided that adding a check to CI was to much of a hassle and backed it out. I know I had problems getting my PR to pass CI, as my cargo +nightly fmt would format things differently then the cargo +stable fmt used on CI, but this may be a problem of the past. (@alexcrichton, for more details.) I know that some of the other projects are using it, like Crates.io. It may get more usable as rustfmt stabilizes.

[withoutboats]

As a very irregular contributor to the Rust repo who doesn’t intend to start using rustfmt in my other code, I don’t understand the objections to having bors fmt code when it merges (not just saying it would be harder to implement, but that it would be a bad idea). You’re running CI after the merge anyway, it seems astronomically unlikely that rust fmt could introduce a bug that would not be caught by CI.

My own personal experience is that waiting for CI and then having it fail because of a tidy check is a frustrating waste of time. CI failures are a barrier to entry, the ideal situation would be to have the outcome we want (tidy code that works) without ever giving a contributor a red result. This seems like an axis on which we could achieve that without contributors having to change their behavior.

[alexcrichton]

I personally haven’t found the “CI checks style” strategy to work too well in the past for the reasons mentioned here, it’s just too annoying for all contributors to follow the same standard (matching rustfmt, forgetting to run rustfmt, etc).

I would, however, certainly be an advocate of uniform style across all our crates! I think a great first step would be to simply run rustfmt over everything. We don’t have to run checks that style remains consistent, but it’d be good to get an initial “restyle everything” commit out of the way.

After that’s done, I think we can figure out a good strategy for enforcing style. The ideal end state is that the relevant repositories (including rust-lang/rust) are 100% rustfmt’d at all times, like we have 100% green tests on master at all times. One way to achieve this goal is, like with tests, to block merges on non-conformant style. A different way, however, is the strategy of either rustfmt-on-merge or having a bot push to your PR.

My personal perfect world I think would involve two pieces:

• First, the tidy checks in rust-lang/rust (and all other rust-lang/rust-lang-nursery repositories) will block landing PRs if style is not conformant.
• Second, to make this bearable, a bot would be watching all PRs to all repositories. It would, upon any update of any PR, check out the PR and run rustfmt. If any change was made it would automatically commit to the PR.

Something like this I think is quite nice in that the code is always styled the same way. It’s also relatively painless because a bot will clean up after you if you mess up. Finally, it also allows us to see what’s going on too! Authors can see the activity on their PR and decide to remove the commit and fold it into their own or othewise somehow enable rustfmt in their editor.

I also suspect the amount of work needed to integrate this into bors is basically the same as an independent bot.

[matklad]

I think the main pain point with gating CI on rustfmt is that contributors don’t have “one command” setup to fix formatting. cargo fmt does not always work: you need to install rustfmt first (I think?), and there might be differences between rustfmt versions.

That is, if readme says “run ./x.py format”, and that does everything, including downloading the appropriate version of rustfmt, than perhaps CI gating might be feasible?

I’ve never tried this approach, I want to adapt it for rust-analyzer: https://github.com/rust-analyzer/rust-analyzer/pull/163

[withoutboats]

Something like this I think is quite nice in that the code is always styled the same way. It’s also relatively painless because a bot will clean up after you if you mess up. Finally, it also allows us to see what’s going on too! Authors can see the activity on their PR and decide to remove the commit and fold it into their own or othewise somehow enable rustfmt in their editor.

This seems nice, the only downside is that since the bot pushes to the branch, users who need to push more updates to their PR will have to --force or remember to pull the bot’s updates before they start working, the git shenanigans here could be problematic for contributors who aren’t super familiar with git.

[nikomatsakis]

That is, if readme says “run ./x.py format”, and that does everything, including downloading the appropriate version of rustfmt, than perhaps CI gating might be feasible?

Given that ./x.py test already runs tidy, it could instead run ./x.py format (or that could be a side-effect of tidy), in which case the act of testing (which I hope all contributors do =) would also apply formatting changes.

We could then make tidy run more often, too, so that e.g. ./x.py test src/test/ui would run tidy without an explicit opt-out (--no-tidy). I think that would be fine so long as 99% of tidy errors are getting fixed automatically anyway, no?

[alexcrichton]

True! We could tailor the bot though to post a message when it pushes a commit pointing to documentation for --force and such, and also perhaps docs about enabling rustfmt in an editor

[Nemo157]

Another alternative for a bot based solution would be to generate a fixup commit, but then push it to a different ref and post a message saying that it will be applied automatically during merge, unless you fix the formatting yourself (along with links to documentation on how to setup a nice local workflow for formatting as you go).

For which version of formatting to use it seems like rust-lang/rust could pin to the rustfmt released with the current stage0 compiler, if there are any incompatible formatting changes then bumping that would be a good point at which to apply them. That doesn’t really help with other official repositories though.

[nikomatsakis]

Is there anyone volunteering to write these fancy bots? hint, hint.

I feel like I’d be happy with any of these options, I just want to see this happen.

[davidtwco]

I think that there are two main considerations when making a change like this - how it affects irregular or new contributors and how it affects regular contributors - and what balance to strike between these when choosing a solution. I tend to lean toward making things easier for regular contributors at the expense of irregular contributors up until a point (or only when I think it is a particularly worthwhile change to make).

I’d propose that we have tidy check that code is correctly formatted and fail the CI if it is not. If that happens, then attempt to detect this (in some fashion similar to the log analyzer that is currently in use or an extension of this) and post a comment letting the contributor know that’s why there was a failure.

This comment would inform the contributor that they have two options - run x.py fmt locally which would use the correct rustfmt version and settings (which, as I understand it, would deal with a majority of the problems that people have found in requiring rustfmt on changes) and then push that; or mention @rustfmt apply which would add a new commit to their PR with that done for them.

This allows regular contributors to configure their local development environment to always run rustfmt and never need to worry about it - much like most contributors will already have set up with line lengths and trailing whitespace. If they aren’t comfortable with commits being added by bots unprompted to their PRs then that won’t happen - for these contributors, the workflow won’t change - they might need to modify their local environment slightly to apply formatting, but otherwise, everything is the same as it has been.

This also enables irregular contributors to deal with it themselves in a simple way - running one command and they’ve got the correct result; or have a bot deal with it for them.

By not having a bot automatically add formatting commits, this has a handful of advantages:

• Unrequested formatting commits won’t require PR authors pull and rebuild their own changes.
• Any review feedback won’t immediately be marked as outdated by these automatic commits.
• There will be a clear understanding and simple mental model for when commits are added - only when the bot is mentioned - not automatically, or only when it is detected as being required, etc.
• It avoids any confusion. In my opinion, I think it will be more confusing/frustrating for a new contributor or as someone unfamiliar with Git/GitHub when commits are being added automatically by bots than when CI is failing because of incorrect formatting.

This isn’t a be-all-end-all solution, but I think it is a decent compromise between approaches previously proposed. It could also be started straight away with the introduction of a tidy check and then the bot could be introduced later when it is ready or if there’s a lot of demand.

That said, in response to other solutions proposed above:

• I don’t think we should mix the responsibilities of x.py subcommands by having x.py test also run formatting.
• However unlikely it is that it causes a problem, having code get changed automatically by bors after reviewing doesn’t sit well with me.

[oli-obk]

Couldn’t we just make x.py run formatting on everything before it starts doing any compilation or otherwise?

[davidtwco]

I don’t mind this. Of course, as long as the CI still asserted that the code was formatted before running the build and having the formatting applied by that. Otherwise, someone might push without running x.py and that’d slip in under the radar.

[petrochenkov]

Question: how much time x.py fmt is expected to require?
Running an empty build (e.g. to run one test) already takes too much time, we cannot add another several seconds to that.

[nikomatsakis]

Good question, @petrochenkov. It’s definitely preferable to be rustfmt’ing as you go, and if you are doing that, then ./x.py fmt might be just wasted time. I wonder if we could use git diff somehow to help.