Misc clippy_dev changes (#14896)

Jarcho · web-flow · commit fc42a205e9e4 · 2025-08-17T11:26:46.000Z
changelog: none
diff --git a/clippy_dev/src/dogfood.rs b/clippy_dev/src/dogfood.rs
@@ -1,35 +1,28 @@
-use crate::utils::exit_if_err;
-use std::process::Command;
+use crate::utils::{cargo_cmd, run_exit_on_err};
+use itertools::Itertools;
 
 /// # Panics
 ///
 /// Panics if unable to run the dogfood test
 #[allow(clippy::fn_params_excessive_bools)]
 pub fn dogfood(fix: bool, allow_dirty: bool, allow_staged: bool, allow_no_vcs: bool) {
-    let mut cmd = Command::new("cargo");
-
-    cmd.args(["test", "--test", "dogfood"])
-        .args(["--features", "internal"])
-        .args(["--", "dogfood_clippy", "--nocapture"]);
-
-    let mut dogfood_args = Vec::new();
-    if fix {
-        dogfood_args.push("--fix");
-    }
-
-    if allow_dirty {
-        dogfood_args.push("--allow-dirty");
-    }
-
-    if allow_staged {
-        dogfood_args.push("--allow-staged");
-    }
-
-    if allow_no_vcs {
-        dogfood_args.push("--allow-no-vcs");
-    }
-
-    cmd.env("__CLIPPY_DOGFOOD_ARGS", dogfood_args.join(" "));
-
-    exit_if_err(cmd.status());
+    run_exit_on_err(
+        "cargo test",
+        cargo_cmd()
+            .args(["test", "--test", "dogfood"])
+            .args(["--features", "internal"])
+            .args(["--", "dogfood_clippy", "--nocapture"])
+            .env(
+                "__CLIPPY_DOGFOOD_ARGS",
+                [
+                    if fix { "--fix" } else { "" },
+                    if allow_dirty { "--allow-dirty" } else { "" },
+                    if allow_staged { "--allow-staged" } else { "" },
+                    if allow_no_vcs { "--allow-no-vcs" } else { "" },
+                ]
+                .iter()
+                .filter(|x| !x.is_empty())
+                .join(" "),
+            ),
+    );
 }
diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs
@@ -15,8 +15,7 @@
 )]
 #![allow(clippy::missing_panics_doc)]
 
-// The `rustc_driver` crate seems to be required in order to use the `rust_lexer` crate.
-#[allow(unused_extern_crates)]
+#[expect(unused_extern_crates, reason = "required to link to rustc crates")]
 extern crate rustc_driver;
 extern crate rustc_lexer;
 extern crate rustc_literal_escaper;
@@ -32,4 +31,6 @@ pub mod serve;
 pub mod setup;
 pub mod sync;
 pub mod update_lints;
-pub mod utils;
+
+mod utils;
+pub use utils::{ClippyInfo, UpdateMode};
diff --git a/clippy_dev/src/lint.rs b/clippy_dev/src/lint.rs
@@ -1,44 +1,44 @@
-use crate::utils::{cargo_clippy_path, exit_if_err};
-use std::process::{self, Command};
+use crate::utils::{ErrAction, cargo_cmd, expect_action, run_exit_on_err};
+use std::process::Command;
 use std::{env, fs};
 
