Skip to content

Commit

Permalink
Auto merge of #6441 - ebroto:use_rustflags, r=flip1995
Browse files Browse the repository at this point in the history
Pass Clippy args also trough RUSTFLAGS

This removes a hack (\_\_CLIPPY_HACKERY\_\_) to add another one :)

It allows this workflow to work:
```terminal
cargo clippy                             # warning: empty `loop {}` wastes CPU cycles
cargo clippy -- -A clippy::empty_loop    # no warnings emitted
```

Before this change the new flag was not taken into consideration in cargo's fingerprint and the warning was emitted again. I guess that ideally we could add a specific env var for compiler wrapper arguments, but in the meantime this should do the job.

changelog: Pass clippy arguments through RUSTFLAGS so that changing them will trigger a rebuild

r? `@flip1995`
cc `@ehuss` (I think this may count as another step towards stabilizing `RUSTC_WORKSPACE_WRAPPER` 😄)

Fixes #5214 and avoids frustration for users unfamiliar with the issue
  • Loading branch information
bors committed Dec 13, 2020
2 parents b7db5bf + f93d965 commit 684f17e
Show file tree
Hide file tree
Showing 5 changed files with 171 additions and 47 deletions.
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

0 comments on commit 684f17e

Please sign in to comment.