Clarify internal programming style

[varkor]

Every so often someone will submit a pull request that is the result of running rustfmt on a particular file, or set of files (examples: #49360, #38567, #30684).

Presumably we want to encourage fixing mistakes in the code style, though at the moment rustfmt can produce undesirable changes (e.g. see the comments in #37349). Without guidelines, these can either be merged directly, or it left up to the individual reviewer to decide which changes are preferred.

It seems to me it would be better to do one of two things:

• Add rustfmt to tidy and ensure code always adheres to one style.
• Explicate style guidelines, and then only allow PRs fixing mistakes that haven't been spotted from earlier changes (for example, if rustfmt is too strict or not good enough). I.e. not changing style choices that were the original intention of the author if they're not specified by the style guide.

At the moment, I think style changes are being made, through blind use of rustfmt, that leave the code in a worse state than before. I think it'd be preferable if we could avoid this; does anyone else have thoughts?

[steveklabnik]

The intention has always been that, when rustfmt is ready, projects by the Rust team will use rustfmt with no configuration for its projects. However, rustfmt isn't ready yet, so we haven't gotten there yet.

I think it'd be preferable if we could avoid this; does anyone else have thoughts?

Generally, discussions are best had on https://internals.rust-lang.org/ rather than the issue tracker; I'll leave @rust-lang/compiler to decide if this should be moved there, though.

[varkor]

However, rustfmt isn't ready yet, so we haven't gotten there yet.

In this case, it would seem sensible to discourage "Run rustfmt on [X]"-style PRs in general.

Generally, discussions are best had on internals.rust-lang.org rather than the issue tracker

Ah, that's true. I was thinking of it more as a "contribution documentation" omission (i.e. that this should be made clearer somewhere in the repository), but more nuanced discussions (if anyone would be opposed to stating that these kinds of PRs are discouraged until rustfmt better fits rustc's use cases) would best be taken elsewhere.

[steveklabnik]

In this case, it would seem sensible to discourage "Run rustfmt on [X]"-style PRs in general.

Historically, this has been helpful to point out issues in rustfmt to get it to be good enough to get to 1.0 :)

I was thinking of it more as a "contribution documentation" omission (i.e. that this should be made clearer somewhere in the repository),

Yes, this is exactly why I didn't close it; if the team wants to document some kind of policy, this is totally a bug, but if there's more disagreement about the policy than I'm aware of (as I'm not on the team) and it needs to be discussed before documenting it, then it'd better be on internals. We'll see!

[ehuss]

Cargo recently attempted this, but just today it was disabled (rust-lang/cargo#5278). It can cause friction with PRs since most people aren't used to running rustfmt all the time.

[oli-obk]

Clippy tried it a while back, but we disabled it out of the same reason that cargo did so.

Options that I see:

• have a bot that pushes a rustfmt commit after every not formatted commit (on open PRs )
• have bors rustfmt before test
• run rustfmt regularly with a bot and open rustfmt PRs

[nrc]

We might also be able to solve this on the rustfmt side by pinning to a version number.

[nrc]

At this stage I think that rustfmt formatting is stable enough that re-formatting crates is unlikely to cause much churn, therefore it is fine to land formatting PRs. However, I wouldn't go so far as to encourage it because reviewing and landing them can be a pain.

[nikomatsakis]

Closing in favor of discussing on internals