Rustfmt stability

[nrc]

With any luck, Rustfmt 1.0 will happen very soon. The Rust community takes promises of stability very seriously, and Rustfmt (due to being a tool as well as a library) has some interesting constraints on stability. Users should be able to update Rustfmt without hurting their workflow. Where it is used in scripts or on CI, updating Rustfmt should not cause operational errors or unexpected failure.

Some changes would clearly be non-breaking (e.g., performance improvements) or clearly breaking (e.g., removing an API function or changing formatting in violation of the specification). However, there is a large grey area of changes (e.g., changing unspecified formatting) that must be addressed so that Rustfmt can evolve without hurting users.

The goal is for formatting to only ever change when a user deliberately upgrades Rustfmt. For a project using Rustfmt, the version of Rustfmt (and thus the exact formatting) can be controlled by some artifact which can be checked-in to version control; thus all project developers and continuous integration will have the same formatting (until Rustfmt is explicitly upgraded).

I propose two possible solutions: versioning is handled by Rustfmt (by formatting according to the rules of previous versions), or versioning is handled by Cargo (by treating Rustfmt as a dev dependency).

Rendered
Tracking issue

[Screwtapello]

I suspect that anybody worried about rustfmt changes breaking their CI should be equally worried about updates to rustc and cargo. Since all three come from the same source (rustup), it seems reasonable to expect people to pin them all in the same way.

QUESTION - how do we distinguish API breaking changes from major formatting changes?

I feel like major formatting changes probably are API breaking changes, since a formatting change implies new configurable options to tweak, and if the new options can't be set to mimic the previous behaviour then they can't default to that behaviour, which means the API has changed in an incompatible way.

