rustbuild: x.py fmt src/tools/rustfmt doesn't format code in the rustfmt subtree

[petrochenkov]

rustfmt's tests require its source code to be properly formatted, but when making changes to rustfmt through its subtree in rust-lang/rust, it seems like there's no way to do such formatting automatically.

@jyn514 suggests that x.py fmt src/tools/rustfmt should just work and do the formatting, and it's probably just a missing step in src/bootstrap/fmt.rs that prevents it from working.

[Mark-Simulacrum]

It's explicitly excluded (like other subtrees), because rustfmt's tests probably are using a different rustfmt to x.py fmt? https://github.com/rust-lang/rust/blob/master/rustfmt.toml#L36

Do you have a pointer to the tests in rustfmt that check it's own source code?

[jyn514]

@Mark-Simulacrum

$ x test src/tools/rustfmt
Mismatch at src/lib.rs:25:
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::fmt;
-use std::io:: {self, Write};
+use std::io::{self, Write};
 use std::mem;
 use std::panic;
 use std::path::PathBuf;
test test::self_tests ... FAILED
test test::system_tests ... ok
test test::idempotence_tests ... ok

failures:

---- test::self_tests stdout ----
Ran 5 self tests.
thread 'test::self_tests' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: 1 self tests failed', src/tools/rustfmt/src/test/mod.rs:357:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test::self_tests

test result: FAILED. 154 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.96s

[Mark-Simulacrum]

Hm. Yeah, so looks like we will probably end up fighting ourselves at some point if we try to format with x.py fmt, but if we don't then this kind of error will also appear. @rust-lang/wg-rustfmt, do you think this test adds a lot of value? It may make sense to just remove it -- the rustfmt source code is probably not terribly interesting from a formatting perspective.

[calebcartwright]

I've mentioned before that I think we might want to consider using the in-tree version of rustfmt to format the rustfmt tree in this repo now that it's a submodule subtree. However, I'm pretty strongly opposed to dropping the test here given that r-l/rust uses an older, pinned version and the bidirectional nature of substree syncs.

If this is dropped and/or rustfmt source is formatted with a different/older version then what happens in r-l/rustfmt then I see it adding undo complexity and increased merge conflicts on those syncs

[calebcartwright]

It may make sense to just remove it -- the rustfmt source code is probably not terribly interesting from a formatting perspective.

that test is also the only viable mechanism we have for enforcing formatting consistency of our own code in the main repo where the development happens on that code, so it's pretty important from that perspective too 😄

[Mark-Simulacrum]

I've mentioned before that I think we might want to consider using the in-tree version of rustfmt to format the rustfmt tree in this repo now that it's a submodule subtree.

That would require everyone building rustfmt locally, right? That doesn't seem feasible.

However, I'm pretty strongly opposed to dropping the test here given that r-l/rust uses an older, pinned version and the bidirectional nature of substree syncs.

I was suggesting we remove the test entirely, and that upstream rustfmt repo check formatting in CI against the same version used locally in rust-lang/rust (perhaps by reading - via GitHub API - src/stage0.json or something; or just pinning and manually bumping from time to time the nightly used).

[jyn514]

That would require everyone building rustfmt locally, right? That doesn't seem feasible.

FWIW it might make sense to format src/tools/rustfmt with itself, but use the beta rustfmt for the rest of the tree.
Another alternative is to turn download-rustc = "if-unchanged" on by default so building rustfmt doesn't take too much time.

[calebcartwright]

FWIW it might make sense to format src/tools/rustfmt with itself, but use the beta rustfmt for the rest of the tree

That's the type of thing I was referring to as well, though not sure how feasible/difficult it would be to have it only run on some explicit invocation/conditional flag.

I was suggesting we remove the test entirely, and that upstream rustfmt repo check formatting in CI against the same version used locally in rust-lang/rust (perhaps by reading - via GitHub API - src/stage0.json or something; or just pinning and manually bumping from time to time the nightly used).

I still feel the same, and am not keen on adding this type of complexity and sacrificing a test that is important to rustfmt.

In general I don't think folks should be making too many changes to subtrees over here, certainly not to the extent that this is a significant problem as opposed to an occasional paper cut.

We'd have a new chunk of work to do on the rustfmt side (some x.py equivalent to ensure folks can locally run the same, older version) and we'd lose the only test that has idempotency checks against an actual codebase. That's quite literally a step in the wrong direction of where I want to take our testing.

