Now that the Rust 2018 release is approaching, I think we should talk about how to apply rustfmt to the rust repository, and how to keep it formatted.
Using rustfmt would have several wins:
Adopt a uniform style, of course
But also resolve problems that contributors have either:
trying to figure out our style (heh, good luck)
accidentally running rustfmt on stuff that drags in a lot of auxiliary diffs
There are obviously two main approaches:
Flag day
Gradual transition
I am partial to “flag day” myself, but I could go either way.
There is another question too: how to keep things formatted? In my ideal world, this would be fairly painless, but I’m not sure how best to achieve that (one idea I had was a rustfmtbot that runs rustfmt on people’s PRs and pushes commits automatically, but that does not exist afaik).
Running Rustfmt on CI is suoer-easy, though it is probably a bit more interesting on the rust repo since it is not a simple Cargo project. rustfmtbot, is easy on the rustfmt side - there is already a version which knows about git diffs, but I'm not sure where that could run and the GH integration (I'm pretty sure that's is not very hard though). Still non-trivial work.
One thought I had is that Homu could run rustfmt on its merge commits. So we don’t have CI failing because someone forgot to run rustfmt, but master is always formated.
Yes, I would be ok with this, though I sort of prefer the rustfmtbot idea – in particular, if there is a bug in rustfmt, it would be detected more readily if there is an actual commit that gets tested as normal (vs being “hidden” in a homu commit).
Mostly though I don’t want to “hassle” contributors to run rustfmt – but maybe it’s ok if we can get everyone to enable “rustfmt on save”.
What does it mean to “run rustfmt on CI”? What happens if the code is ill-formatted?
Rustfmt has a check mode which just gives you a true/false answer as to whether the project would be changed by a full rustfmt or not. I.e., a PR which has not been correctly formatted would fail CI.
OK, I see. I guess it would be quite similar to our x.py tidy scenario today then in some sense. I was hoping we could make it more automatic but then…maybe it’s fine.
I’m not opinionated about the specifics, but I’m partial to any solution that doesn’t fail CI if things are ill-formated and instead has bots deal with it. That saves everyone time and manages to keep the code uniform (and diffs smaller). I think having bors running rustfmt after PRs are run could be better because it reduces the need for having to pull changes to your own PR that you never made.
I guess the real question is whether you trust rustfmt 100% not to mess up code correctness.
To maintain the not rocket science rule, every commit posted to master should already be tested. Even if we want to keep the format commit separate from the bors merge commit, we should still have the format commit before the test.
Speaking hypothetically based on pseudoscript and a regular cargo project, the bors workflow shifts from
git merge
if cargo test
git push
to
git merge
if not cargo fmt --check
cargo fmt
git commit
if cargo test
git push
This “should” be a relatively “small” change: add a rustfmt invocation to bors after the merge, and commit if changed.
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.
The tradeoff here it that it would require committers to install the git hooks once, but the upside is that there is no deluge of “formatted the source code again”-like commits. Those separate commits are noise and I don’t think anyone inherently wants them.
And then there’s the point @CAD97 made that commits to master should already be tested before the commit takes place; formatting commits go against that full-force.
Instead of the user downloading the hook and running it, I would prefer if bors is integrated (somehow, not sure if this is possible) with rustfmt so that it runs rustfmt and pushes a commit with the changes before merging or perhaps on a bors: try so that we can identify if there is a bug with rustfmt (though we may need an override if it is a bug and we don’t want to be blocked by it).
I much prefer having tidy ensure that PRs have been formatted correctly than have it happen automatically when merged via a bot. There’s something about having code changes made automatically by a bot during merging that seems off to me but if there’s precedent then I don’t mind that much.
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. It would be a little bit of a hurdle for new contributors, but it certainly wouldn’t be insurmountable and is quite easily explained - a x.py fmt command that does it for you could handle it for irregular contributors who don’t have a development environment set up with all the bells and whistles.
I’m also not partial to the idea of a new commit being added. As soon as we’re getting new commits added by bots that means that my local build no longer matches the PR - it should be functionally equivalent but it won’t be the same diff being reviewed.
This means after pushing any PR (unless I’m running rustfmt locally?), I would need to pull and rebuild straight away. This could be mitigated by only adding the rustfmt commit after approval but it still complicates things like rebasing - do I rebase with or without the rustfmt commit? What if the rebase of the rustfmt commit ends up not being the same as if the code was now formatted again? Does another commit get added by the bot?
It just seems a little contrived to me. I like to keep the invariant that my local builds match the current state in my PRs (useful if asked to quickly run a test against the branch, for example) and this adds another build per PR to do that.
I’m not completely against this idea but I’m not convinced it is easier for someone to regularly contribute to work around than a tidy check would be.
I would also very much prefer to have CI verify proper formatting rather than changing formatting.
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. What if the hypothetical rustfmtbot submitted review feedback (and indicated that the changes were required) that included those suggested changes, allowing either applying them with one click or rustfmt locally, amending/rebasing, and re-pushing?
that the tests cover any semantic changes I guess. 