QUESTION - could there be incompatabilites with the toolchain (e.g., Rustfmt at the version specified can't handle a Rust feature used in the project)?

I guess, if (for example) a new syntax feature like async/await or impl Trait was added to Rust and nobody got around to teaching rustfmt about it during the nightly and beta phases. I feel like that should be avoided anyway, though, regardless of the documented compatibility guarantees.

QUESTION - how do we handle RLS integration?

RLS comes from the same place as cargo and rustfmt, so it's presumably built from the same source and should always be compatible with the standalone command.

[SimonSapin]

I suspect that anybody worried about rustfmt changes breaking their CI should be equally worried about updates to rustc and cargo.

For projects using unstable features definitely. Otherwise, not worrying about this is the point of the whole stability thing.

[Screwtapello]

"Worrying" is a relative concept. Rust's tolerance for incompatible changes is not quite zero, and stable-channel breakage does occur occasionally. As a concrete example, I recently encountered a project that broke because the standard library added a trait implementation.

While that project was easy to fix manually, rustfmt breaking is even easier to fix, since you can just run cargo fmt and commit the result without having to think. If anything, people should be more worried about rustc and cargo changes than rustfmt.

[joshtriplett]

I don't think we need to rev rustfmt's major version if formatting changes somewhat. With respect to formatting alone (ignoring API changes and similar), I would suggest the following versioning scheme:

• Micro version: changed for minor bugfixes, with little to no impact on the formatting of code other than in unusual corner cases addressed by such bugfixes.
• Minor version: changed when introducing formatting of new language constructs that didn't previously exist, or when changing formatting in non-trivial ways. (For instance, if we came up with a new heuristic for one of the heuristic-driven bits of formatting.)
• Major version: should likely never change due to formatting alone, only if some API break occurred.

[Diggsey]

I think the largest pain-point here is for contributors to projects using rustfmt as a CI step (diesel being one example of a project doing this).

In this case, the user has to make sure they're using the exact same version of rustfmt as is running on CI or else they'll have to manually apply the formatting changes. When contributing to multiple projects with differing rustfmt versions this is quite annoying.

Anything that can be done to make this case easier would be great.

[Ixrec]

I can't think of any way to remove the hassle of "my rustfmt doesn't match CI" other than forcing rustfmt to support a --ruleset or --version or --edition argument and just guaranteeing that rustfmt --ruleset 1 will always produce the same source code edits on any input in all future rustfmt versions, maybe modulo bugfixes or new syntax support, and let each repo have a .rustfmtrc file specifying the default ruleset. Not sure if I actually want that, but no one's suggested it yet.

[killercup]

the user has to make sure they're using the exact same version of rustfmt as is running on CI or else they'll have to manually apply the formatting changes. When contributing to multiple projects with differing rustfmt versions this is quite annoying.

@Diggsey is this really annoying? I've faired quite well with cargo +stable fmt. (Assuming I can easily see which toolchain is used on CI when running rustfmt). I imagine that if this was a seamless experience (using internal versioning), the most obvious pain point for contributors would go away.

[nrc]

I suspect that anybody worried about rustfmt changes breaking their CI should be equally worried about updates to rustc and cargo. Since all three come from the same source (rustup), it seems reasonable to expect people to pin them all in the same way

rustc and cargo have very strong backwards compatibility guarantees and we should have them for rustfmt too.

I feel like major formatting changes probably are API breaking changes...

I was referring to changes to formatting which don't add or change configuration options, i.e., affect users using the default options

While that project was easy to fix manually, rustfmt breaking is even easier to fix, since you can just run cargo fmt and commit the result without having to think. If anything, people should be more worried about rustc and cargo changes than rustfmt.

This is true, however, the case we're trying to avoid is that a PR fails its CI because the Rust version (and thus Rustfmt version) got bumped, we'd like some kind of pinning or back compat guarantee which means users have to opt in to breakage.

I don't think we need to rev rustfmt's major version if formatting changes somewhat. With respect to formatting alone (ignoring API changes and similar), I would suggest the following versioning scheme

This is ignorning the CI use case in the motivation. Consider CI under this proposal: the repo is formatted using rustfmt 1.0, the CI checks each PR is formatted correctly using rustfmt 1.0. A Rust release happens and that bumps rustfmt to 1.1 which changes its formatting algorithm. The next PR to run is still using rustfmt 1.0 and now fails the CI, this innocent contributor has to format the entire repo for their PR to pass CI. Similarly, if someone downloads the repo and runs the CI locally with rustfmt 1.1, they'll see that the repo is failing tests.

is this really annoying?

We're hearing from multiple users that this is very annoying. I think it can be addressed with either pinning or internal versioning though.

[Screwtapello]

I guess I fundamentally disagree with/don't understand the premise. Right now, rustfmt is bundled with rustc and the rest of the toolchain; while I guess you could do something like install two separate toolchains and use rustfmt from one and rustc/cargo from the other, it's not generally practical to opt in (or out!) to a rustfmt upgrade separately from rustc. It doesn't much matter whether rustfmt bumps its major, minor or micro version, the user can't make an informed decision whether to upgrade because they don't get to make that decision at all.

So if we assume that rustfmt's version number is effectively identical to rustc's version number (much like cargo's is, these days), we can talk about the CI use-case in terms of "how much formatting churn should rustfmt allow from version to version?":

• None: Every formatting change must have a configuration option whose default replicates the previous behaviour. Instead of, or in addition to individual formatting options, there may be an omnibus option that changes many aspects of formatting, mirroring the edition="2018" setting in Cargo.toml. This is the friendliest option for end-users, but involves the most work for rustfmt maintainers.
• All: Although rustc has a strong compatibility guarantee, it (deliberately) does not guarantee perfect compatibility, but allows incompatible changes that are easy to fix. All automated formatting changes are easy to fix (you just re-run the formatter), therefore all formatting changes are permissible. People who are annoyed by this should be pinning rustc (and therefore rustfmt) anyway.
• Some: Part of the reason people are OK with rustc's imperfect compatibility guarantee is that it rarely causes problems, so if rustfmt rarely causes problems people are likely to be OK with that too. However, rustc achieves this by only changing the meaning of semantically rare pieces of code; Rust's syntax is simple enough that there aren't many syntactically rare constructs for rustfmt to fix. In practice, this option probably means nominating one release every six months as The Release With Formatting Changes, and postponing merging such changes until then.

I think I'd be happy with any of those scenarios.

[nrc]

So if we assume that rustfmt's version number is effectively identical to rustc's version number (much like cargo's is, these days), we can talk about the CI use-case in terms of "how much formatting churn should rustfmt allow from version to version?":

To clarify, you don't need different versions of rustfmt, in fact I'm not even considering that case. The case I'm considering is just a regular 6-weekly update, say from version 1.29 to 1.30 (Rust and Rustfmt versions).

So just like rustc there aren't API breaking changes, but there are other important facets of stability. For example, imagine if from 1.29 and 1.30 we swapped the meaning of true and false - no API is changing, but a whole lot of code would start failing tests. Likewise if we let formatting change between 1.29 and 1.30, then a whole bunch of tests would fail, and I propose that that is not acceptable as a backwards compatibility consideration.

Some

This is basically what I am proposing, except that the 6-monthly breaking release would actually have a major version number change.

[topecongiro]

Thank you @nrc for writing up the RFC!

I would rather manage stability externally, as doing it internally seems to add a considerable complexity to the code (e.g. gating formatting fixes based on versioning - what happens when we decide to revert the change after the later version?).

I would like to propose supporting different level of stability in different channels: rustfmt on stable channel should be as stable as possible, whereas rustfmt on nightly channel may introduce breaking formatting changes in minor version updates.

I would suggest the following version scheme after 1.0:

• Major version: API breaking changes and/or formatting changes. On stable rustfmt, any formatting changes should only be introduced through major version update.
• Minor version: formatting changes, critical bug fixes like panicking on certain kind of code and/or supporting a new syntax. On stable rustfmt, we have no minor version update.
• Micro version: critical bug fixes and/or supporting a new syntax. No formatting changes should be introduced with micro version updates.

E.g. rustfmt-stable 1.0.13 and rustfmt-nightly 1.2.1.

We should only bump the major version once a few years or so (maybe when the new epoch is introduced?). We do not support stability between major versions (no LTS release). We do not support stability between minor versions (e.g. rustfmt-nightly 1.3.0 does not support rustfmt-nightly 1.2.0).

The benefit of the above scheme is that

• Users can easily benefit the stability support of rustfmt in the stable channel with ease, as users need not specify rustfmt version
• Maintainers can keep updating rustfmt incrementally in the nightly channel

The downside I am aware of is that

• Formatting changes that comes with major version updates may be quite large
• Users using stable channel cannot benefit from formatting fixes without major version releases (though AFAICT we are opted to sacrifice this in favor of stability)
• Users using nightly channel cannot benefit from stability guarantee
• Backporting could be rough

[nrc]

Thanks for the comments @topecongiro !

The downside of the external scheme is that we would have to maintain multiple branches (possibly only two, depending on how we decide on the details). In that scenario we would have to backport, and I don't think it would be any worse in your proposal than in my external proposal.

The major downside I see to your proposal is that Rustfmt wouldn't ride the trains in the usual way for Rust components. I'm not sure how we would implement that. I would also worry that minor version increments would not get enough testing. It could also cause people problems on CI - they would not be able to run Rustfmt on CI on all branches even if they can build on all branches because it might require different formatting. That is not the end of the world, but would be confusing.

Maintainers can keep updating rustfmt incrementally in the nightly channel

I expect maintainers would build from source?

We could also distribute a different version on Cargo vs Rustup.

Minor version: formatting changes, critical bug fixes like panicking on certain kind of code and/or supporting a new syntax. On stable rustfmt, we have no minor version update.

I think we need to backport bug fixes and new code formatting to the stable version, however, we cannot change the formatting.

[nrc]

@rfcbot fcp merge

Summary: The RFC proposes handling versioning internally to Rustfmt and having users opt-in to breaking formatting changes (similar to how the editions will work for Rust). This means running Rustfmt on a project in CI versus locally should have the same results no matter which version of formatting is used.

[rfcbot]

Team member @nrc has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Manishearth
• @fitzgen
• @japaric
• @killercup
• @nrc
• @steveklabnik
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Manishearth]

Might be worth clarifying, this is strictly talking about the existence of options and not their behavior.

As it stands this line is a bit confusing, at first I interpreted it as "changing how stable options format code", which is not a breaking change as long as the options that are changed are not the default ones.

[Manishearth]

@rfcbot reviewed

[gnzlbg]

I've raised my concerns on the internals announcement (https://internals.rust-lang.org/t/2018-edition-end-of-week-post-2018-08-04/8123/2?u=gnzlbg). They can be summarized in: if rustfmt isn't currently good enough for the rust-lang and rust-lang-nursery crates, why should it be good enough for users?

rustfmt is a great tool, but it still feels beta quality at best:

• it cannot really format macros (playground )
• it cannot check that code is properly formatted: that is, if you all cargo fmt --all and then call cargo fmt --all -- --check, the first command can succeed, formatting all your code, but the checking command does still fail ([stdsimd] cargo fmt --all succeeds but subsequent cargo fmt --all -- --check fails rustfmt#2787 )
• it cannot format some of the rust-lang-nursery crates without breaking their build, or breaking their tests (or both): https://github.com/rust-lang-nursery/rustfmt/blob/master/.travis.yml#L36

I use rustfmt every day, and on CI, yet I still need to format a lot of code manually (and cargo fmt --all -- --check can return succeed both for the automatically formatted code, and then also when I manually format it to fix issues like with macros - this doesn't feel idempotent).

I would rate the experience of using rustfmt for 1) formatting code, and 2) verifying that code is properly formatted as frustrating.

A rustfmt 1.0 release conveys that the tool is good enough for end users, but I'd bet that if we tried using it on rust-lang/rust to format the whole code base and then on CI to reject improperly formatted PRs we would turn the enforcement off in a week because of the amount of frustration that this would generate due to all current issues with rustfmt.

If we can't dogfood it yet, doing a 1.0 release just feels wrong and the only thing it will achieve is frustrate end users and generate reactions about how bad rustfmt when compared to gofmt or clang-format. Having been an early adopted of clang-format on a large C++ code base, it was really bad at its job when it was released (it had lots of bugs), but it was better than rustfmt currently is: it could format all of the C++ language, even though not very properly, the result made sense, checking the formatting worked, and it did not break code.

[Manishearth]

I don't think it not being dogfooded in rustc or servo means anything relevant.

Rustc and servo are huge. Moving them to rustfmt is a nontrivial process if you want to review the code, and it will also break all the PRs in flight.

The reason servo hasn't rustfmted yet isn't that rustfmt isn't ready, it's that we haven't really decided what defaults we want and that's low priority. I brought this up once on the mailing list.

It's used on most of the other small crates.

[burdges]

It's best to do nothing than to mess stuff up: I'd think breaking builds/tests should prevent rustfmt being declared v1.0, irrespective of whether it's used on some distinguished project or not. Also, if it mangles simple macros, then it should avoid touching macros all together, unless users specifically request macros.

[killercup]

@rfcbot reviewed

[japaric]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[mathstuf]

A rustfmt 1.0 release conveys that the tool is good enough for end users, but I'd bet that if we tried using it on rust-lang/rust to format the whole code base and then on CI to reject improperly formatted PRs we would turn the enforcement off in a week because of the amount of frustration that this would generate due to all current issues with rustfmt.

Note that it is possible to automate the reformatting process (without introducing new commits or other such things). As deployed, this is currently only done when requested rather than as soon as errors are found. The logic we use to do this starts here though there are some parts "hidden" in dependent crates (namely git-work-area) which makes the operation performant even on gigantic repositories. I don't know what kind of work it would take to implement this in bors (and that tool doesn't support Github…yet). It does assume the formatting tool is idempotent though.

[alanhdu]

This is jumping in a little late, but I want to make an argument for externally-managed versions for a couple of reasons:

Minor formatting changes are pretty common (both in rustfmt but in other tools too)

I've used a couple of different auto-formatters before (mostly YAPF and Black for Python, gofmt, and a little bit of clang-format), and I've found that minor formatting changes happen fairly regularly. For the Python auto-formatters (possibly because they're new), there are probably minor formatting changes every two months or so. Go, for both 1.10 and 1.11 made formatting changes to go fmt.

Looking at the Rustfmt repo and looking at the issues tagged with "poor-formatting", and assuming closing those issues means making formatting changes, it looks like Rustfmt also makes minor formatting changes at least once a month.

If I understand the current proposal correctly, that means that either rustfmt would have to release major versions pretty regularly and handle a lot of complexity about choosing the right "format version" internally, or it would have to batch formatting fixes together at a much slower cadence than their actually fixed. Neither one of those seems that nice to me.

For CI, I think pinning a rustc version is ok

I agree with @Screwtapello that in-practice, I think that rustfmt is going to be mostly tied to the version distrusted through rustup (which is linked to the rustc version). At least, that's how my CI works for Rust and how my CI works for go.

Right now, for both Rust and Go, I only run rustfmt for a fixed language version (e.g. 1.25 or the 2018-08-01, and not stable or nightly). I then opt-in to formatting changes by upgrading my rustc version that I run rustfmt with. I think this model works great, and I'm not sure why

if we let formatting change between 1.29 and 1.30, then a whole bunch of tests would fail

should be true (if the "test" is just that cargo fmt --check fails), when it's so easy to automatically fix the tests on upgrades.

(In Python, we just directly pin yapf==0.22.0 or whatever in our development environment, which also works great).

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Centril]

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#54504