Revert "Pass Clippy args also trough RUSTFLAGS"

rust-lang · Dec 23, 2020 · e898015 · e898015
1 parent 4911ab1
commit e898015
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 171 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -20,6 +20,7 @@ publish = false
 
 [[bin]]
 name = "cargo-clippy"
+test = false
 path = "src/main.rs"
 
 [[bin]]

diff --git a/README.md b/README.md
@@ -208,6 +208,7 @@ 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
@@ -1,6 +1,5 @@
 #![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)]
@@ -20,7 +19,6 @@ 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;
@@ -49,6 +47,20 @@ fn arg_value<'a, T: Deref<Target = str>>(
     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 {}
 
@@ -170,28 +182,6 @@ fn toolchain_path(home: Option<String>, toolchain: Option<String>) -> Option<Pat
     })
 }
 
-fn remove_clippy_args<'a, T, U, I>(args: &mut Vec<T>, clippy_args: I)
-where
-    T: AsRef<str>,
-    U: AsRef<str> + ?Sized + 'a,
-    I: Iterator<Item = &'a U> + 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();
@@ -288,9 +278,20 @@ pub fn main() {
             args.extend(vec!["--sysroot".into(), sys_root]);
         };
 
-        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");
+        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::<Vec<String>>();
 
         // We enable Clippy if one of the following conditions is met
         // - IF Clippy is run on its test suite OR
@@ -303,11 +304,7 @@ pub fn main() {
 
         let clippy_enabled = clippy_tests_set || (!cap_lints_allow && (!no_deps || in_primary_package));
         if clippy_enabled {
-            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);
+            args.extend(clippy_args);
         }
 
         let mut clippy = ClippyCallbacks;
@@ -318,58 +315,3 @@ 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
@@ -1,5 +1,3 @@
-#![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)]
@@ -64,7 +62,7 @@ struct ClippyCmd {
     unstable_options: bool,
     cargo_subcommand: &'static str,
     args: Vec<String>,
-    clippy_args: Option<String>,
+    clippy_args: Vec<String>,
 }
 
 impl ClippyCmd {
@@ -101,17 +99,16 @@ impl ClippyCmd {
             args.insert(0, "+nightly".to_string());
         }
 
-        let mut clippy_args = old_args.collect::<Vec<String>>().join(" ");
-        if cargo_subcommand == "fix" && !clippy_args.contains("--no-deps") {
-            clippy_args = format!("{} --no-deps", clippy_args);
+        let mut clippy_args: Vec<String> = old_args.collect();
+        if cargo_subcommand == "fix" && !clippy_args.iter().any(|arg| arg == "--no-deps") {
+            clippy_args.push("--no-deps".into());
         }
 
-        let has_args = !clippy_args.is_empty();
         ClippyCmd {
             unstable_options,
             cargo_subcommand,
             args,
-            clippy_args: has_args.then_some(clippy_args),
+            clippy_args,
         }
     }
 
@@ -151,24 +148,20 @@ impl ClippyCmd {
             .map(|p| ("CARGO_TARGET_DIR", p))
     }
 
-    fn into_std_cmd(self, rustflags: Option<String>) -> Command {
+    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) = self.clippy_args {
-            cmd.env(
-                "RUSTFLAGS",
-                rustflags.map_or(clippy_args.clone(), |flags| format!("{} {}", clippy_args, flags)),
-            );
-            cmd.env("CLIPPY_ARGS", clippy_args);
-        }
-
         cmd
     }
 }
@@ -179,7 +172,7 @@ where
 {
     let cmd = ClippyCmd::new(old_args);
 
-    let mut cmd = cmd.into_std_cmd(env::var("RUSTFLAGS").ok());
+    let mut cmd = cmd.into_std_cmd();
 
     let exit_status = cmd
         .spawn()
@@ -197,7 +190,6 @@ where
 #[cfg(test)]
 mod tests {
     use super::ClippyCmd;
-    use std::ffi::OsStr;
 
     #[test]
     #[should_panic]
@@ -212,7 +204,6 @@ mod tests {
             .split_whitespace()
             .map(ToString::to_string);
         let cmd = ClippyCmd::new(args);
-
         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")));
@@ -224,8 +215,7 @@ mod tests {
             .split_whitespace()
             .map(ToString::to_string);
         let cmd = ClippyCmd::new(args);
-
-        assert!(cmd.clippy_args.unwrap().contains("--no-deps"));
+        assert!(cmd.clippy_args.iter().any(|arg| arg == "--no-deps"));
     }
 
     #[test]
@@ -234,15 +224,13 @@ mod tests {
             .split_whitespace()
             .map(ToString::to_string);
         let cmd = ClippyCmd::new(args);
-
-        assert_eq!(1, cmd.clippy_args.unwrap().matches("--no-deps").count());
+        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);
         let cmd = ClippyCmd::new(args);
-
         assert_eq!("check", cmd.cargo_subcommand);
         assert_eq!("RUSTC_WRAPPER", cmd.path_env());
     }
@@ -253,63 +241,7 @@ mod tests {
             .split_whitespace()
             .map(ToString::to_string);
         let cmd = ClippyCmd::new(args);
-
         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 cmd = ClippyCmd::new(args);
-
-        let rustflags = None;
-        let cmd = cmd.into_std_cmd(rustflags);
-
-        assert!(cmd
-            .get_envs()
-            .any(|(key, val)| key == "RUSTFLAGS" && val == Some(OsStr::new("-W clippy::as_conversions"))));
-    }
-
-    #[test]
-    fn clippy_args_respect_existing_rustflags() {
-        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 = cmd.into_std_cmd(rustflags);
-
-        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 = cmd.into_std_cmd(rustflags);
-
-        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 = cmd.into_std_cmd(rustflags);
-
-        assert!(!cmd
-            .get_envs()
-            .any(|(key, _)| key == "RUSTFLAGS" || key == "CLIPPY_ARGS"))
-    }
 }
diff --git 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")
+        .arg("clippy-preview")
         .arg("--all-targets")
         .arg("--all-features")
         .arg("--")