Remove path choice from x fmt and add --all option.

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 #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!
rust-lang · bors · May 30, 2024 · May 28, 2024 · May 29, 2024 · May 29, 2024
commit 5cf198d0d6dc64b9c255d1eaec48f0fd5802e47f
diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs
@@ -97,10 +97,21 @@ struct RustfmtConfig {
     ignore: Vec<String>,
 }
 
-pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
+pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
+    if !paths.is_empty() {
+        eprintln!("path arguments are not accepted");
+        crate::exit!(1);
+    };
     if build.config.dry_run() {
         return;
     }
+
+    // By default, we only check modified files locally to speed up runtime. Exceptions are if
+    // `--all` is specified or we are in CI. We check all files in CI to avoid bugs in
+    // `get_modified_rs_files` letting regressions slip through; we also care about CI time less
+    // since this is still very fast compared to building the compiler.
+    let all = all || CiEnv::is_ci();
+
     let mut builder = ignore::types::TypesBuilder::new();
     builder.add_defaults();
     builder.select("rust");
@@ -175,11 +186,7 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
                 untracked_count += 1;
                 fmt_override.add(&format!("!/{untracked_path}")).expect(untracked_path);
             }
-            // Only check modified files locally to speed up runtime. We still check all files in
-            // CI to avoid bugs in `get_modified_rs_files` letting regressions slip through; we
-            // also care about CI time less since this is still very fast compared to building the
-            // compiler.
-            if !CiEnv::is_ci() && paths.is_empty() {
+            if !all {
                 match get_modified_rs_files(build) {
                     Ok(Some(files)) => {
                         if files.len() <= 10 {
@@ -233,55 +240,8 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
     assert!(rustfmt_path.exists(), "{}", rustfmt_path.display());
     let src = build.src.clone();
     let (tx, rx): (SyncSender<PathBuf>, _) = std::sync::mpsc::sync_channel(128);
-    let walker = match paths.first() {
-        Some(first) => {
-            let find_shortcut_candidates = |p: &PathBuf| {
-                let mut candidates = Vec::new();
-                for entry in
-                    WalkBuilder::new(src.clone()).max_depth(Some(3)).build().map_while(Result::ok)
-                {
-                    if let Some(dir_name) = p.file_name() {
-                        if entry.path().is_dir() && entry.file_name() == dir_name {
-                            candidates.push(entry.into_path());
-                        }
-                    }
-                }
-                candidates
-            };
-
-            // Only try to look for shortcut candidates for single component paths like
-            // `std` and not for e.g. relative paths like `../library/std`.
-            let should_look_for_shortcut_dir = |p: &PathBuf| p.components().count() == 1;
-
-            let mut walker = if should_look_for_shortcut_dir(first) {
-                if let [single_candidate] = &find_shortcut_candidates(first)[..] {
-                    WalkBuilder::new(single_candidate)
-                } else {
-                    WalkBuilder::new(first)
-                }
-            } else {
-                WalkBuilder::new(src.join(first))
-            };
-
-            for path in &paths[1..] {
-                if should_look_for_shortcut_dir(path) {
-                    if let [single_candidate] = &find_shortcut_candidates(path)[..] {
-                        walker.add(single_candidate);
-                    } else {
-                        walker.add(path);
-                    }
-                } else {
-                    walker.add(src.join(path));
-                }
-            }
-
-            walker
-        }
-        None => WalkBuilder::new(src.clone()),
-    }
-    .types(matcher)
-    .overrides(fmt_override)
-    .build_parallel();
+    let walker =
+        WalkBuilder::new(src.clone()).types(matcher).overrides(fmt_override).build_parallel();
 
     // There is a lot of blocking involved in spawning a child process and reading files to format.
     // Spawn more processes than available concurrency to keep the CPU busy.

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
@@ -1140,7 +1140,13 @@ HELP: to skip test's attempt to check tidiness, pass `--skip src/tools/tidy` to
                 );
                 crate::exit!(1);
             }
-            crate::core::build_steps::format::format(builder, !builder.config.cmd.bless(), &[]);
+            let all = false;
+            crate::core::build_steps::format::format(
+                builder,
+                !builder.config.cmd.bless(),
+                all,
+                &[],
+            );
         }
 
         builder.info("tidy check");

diff --git a/src/bootstrap/src/core/config/flags.rs b/src/bootstrap/src/core/config/flags.rs
@@ -284,8 +284,8 @@ pub enum Subcommand {
         name = "fmt",
         long_about = "\n
     Arguments:
-        This subcommand optionally accepts a `--check` flag which succeeds if formatting is correct and
-        fails if it is not. For example:
+        This subcommand optionally accepts a `--check` flag which succeeds if
+        formatting is correct and fails if it is not. For example:
             ./x.py fmt
             ./x.py fmt --check"
     )]
@@ -294,6 +294,10 @@ pub enum Subcommand {
         /// check formatting instead of applying
         #[arg(long)]
         check: bool,
+
+        /// apply to all appropriate files, not just those that have been modified
+        #[arg(long)]
+        all: bool,
     },
     #[command(aliases = ["d"], long_about = "\n
     Arguments:

diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs
@@ -660,10 +660,11 @@ impl Build {
 
         // hardcoded subcommands
         match &self.config.cmd {
-            Subcommand::Format { check } => {
+            Subcommand::Format { check, all } => {
                 return core::build_steps::format::format(
                     &builder::Builder::new(self),
                     *check,
+                    *all,
                     &self.config.paths,
                 );
             }

diff --git a/src/etc/completions/x.py.fish b/src/etc/completions/x.py.fish
@@ -216,6 +216,7 @@ complete -c x.py -n "__fish_seen_subcommand_from fmt" -l llvm-profile-use -d 'us
 complete -c x.py -n "__fish_seen_subcommand_from fmt" -l reproducible-artifact -d 'Additional reproducible artifacts that should be added to the reproducible artifacts archive' -r
 complete -c x.py -n "__fish_seen_subcommand_from fmt" -l set -d 'override options in config.toml' -r -f
 complete -c x.py -n "__fish_seen_subcommand_from fmt" -l check -d 'check formatting instead of applying'
+complete -c x.py -n "__fish_seen_subcommand_from fmt" -l all -d 'apply to all appropriate files, not just those that have been modified'
 complete -c x.py -n "__fish_seen_subcommand_from fmt" -s v -l verbose -d 'use verbose output (-vv for very verbose)'
 complete -c x.py -n "__fish_seen_subcommand_from fmt" -s i -l incremental -d 'use incremental compilation'
 complete -c x.py -n "__fish_seen_subcommand_from fmt" -l include-default-paths -d 'include default paths in addition to the provided ones'

diff --git a/src/etc/completions/x.py.ps1 b/src/etc/completions/x.py.ps1
@@ -275,6 +275,7 @@ Register-ArgumentCompleter -Native -CommandName 'x.py' -ScriptBlock {
             [CompletionResult]::new('--reproducible-artifact', 'reproducible-artifact', [CompletionResultType]::ParameterName, 'Additional reproducible artifacts that should be added to the reproducible artifacts archive')
             [CompletionResult]::new('--set', 'set', [CompletionResultType]::ParameterName, 'override options in config.toml')
             [CompletionResult]::new('--check', 'check', [CompletionResultType]::ParameterName, 'check formatting instead of applying')
+            [CompletionResult]::new('--all', 'all', [CompletionResultType]::ParameterName, 'apply to all appropriate files, not just those that have been modified')
             [CompletionResult]::new('-v', 'v', [CompletionResultType]::ParameterName, 'use verbose output (-vv for very verbose)')
             [CompletionResult]::new('--verbose', 'verbose', [CompletionResultType]::ParameterName, 'use verbose output (-vv for very verbose)')
             [CompletionResult]::new('-i', 'i', [CompletionResultType]::ParameterName, 'use incremental compilation')

diff --git a/src/etc/completions/x.py.sh b/src/etc/completions/x.py.sh
@@ -1077,7 +1077,7 @@ _x.py() {
             return 0
             ;;
         x.py__fmt)
-            opts="-v -i -j -h --check --verbose --incremental --config --build-dir --build --host --target --exclude --skip --include-default-paths --rustc-error-format --on-fail --dry-run --dump-bootstrap-shims --stage --keep-stage --keep-stage-std --src --jobs --warnings --error-format --json-output --color --bypass-bootstrap-lock --llvm-skip-rebuild --rust-profile-generate --rust-profile-use --llvm-profile-use --llvm-profile-generate --enable-bolt-settings --skip-stage0-validation --reproducible-artifact --set --help [PATHS]... [ARGS]..."
+            opts="-v -i -j -h --check --all --verbose --incremental --config --build-dir --build --host --target --exclude --skip --include-default-paths --rustc-error-format --on-fail --dry-run --dump-bootstrap-shims --stage --keep-stage --keep-stage-std --src --jobs --warnings --error-format --json-output --color --bypass-bootstrap-lock --llvm-skip-rebuild --rust-profile-generate --rust-profile-use --llvm-profile-use --llvm-profile-generate --enable-bolt-settings --skip-stage0-validation --reproducible-artifact --set --help [PATHS]... [ARGS]..."
             if [[ ${cur} == -* || ${COMP_CWORD} -eq 2 ]] ; then
                 COMPREPLY=( $(compgen -W "${opts}" -- "${cur}") )
                 return 0

diff --git a/src/etc/completions/x.py.zsh b/src/etc/completions/x.py.zsh
@@ -271,6 +271,7 @@ _arguments "${_arguments_options[@]}" \
 '*--reproducible-artifact=[Additional reproducible artifacts that should be added to the reproducible artifacts archive]:REPRODUCIBLE_ARTIFACT: ' \
 '*--set=[override options in config.toml]:section.option=value:( )' \
 '--check[check formatting instead of applying]' \
+'--all[apply to all appropriate files, not just those that have been modified]' \
 '*-v[use verbose output (-vv for very verbose)]' \
 '*--verbose[use verbose output (-vv for very verbose)]' \
 '-i[use incremental compilation]' \