Auto merge of rust-lang#56595 - ljedrz:x_py_clippy_fix, r=oli-obk

Add clippy and fix commands to x.py

Since they are kind of similar in nature, I have used the same approach as for `cargo check`. At least some of the boilerplate could probably be shared, but I'd prefer to gather some feedback before I decide to merge them more aggressively.

This works reasonably well for `clippy`; with `-A clippy::all` and some extra `#![feature(rustc_private)]`s almost the whole codebase can be processed. There are some concerns, though:
- unlike `check`, in order to be able to traverse all the crates, some of them need to be marked with the `#![feature(rustc_private)]` attribute
- `-W clippy::all` breaks on any error. Is there a way to produce errors but not have them break the progress?
- I'm not sure how to redirect the errors in a way that would show colors; for now I was able to de-jsonize and print them (something not needed for `check`)

`cargo fix` is much more stubborn; it refuses to acknowledge crates like `core` and `std`, so it doesn't progress much at all.

Since this is a bit more tricky than I have envisioned, I need some guidance:
- is this the right approach or am I doing something very wrong ^^?
- why are the extra `rustc_private` features necessary? I was hoping for the same treatment as `check`
- are changes in `clippy` and `cargo fix` needed e.g. in order to produce errors in the same manner as `check` or did I miss something?
- do we need this level of file granularity (e.g. for futureproofing) or can `check`, `clippy` and `fix` files be condensed?

Hopes-to-fix: rust-lang#53896

Cc @alexcrichton, @zackmdavis

Author: bors

src/bootstrap/builder.rs