[calebcartwright]

I could live with an update to the test that conditionally skips execution, e.g. when RUSTC_BOOTSTRAP is set so that it doesn't impact folks over here.

I would feel even more comfortable about that if we could get the bots to still ping teams if submod/subtree changes are added after the PR opens and/or if we could find another way to be more proactively kept in the loop on subtree changes that get made in this repo

[jyn514]

FWIW it might make sense to format src/tools/rustfmt with itself, but use the beta rustfmt for the rest of the tree

I looked into this and it was trickier than expected. If we build rustfmt unconditionally, it means building the whole compiler on each x.py fmt, which is more of a compile time hit than I'm willing to accept, but if we only build it on changes to rustfmt we have to be considerably smarter about formatting than we are today ...

@Mark-Simulacrum at one point suggested having ~everything be downloadable from CI if it hasn't been modified. I wonder if we could start downloading rustfmt and using stage 2 unconditionally (falling back to building when it's been changed)? Then we naturally get what we want:

• there's a built-in check for @calebcartwright that rustfmt doesn't change its behavior when modified
• we can format the subtree so the rustfmt tests pass for @petrochenkov on changes
• we don't impact compile times for x.py fmt in the common case when rustfmt hasn't been modified
• as a bonus, bootstrap gets to be less special since we no longer have to use a beta version that differs from the rest of the stage0 toolchain. this helps with my long-term plan to have downloads be managed by rustup/RTIM instead of bootstrap.

[Mark-Simulacrum]

we can format the subtree so the rustfmt tests pass for @petrochenkov on changes

I suspect that in many cases this wouldn't be true? rustfmt doesn't dynamically link against the locally compiled code (and probably that's not really possible), so any changes in rustc wouldn't be picked up by rustfmt.

Changes to rustfmt code that rely on rustc would cause a recompile and reformat, but at that point you might as well just build rustfmt (via x.py) and adjust config.toml to point at the built copy temporarily.

In CI, I'm happy to have us have some builder that does this (builds rustfmt and then re-formats the repo, failing if there are any changes), it should be relatively fast to add that to e.g. the tools builder, since it's not doing much work on top of building rustfmt to format things.

This could even be part of the rustfmt test step, for example.

[jyn514]

rustfmt doesn't dynamically link against the locally compiled code (and probably that's not really possible), so any changes in rustc wouldn't be picked up by rustfmt.

Oh I see, you're worried about changes to rustc_parser or lexer impacting rustfmt. We could have some logic to rebuild from source if those crates change (no need to rebuild if any part of compiler/ changed IMO).

at that point you might as well just build rustfmt (via x.py) and adjust config.toml to point at the built copy temporarily.

That requires you to
a) know what's going wrong (i.e. that there's even a possibility you could have broken rustfmt semantically)
b) know how to configure initial_rustfmt
c) mess with a bunch of config files to test your change

It's certainly better than nothing, but I think doing this automatically with bootstrap is not too terribly much effort and a lot more user friendly.

That said, a) is solved by having a builder that enforces you do this ... willing to be convinced.

[Mark-Simulacrum]

Parser and lexer are part of compiler/, and I'd rather not maintain a mapping of interesting crates -- we can't readily extract it from rustfmt due to it pulling from sysroot rather than declaring in Cargo.toml. The list is somewhat longer than just those two:

rust/src/tools/rustfmt/src/lib.rs

Lines 17 to 25 in 1f5d8d4

	extern crate rustc_ast;
	extern crate rustc_ast_pretty;
	extern crate rustc_builtin_macros;
	extern crate rustc_data_structures;
	extern crate rustc_errors;
	extern crate rustc_expand;
	extern crate rustc_parse;
	extern crate rustc_session;
	extern crate rustc_span;

I think, in general, the likelihood that your changes affect rustfmt is low. Forcing anyone editing these crates to rebuild rustfmt just to format their code is a huge speed bump, and the flow with a CI check (we can make it happen even in the llvm-12 builder, I guess) would be just "x.py test src/tools/rustfmt", which gives you a diff you need to remove. I think the comparative benefit of catching this "earlier" and the cost of forcing re-building format is not a tradeoff that makes sense to make in favor of rebuilding.

