Changing rustfmt from submod to subtree

[calebcartwright]

rustfmt is currently a submodule in this repository, but there's been ongoing discussion and experimentation for a while now to investigate the possibility of switching rustfmt to a subtree in a similar vein as #70651 (additional background/motivation at the bottom of the issue description)

Thanks to @jyn514, the bulk of the changes that would be needed for this conversion have been identified (e.g. #82208), but there's a number of items in my mind that would still need to be resolved/answered before we could proceed. Opening this issue to facilitate those discussions separately from the PR given the quantity of folks that would get notified there.

A non-exhuastive list of questions/steps that I think would need to be discussed before converting:

rust-side

• would x.py fmt continue to use an older/pinned nightly or use the rustfmt version in tree?
• would any additional CI checks be necessary or helpful? For example within the rust-lang/rustfmt repo our CI has some functional-like tests that run the updated rustfmt version against different repositories (it's very easy to make seemingly innocuous changes in rustfmt that result in different formatting being emitted)
• any updates needed to the rustc dev guide? For example I'd imagine the submod section in the conributing intro should be updated prior to the conversion or at least very shortly after
• hooking into high-five config to notify rustfmt team (or at least @calebcartwright) on changes to src/tools/rustfmt (done Notify myself on rustfmt updates highfive#311)

rustfmt-side

• change default branch (current default branch in rust-lang/rustfmt is set to a previously planned but subsequently indefinitely postponed 2.0 version of rustfmt for reasons I won't go into here)
  • I don't have access to change this currently
• update documentation for contributors
• CI review/updates (we currently have a job that runs with the beta toolchain)

general thoughts/questions

• switching rustfmt to a subtree will add overhead to compiler contributors. is this acceptable?
• will PRs that modify rustfmt in rust-lang/rust (outside fixes/adjustments required due to rustc mod changes) be rejected? E.g. if someone opens a PR here to add a new config option
  • I assume this is the case but want to be explicit about it, as changes to rustfmt made in rust-lang/rust that bypassed the rustfmt team would be rather problematic IMO
• how easily would we be able to revert back to a submodule should the need arise?
• what should the sync frequency be? 2 weeks?
• I need to educate myself on the process for sync, if conflicts are possible/how best to resolve, etc.

background/motivation

rustfmt is heavily reliant on internal rustc mods (particularly rustc_parse, rustc_ast, and rustc_span) and this is unlikely to change for the foreseeable future. rustfmt currently consumes these via the auto publish crates which ends up being the root cause of several challenges (many discussed in #82208), notably the frequent and often extended broken toolstate on nightly.

The thinking is that switching to a subtree would greatly reduce the toolstate issues and associated fallout. Additionally, the other benefit in leveraging the knowledge and expertise of the compiler contributors to apply rustc mod changes in the rustfmt codebase is that it would help the rustfmt development process, since the rustfmt team would no longer have to retroactively apply large batches of rustc mod changes themselves.

It's worth noting that rustfmt is also published to crates.io (though admittedly not for a long time, as I don't have access to that either). Switching rustfmt to a subtree would of course result in no longer being able to publish rustfmt directly to crates.io, however, we would still have the option of utilizing and extending the existing rustc auto publish process to continue publishing rustfmt to crates.io just like the other rustc mods

cc @jyn514 @Manishearth @Mark-Simulacrum

[Manishearth]

would x.py fmt continue to use an older/pinned nightly or use the rustfmt version in tree?

It's supposed to be fast, so pinned nightly IMO.

would any additional CI checks be necessary or helpful? For example within the rust-lang/rustfmt repo our CI has some functional-like tests that run the updated rustfmt version against different repositories (it's very easy to make seemingly innocuous changes in rustfmt that result in different formatting being emitted)

Don't what to be prescriptive, but clippy runs its normal tests within rustc and an extended set of tests against external crates in its own CI. We're hesitant to include external crates as a part of Rust's main CI pipeline.

any updates needed to the rustc dev guide? For example I'd imagine the submod section in the conributing intro should be updated prior to the conversion or at least very shortly after

Minor, I think. It's "yet another folder of code that should be updated", like rustdoc.

I don't have access to change this currently

I can handle access issues if we have to, though I'd love to talk to topecongiro first.

switching rustfmt to a subtree will add overhead to compiler contributors. is this acceptable?

I think it adds minor overhead, equal to the overhead of updating rustdoc when you make a breaking AST change. Folks have to do this for plenty of internal consumers, and rustfmt uses much less than rustdoc does. Worth checking with the compiler team, but as long as they mostly have to jsut propagate their refactors, it should be fine

[jyn514]

would any additional CI checks be necessary or helpful? For example within the rust-lang/rustfmt repo our CI has some functional-like tests that run the updated rustfmt version against different repositories (it's very easy to make seemingly innocuous changes in rustfmt that result in different formatting being emitted)

I have no opinion on this either way, but if we do add it it might make sense to make it part of cargotest.

would x.py fmt continue to use an older/pinned nightly or use the rustfmt version in tree?

We discussed this some on Discord. The main relevant points:

• If we use in-tree rustfmt, then x.py fmt suddenly has to build the whole compiler to run. We could reduce the build time by only building the crates it depends on (Don't build rustc_middle for x.py build src/tools/rustfmt #82206) or by downloading it from an earlier commit, the way download-ci-llvm works.
• Using in-tree rustfmt will be a good integration test for rustfmt itself.
• Fixing rustfmt bugs occasionally causes the formatting to change (e.g. formatting within macros). Using an in-tree rustfmt will mean the formatting changes slightly more often than if we pinned a version.

hooking into high-five config to notify rustfmt team (or at least @calebcartwright) on changes to src/tools/rustfmt

Here's an example of how to do that: rust-lang/highfive#274.

how easily would we be able to revert back to a submodule should the need arise?

The main downside is that the subtree history will still be part of the rust-lang/rust history even after it's no longer used. This isn't a giant deal, but it seems a shame. I don't see this being too hard otherwise.

I need to educate myself on the process for sync, if conflicts are possible/how best to resolve, etc.

You might find https://github.com/rust-lang/rust-clippy/blob/master/CONTRIBUTING.md#syncing-changes-between-clippy-and-rust-langrust helpful.

[calebcartwright]

Minor, I think. It's "yet another folder of code that should be updated", like rustdoc.

Agreed that the rustc dev guide item is minor, though there's multiple sections of text just on that one page which refer to rustfmt as a submodule, which use rustfmt as the example for allowed breaking changes, how to update a submodule, etc.

[calebcartwright]

Also @topecongiro know you've been busy and away for a while, but this is a fairly significant change so pinging to see if you have any concerns or objections

[calebcartwright]

#82640 is a good example of another benefit for making this change (syntax updates being supported more quickly)

[jyn514]

Agreed that the rustc dev guide item is minor, though there's multiple sections of text just on that one page which refer to rustfmt as a submodule, which use rustfmt as the example for allowed breaking changes, how to update a submodule, etc.

IMO the ideal is that we no longer have toolstate as a concept and all tools always build. https://github.com/rust-lang-nursery/rust-toolstate/ says the only tools other than rustfmt using compiler APIs are miri and rls, both of which I think could benefit from being subtrees (#70651), and the remaining books aren't mutually dependent on rust-lang/rust the way tools are.

That said, for now just changing all mentions of "rustfmt" to "miri" should be easy enough.

[calebcartwright]

That said, for now just changing all mentions of "rustfmt" to "miri" should be easy enough.

Agreed. In hindsight I probably shouldn't have used the "addressed" wording in the issue description, and will tweak accordingly. My intent was to enumerate everything I could think of in the spirit of thoroughness so that things could at least be discussed and planned for, even if just deciding to ignore or defer.

would x.py fmt continue to use an older/pinned nightly or use the rustfmt version in tree?

I'm now convinced that this makes sense to keep pinned. I think there may still be some potential benefit for an additional check that uses the in-tree version, even if it was just a conditionally executed step (only on changes to src/tools/rustfmt) in CI and was allowed fail. That's another optional, non-blocking item for the submod->subtree change that could be done after or even independently (could have utility with current submod approach too)

---

IMO the biggest blocking item left is our current branching strategy in rust-lang/rustfmt. I suppose it's not a literal blocker, but before making such a change I'd very much prefer getting our default branch in rust-lang/rustfmt back inline with the version of rustfmt that's released/used and that matches what is here in tree.

It's already a source of confusion for rustfmt contributors and occasionally users, and I feel like making the subtree switch before addressing the branches will only cause more confusion on both sides

[jyn514]

@calebcartwright what's the current status of this? It looks like you've since changed the default branch away from rustup 2.0 and to the version published by rustup?

[calebcartwright]

@calebcartwright what's the current status of this?

I do not know whether any discussion/thumbsup/etc. is needed (and if so from whom) here, but moving ahead on the rustfmt side under the presumption that we're going to move to the subtree model. Just moving somewhat slowly, and lost a couple cycles dealing with the current broken tool state.

It looks like you've since changed the default branch away from rustup 2.0 and to the version published by rustup?

Not yet but planning to do so shortly. We do have a few folks building the 2.0rc directly from source and want to give them sufficient heads up about the branch change

[Manishearth]

I do not know whether any discussion/thumbsup/etc. is needed

Mostly just check with the infra team to make sure they're okay with the plan, but I think it's fine to move forward.

[Mark-Simulacrum]

I don't think there's any blockers on the infra side of things, should be a pretty transparent change to our build infrastructure and rustbuild.