@@ -318,6 +318,8 @@ impl<'a> ShouldRun<'a> {
 pub enum Kind {
     Build,
     Check,
+    Clippy,
+    Fix,
     Test,
     Bench,
     Dist,
@@ -359,7 +361,7 @@ impl<'a> Builder<'a> {
                 tool::Miri,
                 native::Lld
             ),
-            Kind::Check => describe!(
+            Kind::Check | Kind::Clippy | Kind::Fix => describe!(
                 check::Std,
                 check::Test,
                 check::Rustc,
@@ -520,6 +522,8 @@ impl<'a> Builder<'a> {
         let (kind, paths) = match build.config.cmd {
             Subcommand::Build { ref paths } => (Kind::Build, &paths[..]),
             Subcommand::Check { ref paths } => (Kind::Check, &paths[..]),
+            Subcommand::Clippy { ref paths } => (Kind::Clippy, &paths[..]),
+            Subcommand::Fix { ref paths } => (Kind::Fix, &paths[..]),
             Subcommand::Doc { ref paths } => (Kind::Doc, &paths[..]),
             Subcommand::Test { ref paths, .. } => (Kind::Test, &paths[..]),
             Subcommand::Bench { ref paths, .. } => (Kind::Bench, &paths[..]),
@@ -757,17 +761,17 @@ impl<'a> Builder<'a> {
         };
 
         let libstd_stamp = match cmd {
-            "check" => check::libstd_stamp(self, cmp, target),
+            "check" | "clippy" | "fix" => check::libstd_stamp(self, cmp, target),
             _ => compile::libstd_stamp(self, cmp, target),
         };
 
         let libtest_stamp = match cmd {
-            "check" => check::libtest_stamp(self, cmp, target),
+            "check" | "clippy" | "fix" => check::libtest_stamp(self, cmp, target),
             _ => compile::libstd_stamp(self, cmp, target),
         };
 
         let librustc_stamp = match cmd {
-            "check" => check::librustc_stamp(self, cmp, target),
+            "check" | "clippy" | "fix" => check::librustc_stamp(self, cmp, target),
             _ => compile::librustc_stamp(self, cmp, target),
         };
 
@@ -831,9 +835,9 @@ impl<'a> Builder<'a> {
             assert_eq!(target, compiler.host);
         }
 
-        // Set a flag for `check` so that certain build scripts can do less work
-        // (e.g., not building/requiring LLVM).
-        if cmd == "check" {
+        // Set a flag for `check`/`clippy`/`fix`, so that certain build
+        // scripts can do less work (e.g. not building/requiring LLVM).
+        if cmd == "check" || cmd == "clippy" || cmd == "fix" {
             cargo.env("RUST_CHECK", "1");
         }
 
@@ -898,6 +902,11 @@ impl<'a> Builder<'a> {
             extra_args.push_str(&s);
         }
 
+        if cmd == "clippy" {
+            extra_args.push_str("-Zforce-unstable-if-unmarked -Zunstable-options \
+                --json-rendered=termcolor");
+        }
+
         if !extra_args.is_empty() {
             cargo.env(
                 "RUSTFLAGS",
@@ -966,7 +975,7 @@ impl<'a> Builder<'a> {
         if let Some(ref error_format) = self.config.rustc_error_format {
             cargo.env("RUSTC_ERROR_FORMAT", error_format);
         }
-        if cmd != "build" && cmd != "check" && cmd != "rustc" && want_rustdoc {
+        if !(["build", "check", "clippy", "fix", "rustc"].contains(&cmd)) && want_rustdoc {
             cargo.env("RUSTDOC_LIBDIR", self.rustc_libdir(compiler));
         }
 

src/bootstrap/check.rs

@@ -1,8 +1,8 @@
-//! Implementation of compiling the compiler and standard library, in "check" mode.
+//! Implementation of compiling the compiler and standard library, in "check"-based modes.
 
 use crate::compile::{run_cargo, std_cargo, test_cargo, rustc_cargo, rustc_cargo_env,
                      add_to_sysroot};
-use crate::builder::{RunConfig, Builder, ShouldRun, Step};
+use crate::builder::{RunConfig, Builder, Kind, ShouldRun, Step};
 use crate::tool::{prepare_tool_cargo, SourceType};
 use crate::{Compiler, Mode};
 use crate::cache::{INTERNER, Interned};
@@ -13,6 +13,22 @@ pub struct Std {
     pub target: Interned<String>,
 }
 
+fn args(kind: Kind) -> Vec<String> {
+    match kind {
+        Kind::Clippy => vec!["--".to_owned(), "--cap-lints".to_owned(), "warn".to_owned()],
+        _ => Vec::new()
+    }
+}
+
+fn cargo_subcommand(kind: Kind) -> &'static str {
+    match kind {
+        Kind::Check => "check",
+        Kind::Clippy => "clippy",
+        Kind::Fix => "fix",
+        _ => unreachable!()
+    }
+}
+
 impl Step for Std {
     type Output = ();
     const DEFAULT: bool = true;
@@ -31,13 +47,14 @@ impl Step for Std {
         let target = self.target;
         let compiler = builder.compiler(0, builder.config.build);
 
-        let mut cargo = builder.cargo(compiler, Mode::Std, target, "check");
+        let mut cargo = builder.cargo(compiler, Mode::Std, target, cargo_subcommand(builder.kind));
         std_cargo(builder, &compiler, target, &mut cargo);
 
         let _folder = builder.fold_output(|| format!("stage{}-std", compiler.stage));
         builder.info(&format!("Checking std artifacts ({} -> {})", &compiler.host, target));
         run_cargo(builder,
                   &mut cargo,
+                  args(builder.kind),
                   &libstd_stamp(builder, compiler, target),
                   true);
 
@@ -78,13 +95,15 @@ impl Step for Rustc {
 
         builder.ensure(Test { target });
 
-        let mut cargo = builder.cargo(compiler, Mode::Rustc, target, "check");
+        let mut cargo = builder.cargo(compiler, Mode::Rustc, target,
+            cargo_subcommand(builder.kind));
         rustc_cargo(builder, &mut cargo);
 
         let _folder = builder.fold_output(|| format!("stage{}-rustc", compiler.stage));
         builder.info(&format!("Checking compiler artifacts ({} -> {})", &compiler.host, target));
         run_cargo(builder,
                   &mut cargo,
+                  args(builder.kind),
                   &librustc_stamp(builder, compiler, target),
                   true);
 
@@ -127,7 +146,8 @@ impl Step for CodegenBackend {
 
         builder.ensure(Rustc { target });
 
-        let mut cargo = builder.cargo(compiler, Mode::Codegen, target, "check");
+        let mut cargo = builder.cargo(compiler, Mode::Codegen, target,
+            cargo_subcommand(builder.kind));
         cargo.arg("--manifest-path").arg(builder.src.join("src/librustc_codegen_llvm/Cargo.toml"));
         rustc_cargo_env(builder, &mut cargo);
 
@@ -136,6 +156,7 @@ impl Step for CodegenBackend {
         let _folder = builder.fold_output(|| format!("stage{}-rustc_codegen_llvm", compiler.stage));
         run_cargo(builder,
                   &mut cargo,
+                  args(builder.kind),
                   &codegen_backend_stamp(builder, compiler, target, backend),
                   true);
     }
@@ -166,13 +187,14 @@ impl Step for Test {
 
         builder.ensure(Std { target });
 
-        let mut cargo = builder.cargo(compiler, Mode::Test, target, "check");
+        let mut cargo = builder.cargo(compiler, Mode::Test, target, cargo_subcommand(builder.kind));
         test_cargo(builder, &compiler, target, &mut cargo);
 
         let _folder = builder.fold_output(|| format!("stage{}-test", compiler.stage));
         builder.info(&format!("Checking test artifacts ({} -> {})", &compiler.host, target));
         run_cargo(builder,
                   &mut cargo,
+                  args(builder.kind),
                   &libtest_stamp(builder, compiler, target),
                   true);
 
@@ -212,7 +234,7 @@ impl Step for Rustdoc {
                                            compiler,
                                            Mode::ToolRustc,
                                            target,
-                                           "check",
+                                           cargo_subcommand(builder.kind),
                                            "src/tools/rustdoc",
                                            SourceType::InTree,
                                            &[]);
@@ -221,6 +243,7 @@ impl Step for Rustdoc {
         println!("Checking rustdoc artifacts ({} -> {})", &compiler.host, target);
         run_cargo(builder,
                   &mut cargo,
+                  args(builder.kind),
                   &rustdoc_stamp(builder, compiler, target),
                   true);
 

src/bootstrap/compile.rs

@@ -100,6 +100,7 @@ impl Step for Std {
                 &compiler.host, target));
         run_cargo(builder,
                   &mut cargo,
+                  vec![],
                   &libstd_stamp(builder, compiler, target),
                   false);
 
@@ -425,6 +426,7 @@ impl Step for Test {
                 &compiler.host, target));
         run_cargo(builder,
                   &mut cargo,
+                  vec![],
                   &libtest_stamp(builder, compiler, target),
                   false);
 
@@ -556,6 +558,7 @@ impl Step for Rustc {
                  compiler.stage, &compiler.host, target));
         run_cargo(builder,
                   &mut cargo,
+                  vec![],
                   &librustc_stamp(builder, compiler, target),
                   false);
 
@@ -707,6 +710,7 @@ impl Step for CodegenBackend {
         let _folder = builder.fold_output(|| format!("stage{}-rustc_codegen_llvm", compiler.stage));
         let files = run_cargo(builder,
                               cargo.arg("--features").arg(features),
+                              vec![],
                               &tmp_stamp,
                               false);
         if builder.config.dry_run {
@@ -1077,6 +1081,7 @@ pub fn add_to_sysroot(
 
 pub fn run_cargo(builder: &Builder<'_>,
                  cargo: &mut Command,
+                 tail_args: Vec<String>,
                  stamp: &Path,
                  is_check: bool)
     -> Vec<PathBuf>
@@ -1099,7 +1104,7 @@ pub fn run_cargo(builder: &Builder<'_>,
     // files we need to probe for later.
     let mut deps = Vec::new();
     let mut toplevel = Vec::new();
-    let ok = stream_cargo(builder, cargo, &mut |msg| {
+    let ok = stream_cargo(builder, cargo, tail_args, &mut |msg| {
         let (filenames, crate_types) = match msg {
             CargoMessage::CompilerArtifact {
                 filenames,
@@ -1108,6 +1113,10 @@ pub fn run_cargo(builder: &Builder<'_>,
                 },
                 ..
             } => (filenames, crate_types),
+            CargoMessage::CompilerMessage { message } => {
+                eprintln!("{}", message.rendered);
+                return;
+            }
             _ => return,
         };
         for filename in filenames {
@@ -1235,6 +1244,7 @@ pub fn run_cargo(builder: &Builder<'_>,
 pub fn stream_cargo(
     builder: &Builder<'_>,
     cargo: &mut Command,
+    tail_args: Vec<String>,
     cb: &mut dyn FnMut(CargoMessage<'_>),
 ) -> bool {
     if builder.config.dry_run {
@@ -1245,6 +1255,10 @@ pub fn stream_cargo(
     cargo.arg("--message-format").arg("json")
          .stdout(Stdio::piped());
 
+    for arg in tail_args {
+        cargo.arg(arg);
+    }
+
     builder.verbose(&format!("running: {:?}", cargo));
     let mut child = match cargo.spawn() {
         Ok(child) => child,
@@ -1291,5 +1305,13 @@ pub enum CargoMessage<'a> {
     },
     BuildScriptExecuted {
         package_id: Cow<'a, str>,
+    },
+    CompilerMessage {
+        message: ClippyMessage<'a>
     }
 }
+
+#[derive(Deserialize)]
+pub struct ClippyMessage<'a> {
+    rendered: Cow<'a, str>,
+}

src/bootstrap/flags.rs

@@ -44,6 +44,12 @@ pub enum Subcommand {
     Check {
         paths: Vec<PathBuf>,
     },
+    Clippy {
+        paths: Vec<PathBuf>,
+    },
+    Fix {
+        paths: Vec<PathBuf>,
+    },
     Doc {
         paths: Vec<PathBuf>,
     },
@@ -90,6 +96,8 @@ Usage: x.py <subcommand> [options] [<paths>...]
 Subcommands:
     build       Compile either the compiler or libraries
     check       Compile either the compiler or libraries, using cargo check
+    clippy      Run clippy
+    fix         Run cargo fix
     test        Build and run some test suites
     bench       Build and run some benchmarks
     doc         Build documentation
@@ -146,6 +154,8 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`"
         let subcommand = args.iter().find(|&s| {
             (s == "build")
                 || (s == "check")
+                || (s == "clippy")
+                || (s == "fix")
                 || (s == "test")
                 || (s == "bench")
                 || (s == "doc")
@@ -281,6 +291,28 @@ Arguments:
     the compiler.",
                 );
             }
+            "clippy" => {
+                subcommand_help.push_str(
+                    "\n
+Arguments:
+    This subcommand accepts a number of paths to directories to the crates
+    and/or artifacts to run clippy against. For example:
+
+        ./x.py clippy src/libcore
+        ./x.py clippy src/libcore src/libproc_macro",
+                );
+            }
+            "fix" => {
+                subcommand_help.push_str(
+                    "\n
+Arguments:
+    This subcommand accepts a number of paths to directories to the crates
+    and/or artifacts to run `cargo fix` against. For example:
+
+        ./x.py fix src/libcore
+        ./x.py fix src/libcore src/libproc_macro",
+                );
+            }
             "test" => {
                 subcommand_help.push_str(
                     "\n
@@ -363,6 +395,8 @@ Arguments:
         let cmd = match subcommand.as_str() {
             "build" => Subcommand::Build { paths },
             "check" => Subcommand::Check { paths },
+            "clippy" => Subcommand::Clippy { paths },
+            "fix" => Subcommand::Fix { paths },
             "test" => Subcommand::Test {
                 paths,
                 bless: matches.opt_present("bless"),

src/bootstrap/tool.rs

@@ -77,7 +77,7 @@ impl Step for ToolBuild {
         let _folder = builder.fold_output(|| format!("stage{}-{}", compiler.stage, tool));
         builder.info(&format!("Building stage{} tool {} ({})", compiler.stage, tool, target));
         let mut duplicates = Vec::new();
-        let is_expected = compile::stream_cargo(builder, &mut cargo, &mut |msg| {
+        let is_expected = compile::stream_cargo(builder, &mut cargo, vec![], &mut |msg| {
             // Only care about big things like the RLS/Cargo for now
             match tool {
                 | "rls"