Skip to content

Commit

Permalink
Auto merge of rust-lang#6834 - hyd-dev:clippy-args, r=phansch,flip199…
Browse files Browse the repository at this point in the history 
…5,oli-obk

Let Cargo track CLIPPY_ARGS

This PR makes `clippy-driver` emit `CLIPPY_ARGS` in its `dep-info` output.

Just like rust-lang#6441, this allows this workflow to work:
```shell
cargo clippy # warning: empty `loop {}` wastes CPU cycles
cargo clippy -- -A clippy::empty_loop # no warnings emitted
```
But without rebuilding all dependencies.

cc https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/CLIPPY_ARGS.20is.20not.20tracked.20by.20Cargo/near/228599088

changelog: Cargo now re-runs Clippy if arguments after `--` provided to `cargo clippy` are changed.
  • Loading branch information
@bors
bors committed Mar 8, 2021
2 parents e6c643f + 3cd5f44 commit d0d5232
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 42 deletions.
1 change: 0 additions & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,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
54 changes: 46 additions & 8 deletions src/driver.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@
extern crate rustc_driver;
extern crate rustc_errors;
extern crate rustc_interface;
extern crate rustc_session;
extern crate rustc_span;

use rustc_interface::interface;
use rustc_session::Session;
use rustc_span::symbol::Symbol;
use rustc_tools_util::VersionInfo;

use std::borrow::Cow;
Expand Down Expand Up @@ -59,20 +63,53 @@ fn test_arg_value() {
assert_eq!(arg_value(args, "--foo", |_| true), None);
}

fn track_clippy_args(sess: &Session, args_env_var: &Option<String>) {
sess.parse_sess.env_depinfo.borrow_mut().insert((
Symbol::intern("CLIPPY_ARGS"),
args_env_var.as_deref().map(Symbol::intern),
));
}

struct DefaultCallbacks;
impl rustc_driver::Callbacks for DefaultCallbacks {}

struct ClippyCallbacks;
/// This is different from `DefaultCallbacks` that it will inform Cargo to track the value of
/// `CLIPPY_ARGS` environment variable.
struct RustcCallbacks {
clippy_args_var: Option<String>,
}

impl rustc_driver::Callbacks for RustcCallbacks {
fn config(&mut self, config: &mut interface::Config) {
let previous = config.register_lints.take();
let clippy_args_var = self.clippy_args_var.take();
config.register_lints = Some(Box::new(move |sess, lint_store| {
if let Some(ref previous) = previous {
(previous)(sess, lint_store);
}

track_clippy_args(sess, &clippy_args_var);
}));
}
}

struct ClippyCallbacks {
clippy_args_var: Option<String>,
}

impl rustc_driver::Callbacks for ClippyCallbacks {
fn config(&mut self, config: &mut interface::Config) {
let previous = config.register_lints.take();
let clippy_args_var = self.clippy_args_var.take();
config.register_lints = Some(Box::new(move |sess, mut lint_store| {
// technically we're ~guaranteed that this is none but might as well call anything that
// is there already. Certainly it can't hurt.
if let Some(previous) = &previous {
(previous)(sess, lint_store);
}

track_clippy_args(sess, &clippy_args_var);

let conf = clippy_lints::read_conf(&[], &sess);
clippy_lints::register_plugins(&mut lint_store, &sess, &conf);
clippy_lints::register_pre_expansion_lints(&mut lint_store);
Expand Down Expand Up @@ -277,7 +314,9 @@ pub fn main() {
};

let mut no_deps = false;
let clippy_args = env::var("CLIPPY_ARGS")
let clippy_args_var = env::var("CLIPPY_ARGS").ok();
let clippy_args = clippy_args_var
.as_deref()
.unwrap_or_default()
.split("__CLIPPY_HACKERY__")
.filter_map(|s| match s {
Expand Down Expand Up @@ -305,11 +344,10 @@ pub fn main() {
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 };

rustc_driver::RunCompiler::new(&args, callbacks).run()
if clippy_enabled {
rustc_driver::RunCompiler::new(&args, &mut ClippyCallbacks { clippy_args_var }).run()
} else {
rustc_driver::RunCompiler::new(&args, &mut RustcCallbacks { clippy_args_var }).run()
}
}))
}
101 changes: 68 additions & 33 deletions tests/dogfood.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#![feature(once_cell)]

use std::lazy::SyncLazy;
use std::path::{Path, PathBuf};
use std::path::PathBuf;
use std::process::Command;

mod cargo;
Expand Down Expand Up @@ -45,40 +45,53 @@ fn dogfood_clippy() {
assert!(output.status.success());
}

#[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");
fn test_no_deps_ignores_path_deps_in_workspaces() {
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
Command::new("cargo")
.current_dir(&cwd)
.env("CARGO_TARGET_DIR", &target_dir)
.arg("clean")
.args(&["-p", "subcrate"])
.args(&["-p", "path_dep"])
.output()
.unwrap();

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

// Make sure we start with a clean state
clean(&cwd, &target_dir);
assert!(output.status.success());

// `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 lint_path_dep = || {
// 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("--no-deps")
.arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir
.args(&["--cfg", r#"feature="primary_package_test""#])
.output()
Expand All @@ -87,12 +100,18 @@ fn dogfood_subprojects() {
println!("stdout: {}", String::from_utf8_lossy(&output.stdout));
println!("stderr: {}", String::from_utf8_lossy(&output.stderr));

assert!(output.status.success());
assert!(!output.status.success());
assert!(
String::from_utf8(output.stderr)
.unwrap()
.contains("error: empty `loop {}` wastes CPU cycles")
);
};

// Make sure we start with a clean state
clean(&cwd, &target_dir);
// Make sure Cargo is aware of the removal of `--no-deps`.
lint_path_dep();

// Test that without the `--no-deps` argument, `path_dep` is linted.
let successful_build = || {
let output = Command::new(&*CLIPPY_PATH)
.current_dir(&cwd)
.env("CLIPPY_DOGFOOD", "1")
Expand All @@ -101,16 +120,32 @@ fn dogfood_subprojects() {
.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());
}
assert!(output.status.success());

output
};

// Trigger a sucessful build, so Cargo would like to cache the build result.
successful_build();

// Make sure there's no spurious rebuild when nothing changes.
let stderr = String::from_utf8(successful_build().stderr).unwrap();
assert!(!stderr.contains("Compiling"));
assert!(!stderr.contains("Checking"));
assert!(stderr.contains("Finished"));

// Make sure Cargo is aware of the new `--cfg` flag.
lint_path_dep();
}

#[test]
fn dogfood_subprojects() {
// run clippy on remaining subprojects and fail the test if lint warnings are reported
if cargo::is_rustc_test_suite() {
return;
Expand Down

0 comments on commit d0d5232

Please sign in to comment.