From 03e72d5446bd104ac4480032ea44bd7419ce5694 Mon Sep 17 00:00:00 2001 From: hyd-dev Date: Wed, 3 Mar 2021 22:06:36 +0800 Subject: [PATCH 1/4] Let Cargo track `CLIPPY_ARGS` --- README.md | 1 - src/driver.rs | 60 +++++++++++++++++++++++++----- tests/dogfood.rs | 97 ++++++++++++++++++++++++++++++++---------------- 3 files changed, 117 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 3cc03cf36033..63057609bb6f 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/src/driver.rs b/src/driver.rs index d5143e1438ee..081a2ddeb164 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -11,12 +11,16 @@ 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; -use std::env; +use std::env::{self, VarError}; use std::lazy::SyncLazy; use std::ops::Deref; use std::panic; @@ -59,13 +63,42 @@ fn test_arg_value() { assert_eq!(arg_value(args, "--foo", |_| true), None); } +fn track_clippy_args(sess: &Session, args_env_var: &Option) { + 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; +struct ClippyArgsCallbacks { + clippy_args_var: Option, +} + +impl rustc_driver::Callbacks for ClippyArgsCallbacks { + 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, +} + 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. @@ -73,6 +106,8 @@ impl rustc_driver::Callbacks for ClippyCallbacks { (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); @@ -277,7 +312,15 @@ pub fn main() { }; let mut no_deps = false; - let clippy_args = env::var("CLIPPY_ARGS") + let clippy_args_var = env::var("CLIPPY_ARGS").map_or_else( + |e| match e { + VarError::NotPresent => None, + VarError::NotUnicode(s) => panic!("CLIPPY_ARGS is not valid Unicode: {:?}", s), + }, + Some, + ); + let clippy_args = clippy_args_var + .as_deref() .unwrap_or_default() .split("__CLIPPY_HACKERY__") .filter_map(|s| match s { @@ -305,11 +348,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 ClippyArgsCallbacks { clippy_args_var }).run() + } })) } diff --git a/tests/dogfood.rs b/tests/dogfood.rs index 8fe48a67beb5..2505836a5ed8 100644 --- a/tests/dogfood.rs +++ b/tests/dogfood.rs @@ -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; @@ -49,17 +49,6 @@ fn dogfood_clippy() { #[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; } @@ -68,7 +57,14 @@ fn dogfood_subprojects() { let cwd = root.join("clippy_workspace_tests"); // Make sure we start with a clean state - clean(&cwd, &target_dir); + 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`. @@ -90,26 +86,65 @@ fn dogfood_subprojects() { assert!(output.status.success()); - // Make sure we start with a clean state - clean(&cwd, &target_dir); + 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("-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!( + String::from_utf8(output.stderr) + .unwrap() + .contains("error: empty `loop {}` wastes CPU cycles") + ); + }; + + // Make sure Cargo is aware of the removal of `--no-deps`. + lint_path_dep(); + + let successful_build = || { + let output = Command::new(&*CLIPPY_PATH) + .current_dir(&cwd) + .env("CLIPPY_DOGFOOD", "1") + .env("CARGO_INCREMENTAL", "0") + .arg("clippy") + .args(&["-p", "subcrate"]) + .arg("--") + .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir + .output() + .unwrap(); + println!("status: {}", output.status); + println!("stdout: {}", String::from_utf8_lossy(&output.stdout)); + println!("stderr: {}", String::from_utf8_lossy(&output.stderr)); - // 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("-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()); + + 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")); - assert!(!output.status.success()); + // Make sure Cargo is aware of the new `--cfg` flag. + lint_path_dep(); } // run clippy on remaining subprojects and fail the test if lint warnings are reported From 2d07c33c86c2576326c97bf6641621bdd90e934a Mon Sep 17 00:00:00 2001 From: hyd-dev Date: Mon, 8 Mar 2021 18:28:43 +0800 Subject: [PATCH 2/4] Rename `ClippyArgsCallbacks` to `RustcCallbacks` --- src/driver.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/driver.rs b/src/driver.rs index 081a2ddeb164..0fba41775aaf 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -73,11 +73,13 @@ fn track_clippy_args(sess: &Session, args_env_var: &Option) { struct DefaultCallbacks; impl rustc_driver::Callbacks for DefaultCallbacks {} -struct ClippyArgsCallbacks { +/// 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, } -impl rustc_driver::Callbacks for ClippyArgsCallbacks { +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(); @@ -351,7 +353,7 @@ pub fn main() { if clippy_enabled { rustc_driver::RunCompiler::new(&args, &mut ClippyCallbacks { clippy_args_var }).run() } else { - rustc_driver::RunCompiler::new(&args, &mut ClippyArgsCallbacks { clippy_args_var }).run() + rustc_driver::RunCompiler::new(&args, &mut RustcCallbacks { clippy_args_var }).run() } })) } From 2d53b6b82412a18734478c0086090dc3a2b5c5cc Mon Sep 17 00:00:00 2001 From: hyd-dev Date: Mon, 8 Mar 2021 18:29:36 +0800 Subject: [PATCH 3/4] Move `test_no_deps_ignores_path_deps_in_workspaces()` out of `dogfood_subprojects()` --- tests/dogfood.rs | 158 +++++++++++++++++++++++------------------------ 1 file changed, 79 insertions(+), 79 deletions(-) diff --git a/tests/dogfood.rs b/tests/dogfood.rs index 2505836a5ed8..296eeb4aabd9 100644 --- a/tests/dogfood.rs +++ b/tests/dogfood.rs @@ -46,28 +46,73 @@ fn dogfood_clippy() { assert!(output.status.success()); } -#[test] -fn dogfood_subprojects() { - 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") +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)); + + assert!(output.status.success()); + + 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("CARGO_TARGET_DIR", &target_dir) - .arg("clean") + .env("CLIPPY_DOGFOOD", "1") + .env("CARGO_INCREMENTAL", "0") + .arg("clippy") .args(&["-p", "subcrate"]) - .args(&["-p", "path_dep"]) + .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!( + String::from_utf8(output.stderr) + .unwrap() + .contains("error: empty `loop {}` wastes CPU cycles") + ); + }; + + // Make sure Cargo is aware of the removal of `--no-deps`. + lint_path_dep(); - // `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 successful_build = || { let output = Command::new(&*CLIPPY_PATH) .current_dir(&cwd) .env("CLIPPY_DOGFOOD", "1") @@ -75,9 +120,7 @@ fn dogfood_subprojects() { .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); @@ -86,67 +129,24 @@ fn dogfood_subprojects() { assert!(output.status.success()); - 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("-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!( - String::from_utf8(output.stderr) - .unwrap() - .contains("error: empty `loop {}` wastes CPU cycles") - ); - }; - - // Make sure Cargo is aware of the removal of `--no-deps`. - lint_path_dep(); - - let successful_build = || { - let output = Command::new(&*CLIPPY_PATH) - .current_dir(&cwd) - .env("CLIPPY_DOGFOOD", "1") - .env("CARGO_INCREMENTAL", "0") - .arg("clippy") - .args(&["-p", "subcrate"]) - .arg("--") - .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir - .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()); - - 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(); - } + 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; From 3cd5f44ec463fa0c9376ad7e3d7b8e19e662bd8c Mon Sep 17 00:00:00 2001 From: hyd-dev Date: Mon, 8 Mar 2021 18:49:41 +0800 Subject: [PATCH 4/4] Don't panic if `CLIPPY_ARGS` is not Unicode --- src/driver.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/driver.rs b/src/driver.rs index 0fba41775aaf..82582db0b5ef 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -20,7 +20,7 @@ use rustc_span::symbol::Symbol; use rustc_tools_util::VersionInfo; use std::borrow::Cow; -use std::env::{self, VarError}; +use std::env; use std::lazy::SyncLazy; use std::ops::Deref; use std::panic; @@ -314,13 +314,7 @@ pub fn main() { }; let mut no_deps = false; - let clippy_args_var = env::var("CLIPPY_ARGS").map_or_else( - |e| match e { - VarError::NotPresent => None, - VarError::NotUnicode(s) => panic!("CLIPPY_ARGS is not valid Unicode: {:?}", s), - }, - Some, - ); + let clippy_args_var = env::var("CLIPPY_ARGS").ok(); let clippy_args = clippy_args_var .as_deref() .unwrap_or_default()