From 0387981f2b5e41c982ec4a1b102f0c54997361ff Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Sat, 17 Oct 2020 19:48:40 +0200 Subject: [PATCH 1/5] Add --no-deps command-line argument --- clippy_workspace_tests/path_dep/Cargo.toml | 3 + clippy_workspace_tests/path_dep/src/lib.rs | 6 ++ clippy_workspace_tests/subcrate/Cargo.toml | 3 + src/driver.rs | 37 ++++++----- tests/dogfood.rs | 71 +++++++++++++++++++++- 5 files changed, 104 insertions(+), 16 deletions(-) create mode 100644 clippy_workspace_tests/path_dep/Cargo.toml create mode 100644 clippy_workspace_tests/path_dep/src/lib.rs diff --git a/clippy_workspace_tests/path_dep/Cargo.toml b/clippy_workspace_tests/path_dep/Cargo.toml new file mode 100644 index 000000000000..85a91cd2decd --- /dev/null +++ b/clippy_workspace_tests/path_dep/Cargo.toml @@ -0,0 +1,3 @@ +[package] +name = "path_dep" +version = "0.1.0" diff --git a/clippy_workspace_tests/path_dep/src/lib.rs b/clippy_workspace_tests/path_dep/src/lib.rs new file mode 100644 index 000000000000..35ce524f2b10 --- /dev/null +++ b/clippy_workspace_tests/path_dep/src/lib.rs @@ -0,0 +1,6 @@ +#![deny(clippy::empty_loop)] + +#[cfg(feature = "primary_package_test")] +pub fn lint_me() { + loop {} +} diff --git a/clippy_workspace_tests/subcrate/Cargo.toml b/clippy_workspace_tests/subcrate/Cargo.toml index 83ea5868160b..45362c11b856 100644 --- a/clippy_workspace_tests/subcrate/Cargo.toml +++ b/clippy_workspace_tests/subcrate/Cargo.toml @@ -1,3 +1,6 @@ [package] name = "subcrate" version = "0.1.0" + +[dependencies] +path_dep = { path = "../path_dep" } diff --git a/src/driver.rs b/src/driver.rs index ef31c72481a2..bbe9ce739368 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -277,27 +277,34 @@ pub fn main() { args.extend(vec!["--sysroot".into(), sys_root]); }; + let mut no_deps = false; + let clippy_args = env::var("CLIPPY_ARGS") + .unwrap_or_default() + .split("__CLIPPY_HACKERY__") + .filter_map(|s| match s { + "" => None, + "--no-deps" => { + no_deps = true; + None + }, + _ => Some(s.to_string()), + }) + .chain(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()]) + .collect::>(); + // this check ensures that dependencies are built but not linted and the final // crate is linted but not built - let clippy_enabled = env::var("CLIPPY_TESTS").map_or(false, |val| val == "true") - || arg_value(&orig_args, "--cap-lints", |val| val == "allow").is_none(); - - if clippy_enabled { - args.extend(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()]); - if let Ok(extra_args) = env::var("CLIPPY_ARGS") { - args.extend(extra_args.split("__CLIPPY_HACKERY__").filter_map(|s| { - if s.is_empty() { - None - } else { - Some(s.to_string()) - } - })); - } + let clippy_disabled = env::var("CLIPPY_TESTS").map_or(false, |val| val != "true") + || arg_value(&orig_args, "--cap-lints", |val| val == "allow").is_some() + || no_deps && env::var("CARGO_PRIMARY_PACKAGE").is_err(); + + if !clippy_disabled { + args.extend(clippy_args); } let mut clippy = ClippyCallbacks; let mut default = DefaultCallbacks; let callbacks: &mut (dyn rustc_driver::Callbacks + Send) = - if clippy_enabled { &mut clippy } else { &mut default }; + if clippy_disabled { &mut default } else { &mut clippy }; rustc_driver::RunCompiler::new(&args, callbacks).run() })) } diff --git a/tests/dogfood.rs b/tests/dogfood.rs index 48e0478f1699..b166a6b7c1ff 100644 --- a/tests/dogfood.rs +++ b/tests/dogfood.rs @@ -3,7 +3,7 @@ #![feature(once_cell)] use std::lazy::SyncLazy; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Command; mod cargo; @@ -41,12 +41,77 @@ fn dogfood_clippy() { #[test] fn dogfood_subprojects() { + fn test_no_deps_ignores_path_deps_in_workspaces() { + fn clean(cwd: &Path, target_dir: &Path) { + Command::new("cargo") + .current_dir(cwd) + .env("CARGO_TARGET_DIR", target_dir) + .arg("clean") + .args(&["-p", "subcrate"]) + .args(&["-p", "path_dep"]) + .output() + .unwrap(); + } + + if cargo::is_rustc_test_suite() { + return; + } + let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let target_dir = root.join("target").join("dogfood"); + let cwd = root.join("clippy_workspace_tests"); + + // Make sure we start with a clean state + clean(&cwd, &target_dir); + + // `path_dep` is a path dependency of `subcrate` that would trigger a denied lint. + // Make sure that with the `--no-deps` argument Clippy does not run on `path_dep`. + let output = Command::new(&*CLIPPY_PATH) + .current_dir(&cwd) + .env("CLIPPY_DOGFOOD", "1") + .env("CARGO_INCREMENTAL", "0") + .arg("clippy") + .args(&["-p", "subcrate"]) + .arg("--") + .arg("--no-deps") + .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir + .args(&["--cfg", r#"feature="primary_package_test""#]) + .output() + .unwrap(); + println!("status: {}", output.status); + println!("stdout: {}", String::from_utf8_lossy(&output.stdout)); + println!("stderr: {}", String::from_utf8_lossy(&output.stderr)); + + assert!(output.status.success()); + + // Make sure we start with a clean state + clean(&cwd, &target_dir); + + // Test that without the `--no-deps` argument, `path_dep` is linted. + let output = Command::new(&*CLIPPY_PATH) + .current_dir(&cwd) + .env("CLIPPY_DOGFOOD", "1") + .env("CARGO_INCREMENTAL", "0") + .arg("clippy") + .args(&["-p", "subcrate"]) + .arg("--") + .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir + .args(&["--cfg", r#"feature="primary_package_test""#]) + .output() + .unwrap(); + println!("status: {}", output.status); + println!("stdout: {}", String::from_utf8_lossy(&output.stdout)); + println!("stderr: {}", String::from_utf8_lossy(&output.stderr)); + + assert!(!output.status.success()); + } + // run clippy on remaining subprojects and fail the test if lint warnings are reported if cargo::is_rustc_test_suite() { return; } let root_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + // NOTE: `path_dep` crate is omitted on purpose here for d in &[ "clippy_workspace_tests", "clippy_workspace_tests/src", @@ -72,4 +137,8 @@ fn dogfood_subprojects() { assert!(output.status.success()); } + + // NOTE: Since tests run in parallel we can't run cargo commands on the same workspace at the + // same time, so we test this immediately after the dogfood for workspaces. + test_no_deps_ignores_path_deps_in_workspaces(); } From 192ccfb4efc0a38378f4564b26e6033dec432bdb Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Sat, 17 Oct 2020 19:55:03 +0200 Subject: [PATCH 2/5] Update README.md --- README.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/README.md b/README.md index fddf0614a0b8..aaa55e11c7db 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,22 @@ Note that this is still experimental and only supported on the nightly channel: cargo clippy --fix -Z unstable-options ``` +#### Workspaces + +All the usual workspace options should work with Clippy. For example the following command +will run Clippy on the `example` crate: + +```terminal +cargo clippy -p example +``` + +As with `cargo check`, this includes dependencies that are members of the workspace, like path dependencies. +If you want to run Clippy **only** on the given crate, use the `--no-deps` option like this: + +```terminal +cargo clippy -p example -- --no-deps +``` + ### Running Clippy from the command line without installing it To have cargo compile your crate with Clippy without Clippy installation From 7eda421e9629a717d31ec03d12b4befd03f5fb50 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Wed, 25 Nov 2020 15:02:47 +0100 Subject: [PATCH 3/5] Apply suggestion regarding clippy_enabled bool --- src/driver.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/driver.rs b/src/driver.rs index bbe9ce739368..03381106de1a 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -182,6 +182,7 @@ fn toolchain_path(home: Option, toolchain: Option) -> Option Date: Fri, 27 Nov 2020 16:53:30 +0100 Subject: [PATCH 4/5] Make --fix imply --no-deps --- src/main.rs | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/src/main.rs b/src/main.rs index 6739a4cf2245..ea06743394d1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -62,7 +62,7 @@ struct ClippyCmd { unstable_options: bool, cargo_subcommand: &'static str, args: Vec, - clippy_args: String, + clippy_args: Vec, } impl ClippyCmd { @@ -99,7 +99,10 @@ impl ClippyCmd { args.insert(0, "+nightly".to_string()); } - let clippy_args: String = old_args.map(|arg| format!("{}__CLIPPY_HACKERY__", arg)).collect(); + let mut clippy_args: Vec = old_args.collect(); + if cargo_subcommand == "fix" && !clippy_args.iter().any(|arg| arg == "--no-deps") { + clippy_args.push("--no-deps".into()); + } ClippyCmd { unstable_options, @@ -147,10 +150,15 @@ impl ClippyCmd { fn into_std_cmd(self) -> Command { let mut cmd = Command::new("cargo"); + let clippy_args: String = self + .clippy_args + .iter() + .map(|arg| format!("{}__CLIPPY_HACKERY__", arg)) + .collect(); cmd.env(self.path_env(), Self::path()) .envs(ClippyCmd::target_dir()) - .env("CLIPPY_ARGS", self.clippy_args) + .env("CLIPPY_ARGS", clippy_args) .arg(self.cargo_subcommand) .args(&self.args); @@ -201,6 +209,24 @@ mod tests { assert!(cmd.args.iter().any(|arg| arg.ends_with("unstable-options"))); } + #[test] + fn fix_implies_no_deps() { + let args = "cargo clippy --fix -Zunstable-options" + .split_whitespace() + .map(ToString::to_string); + let cmd = ClippyCmd::new(args); + assert!(cmd.clippy_args.iter().any(|arg| arg == "--no-deps")); + } + + #[test] + fn no_deps_not_duplicated_with_fix() { + let args = "cargo clippy --fix -Zunstable-options -- --no-deps" + .split_whitespace() + .map(ToString::to_string); + let cmd = ClippyCmd::new(args); + assert_eq!(cmd.clippy_args.iter().filter(|arg| *arg == "--no-deps").count(), 1); + } + #[test] fn check() { let args = "cargo clippy".split_whitespace().map(ToString::to_string); From 952b731fb9134e44e4c99bae46e6a917c944e77e Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Fri, 27 Nov 2020 18:04:30 +0100 Subject: [PATCH 5/5] Reword bitrotten comment --- src/driver.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/driver.rs b/src/driver.rs index 03381106de1a..e490ee54c0be 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -293,8 +293,11 @@ pub fn main() { .chain(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()]) .collect::>(); - // this check ensures that dependencies are built but not linted and the final - // crate is linted but not built + // We enable Clippy if one of the following conditions is met + // - IF Clippy is run on its test suite OR + // - IF Clippy is run on the main crate, not on deps (`!cap_lints_allow`) THEN + // - IF `--no-deps` is not set (`!no_deps`) OR + // - IF `--no-deps` is set and Clippy is run on the specified primary package let clippy_tests_set = env::var("CLIPPY_TESTS").map_or(false, |val| val == "true"); let cap_lints_allow = arg_value(&orig_args, "--cap-lints", |val| val == "allow").is_some(); let in_primary_package = env::var("CARGO_PRIMARY_PACKAGE").is_ok();