-pub fn run<'a>(path: &str, edition: &str, args: impl Iterator<Item = &'a String>) {
-    let is_file = match fs::metadata(path) {
-        Ok(metadata) => metadata.is_file(),
-        Err(e) => {
-            eprintln!("Failed to read {path}: {e:?}");
-            process::exit(1);
-        },
-    };
+#[cfg(not(windows))]
+static CARGO_CLIPPY_EXE: &str = "cargo-clippy";
+#[cfg(windows)]
+static CARGO_CLIPPY_EXE: &str = "cargo-clippy.exe";
 
+pub fn run<'a>(path: &str, edition: &str, args: impl Iterator<Item = &'a String>) {
+    let is_file = expect_action(fs::metadata(path), ErrAction::Read, path).is_file();
     if is_file {
-        exit_if_err(
-            Command::new(env::var("CARGO").unwrap_or_else(|_| "cargo".into()))
+        run_exit_on_err(
+            "cargo run",
+            cargo_cmd()
                 .args(["run", "--bin", "clippy-driver", "--"])
                 .args(["-L", "./target/debug"])
                 .args(["-Z", "no-codegen"])
                 .args(["--edition", edition])
                 .arg(path)
                 .args(args)
                 // Prevent rustc from creating `rustc-ice-*` files the console output is enough.
-                .env("RUSTC_ICE", "0")
-                .status(),
+                .env("RUSTC_ICE", "0"),
         );
     } else {
-        exit_if_err(
-            Command::new(env::var("CARGO").unwrap_or_else(|_| "cargo".into()))
-                .arg("build")
-                .status(),
-        );
-
-        let status = Command::new(cargo_clippy_path())
-            .arg("clippy")
-            .args(args)
-            // Prevent rustc from creating `rustc-ice-*` files the console output is enough.
-            .env("RUSTC_ICE", "0")
-            .current_dir(path)
-            .status();
+        // Ideally this would just be `cargo run`, but the working directory needs to be
+        // set to clippy's directory when building, and the target project's directory
+        // when running clippy. `cargo` can only set a single working directory for both
+        // when using `run`.
+        run_exit_on_err("cargo build", cargo_cmd().arg("build"));
 
-        exit_if_err(status);
+        let mut exe = env::current_exe().expect("failed to get current executable name");
+        exe.set_file_name(CARGO_CLIPPY_EXE);
+        run_exit_on_err(
+            "cargo clippy",
+            Command::new(exe)
+                .arg("clippy")
+                .args(args)
+                // Prevent rustc from creating `rustc-ice-*` files the console output is enough.
+                .env("RUSTC_ICE", "0")
+                .current_dir(path),
+        );
     }
 }
diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs
@@ -4,14 +4,15 @@
 
 use clap::{Args, Parser, Subcommand};
 use clippy_dev::{
-    deprecate_lint, dogfood, fmt, lint, new_lint, release, rename_lint, serve, setup, sync, update_lints, utils,
+    ClippyInfo, UpdateMode, deprecate_lint, dogfood, fmt, lint, new_lint, release, rename_lint, serve, setup, sync,
+    update_lints,
 };
 use std::convert::Infallible;
 use std::env;
 
 fn main() {
     let dev = Dev::parse();
-    let clippy = utils::ClippyInfo::search_for_manifest();
+    let clippy = ClippyInfo::search_for_manifest();
     if let Err(e) = env::set_current_dir(&clippy.path) {
         panic!("error setting current directory to `{}`: {e}", clippy.path.display());
     }
@@ -26,16 +27,16 @@ fn main() {
             allow_staged,
             allow_no_vcs,
         } => dogfood::dogfood(fix, allow_dirty, allow_staged, allow_no_vcs),
-        DevCommand::Fmt { check } => fmt::run(utils::UpdateMode::from_check(check)),
-        DevCommand::UpdateLints { check } => update_lints::update(utils::UpdateMode::from_check(check)),
+        DevCommand::Fmt { check } => fmt::run(UpdateMode::from_check(check)),
+        DevCommand::UpdateLints { check } => update_lints::update(UpdateMode::from_check(check)),
         DevCommand::NewLint {
             pass,
             name,
             category,
             r#type,
             msrv,
         } => match new_lint::create(clippy.version, pass, &name, &category, r#type.as_deref(), msrv) {
-            Ok(()) => update_lints::update(utils::UpdateMode::Change),
+            Ok(()) => update_lints::update(UpdateMode::Change),
             Err(e) => eprintln!("Unable to create lint: {e}"),
         },
         DevCommand::Setup(SetupCommand { subcommand }) => match subcommand {
diff --git a/clippy_dev/src/serve.rs b/clippy_dev/src/serve.rs
@@ -1,7 +1,11 @@
+use crate::utils::{ErrAction, cargo_cmd, expect_action};
+use core::fmt::Display;
+use core::mem;
 use std::path::Path;
 use std::process::Command;
 use std::time::{Duration, SystemTime};
-use std::{env, thread};
+use std::{fs, thread};
+use walkdir::WalkDir;
 
 #[cfg(windows)]
 const PYTHON: &str = "python";
@@ -18,56 +22,83 @@ pub fn run(port: u16, lint: Option<String>) -> ! {
         Some(lint) => format!("http://localhost:{port}/#{lint}"),
     });
 
+    let mut last_update = mtime("util/gh-pages/index.html");
     loop {
-        let index_time = mtime("util/gh-pages/index.html");
-        let times = [
-            "clippy_lints/src",
-            "util/gh-pages/index_template.html",
-            "tests/compile-test.rs",
-        ]
-        .map(mtime);
-
-        if times.iter().any(|&time| index_time < time) {
-            Command::new(env::var("CARGO").unwrap_or_else(|_| "cargo".into()))
-                .arg("collect-metadata")
-                .spawn()
-                .unwrap()
-                .wait()
-                .unwrap();
+        if is_metadata_outdated(mem::replace(&mut last_update, SystemTime::now())) {
+            // Ignore the command result; we'll fall back to displaying the old metadata.
+            let _ = expect_action(
+                cargo_cmd().arg("collect-metadata").status(),
+                ErrAction::Run,
+                "cargo collect-metadata",
+            );
+            last_update = SystemTime::now();
         }
+
+        // Only start the web server the first time around.
         if let Some(url) = url.take() {
             thread::spawn(move || {
-                let mut child = Command::new(PYTHON)
-                    .arg("-m")
-                    .arg("http.server")
-                    .arg(port.to_string())
-                    .current_dir("util/gh-pages")
-                    .spawn()
-                    .unwrap();
+                let mut child = expect_action(
+                    Command::new(PYTHON)
+                        .args(["-m", "http.server", port.to_string().as_str()])
+                        .current_dir("util/gh-pages")
+                        .spawn(),
+                    ErrAction::Run,
+                    "python -m http.server",
+                );
                 // Give some time for python to start
                 thread::sleep(Duration::from_millis(500));
                 // Launch browser after first export.py has completed and http.server is up
                 let _result = opener::open(url);
-                child.wait().unwrap();
+                expect_action(child.wait(), ErrAction::Run, "python -m http.server");
             });
         }
+
+        // Delay to avoid updating the metadata too aggressively.
         thread::sleep(Duration::from_millis(1000));
     }
 }
 
-fn mtime(path: impl AsRef<Path>) -> SystemTime {
-    let path = path.as_ref();
-    if path.is_dir() {
-        path.read_dir()
-            .into_iter()
-            .flatten()
-            .flatten()
-            .map(|entry| mtime(entry.path()))
-            .max()
-            .unwrap_or(SystemTime::UNIX_EPOCH)
-    } else {
-        path.metadata()
-            .and_then(|metadata| metadata.modified())
-            .unwrap_or(SystemTime::UNIX_EPOCH)
+fn log_err_and_continue<T>(res: Result<T, impl Display>, path: &Path) -> Option<T> {
+    match res {
+        Ok(x) => Some(x),
+        Err(ref e) => {
+            eprintln!("error reading `{}`: {e}", path.display());
+            None
+        },
     }
 }
+
+fn mtime(path: &str) -> SystemTime {
+    log_err_and_continue(fs::metadata(path), path.as_ref())
+        .and_then(|metadata| log_err_and_continue(metadata.modified(), path.as_ref()))
+        .unwrap_or(SystemTime::UNIX_EPOCH)
+}
+
+fn is_metadata_outdated(time: SystemTime) -> bool {
+    // Ignore all IO errors here. We don't want to stop them from hosting the server.
+    if time < mtime("util/gh-pages/index_template.html") || time < mtime("tests/compile-test.rs") {
+        return true;
+    }
+    let Some(dir) = log_err_and_continue(fs::read_dir("."), ".".as_ref()) else {
+        return false;
+    };
+    dir.map_while(|e| log_err_and_continue(e, ".".as_ref())).any(|e| {
+        let name = e.file_name();
+        let name_bytes = name.as_encoded_bytes();
+        if (name_bytes.starts_with(b"clippy_lints") && name_bytes != b"clippy_lints_internal")
+            || name_bytes == b"clippy_config"
+        {
+            WalkDir::new(&name)
+                .into_iter()
+                .map_while(|e| log_err_and_continue(e, name.as_ref()))
+                .filter(|e| e.file_type().is_file())
+                .filter_map(|e| {
+                    log_err_and_continue(e.metadata(), e.path())
+                        .and_then(|m| log_err_and_continue(m.modified(), e.path()))
+                })
+                .any(|ftime| time < ftime)
+        } else {
+            false
+        }
+    })
+}
diff --git a/clippy_dev/src/setup/git_hook.rs b/clippy_dev/src/setup/git_hook.rs
@@ -1,8 +1,6 @@
 use std::fs;
 use std::path::Path;
 
-use super::verify_inside_clippy_dir;
-
 /// Rusts setup uses `git rev-parse --git-common-dir` to get the root directory of the repo.
 /// I've decided against this for the sake of simplicity and to make sure that it doesn't install
 /// the hook if `clippy_dev` would be used in the rust tree. The hook also references this tool
@@ -35,10 +33,6 @@ pub fn install_hook(force_override: bool) {
 }
 
 fn check_precondition(force_override: bool) -> bool {
-    if !verify_inside_clippy_dir() {
-        return false;
-    }
-
     // Make sure that we can find the git repository
     let git_path = Path::new(REPO_GIT_DIR);
     if !git_path.exists() || !git_path.is_dir() {
diff --git a/clippy_dev/src/setup/mod.rs b/clippy_dev/src/setup/mod.rs
@@ -2,23 +2,3 @@ pub mod git_hook;
 pub mod intellij;
 pub mod toolchain;
 pub mod vscode;
-
-use std::path::Path;
-
-const CLIPPY_DEV_DIR: &str = "clippy_dev";
-
-/// This function verifies that the tool is being executed in the clippy directory.
-/// This is useful to ensure that setups only modify Clippy's resources. The verification
-/// is done by checking that `clippy_dev` is a sub directory of the current directory.
-///
-/// It will print an error message and return `false` if the directory could not be
-/// verified.
-fn verify_inside_clippy_dir() -> bool {
-    let path = Path::new(CLIPPY_DEV_DIR);
-    if path.exists() && path.is_dir() {
-        true
-    } else {
-        eprintln!("error: unable to verify that the working directory is clippy's directory");
-        false
-    }
-}
diff --git a/clippy_dev/src/setup/toolchain.rs b/clippy_dev/src/setup/toolchain.rs
diff --git a/clippy_dev/src/setup/vscode.rs b/clippy_dev/src/setup/vscode.rs
diff --git a/clippy_dev/src/utils.rs b/clippy_dev/src/utils.rs
