Improve speed of cargo dev fmt.

Author: Jarcho

clippy_dev/src/fmt.rs

@@ -1,5 +1,6 @@
 use crate::utils::{
-    ClippyInfo, ErrAction, FileUpdater, UpdateMode, UpdateStatus, panic_action, run_with_args_split, run_with_output,
+    ErrAction, FileUpdater, UpdateMode, UpdateStatus, panic_action, run_with_output, split_args_for_threads,
+    walk_dir_no_dot_or_target,
 };
 use itertools::Itertools;
 use rustc_lexer::{TokenKind, tokenize};
@@ -9,7 +10,6 @@ use std::io::{self, Read};
 use std::ops::ControlFlow;
 use std::path::PathBuf;
 use std::process::{self, Command, Stdio};
-use walkdir::WalkDir;
 
 pub enum Error {
     Io(io::Error),
@@ -260,50 +260,27 @@ fn fmt_syms(update_mode: UpdateMode) {
     );
 }
 
-fn run_rustfmt(clippy: &ClippyInfo, update_mode: UpdateMode) {
+fn run_rustfmt(update_mode: UpdateMode) {
     let mut rustfmt_path = String::from_utf8(run_with_output(
         "rustup which rustfmt",
         Command::new("rustup").args(["which", "rustfmt"]),
     ))
     .expect("invalid rustfmt path");
     rustfmt_path.truncate(rustfmt_path.trim_end().len());
 
-    let mut cargo_path = String::from_utf8(run_with_output(
-        "rustup which cargo",
-        Command::new("rustup").args(["which", "cargo"]),
-    ))
-    .expect("invalid cargo path");
-    cargo_path.truncate(cargo_path.trim_end().len());
-
-    // Start all format jobs first before waiting on the results.
-    let mut children = Vec::with_capacity(16);
-    for &path in &[
-        ".",
-        "clippy_config",
-        "clippy_dev",
-        "clippy_lints",
-        "clippy_lints_internal",
-        "clippy_utils",
-        "rustc_tools_util",
-        "lintcheck",
-    ] {
-        let mut cmd = Command::new(&cargo_path);
-        cmd.current_dir(clippy.path.join(path))
-            .args(["fmt"])
-            .env("RUSTFMT", &rustfmt_path)
-            .stdout(Stdio::null())
-            .stdin(Stdio::null())
-            .stderr(Stdio::piped());
-        if update_mode.is_check() {
-            cmd.arg("--check");
-        }
-        match cmd.spawn() {
-            Ok(x) => children.push(("cargo fmt", x)),
-            Err(ref e) => panic_action(&e, ErrAction::Run, "cargo fmt".as_ref()),
-        }
-    }
+    let args: Vec<_> = walk_dir_no_dot_or_target()
+        .filter_map(|e| {
+            let e = e.expect("error reading `.`");
+            e.path()
+                .as_os_str()
+                .as_encoded_bytes()
+                .ends_with(b".rs")
+                .then(|| e.into_path().into_os_string())
+        })
+        .collect();
 
-    run_with_args_split(
+    let mut children: Vec<_> = split_args_for_threads(
+        32,
         || {
             let mut cmd = Command::new(&rustfmt_path);
             if update_mode.is_check() {
@@ -312,27 +289,18 @@ fn run_rustfmt(clippy: &ClippyInfo, update_mode: UpdateMode) {
             cmd.stdout(Stdio::null())
                 .stdin(Stdio::null())
                 .stderr(Stdio::piped())
-                .args(["--config", "show_parse_errors=false"]);
+                .args(["--unstable-features", "--skip-children"]);
             cmd
         },
-        |cmd| match cmd.spawn() {
-            Ok(x) => children.push(("rustfmt", x)),
-            Err(ref e) => panic_action(&e, ErrAction::Run, "rustfmt".as_ref()),
-        },
-        WalkDir::new("tests")
-            .into_iter()
-            .filter_entry(|p| p.path().file_name().is_none_or(|x| x != "skip_rustfmt"))
-            .filter_map(|e| {
-                let e = e.expect("error reading `tests`");
-                e.path()
-                    .as_os_str()
-                    .as_encoded_bytes()
-                    .ends_with(b".rs")
-                    .then(|| e.into_path().into_os_string())
-            }),
-    );
+        args.iter(),
+    )
+    .map(|mut cmd| match cmd.spawn() {
+        Ok(x) => x,
+        Err(ref e) => panic_action(&e, ErrAction::Run, "rustfmt".as_ref()),
+    })
+    .collect();
 
-    for (name, child) in &mut children {
+    for child in &mut children {
         match child.wait() {
             Ok(status) => match (update_mode, status.exit_ok()) {
                 (UpdateMode::Check | UpdateMode::Change, Ok(())) => {},
@@ -353,25 +321,17 @@ fn run_rustfmt(clippy: &ClippyInfo, update_mode: UpdateMode) {
                     {
                         eprintln!("{s}");
                     }
-                    panic_action(&e, ErrAction::Run, name.as_ref());
+                    panic_action(&e, ErrAction::Run, "rustfmt".as_ref());
                 },
             },
-            Err(ref e) => panic_action(e, ErrAction::Run, name.as_ref()),
+            Err(ref e) => panic_action(e, ErrAction::Run, "rustfmt".as_ref()),
         }
     }
 }
 
 // the "main" function of cargo dev fmt
-pub fn run(clippy: &ClippyInfo, update_mode: UpdateMode) {
-    if clippy.has_intellij_hook {
-        eprintln!(
-            "error: a local rustc repo is enabled as path dependency via `cargo dev setup intellij`.\n\
-            Not formatting because that would format the local repo as well!\n\
-            Please revert the changes to `Cargo.toml`s with `cargo dev remove intellij`."
-        );
-        return;
-    }
-    run_rustfmt(clippy, update_mode);
+pub fn run(update_mode: UpdateMode) {
+    run_rustfmt(update_mode);
     fmt_syms(update_mode);
     if let Err(e) = fmt_conf(update_mode.is_check()) {
         e.display();

clippy_dev/src/main.rs

@@ -26,7 +26,7 @@ fn main() {
             allow_staged,
             allow_no_vcs,
         } => dogfood::dogfood(fix, allow_dirty, allow_staged, allow_no_vcs),
-        DevCommand::Fmt { check } => fmt::run(&clippy, utils::UpdateMode::from_check(check)),
+        DevCommand::Fmt { check } => fmt::run(utils::UpdateMode::from_check(check)),
         DevCommand::UpdateLints { check } => update_lints::update(utils::UpdateMode::from_check(check)),
         DevCommand::NewLint {
             pass,

clippy_dev/src/rename_lint.rs

@@ -1,13 +1,12 @@
 use crate::update_lints::{RenamedLint, find_lint_decls, generate_lint_files, read_deprecated_lints};
 use crate::utils::{
     FileUpdater, RustSearcher, Token, UpdateMode, UpdateStatus, Version, delete_dir_if_exists, delete_file_if_exists,
-    try_rename_dir, try_rename_file,
+    try_rename_dir, try_rename_file, walk_dir_no_dot_or_target,
 };
 use rustc_lexer::TokenKind;
 use std::ffi::OsString;
 use std::fs;
 use std::path::Path;
-use walkdir::WalkDir;
 
 /// Runs the `rename_lint` command.
 ///
@@ -133,15 +132,8 @@ pub fn rename(clippy_version: Version, old_name: &str, new_name: &str, uplift: b
     }
 
     let mut update_fn = file_update_fn(old_name, new_name, mod_edit);
-    for file in WalkDir::new(".").into_iter().filter_entry(|e| {
-        // Skip traversing some of the larger directories.
-        e.path()
-            .as_os_str()
-            .as_encoded_bytes()
-            .get(2..)
-            .is_none_or(|x| x != "target".as_bytes() && x != ".git".as_bytes())
-    }) {
-        let file = file.expect("error reading clippy directory");
+    for file in walk_dir_no_dot_or_target() {
+        let file = file.expect("error reading `.`");
         if file.path().as_os_str().as_encoded_bytes().ends_with(b".rs") {
             updater.update_file(file.path(), &mut update_fn);
         }

clippy_dev/src/utils.rs

@@ -1,14 +1,16 @@
 use core::fmt::{self, Display};
+use core::num::NonZero;
 use core::ops::Range;
 use core::slice;
 use core::str::FromStr;
 use rustc_lexer::{self as lexer, FrontmatterAllowed};
-use std::env;
 use std::ffi::OsStr;
 use std::fs::{self, OpenOptions};
 use std::io::{self, Read as _, Seek as _, SeekFrom, Write};
 use std::path::{Path, PathBuf};
 use std::process::{self, Command, ExitStatus, Stdio};
+use std::{env, thread};
+use walkdir::WalkDir;
 
 #[cfg(not(windows))]
 static CARGO_CLIPPY_EXE: &str = "cargo-clippy";
@@ -682,25 +684,71 @@ pub fn run_with_output(path: &(impl AsRef<Path> + ?Sized), cmd: &mut Command) ->
     f(path.as_ref(), cmd)
 }
 
-pub fn run_with_args_split(
-    mut make_cmd: impl FnMut() -> Command,
-    mut run_cmd: impl FnMut(&mut Command),
-    args: impl Iterator<Item: AsRef<OsStr>>,
-) {
-    let mut cmd = make_cmd();
-    let mut len = 0;
-    for arg in args {
-        len += arg.as_ref().len();
-        cmd.arg(arg);
-        // Very conservative limit
-        if len > 10000 {
-            run_cmd(&mut cmd);
-            cmd = make_cmd();
-            len = 0;
+/// Splits an argument list across multiple `Command` invocations.
+///
+/// The argument list will be split into a number of batches based on
+/// `thread::available_parallelism`, with `min_batch_size` setting a lower bound on the size of each
+/// batch.
+///
+/// If the size of the arguments would exceed the system limit additional batches will be created.
+pub fn split_args_for_threads(
+    min_batch_size: usize,
+    make_cmd: impl FnMut() -> Command,
+    args: impl ExactSizeIterator<Item: AsRef<OsStr>>,
+) -> impl Iterator<Item = Command> {
+    struct Iter<F, I> {
+        make_cmd: F,
+        args: I,
+        min_batch_size: usize,
+        batch_size: usize,
+        thread_count: usize,
+    }
+    impl<F, I> Iterator for Iter<F, I>
+    where
+        F: FnMut() -> Command,
+        I: ExactSizeIterator<Item: AsRef<OsStr>>,
+    {
+        type Item = Command;
+        fn next(&mut self) -> Option<Self::Item> {
+            if self.thread_count > 1 {
+                self.thread_count -= 1;
+            }
+            let mut cmd = (self.make_cmd)();
+            let mut cmd_len = 0usize;
+            for arg in self.args.by_ref().take(self.batch_size) {
+                cmd.arg(arg.as_ref());
+                // `+ 8` to account for the `argv` pointer on unix.
+                // Windows is complicated since the arguments are first converted to UTF-16ish,
+                // but this needs to account for the space between arguments and whatever additional
+                // is needed to escape within an argument.
+                cmd_len += arg.as_ref().len() + 8;
+                cmd_len += 8;
+
+                // Windows has a command length limit of 32767. For unix systems this is more
+                // complicated since the limit includes environment variables and room needs to be
+                // left to edit them once the program starts, but the total size comes from
+                // `getconf ARG_MAX`.
+                //
+                // For simplicity we use 30000 here under a few assumptions.
+                // * Individual arguments aren't super long (the final argument is still added)
+                // * `ARG_MAX` is set to a reasonable amount. Basically every system will be configured way above
+                //   what windows supports, but POSIX only requires `4096`.
+                if cmd_len > 30000 {
+                    self.batch_size = self.args.len().div_ceil(self.thread_count).max(self.min_batch_size);
+                    break;
+                }
+            }
+            (cmd_len != 0).then_some(cmd)
         }
     }
-    if len != 0 {
-        run_cmd(&mut cmd);
+    let thread_count = thread::available_parallelism().map_or(1, NonZero::get);
+    let batch_size = args.len().div_ceil(thread_count).max(min_batch_size);
+    Iter {
+        make_cmd,
+        args,
+        min_batch_size,
+        batch_size,
+        thread_count,
     }
 }
 
@@ -720,3 +768,12 @@ pub fn delete_dir_if_exists(path: &Path) {
         Err(ref e) => panic_action(e, ErrAction::Delete, path),
     }
 }
+
+/// Walks all items excluding top-level dot files/directories and any target directories.
+pub fn walk_dir_no_dot_or_target() -> impl Iterator<Item = ::walkdir::Result<::walkdir::DirEntry>> {
+    WalkDir::new(".").into_iter().filter_entry(|e| {
+        e.path()
+            .file_name()
+            .is_none_or(|x| x != "target" && x.as_encoded_bytes().first().copied() != Some(b'.'))
+    })
+}

rustfmt.toml

@@ -6,4 +6,8 @@ edition = "2024"
 error_on_line_overflow = true
 imports_granularity = "Module"
 style_edition = "2024"
-ignore = ["tests/ui/crashes/ice-10912.rs"]
+ignore = [
+    "tests/ui/crashes/ice-9405.rs",
+    "tests/ui/crashes/ice-10912.rs",
+    "tests/ui/non_expressive_names_error_recovery.rs",
+]

tests/ui/skip_rustfmt/non_expressive_names_error_recovery.fixed renamed to tests/ui/non_expressive_names_error_recovery.fixed

@@ -1,5 +1,5 @@
 error: expected one of `!`, `(`, `+`, `,`, `::`, `<`, or `>`, found `)`
-  --> tests/ui/skip_rustfmt/non_expressive_names_error_recovery.rs:6:19
+  --> tests/ui/non_expressive_names_error_recovery.rs:6:19
    |
 LL | fn aa(a: Aa<String) {
    |                   ^ expected one of 7 possible tokens