Skip to content
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

Merged
merged 5 commits into from
May 30, 2024

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented May 29, 2024

  • 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

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.
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 29, 2024

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label May 29, 2024
- Avoid calling `try_wait` followed immediately by `wait`.
- Make the match exhaustive.
- Improve the comment.
Comment on lines +294 to +298
if let Ok(cwd) = cwd {
if let Ok(path2) = path.strip_prefix(cwd) {
path = path2.to_path_buf();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit:

Suggested change
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();
}

Copy link
Contributor Author

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 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>`

Copy link
Contributor Author

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too...

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a 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.

@nnethercote
Copy link
Contributor Author

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented May 29, 2024

📌 Commit 4dec0a0 has been approved by GuillaumeGomez

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2024
@nnethercote
Copy link
Contributor Author

@bors rollup

fmease added a commit to fmease/rust that referenced this pull request May 29, 2024
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2024
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
@bors bors merged commit c62fa82 into rust-lang:master May 30, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 30, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2024
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``
@nnethercote nnethercote deleted the streamline-rustfmt branch May 30, 2024 05:45
@ChrisDenton
Copy link
Member

Removes the ability to pass paths to x fmt, because it's complicated and not useful

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 ./x fmt library/std to only format std and not the whole code base.

@nnethercote
Copy link
Contributor Author

Can you give me more details about your use case, especially the generated code?

And with the x fmt library/std case, does running x fmt --all give a different outcome?

@ChrisDenton
Copy link
Member

Specifically in my case std generates the Windows API bindings using src/tools/generate-windows-sys, which writes to library/std/src/sys/pal/windows/c/windows_sys.rs that then needs to be formatted according to std's formatting config.

And with the x fmt library/std case, does running x fmt --all give a different outcome?

That seems to work. Though that is noticeably slower instead of being near enough instant.

@nnethercote
Copy link
Contributor Author

x fmt --all is slower, to be sure. On my machine x fmt with very few changes takes about 2.2 seconds, and x fmt --all takes about 3.3 seconds. So bootstrap stuff before formatting takes more time than the actual formatting. But I do have a fast machine, so your results may be different. How often do you generate the windows bindings?

@ChrisDenton
Copy link
Member

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.

@nnethercote
Copy link
Contributor Author

Thanks for the info. The overall calculus was:

  • Path support is rarely needed, --all subsumes it and is only marginally slower.
  • Path support is surprisingly complicated. It was completely broken for a while until rustfmt fixes #125637 fixed it again, and a number of non-formatted files slipped into the repository in this time. The ignore library has some surprising behaviours when you use combinations of explicitly excluded and explicitly included paths.
  • So overall, it's not worth keeping.

I have filed #126051 to improve the error message given when x fmt is given a path. I agree the current one is not clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants