Streamline x fmt and improve its output#125699
Conversation
By default, `x fmt` formats/checks modified files. But it also lets you choose one or more paths instead. This adds significant complexity to `x fmt`. Explicit paths are specified via `WalkBuilder::add` rather than `OverrideBuilder::add`. The `ignore` library is not simple, and predicting the interactions between the two mechanisms is difficult. Here's a particularly interesting case. - You can request a path P that is excluded by the `ignore` list in the `rustfmt.toml`. E.g. `x fmt tests/ui/` or `x fmt tests/ui/bitwise.rs`. - `x fmt` will add P to the walker (via `WalkBuilder::add`), traverse it (paying no attention to the `ignore` list from the `rustfmt.toml` file, due to the different mechanism), and call `rustfmt` on every `.rs` file within it. - `rustfmt` will do nothing to those `.rs` files, because it *also* reads `rustfmt.toml` and sees that they match the `ignore` list! It took me *ages* to debug and understand this behaviour. Not good! `x fmt` even lets you name a path below the current directory. This was intended to let you do things like `x fmt std` that mirror things like `x test std`. This works by looking for `std` and finding `library/std`, and then formatting that. Unfortuantely, this motivating case now gives an error. When support was added in rust-lang#107944, `library/std` was the only directory named `std`. Since then, `tests/ui/std` was added, and so `x fmt std` now gives an error. In general, explicit paths don't seem particularly useful. The only two cases `x fmt` really needs are: - format/check the files I have modified (99% of uses) - format/check all files (While respecting the `ignore` list in `rustfmt.toml`, of course.) So this commit moves to that model. `x fmt` will now give an error if given an explicit path. `x fmt` now also supports a `--all` option. (And running with `GITHUB_ACTIONS=true` also causes everything to be formatted/checked, as before.) Much simpler!
- Precede them all with `fmt` so it's clear where they are coming from. - Use `error:` and `warning:` when appropriate. - Print warnings to stderr instead of stdout
Currently, `x fmt` can print two lists of files. - The untracked files that are skipped. Always done if within a git repo. - The modified files that are formatted. But if you run with `--all` (or with `GITHUB_ACTIONS=true`) it doesn't print anything about which files are formatted. This commit increases consistency. - The formatted/checked files are now always printed. And it makes it clear why a file was formatted, e.g. with "modified". - It uses the same code for both untracked files and formatted/checked files. This means that now if there are a lot of untracked files just the number will be printed, which is like the old behaviour for modified files. Example output: ``` fmt: skipped 31 untracked files fmt: formatted modified file compiler/rustc_mir_transform/src/instsimplify.rs fmt: formatted modified file compiler/rustc_mir_transform/src/validate.rs fmt: formatted modified file library/core/src/ptr/metadata.rs fmt: formatted modified file src/bootstrap/src/core/build_steps/format.rs ``` or (with `--all`): ``` fmt: checked 3148 files ```
It's a weird name, the `fmt_` prefix seems unnecessary.
|
This PR modifies If appropriate, please update |
- Avoid calling `try_wait` followed immediately by `wait`. - Make the match exhaustive. - Improve the comment.
b872585 to
4dec0a0
Compare
| if let Ok(cwd) = cwd { | ||
| if let Ok(path2) = path.strip_prefix(cwd) { | ||
| path = path2.to_path_buf(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Small nit:
| if let Ok(cwd) = cwd { | |
| if let Ok(path2) = path.strip_prefix(cwd) { | |
| path = path2.to_path_buf(); | |
| } | |
| } | |
| if let Ok(path2) = cwd.and_then(|cwd| path.strip_prefix(cwd)) { | |
| path = path2.to_path_buf(); | |
| } |
There was a problem hiding this comment.
It doesn't work because the error types in the two Results don't match:
error[E0308]: mismatched types
--> src/core/build_steps/format.rs:292:45
|
292 | if let Ok(path2) = cwd.and_then(|cwd| path.strip_prefix(cwd)) {
| ^^^^^^^^^^^^^^^^^^^^^^ expected `Result<_, Error>`, found `Result<&Path, ...>`
|
= note: expected enum `Result<_, std::io::Error>`
found enum `Result<&Path, StripPrefixError>`
There was a problem hiding this comment.
I'm looking forward to let-chains reaching stable :)
GuillaumeGomez
left a comment
There was a problem hiding this comment.
Just a small nit but otherwise looks great to me, thanks!
r=me once nit fixed.
|
@bors r=GuillaumeGomez |
|
@bors rollup |
…GuillaumeGomez Streamline `x fmt` and improve its output - Removes the ability to pass paths to `x fmt`, because it's complicated and not useful, and adds `--all`. - Improves `x fmt` output. - Improves `x fmt`'s internal code. r? `@GuillaumeGomez`
Rollup of 7 pull requests Successful merges: - rust-lang#125653 (Migrate `run-make/const-prop-lint` to `rmake.rs`) - rust-lang#125662 (Rewrite `fpic`, `simple-dylib` and `issue-37893` `run-make` tests in `rmake.rs` or ui test format) - rust-lang#125699 (Streamline `x fmt` and improve its output) - rust-lang#125701 ([ACP 362] genericize `ptr::from_raw_parts`) - rust-lang#125723 (Migrate `run-make/crate-data-smoke` to `rmake.rs`) - rust-lang#125733 (Add lang items for `AsyncFn*`, `Future`, `AsyncFnKindHelper`'s associated types) - rust-lang#125734 (ast: Revert a breaking attribute visiting order change) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#125699 - nnethercote:streamline-rustfmt, r=GuillaumeGomez Streamline `x fmt` and improve its output - Removes the ability to pass paths to `x fmt`, because it's complicated and not useful, and adds `--all`. - Improves `x fmt` output. - Improves `x fmt`'s internal code. r? ``@GuillaumeGomez``
I'll just add that I found this useful and was surprised when I rebased and was met with a terse and slightly ambiguous error instead. Passing paths was especially useful for generated code when you only want to automatically format the generated file. But I also fairly often used |
|
Can you give me more details about your use case, especially the generated code? And with the |
|
Specifically in my case std generates the Windows API bindings using
That seems to work. Though that is noticeably slower instead of being near enough instant. |
|
|
|
Not that often. And to be clear, I'm not saying it's a big deal but I wouldn't quite characterise it as useless either. |
|
Thanks for the info. The overall calculus was:
I have filed #126051 to improve the error message given when |
x fmt, because it's complicated and not useful, and adds--all.x fmtoutput.x fmt's internal code.r? @GuillaumeGomez