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

Add --no-deps option to avoid running on path dependencies in workspaces #6188

Merged
merged 5 commits into from
Dec 9, 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
16 changes: 16 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,22 @@ Note that this is still experimental and only supported on the nightly channel:
cargo clippy --fix -Z unstable-options
```

#### Workspaces

All the usual workspace options should work with Clippy. For example the following command
will run Clippy on the `example` crate:

```terminal
cargo clippy -p example
```

As with `cargo check`, this includes dependencies that are members of the workspace, like path dependencies.
If you want to run Clippy **only** on the given crate, use the `--no-deps` option like this:

```terminal
cargo clippy -p example -- --no-deps
```

### Running Clippy from the command line without installing it

To have cargo compile your crate with Clippy without Clippy installation
Expand Down
3 changes: 3 additions & 0 deletions clippy_workspace_tests/path_dep/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[package]
name = "path_dep"
version = "0.1.0"
6 changes: 6 additions & 0 deletions clippy_workspace_tests/path_dep/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#![deny(clippy::empty_loop)]

#[cfg(feature = "primary_package_test")]
pub fn lint_me() {
loop {}
}
3 changes: 3 additions & 0 deletions clippy_workspace_tests/subcrate/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
[package]
name = "subcrate"
version = "0.1.0"

[dependencies]
path_dep = { path = "../path_dep" }
44 changes: 29 additions & 15 deletions src/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ fn toolchain_path(home: Option<String>, toolchain: Option<String>) -> Option<Pat
})
}

#[allow(clippy::too_many_lines)]
pub fn main() {
rustc_driver::init_rustc_env_logger();
SyncLazy::force(&ICE_HOOK);
Expand Down Expand Up @@ -277,27 +278,40 @@ pub fn main() {
args.extend(vec!["--sysroot".into(), sys_root]);
};

// this check ensures that dependencies are built but not linted and the final
// crate is linted but not built
let clippy_enabled = env::var("CLIPPY_TESTS").map_or(false, |val| val == "true")
|| arg_value(&orig_args, "--cap-lints", |val| val == "allow").is_none();

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
// - IF Clippy is run on the main crate, not on deps (`!cap_lints_allow`) THEN
// - IF `--no-deps` is not set (`!no_deps`) OR
// - IF `--no-deps` is set and Clippy is run on the specified primary package
let clippy_tests_set = env::var("CLIPPY_TESTS").map_or(false, |val| val == "true");
let cap_lints_allow = arg_value(&orig_args, "--cap-lints", |val| val == "allow").is_some();
let in_primary_package = env::var("CARGO_PRIMARY_PACKAGE").is_ok();

let clippy_enabled = clippy_tests_set || (!cap_lints_allow && (!no_deps || in_primary_package));
if clippy_enabled {
args.extend(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()]);
if let Ok(extra_args) = env::var("CLIPPY_ARGS") {
args.extend(extra_args.split("__CLIPPY_HACKERY__").filter_map(|s| {
if s.is_empty() {
None
} else {
Some(s.to_string())
}
}));
}
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()
}))
}
32 changes: 29 additions & 3 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ struct ClippyCmd {
unstable_options: bool,
cargo_subcommand: &'static str,
args: Vec<String>,
clippy_args: String,
clippy_args: Vec<String>,
}

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

let clippy_args: String = old_args.map(|arg| format!("{}__CLIPPY_HACKERY__", arg)).collect();
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());
}

ClippyCmd {
unstable_options,
Expand Down Expand Up @@ -147,10 +150,15 @@ impl ClippyCmd {

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", self.clippy_args)
.env("CLIPPY_ARGS", clippy_args)
.arg(self.cargo_subcommand)
.args(&self.args);

Expand Down Expand Up @@ -201,6 +209,24 @@ mod tests {
assert!(cmd.args.iter().any(|arg| arg.ends_with("unstable-options")));
}

#[test]
fn fix_implies_no_deps() {
let args = "cargo clippy --fix -Zunstable-options"
.split_whitespace()
.map(ToString::to_string);
let cmd = ClippyCmd::new(args);
assert!(cmd.clippy_args.iter().any(|arg| arg == "--no-deps"));
}

#[test]
fn no_deps_not_duplicated_with_fix() {
let args = "cargo clippy --fix -Zunstable-options -- --no-deps"
.split_whitespace()
.map(ToString::to_string);
let cmd = ClippyCmd::new(args);
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);
Expand Down
71 changes: 70 additions & 1 deletion tests/dogfood.rs
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::PathBuf;
use std::path::{Path, PathBuf};
use std::process::Command;

mod cargo;
Expand Down Expand Up @@ -41,12 +41,77 @@ 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;
}
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
clean(&cwd, &target_dir);
flip1995 marked this conversation as resolved.
Show resolved Hide resolved

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

// Make sure we start with a clean state
clean(&cwd, &target_dir);

// 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());
}

// run clippy on remaining subprojects and fail the test if lint warnings are reported
if cargo::is_rustc_test_suite() {
return;
}
let root_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"));

// NOTE: `path_dep` crate is omitted on purpose here
for d in &[
"clippy_workspace_tests",
"clippy_workspace_tests/src",
Expand All @@ -72,4 +137,8 @@ fn dogfood_subprojects() {

assert!(output.status.success());
}

// NOTE: Since tests run in parallel we can't run cargo commands on the same workspace at the
// same time, so we test this immediately after the dogfood for workspaces.
test_no_deps_ignores_path_deps_in_workspaces();
}