-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't work because the error types in the two Result
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking forward to let-chains reaching stable :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fmt
output.x fmt
's internal code.r? @GuillaumeGomez