From cc9695543ea8f3973a2be2936df0efc724de1c16 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Fri, 11 Dec 2020 23:54:47 +0100 Subject: [PATCH 1/2] Pass Clippy args also trough RUSTFLAGS --- README.md | 1 - src/driver.rs | 116 +++++++++++++++++++++++++++++++++++------------ src/main.rs | 98 ++++++++++++++++++++++++++++++--------- tests/dogfood.rs | 2 +- 4 files changed, 165 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index aaa55e11c7db1..dc931963726b2 100644 --- a/README.md +++ b/README.md @@ -208,7 +208,6 @@ the lint(s) you are interested in: ```terminal cargo clippy -- -A clippy::all -W clippy::useless_format -W clippy::... ``` -Note that if you've run clippy before, this may only take effect after you've modified a file or ran `cargo clean`. ### Specifying the minimum supported Rust version diff --git a/src/driver.rs b/src/driver.rs index e490ee54c0be0..40f1b802e60e6 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -1,5 +1,6 @@ #![feature(rustc_private)] #![feature(once_cell)] +#![feature(bool_to_option)] #![cfg_attr(feature = "deny-warnings", deny(warnings))] // warn on lints, that are included in `rust-lang/rust`s bootstrap #![warn(rust_2018_idioms, unused_lifetimes)] @@ -19,6 +20,7 @@ use rustc_tools_util::VersionInfo; use std::borrow::Cow; use std::env; +use std::iter; use std::lazy::SyncLazy; use std::ops::Deref; use std::panic; @@ -47,20 +49,6 @@ fn arg_value<'a, T: Deref>( None } -#[test] -fn test_arg_value() { - let args = &["--bar=bar", "--foobar", "123", "--foo"]; - - assert_eq!(arg_value(&[] as &[&str], "--foobar", |_| true), None); - assert_eq!(arg_value(args, "--bar", |_| false), None); - assert_eq!(arg_value(args, "--bar", |_| true), Some("bar")); - assert_eq!(arg_value(args, "--bar", |p| p == "bar"), Some("bar")); - assert_eq!(arg_value(args, "--bar", |p| p == "foo"), None); - assert_eq!(arg_value(args, "--foobar", |p| p == "foo"), None); - assert_eq!(arg_value(args, "--foobar", |p| p == "123"), Some("123")); - assert_eq!(arg_value(args, "--foo", |_| true), None); -} - struct DefaultCallbacks; impl rustc_driver::Callbacks for DefaultCallbacks {} @@ -182,6 +170,28 @@ fn toolchain_path(home: Option, toolchain: Option) -> Option(args: &mut Vec, clippy_args: I) +where + T: AsRef, + U: AsRef + ?Sized + 'a, + I: Iterator + Clone, +{ + let args_iter = clippy_args.map(AsRef::as_ref); + let args_count = args_iter.clone().count(); + + if args_count > 0 { + if let Some(start) = args.windows(args_count).enumerate().find_map(|(current, window)| { + window + .iter() + .map(AsRef::as_ref) + .eq(args_iter.clone()) + .then_some(current) + }) { + args.drain(start..start + args_count); + } + } +} + #[allow(clippy::too_many_lines)] pub fn main() { rustc_driver::init_rustc_env_logger(); @@ -278,20 +288,9 @@ 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::>(); + let clippy_args = env::var("CLIPPY_ARGS").unwrap_or_default(); + let clippy_args = clippy_args.split_whitespace(); + let no_deps = clippy_args.clone().any(|flag| flag == "--no-deps"); // We enable Clippy if one of the following conditions is met // - IF Clippy is run on its test suite OR @@ -304,7 +303,11 @@ pub fn main() { let clippy_enabled = clippy_tests_set || (!cap_lints_allow && (!no_deps || in_primary_package)); if clippy_enabled { - args.extend(clippy_args); + remove_clippy_args(&mut args, iter::once("--no-deps")); + args.extend(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()]); + } else { + // Remove all flags passed through RUSTFLAGS if Clippy is not enabled. + remove_clippy_args(&mut args, clippy_args); } let mut clippy = ClippyCallbacks; @@ -315,3 +318,58 @@ pub fn main() { rustc_driver::RunCompiler::new(&args, callbacks).run() })) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_arg_value() { + let args = &["--bar=bar", "--foobar", "123", "--foo"]; + + assert_eq!(arg_value(&[] as &[&str], "--foobar", |_| true), None); + assert_eq!(arg_value(args, "--bar", |_| false), None); + assert_eq!(arg_value(args, "--bar", |_| true), Some("bar")); + assert_eq!(arg_value(args, "--bar", |p| p == "bar"), Some("bar")); + assert_eq!(arg_value(args, "--bar", |p| p == "foo"), None); + assert_eq!(arg_value(args, "--foobar", |p| p == "foo"), None); + assert_eq!(arg_value(args, "--foobar", |p| p == "123"), Some("123")); + assert_eq!(arg_value(args, "--foo", |_| true), None); + } + + #[test] + fn removes_clippy_args_from_start() { + let mut args = vec!["-D", "clippy::await_holding_lock", "--cfg", r#"feature="some_feat""#]; + let clippy_args = ["-D", "clippy::await_holding_lock"].iter(); + + remove_clippy_args(&mut args, clippy_args); + assert_eq!(args, &["--cfg", r#"feature="some_feat""#]); + } + + #[test] + fn removes_clippy_args_from_end() { + let mut args = vec!["-Zui-testing", "-A", "clippy::empty_loop", "--no-deps"]; + let clippy_args = ["-A", "clippy::empty_loop", "--no-deps"].iter(); + + remove_clippy_args(&mut args, clippy_args); + assert_eq!(args, &["-Zui-testing"]); + } + + #[test] + fn removes_clippy_args_from_middle() { + let mut args = vec!["-Zui-testing", "-W", "clippy::filter_map", "-L", "serde"]; + let clippy_args = ["-W", "clippy::filter_map"].iter(); + + remove_clippy_args(&mut args, clippy_args); + assert_eq!(args, &["-Zui-testing", "-L", "serde"]); + } + + #[test] + fn no_clippy_args_to_remove() { + let mut args = vec!["-Zui-testing", "-L", "serde"]; + let clippy_args: [&str; 0] = []; + + remove_clippy_args(&mut args, clippy_args.iter()); + assert_eq!(args, &["-Zui-testing", "-L", "serde"]); + } +} diff --git a/src/main.rs b/src/main.rs index ea06743394d10..7594ea2c7b1d1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,3 +1,4 @@ +#![feature(bool_to_option)] #![cfg_attr(feature = "deny-warnings", deny(warnings))] // warn on lints, that are included in `rust-lang/rust`s bootstrap #![warn(rust_2018_idioms, unused_lifetimes)] @@ -62,11 +63,12 @@ struct ClippyCmd { unstable_options: bool, cargo_subcommand: &'static str, args: Vec, - clippy_args: Vec, + rustflags: Option, + clippy_args: Option, } impl ClippyCmd { - fn new(mut old_args: I) -> Self + fn new(mut old_args: I, rustflags: Option) -> Self where I: Iterator, { @@ -99,16 +101,19 @@ impl ClippyCmd { args.insert(0, "+nightly".to_string()); } - 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()); + let mut clippy_args = old_args.collect::>().join(" "); + if cargo_subcommand == "fix" && !clippy_args.contains("--no-deps") { + clippy_args = format!("{} --no-deps", clippy_args); } + let has_args = !clippy_args.is_empty(); ClippyCmd { unstable_options, cargo_subcommand, args, - clippy_args, + rustflags: has_args + .then(|| rustflags.map_or_else(|| clippy_args.clone(), |flags| format!("{} {}", clippy_args, flags))), + clippy_args: has_args.then_some(clippy_args), } } @@ -150,18 +155,19 @@ 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", clippy_args) .arg(self.cargo_subcommand) .args(&self.args); + // HACK: pass Clippy args to the driver *also* through RUSTFLAGS. + // This guarantees that new builds will be triggered when Clippy flags change. + if let (Some(clippy_args), Some(rustflags)) = (self.clippy_args, self.rustflags) { + cmd.env("CLIPPY_ARGS", clippy_args); + cmd.env("RUSTFLAGS", rustflags); + } + cmd } } @@ -170,7 +176,7 @@ fn process(old_args: I) -> Result<(), i32> where I: Iterator, { - let cmd = ClippyCmd::new(old_args); + let cmd = ClippyCmd::new(old_args, env::var("RUSTFLAGS").ok()); let mut cmd = cmd.into_std_cmd(); @@ -195,7 +201,7 @@ mod tests { #[should_panic] fn fix_without_unstable() { let args = "cargo clippy --fix".split_whitespace().map(ToString::to_string); - let _ = ClippyCmd::new(args); + let _ = ClippyCmd::new(args, None); } #[test] @@ -203,7 +209,8 @@ mod tests { let args = "cargo clippy --fix -Zunstable-options" .split_whitespace() .map(ToString::to_string); - let cmd = ClippyCmd::new(args); + let cmd = ClippyCmd::new(args, None); + assert_eq!("fix", cmd.cargo_subcommand); assert_eq!("RUSTC_WORKSPACE_WRAPPER", cmd.path_env()); assert!(cmd.args.iter().any(|arg| arg.ends_with("unstable-options"))); @@ -214,8 +221,9 @@ mod tests { 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")); + let cmd = ClippyCmd::new(args, None); + + assert!(cmd.clippy_args.unwrap().contains("--no-deps")); } #[test] @@ -223,14 +231,16 @@ mod tests { 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); + let cmd = ClippyCmd::new(args, None); + + assert_eq!(1, cmd.clippy_args.unwrap().matches("--no-deps").count()); } #[test] fn check() { let args = "cargo clippy".split_whitespace().map(ToString::to_string); - let cmd = ClippyCmd::new(args); + let cmd = ClippyCmd::new(args, None); + assert_eq!("check", cmd.cargo_subcommand); assert_eq!("RUSTC_WRAPPER", cmd.path_env()); } @@ -240,8 +250,54 @@ mod tests { let args = "cargo clippy -Zunstable-options" .split_whitespace() .map(ToString::to_string); - let cmd = ClippyCmd::new(args); + let cmd = ClippyCmd::new(args, None); + assert_eq!("check", cmd.cargo_subcommand); assert_eq!("RUSTC_WORKSPACE_WRAPPER", cmd.path_env()); } + + #[test] + fn clippy_args_into_rustflags() { + let args = "cargo clippy -- -W clippy::as_conversions" + .split_whitespace() + .map(ToString::to_string); + let rustflags = None; + let cmd = ClippyCmd::new(args, rustflags); + + assert_eq!("-W clippy::as_conversions", cmd.rustflags.unwrap()); + } + + #[test] + fn clippy_args_respect_existing_rustflags() { + let args = "cargo clippy -- -D clippy::await_holding_lock" + .split_whitespace() + .map(ToString::to_string); + let rustflags = Some(r#"--cfg feature="some_feat""#.into()); + let cmd = ClippyCmd::new(args, rustflags); + + assert_eq!( + r#"-D clippy::await_holding_lock --cfg feature="some_feat""#, + cmd.rustflags.unwrap() + ); + } + + #[test] + fn no_env_change_if_no_clippy_args() { + let args = "cargo clippy".split_whitespace().map(ToString::to_string); + let rustflags = Some(r#"--cfg feature="some_feat""#.into()); + let cmd = ClippyCmd::new(args, rustflags); + + assert!(cmd.clippy_args.is_none()); + assert!(cmd.rustflags.is_none()); + } + + #[test] + fn no_env_change_if_no_clippy_args_nor_rustflags() { + let args = "cargo clippy".split_whitespace().map(ToString::to_string); + let rustflags = None; + let cmd = ClippyCmd::new(args, rustflags); + + assert!(cmd.clippy_args.is_none()); + assert!(cmd.rustflags.is_none()); + } } diff --git a/tests/dogfood.rs b/tests/dogfood.rs index 052223d6d6ff7..fda1413868e82 100644 --- a/tests/dogfood.rs +++ b/tests/dogfood.rs @@ -23,7 +23,7 @@ fn dogfood_clippy() { .current_dir(root_dir) .env("CLIPPY_DOGFOOD", "1") .env("CARGO_INCREMENTAL", "0") - .arg("clippy-preview") + .arg("clippy") .arg("--all-targets") .arg("--all-features") .arg("--") From f93d9654d2ce012e146b8dfa615ad724f4bb23fd Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Sun, 13 Dec 2020 17:21:53 +0100 Subject: [PATCH 2/2] Address comments from PR review Also: enable tests for cargo-clippy --- Cargo.toml | 1 - src/main.rs | 68 +++++++++++++++++++++++++++++++---------------------- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a765390c6032d..7f9d22e594b9c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,7 +20,6 @@ publish = false [[bin]] name = "cargo-clippy" -test = false path = "src/main.rs" [[bin]] diff --git a/src/main.rs b/src/main.rs index 7594ea2c7b1d1..1c0e04689a9fe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,4 +1,5 @@ #![feature(bool_to_option)] +#![feature(command_access)] #![cfg_attr(feature = "deny-warnings", deny(warnings))] // warn on lints, that are included in `rust-lang/rust`s bootstrap #![warn(rust_2018_idioms, unused_lifetimes)] @@ -63,12 +64,11 @@ struct ClippyCmd { unstable_options: bool, cargo_subcommand: &'static str, args: Vec, - rustflags: Option, clippy_args: Option, } impl ClippyCmd { - fn new(mut old_args: I, rustflags: Option) -> Self + fn new(mut old_args: I) -> Self where I: Iterator, { @@ -111,8 +111,6 @@ impl ClippyCmd { unstable_options, cargo_subcommand, args, - rustflags: has_args - .then(|| rustflags.map_or_else(|| clippy_args.clone(), |flags| format!("{} {}", clippy_args, flags))), clippy_args: has_args.then_some(clippy_args), } } @@ -153,7 +151,7 @@ impl ClippyCmd { .map(|p| ("CARGO_TARGET_DIR", p)) } - fn into_std_cmd(self) -> Command { + fn into_std_cmd(self, rustflags: Option) -> Command { let mut cmd = Command::new("cargo"); cmd.env(self.path_env(), Self::path()) @@ -163,9 +161,12 @@ impl ClippyCmd { // HACK: pass Clippy args to the driver *also* through RUSTFLAGS. // This guarantees that new builds will be triggered when Clippy flags change. - if let (Some(clippy_args), Some(rustflags)) = (self.clippy_args, self.rustflags) { + if let Some(clippy_args) = self.clippy_args { + cmd.env( + "RUSTFLAGS", + rustflags.map_or(clippy_args.clone(), |flags| format!("{} {}", clippy_args, flags)), + ); cmd.env("CLIPPY_ARGS", clippy_args); - cmd.env("RUSTFLAGS", rustflags); } cmd @@ -176,9 +177,9 @@ fn process(old_args: I) -> Result<(), i32> where I: Iterator, { - let cmd = ClippyCmd::new(old_args, env::var("RUSTFLAGS").ok()); + let cmd = ClippyCmd::new(old_args); - let mut cmd = cmd.into_std_cmd(); + let mut cmd = cmd.into_std_cmd(env::var("RUSTFLAGS").ok()); let exit_status = cmd .spawn() @@ -196,12 +197,13 @@ where #[cfg(test)] mod tests { use super::ClippyCmd; + use std::ffi::OsStr; #[test] #[should_panic] fn fix_without_unstable() { let args = "cargo clippy --fix".split_whitespace().map(ToString::to_string); - let _ = ClippyCmd::new(args, None); + let _ = ClippyCmd::new(args); } #[test] @@ -209,7 +211,7 @@ mod tests { let args = "cargo clippy --fix -Zunstable-options" .split_whitespace() .map(ToString::to_string); - let cmd = ClippyCmd::new(args, None); + let cmd = ClippyCmd::new(args); assert_eq!("fix", cmd.cargo_subcommand); assert_eq!("RUSTC_WORKSPACE_WRAPPER", cmd.path_env()); @@ -221,7 +223,7 @@ mod tests { let args = "cargo clippy --fix -Zunstable-options" .split_whitespace() .map(ToString::to_string); - let cmd = ClippyCmd::new(args, None); + let cmd = ClippyCmd::new(args); assert!(cmd.clippy_args.unwrap().contains("--no-deps")); } @@ -231,7 +233,7 @@ mod tests { let args = "cargo clippy --fix -Zunstable-options -- --no-deps" .split_whitespace() .map(ToString::to_string); - let cmd = ClippyCmd::new(args, None); + let cmd = ClippyCmd::new(args); assert_eq!(1, cmd.clippy_args.unwrap().matches("--no-deps").count()); } @@ -239,7 +241,7 @@ mod tests { #[test] fn check() { let args = "cargo clippy".split_whitespace().map(ToString::to_string); - let cmd = ClippyCmd::new(args, None); + let cmd = ClippyCmd::new(args); assert_eq!("check", cmd.cargo_subcommand); assert_eq!("RUSTC_WRAPPER", cmd.path_env()); @@ -250,7 +252,7 @@ mod tests { let args = "cargo clippy -Zunstable-options" .split_whitespace() .map(ToString::to_string); - let cmd = ClippyCmd::new(args, None); + let cmd = ClippyCmd::new(args); assert_eq!("check", cmd.cargo_subcommand); assert_eq!("RUSTC_WORKSPACE_WRAPPER", cmd.path_env()); @@ -261,10 +263,14 @@ mod tests { let args = "cargo clippy -- -W clippy::as_conversions" .split_whitespace() .map(ToString::to_string); + let cmd = ClippyCmd::new(args); + let rustflags = None; - let cmd = ClippyCmd::new(args, rustflags); + let cmd = cmd.into_std_cmd(rustflags); - assert_eq!("-W clippy::as_conversions", cmd.rustflags.unwrap()); + assert!(cmd + .get_envs() + .any(|(key, val)| key == "RUSTFLAGS" && val == Some(OsStr::new("-W clippy::as_conversions")))); } #[test] @@ -272,32 +278,38 @@ mod tests { let args = "cargo clippy -- -D clippy::await_holding_lock" .split_whitespace() .map(ToString::to_string); + let cmd = ClippyCmd::new(args); + let rustflags = Some(r#"--cfg feature="some_feat""#.into()); - let cmd = ClippyCmd::new(args, rustflags); + let cmd = cmd.into_std_cmd(rustflags); - assert_eq!( - r#"-D clippy::await_holding_lock --cfg feature="some_feat""#, - cmd.rustflags.unwrap() - ); + assert!(cmd.get_envs().any(|(key, val)| key == "RUSTFLAGS" + && val == Some(OsStr::new(r#"-D clippy::await_holding_lock --cfg feature="some_feat""#)))); } #[test] fn no_env_change_if_no_clippy_args() { let args = "cargo clippy".split_whitespace().map(ToString::to_string); + let cmd = ClippyCmd::new(args); + let rustflags = Some(r#"--cfg feature="some_feat""#.into()); - let cmd = ClippyCmd::new(args, rustflags); + let cmd = cmd.into_std_cmd(rustflags); - assert!(cmd.clippy_args.is_none()); - assert!(cmd.rustflags.is_none()); + assert!(!cmd + .get_envs() + .any(|(key, _)| key == "RUSTFLAGS" || key == "CLIPPY_ARGS")); } #[test] fn no_env_change_if_no_clippy_args_nor_rustflags() { let args = "cargo clippy".split_whitespace().map(ToString::to_string); + let cmd = ClippyCmd::new(args); + let rustflags = None; - let cmd = ClippyCmd::new(args, rustflags); + let cmd = cmd.into_std_cmd(rustflags); - assert!(cmd.clippy_args.is_none()); - assert!(cmd.rustflags.is_none()); + assert!(!cmd + .get_envs() + .any(|(key, _)| key == "RUSTFLAGS" || key == "CLIPPY_ARGS")) } }