Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass Clippy args also trough RUSTFLAGS #6441

Merged
merged 2 commits into from
Dec 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ publish = false

[[bin]]
name = "cargo-clippy"
test = false
path = "src/main.rs"

[[bin]]
Expand Down
1 change: 0 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
116 changes: 87 additions & 29 deletions src/driver.rs
Original file line number Diff line number Diff line change
@@ -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)]
Expand All @@ -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;
Expand Down Expand Up @@ -47,20 +49,6 @@ 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 {}

Expand Down Expand Up @@ -182,6 +170,28 @@ 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();
Expand Down Expand Up @@ -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::<Vec<String>>();
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
Expand All @@ -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;
Expand All @@ -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"]);
}
}
98 changes: 83 additions & 15 deletions src/main.rs
Original file line number Diff line number Diff line change
@@ -1,3 +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)]
Expand Down Expand Up @@ -62,7 +64,7 @@ struct ClippyCmd {
unstable_options: bool,
cargo_subcommand: &'static str,
args: Vec<String>,
clippy_args: Vec<String>,
clippy_args: Option<String>,
}

impl ClippyCmd {
Expand Down Expand Up @@ -99,16 +101,17 @@ impl ClippyCmd {
args.insert(0, "+nightly".to_string());
}

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 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 has_args = !clippy_args.is_empty();
ClippyCmd {
unstable_options,
cargo_subcommand,
args,
clippy_args,
clippy_args: has_args.then_some(clippy_args),
}
}

Expand Down Expand Up @@ -148,20 +151,24 @@ impl ClippyCmd {
.map(|p| ("CARGO_TARGET_DIR", p))
}

fn into_std_cmd(self) -> Command {
fn into_std_cmd(self, rustflags: Option<String>) -> 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
}
}
Expand All @@ -172,7 +179,7 @@ where
{
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()
Expand All @@ -190,6 +197,7 @@ where
#[cfg(test)]
mod tests {
use super::ClippyCmd;
use std::ffi::OsStr;

#[test]
#[should_panic]
Expand All @@ -204,6 +212,7 @@ 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")));
Expand All @@ -215,7 +224,8 @@ mod tests {
.split_whitespace()
.map(ToString::to_string);
let cmd = ClippyCmd::new(args);
assert!(cmd.clippy_args.iter().any(|arg| arg == "--no-deps"));

assert!(cmd.clippy_args.unwrap().contains("--no-deps"));
}

#[test]
Expand All @@ -224,13 +234,15 @@ mod tests {
.split_whitespace()
.map(ToString::to_string);
let cmd = ClippyCmd::new(args);
assert_eq!(cmd.clippy_args.iter().filter(|arg| *arg == "--no-deps").count(), 1);

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);

assert_eq!("check", cmd.cargo_subcommand);
assert_eq!("RUSTC_WRAPPER", cmd.path_env());
}
Expand All @@ -241,7 +253,63 @@ 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"))
}
}
2 changes: 1 addition & 1 deletion tests/dogfood.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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("--")
Expand Down