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

[cuviper]

Any local x.py formatting should not be required for non-dev builds, please, like distro builds from source tarballs. Or at least, it should be possible to opt out.

[Eh2406]

However unlikely it is that it causes a problem, having code get changed automatically by bors after reviewing doesn’t sit well with me.

But that is exactly what bors is designed to do, it runs a git merge with master creating code that has never been seen before in the universe. If the merge result was unchanged from reviewing then we could just merge if CI is green.

[davidtwco]

That’s a good point. I don’t know, there’s something different in my mind about performing a git merge compared to running a rustfmt, doesn’t feel right - I don’t think it’s necessarily logical.

[mark-i-m]

I think @davidtwco expressed my thoughts.

I only have one other question: are we concerned about the git blame being messed up? If so a potential solution is to only run rustfmt on files touched by the PR. We could then turn on the CI check everywhere and have regular contributors rustfmt the parts they normally touch.

[nikomatsakis]

I only have one other question: are we concerned about the git blame being messed up?

I personally am not very concerned about this. In any case if we do a “flag day” where we rustfmt everything, then from that point forward it wouldn’t be a problem, right?

[nikomatsakis]

I like the @rustfmt fmt idea. =)

Bonus points if we can setup the bot in such a way that random Rust projects can easily use it for their repositories.

[mark-i-m]

Yep, that’s true. I don’t have strong feelings one way or another. I just wanted to bring it up since it didn’t seem to have been discussed anywhere.

[mark-i-m]

@nikomatsakis What was the timeline you were thinking of? I have kind of wanted this for a long time because I have rustfmt-on-save in my editor, and it keeps messing up my PRs.

[gnzlbg]

Why do we need a bot at all?

I mean, we already have a tidy script that checks formatting (in some sense) and breaks the build.

Couldn’t we just update that script to use rustfmt --check instead, and maybe give the script an option to apply the formatting ?

A flag day with a modification to the tidy script and a reformat of the code base appears to me to be the next incremental step. We could add a bot that keeps PRs automatically formatted but that sounds like an orthogonal thing to do, and I could see how that could be annoying. Maybe instead of modifying the PR, the bot could just comment saying that the PR does not pass the style check, and block running travis on that passing to reduce travis load.

[nikomatsakis]

Regarding timeline, I see a few options:

• Now.
• A few weeks after Rust 2018 is released.

The reason to maybe wait is that I expect us to backport fixes to the current beta (Rust 2018 RC2) and it will ease backporting if we don’t have huge format diffs everywhere (i.e., because master has been rustfmt’d).

On the other hand, one can always do that by – when preparing the backport – first running rustfmt on the affected files in the beta branch. Assuming you trust rustfmt, that ought to be zero-risk, though in terms of LOC affected it looks risky.

[Riateche]

What about rustfmt updates? They can cause changes in formatting all over the repository, so updating to a newer rustfmt version could be as painful as introducing rustfmt in the first place. Is it planned to pin rustfmt to a specific version?

[davidtwco]

Presumably when the rustfmt version in use is updated, that same PR would apply any changes that entails to the formatting.

[Centril]

A few weeks after Rust 2018 is released.

This is my preferred option;

We similarly delayed the https://github.com/rust-lang/rust/pull/53654 until after the edition, so we should wait and do them in sequence and avoid closing the tree before.

[nikomatsakis]

We similarly delayed the https://github.com/rust-lang/rust/pull/53654 until after the edition, so we should wait and do them in sequence and avoid closing the tree before.

Oh man, that PR is so exciting.

Yes, that makes sense to combine them.

[sanmai-NL]

rustfmt as Git pre-commit hook is an option. Can be invoked from CI too.

[sanmai-NL]

I suppose it should be pinned to some version constraint and rustfmt shouldn’t change its output unpredictably (e.g., given some versioning rules).

[mark-i-m]

Yes, that makes sense to combine them.

Do you mean do them with the same lock of the tree or do them in the same PR? I would not do them in the same PR. It would be an impossibly huge diff to check.

Also, depending on how much you do or don’t trust formatting tools, it might make sense to do it one file at a time: as files get touched, we require them to be formatted from then on.

[mark-i-m]

@nikomatsakis Are there also plans to run rustfix and switch to 2018 in the compiler?

[Mark_Simulacrum]

I don’t believe it’s been discussed at all, but I do think that is likely to have similar levels of impact as with rustfmt so we’ll probably try and schedule them for around the same time. I hope to do so right around new years (January 1st/December 31st) but we have not yet decided on anything concrete.

[mark-i-m]

Perhaps we can attempt to set a date now?