clippy_dev: refactor rustfmt calls

Author: Jarcho

clippy_dev/Cargo.toml

@@ -11,7 +11,6 @@ clap = { version = "4.4", features = ["derive"] }
 indoc = "1.0"
 itertools = "0.12"
 opener = "0.7"
-shell-escape = "0.1"
 walkdir = "2.3"
 
 [package.metadata.rust-analyzer]

clippy_dev/src/fmt.rs

@@ -1,19 +1,14 @@
+use crate::utils::{ClippyInfo, ErrAction, UpdateMode, panic_action, run_with_args_split, run_with_output};
 use itertools::Itertools;
 use rustc_lexer::{TokenKind, tokenize};
-use shell_escape::escape;
-use std::ffi::{OsStr, OsString};
 use std::ops::ControlFlow;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::process::{self, Command, Stdio};
 use std::{fs, io};
 use walkdir::WalkDir;
 
 pub enum Error {
-    CommandFailed(String, String),
     Io(io::Error),
-    RustfmtNotInstalled,
-    WalkDir(walkdir::Error),
-    IntellijSetupActive,
     Parse(PathBuf, usize, String),
     CheckFailed,
 }
@@ -24,50 +19,22 @@ impl From<io::Error> for Error {
     }
 }
 
-impl From<walkdir::Error> for Error {
-    fn from(error: walkdir::Error) -> Self {
-        Self::WalkDir(error)
-    }
-}
-
 impl Error {
     fn display(&self) {
         match self {
             Self::CheckFailed => {
                 eprintln!("Formatting check failed!\nRun `cargo dev fmt` to update.");
             },
-            Self::CommandFailed(command, stderr) => {
-                eprintln!("error: command `{command}` failed!\nstderr: {stderr}");
-            },
             Self::Io(err) => {
                 eprintln!("error: {err}");
             },
-            Self::RustfmtNotInstalled => {
-                eprintln!("error: rustfmt nightly is not installed.");
-            },
-            Self::WalkDir(err) => {
-                eprintln!("error: {err}");
-            },
-            Self::IntellijSetupActive => {
-                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`."
-                );
-            },
             Self::Parse(path, line, msg) => {
                 eprintln!("error parsing `{}:{line}`: {msg}", path.display());
             },
         }
     }
 }
 
-struct FmtContext {
-    check: bool,
-    verbose: bool,
-    rustfmt_path: String,
-}
-
 struct ClippyConf<'a> {
     name: &'a str,
     attrs: &'a str,
@@ -257,155 +224,90 @@ fn fmt_conf(check: bool) -> Result<(), Error> {
     Ok(())
 }
 
-fn run_rustfmt(context: &FmtContext) -> Result<(), Error> {
-    // if we added a local rustc repo as path dependency to clippy for rust analyzer, we do NOT want to
-    // format because rustfmt would also format the entire rustc repo as it is a local
-    // dependency
-    if fs::read_to_string("Cargo.toml")
-        .expect("Failed to read clippy Cargo.toml")
-        .contains("[target.'cfg(NOT_A_PLATFORM)'.dependencies]")
-    {
-        return Err(Error::IntellijSetupActive);
-    }
-
-    check_for_rustfmt(context)?;
+fn run_rustfmt(clippy: &ClippyInfo, 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());
 
-    cargo_fmt(context, ".".as_ref())?;
-    cargo_fmt(context, "clippy_dev".as_ref())?;
-    cargo_fmt(context, "rustc_tools_util".as_ref())?;
-    cargo_fmt(context, "lintcheck".as_ref())?;
+    // Start all format jobs first before waiting on the results.
+    let mut children = Vec::with_capacity(16);
+    for &path in &[".", "clippy_dev", "rustc_tools_util", "lintcheck"] {
+        let mut cmd = Command::new("cargo");
+        if update_mode.is_check() {
+            cmd.arg("--check");
+        }
+        match cmd
+            .current_dir(clippy.path.join(path))
+            .arg("fmt")
+            .stdout(Stdio::null())
+            .stdin(Stdio::null())
+            .stderr(Stdio::inherit())
+            .spawn()
+        {
+            Ok(x) => children.push(x),
+            Err(ref e) => panic_action(&e, ErrAction::Run, "cargo fmt".as_ref()),
+        }
+    }
 
-    let chunks = WalkDir::new("tests")
-        .into_iter()
-        .filter_map(|entry| {
-            let entry = entry.expect("failed to find tests");
-            let path = entry.path();
-            if path.extension() != Some("rs".as_ref())
-                || path
-                    .components()
-                    .nth_back(1)
-                    .is_some_and(|c| c.as_os_str() == "syntax-error-recovery")
-                || entry.file_name() == "ice-3891.rs"
-            {
-                None
-            } else {
-                Some(entry.into_path().into_os_string())
+    run_with_args_split(
+        || {
+            let mut cmd = Command::new(&rustfmt_path);
+            if update_mode.is_check() {
+                cmd.arg("--check");
             }
-        })
-        .chunks(250);
+            // Inherit stderr https://github.com/rust-lang/rustfmt/issues/6543 is fixed.
+            cmd.stdout(Stdio::null()).stdin(Stdio::null()).stderr(Stdio::null());
+            cmd
+        },
+        |cmd| match cmd.spawn() {
+            Ok(x) => children.push(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())
+            }),
+    );
 
-    for chunk in &chunks {
-        rustfmt(context, chunk)?;
+    for child in &mut children {
+        match child.wait() {
+            Ok(status) => match (update_mode, status.exit_ok()) {
+                (UpdateMode::Check | UpdateMode::Change, Ok(())) => {},
+                (UpdateMode::Check, Err(_)) => {
+                    eprintln!("Formatting check failed!\nRun `cargo dev fmt` to update.");
+                    process::exit(1);
+                },
+                (UpdateMode::Change, Err(e)) => panic_action(&e, ErrAction::Run, "cargo fmt".as_ref()),
+            },
+            Err(ref e) => panic_action(e, ErrAction::Run, "cargo fmt".as_ref()),
+        }
     }
-    Ok(())
 }
 
 // the "main" function of cargo dev fmt
-pub fn run(check: bool, verbose: bool) {
-    let output = Command::new("rustup")
-        .args(["which", "rustfmt"])
-        .stderr(Stdio::inherit())
-        .output()
-        .expect("error running `rustup which rustfmt`");
-    if !output.status.success() {
-        eprintln!("`rustup which rustfmt` did not execute successfully");
-        process::exit(1);
+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;
     }
-    let mut rustfmt_path = String::from_utf8(output.stdout).expect("invalid rustfmt path");
-    rustfmt_path.truncate(rustfmt_path.trim_end().len());
+    run_rustfmt(clippy, update_mode);
 
-    let context = FmtContext {
-        check,
-        verbose,
-        rustfmt_path,
-    };
-    if let Err(e) = run_rustfmt(&context).and_then(|()| fmt_conf(check)) {
+    if let Err(e) = fmt_conf(update_mode.is_check()) {
         e.display();
         process::exit(1);
     }
 }
-
-fn format_command(program: impl AsRef<OsStr>, dir: impl AsRef<Path>, args: &[impl AsRef<OsStr>]) -> String {
-    let arg_display: Vec<_> = args.iter().map(|a| escape(a.as_ref().to_string_lossy())).collect();
-
-    format!(
-        "cd {} && {} {}",
-        escape(dir.as_ref().to_string_lossy()),
-        escape(program.as_ref().to_string_lossy()),
-        arg_display.join(" ")
-    )
-}
-
-fn exec_fmt_command(
-    context: &FmtContext,
-    program: impl AsRef<OsStr>,
-    dir: impl AsRef<Path>,
-    args: &[impl AsRef<OsStr>],
-) -> Result<(), Error> {
-    if context.verbose {
-        println!("{}", format_command(&program, &dir, args));
-    }
-
-    let output = Command::new(&program)
-        .env("RUSTFMT", &context.rustfmt_path)
-        .current_dir(&dir)
-        .args(args.iter())
-        .output()
-        .unwrap();
-    let success = output.status.success();
-
-    match (context.check, success) {
-        (_, true) => Ok(()),
-        (true, false) => Err(Error::CheckFailed),
-        (false, false) => {
-            let stderr = std::str::from_utf8(&output.stderr).unwrap_or("");
-            Err(Error::CommandFailed(
-                format_command(&program, &dir, args),
-                String::from(stderr),
-            ))
-        },
-    }
-}
-
-fn cargo_fmt(context: &FmtContext, path: &Path) -> Result<(), Error> {
-    let mut args = vec!["fmt", "--all"];
-    if context.check {
-        args.push("--check");
-    }
-    exec_fmt_command(context, "cargo", path, &args)
-}
-
-fn check_for_rustfmt(context: &FmtContext) -> Result<(), Error> {
-    let program = "rustfmt";
-    let dir = std::env::current_dir()?;
-    let args = &["--version"];
-
-    if context.verbose {
-        println!("{}", format_command(program, &dir, args));
-    }
-
-    let output = Command::new(program).current_dir(&dir).args(args.iter()).output()?;
-
-    if output.status.success() {
-        Ok(())
-    } else if std::str::from_utf8(&output.stderr)
-        .unwrap_or("")
-        .starts_with("error: 'rustfmt' is not installed")
-    {
-        Err(Error::RustfmtNotInstalled)
-    } else {
-        Err(Error::CommandFailed(
-            format_command(program, &dir, args),
-            std::str::from_utf8(&output.stderr).unwrap_or("").to_string(),
-        ))
-    }
-}
-
-fn rustfmt(context: &FmtContext, paths: impl Iterator<Item = OsString>) -> Result<(), Error> {
-    let mut args = Vec::new();
-    if context.check {
-        args.push(OsString::from("--check"));
-    }
-    args.extend(paths);
-    exec_fmt_command(context, &context.rustfmt_path, std::env::current_dir()?, &args)
-}

clippy_dev/src/lib.rs

@@ -1,4 +1,4 @@
-#![feature(rustc_private, if_let_guard, let_chains)]
+#![feature(rustc_private, exit_status_error, if_let_guard, let_chains)]
 #![warn(
     trivial_casts,
     trivial_numeric_casts,

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, verbose } => fmt::run(check, verbose),
+        DevCommand::Fmt { check } => fmt::run(&clippy, utils::UpdateMode::from_check(check)),
         DevCommand::UpdateLints { check } => update_lints::update(utils::UpdateMode::from_check(check)),
         DevCommand::NewLint {
             pass,
@@ -125,9 +125,6 @@ enum DevCommand {
         #[arg(long)]
         /// Use the rustfmt --check option
         check: bool,
-        #[arg(short, long)]
-        /// Echo commands run
-        verbose: bool,
     },
     #[command(name = "update_lints")]
     /// Updates lint registration and information from the source code

clippy_dev/src/update_lints.rs

@@ -1,5 +1,5 @@
 use crate::utils::{
-    File, FileAction, FileUpdater, RustSearcher, Token, UpdateMode, UpdateStatus, panic_file, update_text_region_fn,
+    ErrAction, File, FileUpdater, RustSearcher, Token, UpdateMode, UpdateStatus, panic_action, update_text_region_fn,
 };
 use itertools::Itertools;
 use rustc_lexer::unescape;
@@ -173,7 +173,7 @@ fn read_src_with_module(src_root: &Path) -> impl use<'_> + Iterator<Item = (DirE
     WalkDir::new(src_root).into_iter().filter_map(move |e| {
         let e = match e {
             Ok(e) => e,
-            Err(ref e) => panic_file(e, FileAction::Read, src_root),
+            Err(ref e) => panic_action(e, ErrAction::Read, src_root),
         };
         let path = e.path().as_os_str().as_encoded_bytes();
         if let Some(path) = path.strip_suffix(b".rs")
@@ -220,9 +220,7 @@ fn parse_clippy_lint_decls(contents: &str, module: &str, lints: &mut Vec<Lint>)
     while searcher.find_token(Ident("declare_clippy_lint")) {
         let start = searcher.pos() as usize - "declare_clippy_lint".len();
         let (mut name, mut group) = ("", "");
-        if searcher.match_tokens(DECL_TOKENS, &mut [&mut name, &mut group])
-            && searcher.find_token(CloseBrace)
-        {
+        if searcher.match_tokens(DECL_TOKENS, &mut [&mut name, &mut group]) && searcher.find_token(CloseBrace) {
             lints.push(Lint {
                 name: name.to_lowercase(),
                 group: group.into(),