It should be even rarer that your changes create a diff in rustfmt's output and you want to re-bless the code. At least according to my understanding, most changes should be treated as bugs in the PR. And we can support re-blessing with x.py test src/tools/rustfmt --bless, since we already do that with e.g. compiletest UI tests.

[jyn514]

Makes sense to me :)

Mentoring instructions: I believe test rustfmt already does the right thing, so this just needs someone to

• Add --bless support to x test rustfmt; i.e. if any of the tests fail, build rustfmt and then use it to format itself.
• Make x fmt rustfmt give a hard error that suggests using test --bless instead.

[Mark-Simulacrum]

I don't think x.py test src/tools/rustfmt reformats the full Rust tree, unless I missed something -- it might have a rustfmt-internal check that reformats rustfmt only, but if that makes sense a x.py fmt w/ the newly built rustfmt probably also does.

I don't think we should make x.py fmt rustfmt hard error; it might print a warning but no more. In 99% of cases it's a perfectly reasonable thing to do after some minor edits to rustfmt code.

[jyn514]

I don't think we should make x.py fmt rustfmt hard error; it might print a warning but no more. In 99% of cases it's a perfectly reasonable thing to do after some minor edits to rustfmt code

Currently we don't format any code in subtrees (there's some config in rustfmt.toml) by the rationale they may be using a different rustfmt than the rest of tree. Either we should start doing that or give an error IMO; it's very confusing to have it silently do nothing.

I don't think we should start formatting with a two different rustfmts, that seems likely to break. I guess this is a question for @calebcartwright though: if a later rustfmt disagrees with an earlier rustfmt on formatting, is that always a bug? I know sometimes rustfmt learns new syntax and that's ok, mostly want to avoid the earlier rustfmt changing it back to how it was before, which would break our CI with the setup @Mark-Simulacrum suggested.

[Mark-Simulacrum]

OK, sure, that makes sense -- we definitely shouldn't break x.py fmt from working; I don't mind us giving an error for explicitly trying to format a path that we know to be ignored (like any subtree), though.

[calebcartwright]

tbh, I'm not entirely sure I follow all of the proposals and associated implications as I don't typically do much work in this repository beyond the subtree syncs.

However, as I see it the original impetus for this is really about the inner dev loop experience when contributors also have to modify the rustfmt source in tree, and that ties back to a test we have in rustfmt that's critical to the CI and inner dev loop experience for rustfmt contributors in r-l/rustfmt (it's our mechanism to ensure the rustfmt source is properly formatted, our equivalent of x.py fmt --check for the rustfmt source code)

I still believe that the most direct path to addressing the original item is what I mentioned in #90672 (comment). We've already got other rustfmt tests that are intentionally skipped during Rust's CI in this repo, and I believe Clippy does this for certain tests too.

I also acknowledge that the broader proposals discussed above around revamping how/where automated formatting is applied in this repo would also have the effect of addressing the original item. However, I feel like those also introduce a broader set of considerations and potential complexity/drawbacks.

If there's a need/desire for those broader changes to provide more general improvements to formatting in this repo then fair enough, but if the main focus is just the original scenario then I'd reiterate my prior recommendation. I think that's the simplest option with a very, very low ceiling of potential impact (worst case scenario is that I occasionally need to add an extra commit on subtree pushes)

---

@jyn514

@calebcartwright though: if a later rustfmt disagrees with an earlier rustfmt on formatting, is that always a bug?

Short answer is no, though there's a lot of caveats and nuance.

I typically recommend folks think about it this way: So long as you are exclusively using default config options, you should never see an earlier version of rustfmt vN-1 want to change formatting emitted by a newer version of rustfmt vN. The reverse mostly holds true as well but that's where the important nuance comes into play that's probably not worth delving into here (especially since this comment is already approaching novel-length).

However, this repo is using several non-default option values, notably version = Two which functionally grants free license to change/improve formatting. This is the part that would give me pause around x.py fmt using the in-tree version of rustfmt to format the entire codebase because it essentially takes the periodic rustfmt updates currently performed atomically, and couples that to the rustfmt subtree pulls.

Again if that's the right change for this repository then I'll learn to deal with it, but I'd worry it could complicate our ability to perform syncs/increase odds of merge conflicts that typically require restarting the entire sync workflow

[oli-obk]

I added x test rustfmt --bless which will compile in-tree rustfmt and run its tests, but instead of failing on an error it will apply the formatting: 841f8dc

While we figure out what to do, this makes it at least easy to bless rustfmt