From 2c0a4df83fde6e5b308f0e67a6d96b3df29c60ff Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 22 Jul 2019 10:33:27 -0700 Subject: [PATCH 1/4] reimplement arg passthrough for clippy-driver Prior to this change, the old cargo subcommand version of `cargo clippy` had a feature to pass trailing args down to clippy-driver but when this the subcommand was reimplemented inside of cargo this feature was left out accidentally. This change readds the feature to to capture all args after a trailing -- and forward them down to clippy-driver via an env variable. --- src/bin/cargo/commands/clippy.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs index c591acfacbc..88a36c32857 100644 --- a/src/bin/cargo/commands/clippy.rs +++ b/src/bin/cargo/commands/clippy.rs @@ -6,6 +6,7 @@ use cargo::util; pub fn cli() -> App { subcommand("clippy-preview") .about("Checks a package to catch common mistakes and improve your Rust code.") + .arg(Arg::with_name("args").multiple(true)) .arg_package_spec( "Package(s) to check", "Check all packages in the workspace", @@ -69,7 +70,15 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { .into()); } - let wrapper = util::process(util::config::clippy_driver()); + let mut wrapper = util::process(util::config::clippy_driver()); + + if let Some(old_args) = args.values_of("args") { + let clippy_args: String = old_args + .map(|arg| format!("{}__CLIPPY_HACKERY__", arg)) + .collect(); + wrapper.env("CLIPPY_ARGS", clippy_args); + } + compile_opts.build_config.primary_unit_rustc = Some(wrapper); compile_opts.build_config.force_rebuild = true; From f4dcb2bd976459bd9519a40d701760d7b7edca16 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 22 Jul 2019 11:14:52 -0700 Subject: [PATCH 2/4] remove clippy_hackery --- src/bin/cargo/commands/clippy.rs | 7 ++----- src/cargo/core/compiler/build_config.rs | 11 ++++++++++- src/cargo/core/compiler/compilation.rs | 6 ++++-- src/cargo/core/compiler/mod.rs | 2 +- src/cargo/ops/fix.rs | 6 +++++- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs index 88a36c32857..135fe51bfcb 100644 --- a/src/bin/cargo/commands/clippy.rs +++ b/src/bin/cargo/commands/clippy.rs @@ -72,11 +72,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { let mut wrapper = util::process(util::config::clippy_driver()); - if let Some(old_args) = args.values_of("args") { - let clippy_args: String = old_args - .map(|arg| format!("{}__CLIPPY_HACKERY__", arg)) - .collect(); - wrapper.env("CLIPPY_ARGS", clippy_args); + if let Some(clippy_args) = args.values_of("args") { + wrapper.args(&clippy_args.collect::>()); } compile_opts.build_config.primary_unit_rustc = Some(wrapper); diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index 7f795c442cd..70aebc6d397 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -25,12 +25,21 @@ pub struct BuildConfig { /// Output a build plan to stdout instead of actually compiling. pub build_plan: bool, /// An optional override of the rustc path for primary units only - pub primary_unit_rustc: Option, + pub primary_unit_rustc: Option, pub rustfix_diagnostic_server: RefCell>, /// Whether or not Cargo should cache compiler output on disk. cache_messages: bool, } +/// Configuration for subcommand specific rustc override +#[derive(Debug, Clone)] +pub struct PrimaryUnitRustc { + /// ProcessBuilder to use instead of the default provided by `Rustc` + pub proc: ProcessBuilder, + /// Configure whether or not to use this as a wrapper around the original rustc process + pub is_wrapper: bool, +} + impl BuildConfig { /// Parses all config files to learn about build configuration. Currently /// configured options are: diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 8c24c2e281b..07fd1683292 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -80,8 +80,10 @@ impl<'cfg> Compilation<'cfg> { let mut primary_unit_rustc_process = bcx.build_config.primary_unit_rustc.clone().map(|mut r| { - r.arg(&bcx.rustc.path); - r + if r.is_wrapper { + r.proc.arg(&bcx.rustc.path); + } + r.proc }); if bcx.config.extra_verbose() { diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index efe332a93eb..f0938d94555 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -24,7 +24,7 @@ use log::debug; use same_file::is_same_file; use serde::Serialize; -pub use self::build_config::{BuildConfig, CompileMode, MessageFormat}; +pub use self::build_config::{BuildConfig, CompileMode, MessageFormat, PrimaryUnitRustc}; pub use self::build_context::{BuildContext, FileFlavor, TargetConfig, TargetInfo}; use self::build_plan::BuildPlan; pub use self::compilation::{Compilation, Doctest}; diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index a43c68687b7..28dc1ff51f3 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -51,6 +51,7 @@ use log::{debug, trace, warn}; use rustfix::diagnostics::Diagnostic; use rustfix::{self, CodeFix}; +use crate::core::compiler::PrimaryUnitRustc; use crate::core::Workspace; use crate::ops::{self, CompileOptions}; use crate::util::diagnostic_server::{Message, RustfixDiagnosticServer}; @@ -132,7 +133,10 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { // primary crates are compiled using a cargo subprocess to do extra work of applying fixes and // repeating build until there are no more changes to be applied - opts.compile_opts.build_config.primary_unit_rustc = Some(wrapper); + opts.compile_opts.build_config.primary_unit_rustc = Some(PrimaryUnitRustc { + proc: wrapper, + is_wrapper: true, + }); ops::compile(ws, &opts.compile_opts)?; Ok(()) From 79480cc21cb38055600fcea73e898cb62d0c0940 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Sun, 4 Aug 2019 12:33:36 -0700 Subject: [PATCH 3/4] remove unnecessary is wrapper struct --- src/cargo/core/compiler/build_config.rs | 11 +---------- src/cargo/core/compiler/compilation.rs | 8 +------- src/cargo/core/compiler/mod.rs | 2 +- src/cargo/ops/fix.rs | 9 ++++----- 4 files changed, 7 insertions(+), 23 deletions(-) diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index 70aebc6d397..7f795c442cd 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -25,21 +25,12 @@ pub struct BuildConfig { /// Output a build plan to stdout instead of actually compiling. pub build_plan: bool, /// An optional override of the rustc path for primary units only - pub primary_unit_rustc: Option, + pub primary_unit_rustc: Option, pub rustfix_diagnostic_server: RefCell>, /// Whether or not Cargo should cache compiler output on disk. cache_messages: bool, } -/// Configuration for subcommand specific rustc override -#[derive(Debug, Clone)] -pub struct PrimaryUnitRustc { - /// ProcessBuilder to use instead of the default provided by `Rustc` - pub proc: ProcessBuilder, - /// Configure whether or not to use this as a wrapper around the original rustc process - pub is_wrapper: bool, -} - impl BuildConfig { /// Parses all config files to learn about build configuration. Currently /// configured options are: diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 07fd1683292..73273996b99 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -78,13 +78,7 @@ impl<'cfg> Compilation<'cfg> { pub fn new<'a>(bcx: &BuildContext<'a, 'cfg>) -> CargoResult> { let mut rustc = bcx.rustc.process(); - let mut primary_unit_rustc_process = - bcx.build_config.primary_unit_rustc.clone().map(|mut r| { - if r.is_wrapper { - r.proc.arg(&bcx.rustc.path); - } - r.proc - }); + let mut primary_unit_rustc_process = bcx.build_config.primary_unit_rustc.clone(); if bcx.config.extra_verbose() { rustc.display_env_vars(); diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index f0938d94555..efe332a93eb 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -24,7 +24,7 @@ use log::debug; use same_file::is_same_file; use serde::Serialize; -pub use self::build_config::{BuildConfig, CompileMode, MessageFormat, PrimaryUnitRustc}; +pub use self::build_config::{BuildConfig, CompileMode, MessageFormat}; pub use self::build_context::{BuildContext, FileFlavor, TargetConfig, TargetInfo}; use self::build_plan::BuildPlan; pub use self::compilation::{Compilation, Doctest}; diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 28dc1ff51f3..415229eca6d 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -51,7 +51,6 @@ use log::{debug, trace, warn}; use rustfix::diagnostics::Diagnostic; use rustfix::{self, CodeFix}; -use crate::core::compiler::PrimaryUnitRustc; use crate::core::Workspace; use crate::ops::{self, CompileOptions}; use crate::util::diagnostic_server::{Message, RustfixDiagnosticServer}; @@ -131,12 +130,12 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { server.configure(&mut wrapper); } + let rustc = opts.compile_opts.config.load_global_rustc(Some(ws))?; + wrapper.arg(&rustc.path); + // primary crates are compiled using a cargo subprocess to do extra work of applying fixes and // repeating build until there are no more changes to be applied - opts.compile_opts.build_config.primary_unit_rustc = Some(PrimaryUnitRustc { - proc: wrapper, - is_wrapper: true, - }); + opts.compile_opts.build_config.primary_unit_rustc = Some(wrapper); ops::compile(ws, &opts.compile_opts)?; Ok(()) From 42a00c1d3979619deb29489c2f5b4df31f4b7bd0 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Thu, 8 Aug 2019 16:39:00 -0700 Subject: [PATCH 4/4] add unit test --- tests/testsuite/clippy.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/testsuite/clippy.rs b/tests/testsuite/clippy.rs index 8348d2816ac..271f6878a02 100644 --- a/tests/testsuite/clippy.rs +++ b/tests/testsuite/clippy.rs @@ -38,3 +38,37 @@ fn clippy_force_rebuild() { .with_stderr_contains("[..]assert!(true)[..]") .run(); } + +#[cargo_test] +fn clippy_passes_args() { + if !clippy_is_available() { + return; + } + + // This is just a random clippy lint (assertions_on_constants) that + // hopefully won't change much in the future. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + "#, + ) + .file("src/lib.rs", "pub fn f() { assert!(true); }") + .build(); + + p.cargo("clippy-preview -Zunstable-options -v -- -Aclippy::assertions_on_constants") + .masquerade_as_nightly_cargo() + .with_stderr_does_not_contain("[..]assert!(true)[..]") + .run(); + + // Make sure it runs again. + p.cargo("clippy-preview -Zunstable-options -v") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") + .run(); +}