Rollup merge of #122883 - onur-ozkan:clippy-build-step, r=albertlarsan68

refactor clippy in bootstrap

Previously, using clippy in bootstrap was not very useful as explained in #122825. In short, regardless of the given path clippy would always check the entire compiler and std tree. This makes it impossible to run clippy on different paths with different set of rules. This PR fixes that by allowing developers to run clippy with specific rules on specific paths (e.g., we can run `x clippy compiler -Aclippy::all -Dclippy::correctness` and `x clippy library/std -Dclippy::all` and none of them will affect each other).

Resolves #122825

src/bootstrap/src/core/build_steps/check.rs

@@ -11,6 +11,16 @@ use crate::core::config::TargetSelection;
 use crate::{Compiler, Mode, Subcommand};
 use std::path::{Path, PathBuf};
 
+pub fn cargo_subcommand(kind: Kind) -> &'static str {
+    match kind {
+        Kind::Check
+        // We ensure check steps for both std and rustc from build_steps/clippy, so handle `Kind::Clippy` as well.
+        | Kind::Clippy => "check",
+        Kind::Fix => "fix",
+        _ => unreachable!(),
+    }
+}
+
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct Std {
     pub target: TargetSelection,
@@ -22,97 +32,6 @@ pub struct Std {
     crates: Vec<String>,
 }
 
-/// Returns args for the subcommand itself (not for cargo)
-fn args(builder: &Builder<'_>) -> Vec<String> {
-    fn strings<'a>(arr: &'a [&str]) -> impl Iterator<Item = String> + 'a {
-        arr.iter().copied().map(String::from)
-    }
-
-    if let Subcommand::Clippy { fix, allow_dirty, allow_staged, allow, deny, warn, forbid } =
-        &builder.config.cmd
-    {
-        // disable the most spammy clippy lints
-        let ignored_lints = [
-            "many_single_char_names", // there are a lot in stdarch
-            "collapsible_if",
-            "type_complexity",
-            "missing_safety_doc", // almost 3K warnings
-            "too_many_arguments",
-            "needless_lifetimes", // people want to keep the lifetimes
-            "wrong_self_convention",
-        ];
-        let mut args = vec![];
-        if *fix {
-            #[rustfmt::skip]
-            args.extend(strings(&[
-                "--fix", "-Zunstable-options",
-                // FIXME: currently, `--fix` gives an error while checking tests for libtest,
-                // possibly because libtest is not yet built in the sysroot.
-                // As a workaround, avoid checking tests and benches when passed --fix.
-                "--lib", "--bins", "--examples",
-            ]));
-
-            if *allow_dirty {
-                args.push("--allow-dirty".to_owned());
-            }
-
-            if *allow_staged {
-                args.push("--allow-staged".to_owned());
-            }
-        }
-
-        args.extend(strings(&["--"]));
-
-        if deny.is_empty() && forbid.is_empty() {
-            args.extend(strings(&["--cap-lints", "warn"]));
-        }
-
-        let all_args = std::env::args().collect::<Vec<_>>();
-        args.extend(get_clippy_rules_in_order(&all_args, allow, deny, warn, forbid));
-
-        args.extend(ignored_lints.iter().map(|lint| format!("-Aclippy::{}", lint)));
-        args.extend(builder.config.free_args.clone());
-        args
-    } else {
-        builder.config.free_args.clone()
-    }
-}
-
-/// We need to keep the order of the given clippy lint rules before passing them.
-/// Since clap doesn't offer any useful interface for this purpose out of the box,
-/// we have to handle it manually.
-pub(crate) fn get_clippy_rules_in_order(
-    all_args: &[String],
-    allow_rules: &[String],
-    deny_rules: &[String],
-    warn_rules: &[String],
-    forbid_rules: &[String],
-) -> Vec<String> {
-    let mut result = vec![];
-
-    for (prefix, item) in
-        [("-A", allow_rules), ("-D", deny_rules), ("-W", warn_rules), ("-F", forbid_rules)]
-    {
-        item.iter().for_each(|v| {
-            let rule = format!("{prefix}{v}");
-            let position = all_args.iter().position(|t| t == &rule).unwrap();
-            result.push((position, rule));
-        });
-    }
-
-    result.sort_by_key(|&(position, _)| position);
-    result.into_iter().map(|v| v.1).collect()
-}
-
-fn cargo_subcommand(kind: Kind) -> &'static str {
-    match kind {
-        Kind::Check => "check",
-        Kind::Clippy => "clippy",
-        Kind::Fix => "fix",
-        _ => unreachable!(),
-    }
-}
-
 impl Std {
     pub fn new(target: TargetSelection) -> Self {
         Self { target, crates: vec![] }
@@ -164,7 +83,7 @@ impl Step for Std {
         run_cargo(
             builder,
             cargo,
-            args(builder),
+            builder.config.free_args.clone(),
             &libstd_stamp(builder, compiler, target),
             vec![],
             true,
@@ -221,7 +140,7 @@ impl Step for Std {
         run_cargo(
             builder,
             cargo,
-            args(builder),
+            builder.config.free_args.clone(),
             &libstd_test_stamp(builder, compiler, target),
             vec![],
             true,
@@ -318,7 +237,7 @@ impl Step for Rustc {
         run_cargo(
             builder,
             cargo,
-            args(builder),
+            builder.config.free_args.clone(),
             &librustc_stamp(builder, compiler, target),
             vec![],
             true,
@@ -384,7 +303,7 @@ impl Step for CodegenBackend {
         run_cargo(
             builder,
             cargo,
-            args(builder),
+            builder.config.free_args.clone(),
             &codegen_backend_stamp(builder, compiler, target, backend),
             vec![],
             true,
@@ -450,7 +369,7 @@ impl Step for RustAnalyzer {
         run_cargo(
             builder,
             cargo,
-            args(builder),
+            builder.config.free_args.clone(),
             &stamp(builder, compiler, target),
             vec![],
             true,
@@ -513,7 +432,7 @@ macro_rules! tool_check_step {
                 run_cargo(
                     builder,
                     cargo,
-                    args(builder),
+                    builder.config.free_args.clone(),
                     &stamp(builder, compiler, target),
                     vec![],
                     true,

src/bootstrap/src/core/build_steps/clippy.rs

@@ -0,0 +1,330 @@
+//! Implementation of running clippy on the compiler, standard library and various tools.
+
+use std::path::Path;
+
+use crate::builder::Builder;
+use crate::builder::ShouldRun;
+use crate::core::builder;
+use crate::core::builder::crate_description;
+use crate::core::builder::Alias;
+use crate::core::builder::Kind;
+use crate::core::builder::RunConfig;
+use crate::core::builder::Step;
+use crate::Mode;
+use crate::Subcommand;
+use crate::TargetSelection;
+
+use super::check;
+use super::compile;
+use super::compile::librustc_stamp;
+use super::compile::libstd_stamp;
+use super::compile::run_cargo;
+use super::compile::rustc_cargo;
+use super::compile::std_cargo;
+use super::tool::prepare_tool_cargo;
+use super::tool::SourceType;
+
+// Disable the most spammy clippy lints
+const IGNORED_RULES_FOR_STD_AND_RUSTC: &[&str] = &[
+    "many_single_char_names", // there are a lot in stdarch
+    "collapsible_if",
+    "type_complexity",
+    "missing_safety_doc", // almost 3K warnings
+    "too_many_arguments",
+    "needless_lifetimes", // people want to keep the lifetimes
+    "wrong_self_convention",
+];
+
+fn lint_args(builder: &Builder<'_>, ignored_rules: &[&str]) -> Vec<String> {
+    fn strings<'a>(arr: &'a [&str]) -> impl Iterator<Item = String> + 'a {
+        arr.iter().copied().map(String::from)
+    }
+
+    let Subcommand::Clippy { fix, allow_dirty, allow_staged, allow, deny, warn, forbid } =
+        &builder.config.cmd
+    else {
+        unreachable!("clippy::lint_args can only be called from `clippy` subcommands.");
+    };
+
+    let mut args = vec![];
+    if *fix {
+        #[rustfmt::skip]
+            args.extend(strings(&[
+                "--fix", "-Zunstable-options",
+                // FIXME: currently, `--fix` gives an error while checking tests for libtest,
+                // possibly because libtest is not yet built in the sysroot.
+                // As a workaround, avoid checking tests and benches when passed --fix.
+                "--lib", "--bins", "--examples",
+            ]));
+
+        if *allow_dirty {
+            args.push("--allow-dirty".to_owned());
+        }
+
+        if *allow_staged {
+            args.push("--allow-staged".to_owned());
+        }
+    }
+
+    args.extend(strings(&["--"]));
+
+    if deny.is_empty() && forbid.is_empty() {
+        args.extend(strings(&["--cap-lints", "warn"]));
+    }
+
+    let all_args = std::env::args().collect::<Vec<_>>();
+    args.extend(get_clippy_rules_in_order(&all_args, allow, deny, warn, forbid));
+
+    args.extend(ignored_rules.iter().map(|lint| format!("-Aclippy::{}", lint)));
+    args.extend(builder.config.free_args.clone());
+    args
+}
+
+/// We need to keep the order of the given clippy lint rules before passing them.
+/// Since clap doesn't offer any useful interface for this purpose out of the box,
+/// we have to handle it manually.
+pub(crate) fn get_clippy_rules_in_order(
+    all_args: &[String],
+    allow_rules: &[String],
+    deny_rules: &[String],
+    warn_rules: &[String],
+    forbid_rules: &[String],
+) -> Vec<String> {
+    let mut result = vec![];
+
+    for (prefix, item) in
+        [("-A", allow_rules), ("-D", deny_rules), ("-W", warn_rules), ("-F", forbid_rules)]
+    {
+        item.iter().for_each(|v| {
+            let rule = format!("{prefix}{v}");
+            let position = all_args.iter().position(|t| t == &rule).unwrap();
+            result.push((position, rule));
+        });
+    }
+
+    result.sort_by_key(|&(position, _)| position);
+    result.into_iter().map(|v| v.1).collect()
+}
+
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct Std {
+    pub target: TargetSelection,
+    /// Whether to lint only a subset of crates.
+    crates: Vec<String>,
+}
+
+impl Step for Std {
+    type Output = ();
+    const DEFAULT: bool = true;
+
+    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+        run.crate_or_deps("sysroot").path("library")
+    }
+
+    fn make_run(run: RunConfig<'_>) {
+        let crates = run.make_run_crates(Alias::Library);
+        run.builder.ensure(Std { target: run.target, crates });
+    }
+
+    fn run(self, builder: &Builder<'_>) {
+        builder.update_submodule(&Path::new("library").join("stdarch"));
+
+        let target = self.target;
+        let compiler = builder.compiler(builder.top_stage, builder.config.build);
+
+        let mut cargo =
+            builder::Cargo::new(builder, compiler, Mode::Std, SourceType::InTree, target, "clippy");
+
+        std_cargo(builder, target, compiler.stage, &mut cargo);
+
+        for krate in &*self.crates {
+            cargo.arg("-p").arg(krate);
+        }
+
+        let _guard =
+            builder.msg_clippy(format_args!("library{}", crate_description(&self.crates)), target);
+
+        run_cargo(
+            builder,
+            cargo,
+            lint_args(builder, IGNORED_RULES_FOR_STD_AND_RUSTC),
+            &libstd_stamp(builder, compiler, target),
+            vec![],
+            true,
+            false,
+        );
+    }
+}
+
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct Rustc {
+    pub target: TargetSelection,
+    /// Whether to lint only a subset of crates.
+    crates: Vec<String>,
+}
+
+impl Step for Rustc {
+    type Output = ();
+    const ONLY_HOSTS: bool = true;
+    const DEFAULT: bool = true;
+
+    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+        run.crate_or_deps("rustc-main").path("compiler")
+    }
+
+    fn make_run(run: RunConfig<'_>) {
+        let crates = run.make_run_crates(Alias::Compiler);
+        run.builder.ensure(Rustc { target: run.target, crates });
+    }
+
+    /// Lints the compiler.
+    ///
+    /// This will lint the compiler for a particular stage of the build using
+    /// the `compiler` targeting the `target` architecture.
+    fn run(self, builder: &Builder<'_>) {
+        let compiler = builder.compiler(builder.top_stage, builder.config.build);
+        let target = self.target;
+
+        if compiler.stage != 0 {
+            // If we're not in stage 0, then we won't have a std from the beta
+            // compiler around. That means we need to make sure there's one in
+            // the sysroot for the compiler to find. Otherwise, we're going to
+            // fail when building crates that need to generate code (e.g., build
+            // scripts and their dependencies).
+            builder.ensure(compile::Std::new(compiler, compiler.host));
+            builder.ensure(compile::Std::new(compiler, target));
+        } else {
+            builder.ensure(check::Std::new(target));
+        }
+
+        let mut cargo = builder::Cargo::new(
+            builder,
+            compiler,
+            Mode::Rustc,
+            SourceType::InTree,
+            target,
+            "clippy",
+        );
+
+        rustc_cargo(builder, &mut cargo, target, &compiler);
+
+        // Explicitly pass -p for all compiler crates -- this will force cargo
+        // to also lint the tests/benches/examples for these crates, rather
+        // than just the leaf crate.
+        for krate in &*self.crates {
+            cargo.arg("-p").arg(krate);
+        }
+
+        let _guard =
+            builder.msg_clippy(format_args!("compiler{}", crate_description(&self.crates)), target);
+
+        run_cargo(
+            builder,
+            cargo,
+            lint_args(builder, IGNORED_RULES_FOR_STD_AND_RUSTC),
+            &librustc_stamp(builder, compiler, target),
+            vec![],
+            true,
+            false,
+        );
+    }
+}
+
+macro_rules! lint_any {
+    ($(
+        $name:ident, $path:expr, $readable_name:expr
+        $(,lint_by_default = $lint_by_default:expr)*
+        ;
+    )+) => {
+        $(
+
+        #[derive(Debug, Clone, Hash, PartialEq, Eq)]
+        pub struct $name {
+            pub target: TargetSelection,
+        }
+
+        impl Step for $name {
+            type Output = ();
+            const DEFAULT: bool = if false $(|| $lint_by_default)* { true } else { false };
+
+            fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+                run.path($path)
+            }
+
+            fn make_run(run: RunConfig<'_>) {
+                run.builder.ensure($name {
+                    target: run.target,
+                });
+            }
+
+            fn run(self, builder: &Builder<'_>) -> Self::Output {
+                let compiler = builder.compiler(builder.top_stage, builder.config.build);
+                let target = self.target;
+
+                builder.ensure(check::Rustc::new(target, builder));
+
+                let cargo = prepare_tool_cargo(
+                    builder,
+                    compiler,
+                    Mode::ToolRustc,
+                    target,
+                    "clippy",
+                    $path,
+                    SourceType::InTree,
+                    &[],
+                );
+
+                let _guard = builder.msg_tool(
+                    Kind::Clippy,
+                    Mode::ToolRustc,
+                    $readable_name,
+                    compiler.stage,
+                    &compiler.host,
+                    &target,
+                );
+
+                let stamp = builder
+                    .cargo_out(compiler, Mode::ToolRustc, target)
+                    .join(format!(".{}-check.stamp", stringify!($name).to_lowercase()));
+
+                run_cargo(
+                    builder,
+                    cargo,
+                    lint_args(builder, &[]),
+                    &stamp,
+                    vec![],
+                    true,
+                    false,
+                );
+            }
+        }
+        )+
+    }
+}
+
+lint_any!(
+    Bootstrap, "src/bootstrap", "bootstrap";
+    BuildHelper, "src/tools/build_helper", "build_helper";
+    BuildManifest, "src/tools/build-manifest", "build-manifest";
+    CargoMiri, "src/tools/miri/cargo-miri", "cargo-miri";
+    Clippy, "src/tools/clippy", "clippy";
+    CollectLicenseMetadata, "src/tools/collect-license-metadata", "collect-license-metadata";
+    Compiletest, "src/tools/compiletest", "compiletest";
+    CoverageDump, "src/tools/coverage-dump", "coverage-dump";
+    Jsondocck, "src/tools/jsondocck", "jsondocck";
+    Jsondoclint, "src/tools/jsondoclint", "jsondoclint";
+    LintDocs, "src/tools/lint-docs", "lint-docs";
+    LlvmBitcodeLinker, "src/tools/llvm-bitcode-linker", "llvm-bitcode-linker";
+    Miri, "src/tools/miri", "miri";
+    MiroptTestTools, "src/tools/miropt-test-tools", "miropt-test-tools";
+    OptDist, "src/tools/opt-dist", "opt-dist";
+    RemoteTestClient, "src/tools/remote-test-client", "remote-test-client";
+    RemoteTestServer, "src/tools/remote-test-server", "remote-test-server";
+    Rls, "src/tools/rls", "rls";
+    RustAnalyzer, "src/tools/rust-analyzer", "rust-analyzer";
+    RustDemangler, "src/tools/rust-demangler", "rust-demangler";
+    Rustdoc, "src/tools/rustdoc", "clippy";
+    Rustfmt, "src/tools/rustfmt", "rustfmt";
+    RustInstaller, "src/tools/rust-installer", "rust-installer";
+    Tidy, "src/tools/tidy", "tidy";
+);

src/bootstrap/src/core/build_steps/mod.rs

@@ -1,5 +1,6 @@
 pub(crate) mod check;
 pub(crate) mod clean;
+pub(crate) mod clippy;
 pub(crate) mod compile;
 pub(crate) mod dist;
 pub(crate) mod doc;

src/bootstrap/src/core/build_steps/tool.rs

@@ -36,8 +36,9 @@ struct ToolBuild {
 
 impl Builder<'_> {
     #[track_caller]
-    fn msg_tool(
+    pub(crate) fn msg_tool(
         &self,
+        kind: Kind,
         mode: Mode,
         tool: &str,
         build_stage: u32,
@@ -47,7 +48,7 @@ impl Builder<'_> {
         match mode {
             // depends on compiler stage, different to host compiler
             Mode::ToolRustc => self.msg_sysroot_tool(
-                Kind::Build,
+                kind,
                 build_stage,
                 format_args!("tool {tool}"),
                 *host,
@@ -100,6 +101,7 @@ impl Step for ToolBuild {
             cargo.allow_features(self.allow_features);
         }
         let _guard = builder.msg_tool(
+            Kind::Build,
             self.mode,
             self.tool,
             self.compiler.stage,
@@ -481,6 +483,7 @@ impl Step for Rustdoc {
         );
 
         let _guard = builder.msg_tool(
+            Kind::Build,
             Mode::ToolRustc,
             "rustdoc",
             build_compiler.stage,

src/bootstrap/src/core/builder.rs

@@ -13,9 +13,9 @@ use std::process::Command;
 use std::sync::OnceLock;
 use std::time::{Duration, Instant};
 
-use crate::core::build_steps::llvm;
 use crate::core::build_steps::tool::{self, SourceType};
 use crate::core::build_steps::{check, clean, compile, dist, doc, install, run, setup, test};
+use crate::core::build_steps::{clippy, llvm};
 use crate::core::config::flags::{Color, Subcommand};
 use crate::core::config::{DryRun, SplitDebuginfo, TargetSelection};
 use crate::prepare_behaviour_dump_dir;
@@ -672,6 +672,7 @@ impl Kind {
             Kind::Doc => "Documenting",
             Kind::Run => "Running",
             Kind::Suggest => "Suggesting",
+            Kind::Clippy => "Linting",
             _ => {
                 let title_letter = self.as_str()[0..1].to_ascii_uppercase();
                 return format!("{title_letter}{}ing", &self.as_str()[1..]);
@@ -726,7 +727,35 @@ impl<'a> Builder<'a> {
                 tool::CoverageDump,
                 tool::LlvmBitcodeLinker
             ),
-            Kind::Check | Kind::Clippy | Kind::Fix => describe!(
+            Kind::Clippy => describe!(
+                clippy::Std,
+                clippy::Rustc,
+                clippy::Bootstrap,
+                clippy::BuildHelper,
+                clippy::BuildManifest,
+                clippy::CargoMiri,
+                clippy::Clippy,
+                clippy::CollectLicenseMetadata,
+                clippy::Compiletest,
+                clippy::CoverageDump,
+                clippy::Jsondocck,
+                clippy::Jsondoclint,
+                clippy::LintDocs,
+                clippy::LlvmBitcodeLinker,
+                clippy::Miri,
+                clippy::MiroptTestTools,
+                clippy::OptDist,
+                clippy::RemoteTestClient,
+                clippy::RemoteTestServer,
+                clippy::Rls,
+                clippy::RustAnalyzer,
+                clippy::RustDemangler,
+                clippy::Rustdoc,
+                clippy::Rustfmt,
+                clippy::RustInstaller,
+                clippy::Tidy,
+            ),
+            Kind::Check | Kind::Fix => describe!(
                 check::Std,
                 check::Rustc,
                 check::Rustdoc,

src/bootstrap/src/core/config/tests.rs

@@ -1,5 +1,5 @@
 use super::{flags::Flags, ChangeIdWrapper, Config};
-use crate::core::build_steps::check::get_clippy_rules_in_order;
+use crate::core::build_steps::clippy::get_clippy_rules_in_order;
 use crate::core::config::{LldMode, TomlConfig};
 
 use clap::CommandFactory;

src/bootstrap/src/lib.rs

@@ -1090,6 +1090,16 @@ impl Build {
         }
     }
 
+    #[must_use = "Groups should not be dropped until the Step finishes running"]
+    #[track_caller]
+    fn msg_clippy(
+        &self,
+        what: impl Display,
+        target: impl Into<Option<TargetSelection>>,
+    ) -> Option<gha::Group> {
+        self.msg(Kind::Clippy, self.config.stage, what, self.config.build, target)
+    }
+
     #[must_use = "Groups should not be dropped until the Step finishes running"]
     #[track_caller]
     fn msg_check(

src/ci/docker/host-x86_64/mingw-check/Dockerfile

@@ -46,7 +46,7 @@ ENV SCRIPT python3 ../x.py --stage 2 test src/tools/expand-yaml-anchors && \
            # We also skip the x86_64-unknown-linux-gnu target as it is well-tested by other jobs.
            python3 ../x.py check --stage 0 --set build.optimized-compiler-builtins=false core alloc std --target=aarch64-unknown-linux-gnu,i686-pc-windows-msvc,i686-unknown-linux-gnu,x86_64-apple-darwin,x86_64-pc-windows-gnu,x86_64-pc-windows-msvc && \
            python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu && \
-           python3 ../x.py clippy compiler -Aclippy::all -Dclippy::correctness && \
+           python3 ../x.py clippy compiler library -Aclippy::all -Dclippy::correctness && \
            python3 ../x.py build --stage 0 src/tools/build-manifest && \
            python3 ../x.py test --stage 0 src/tools/compiletest && \
            python3 ../x.py test --stage 0 core alloc std test proc_macro && \