From c057a0665b0f700fad354de621dc49d8babea0dd Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 24 Jun 2019 16:49:41 -0700 Subject: [PATCH 01/22] initial working version of cargo fix --clippy --- src/bin/cargo/commands/fix.rs | 15 +++++++++++++++ src/cargo/ops/fix.rs | 12 +++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index 4ae59e580e3..75fcb93addf 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -72,6 +72,11 @@ pub fn cli() -> App { .long("allow-staged") .help("Fix code even if the working directory has staged changes"), ) + .arg( + Arg::with_name("clippy") + .long("clippy") + .help("(unstable) get fix suggestions from clippy instead of check"), + ) .after_help( "\ This Cargo subcommand will automatically take rustc's suggestions from @@ -125,6 +130,15 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { // code as we can. let mut opts = args.compile_options(config, mode, Some(&ws))?; + let use_clippy = args.is_present("clippy"); + + if use_clippy && !config.cli_unstable().unstable_options { + return Err(failure::format_err!( + "`cargo fix --clippy` is unstable, pass `-Z unstable-options` to enable it" + ) + .into()); + } + if let CompileFilter::Default { .. } = opts.filter { opts.filter = CompileFilter::Only { all_targets: true, @@ -146,6 +160,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { allow_no_vcs: args.is_present("allow-no-vcs"), allow_staged: args.is_present("allow-staged"), broken_code: args.is_present("broken-code"), + use_clippy }, )?; Ok(()) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index e4c1a6fabe4..19e97e8b6a1 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -63,6 +63,7 @@ const BROKEN_CODE_ENV: &str = "__CARGO_FIX_BROKEN_CODE"; const PREPARE_FOR_ENV: &str = "__CARGO_FIX_PREPARE_FOR"; const EDITION_ENV: &str = "__CARGO_FIX_EDITION"; const IDIOMS_ENV: &str = "__CARGO_FIX_IDIOMS"; +const CLIPPY_FIX_ENV: &str = "__CARGO_FIX_CLIPPY_PLZ"; pub struct FixOptions<'a> { pub edition: bool, @@ -73,6 +74,7 @@ pub struct FixOptions<'a> { pub allow_no_vcs: bool, pub allow_staged: bool, pub broken_code: bool, + pub use_clippy: bool, } pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { @@ -99,6 +101,10 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { wrapper.env(IDIOMS_ENV, "1"); } + if opts.use_clippy { + wrapper.env(CLIPPY_FIX_ENV, "1"); + } + *opts .compile_opts .build_config @@ -582,7 +588,11 @@ impl Default for PrepareFor { impl FixArgs { fn get() -> FixArgs { let mut ret = FixArgs::default(); - ret.rustc = env::args_os().nth(1).map(PathBuf::from); + if env::var(CLIPPY_FIX_ENV).is_ok() { + ret.rustc = Some(PathBuf::from("clippy-driver")); + } else { + ret.rustc = env::args_os().nth(1).map(PathBuf::from); + } for arg in env::args_os().skip(2) { let path = PathBuf::from(arg); if path.extension().and_then(|s| s.to_str()) == Some("rs") && path.exists() { From 0a943be3a661bcb52d067ffb8f822c0211578808 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 24 Jun 2019 18:53:56 -0700 Subject: [PATCH 02/22] initial attempt at argument passthrough --- src/bin/cargo/commands/fix.rs | 17 ++++++++++++++--- src/cargo/ops/fix.rs | 22 ++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index 75fcb93addf..9d983bc1a94 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -75,7 +75,16 @@ pub fn cli() -> App { .arg( Arg::with_name("clippy") .long("clippy") - .help("(unstable) get fix suggestions from clippy instead of check"), + .help("Get fix suggestions from clippy instead of check") + .hidden(true) + ) + .arg( + Arg::with_name("clippy-args") + .long("clippy-args") + .help("Args to pass through to clippy, implies --clippy") + .hidden(true) + .multiple(true) + .number_of_values(1) ) .after_help( "\ @@ -130,7 +139,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { // code as we can. let mut opts = args.compile_options(config, mode, Some(&ws))?; - let use_clippy = args.is_present("clippy"); + let clippy_args = args.values_of_lossy("clippy-args"); + let use_clippy = args.is_present("clippy") || clippy_args.is_some(); if use_clippy && !config.cli_unstable().unstable_options { return Err(failure::format_err!( @@ -160,7 +170,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { allow_no_vcs: args.is_present("allow-no-vcs"), allow_staged: args.is_present("allow-staged"), broken_code: args.is_present("broken-code"), - use_clippy + use_clippy, + clippy_args, }, )?; Ok(()) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 19e97e8b6a1..5f07eaec81c 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -64,6 +64,8 @@ const PREPARE_FOR_ENV: &str = "__CARGO_FIX_PREPARE_FOR"; const EDITION_ENV: &str = "__CARGO_FIX_EDITION"; const IDIOMS_ENV: &str = "__CARGO_FIX_IDIOMS"; const CLIPPY_FIX_ENV: &str = "__CARGO_FIX_CLIPPY_PLZ"; +const CLIPPY_FIX_ARGS: &str = "__CARGO_FIX_CLIPPY_ARGS"; +const CLIPPY_PASSHTHROUGH_SEP: &str = "__CLIPPYSEP__"; pub struct FixOptions<'a> { pub edition: bool, @@ -75,6 +77,7 @@ pub struct FixOptions<'a> { pub allow_staged: bool, pub broken_code: bool, pub use_clippy: bool, + pub clippy_args: Option>, } pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { @@ -105,6 +108,10 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { wrapper.env(CLIPPY_FIX_ENV, "1"); } + if let Some(clippy_args) = &opts.clippy_args { + wrapper.env(CLIPPY_FIX_ARGS, clippy_args.join(CLIPPY_PASSHTHROUGH_SEP)); + } + *opts .compile_opts .build_config @@ -391,6 +398,11 @@ fn rustfix_and_fix( let mut cmd = Command::new(rustc); cmd.arg("--error-format=json"); + + if !args.clippy_args.is_empty() { + cmd.args(&args.clippy_args); + } + args.apply(&mut cmd); let output = cmd .output() @@ -571,6 +583,7 @@ struct FixArgs { other: Vec, primary_package: bool, rustc: Option, + clippy_args: Vec, } enum PrepareFor { @@ -593,6 +606,14 @@ impl FixArgs { } else { ret.rustc = env::args_os().nth(1).map(PathBuf::from); } + + if let Ok(clippy_args) = env::var(CLIPPY_FIX_ARGS) { + ret.clippy_args = clippy_args + .split(CLIPPY_PASSHTHROUGH_SEP) + .map(ToString::to_string) + .collect(); + } + for arg in env::args_os().skip(2) { let path = PathBuf::from(arg); if path.extension().and_then(|s| s.to_str()) == Some("rs") && path.exists() { @@ -618,6 +639,7 @@ impl FixArgs { } else if env::var(EDITION_ENV).is_ok() { ret.prepare_for_edition = PrepareFor::Next; } + ret.idioms = env::var(IDIOMS_ENV).is_ok(); ret.primary_package = env::var("CARGO_PRIMARY_PACKAGE").is_ok(); ret From 4f64aaabc288848cc29fb7d185f20dd500039707 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 24 Jun 2019 19:04:56 -0700 Subject: [PATCH 03/22] nailed it --- src/cargo/ops/fix.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 5f07eaec81c..e939dba31f0 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -399,10 +399,6 @@ fn rustfix_and_fix( let mut cmd = Command::new(rustc); cmd.arg("--error-format=json"); - if !args.clippy_args.is_empty() { - cmd.args(&args.clippy_args); - } - args.apply(&mut cmd); let output = cmd .output() @@ -649,6 +645,11 @@ impl FixArgs { if let Some(path) = &self.file { cmd.arg(path); } + + if !self.clippy_args.is_empty() { + cmd.args(&self.clippy_args); + } + cmd.args(&self.other).arg("--cap-lints=warn"); if let Some(edition) = &self.enabled_edition { cmd.arg("--edition").arg(edition); From c4922052fc7470264f3df491213f74142ac67ebf Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 24 Jun 2019 20:58:01 -0700 Subject: [PATCH 04/22] appease cargo fmt --- src/bin/cargo/commands/fix.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index 9d983bc1a94..7040dd68257 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -76,7 +76,7 @@ pub fn cli() -> App { Arg::with_name("clippy") .long("clippy") .help("Get fix suggestions from clippy instead of check") - .hidden(true) + .hidden(true), ) .arg( Arg::with_name("clippy-args") @@ -84,7 +84,7 @@ pub fn cli() -> App { .help("Args to pass through to clippy, implies --clippy") .hidden(true) .multiple(true) - .number_of_values(1) + .number_of_values(1), ) .after_help( "\ From 6b233cbe4094560255c3e396e6b9c3a969e1d97a Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 24 Jun 2019 21:02:24 -0700 Subject: [PATCH 05/22] rename passthru arg to make it clearer the clippy args passthrough only allows one arg per instance of the flag and does not resplit on spaces, as such it should be more obvious to the user that you need to specify the flag once per clippy argument. --- src/bin/cargo/commands/fix.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index 7040dd68257..e3c3f1ae7d9 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -79,8 +79,8 @@ pub fn cli() -> App { .hidden(true), ) .arg( - Arg::with_name("clippy-args") - .long("clippy-args") + Arg::with_name("clippy-arg") + .long("clippy-arg") .help("Args to pass through to clippy, implies --clippy") .hidden(true) .multiple(true) From 3a9ac3ee00c14c795e5f61eefe842a81848246a3 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 24 Jun 2019 21:53:11 -0700 Subject: [PATCH 06/22] initial clippy tests --- tests/testsuite/cache_messages.rs | 38 --------------- tests/testsuite/clippy.rs | 79 +++++++++++++++++++++++++++++++ tests/testsuite/main.rs | 1 + 3 files changed, 80 insertions(+), 38 deletions(-) create mode 100644 tests/testsuite/clippy.rs diff --git a/tests/testsuite/cache_messages.rs b/tests/testsuite/cache_messages.rs index 79959ed1e80..36072867ffe 100644 --- a/tests/testsuite/cache_messages.rs +++ b/tests/testsuite/cache_messages.rs @@ -239,44 +239,6 @@ fn rustdoc() { assert_eq!(as_str(&rustdoc_output.stderr), rustdoc_stderr); } -#[cargo_test] -fn clippy() { - if !is_nightly() { - // --json-rendered is unstable - return; - } - if let Err(e) = process("clippy-driver").arg("-V").exec_with_output() { - eprintln!("clippy-driver not available, skipping clippy test"); - eprintln!("{:?}", e); - return; - } - - // Caching clippy output. - // This is just a random clippy lint (assertions_on_constants) that - // hopefully won't change much in the future. - let p = project() - .file("src/lib.rs", "pub fn f() { assert!(true); }") - .build(); - - p.cargo("clippy-preview -Zunstable-options -Zcache-messages") - .masquerade_as_nightly_cargo() - .with_stderr_contains("[..]assert!(true)[..]") - .run(); - - // Again, reading from the cache. - p.cargo("clippy-preview -Zunstable-options -Zcache-messages") - .masquerade_as_nightly_cargo() - .with_stderr_contains("[..]assert!(true)[..]") - .run(); - - // FIXME: Unfortunately clippy is sharing the same hash with check. This - // causes the cache to be reused when it shouldn't. - p.cargo("check -Zcache-messages") - .masquerade_as_nightly_cargo() - .with_stderr_contains("[..]assert!(true)[..]") // This should not be here. - .run(); -} - #[cargo_test] fn fix() { if !is_nightly() { diff --git a/tests/testsuite/clippy.rs b/tests/testsuite/clippy.rs new file mode 100644 index 00000000000..d4d935176e3 --- /dev/null +++ b/tests/testsuite/clippy.rs @@ -0,0 +1,79 @@ +use crate::support::{is_nightly, process, project}; + +#[cargo_test] +fn clippy() { + if !is_nightly() { + // --json-rendered is unstable + eprintln!("skipping test: requires nightly"); + return; + } + if let Err(e) = process("clippy-driver").arg("-V").exec_with_output() { + eprintln!("clippy-driver not available, skipping clippy test"); + eprintln!("{:?}", e); + return; + } + + // Caching clippy output. + // This is just a random clippy lint (assertions_on_constants) that + // hopefully won't change much in the future. + let p = project() + .file("src/lib.rs", "pub fn f() { assert!(true); }") + .build(); + + p.cargo("clippy-preview -Zunstable-options -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") + .run(); + + // Again, reading from the cache. + p.cargo("clippy-preview -Zunstable-options -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") + .run(); + + // FIXME: Unfortunately clippy is sharing the same hash with check. This + // causes the cache to be reused when it shouldn't. + p.cargo("check -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") // This should not be here. + .run(); +} + +#[cargo_test] +fn fix_with_clippy() { + if !is_nightly() { + // fix --clippy is unstable + eprintln!("skipping test: requires nightly"); + return; + } + + if let Err(e) = process("clippy-driver").arg("-V").exec_with_output() { + eprintln!("clippy-driver not available, skipping clippy test"); + eprintln!("{:?}", e); + return; + } + + let p = project() + .file( + "src/lib.rs", + " + pub fn foo() { + let mut v = Vec::::new(); + let _ = v.iter_mut().filter(|&ref a| a.is_empty()); + } + ", + ) + .build(); + + let stderr = "\ +[CHECKING] foo v0.0.1 ([..]) +[FIXING] src/lib.rs (1 fix) +[FINISHED] [..] +"; + + p.cargo("fix -Zunstable-options --clippy --allow-no-vcs") + .masquerade_as_nightly_cargo() + .with_stderr(stderr) + .with_stdout("") + .run(); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index c1a7f06f0dd..618c92ceb23 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -29,6 +29,7 @@ mod cargo_features; mod cfg; mod check; mod clean; +mod clippy; mod collisions; mod concurrent; mod config; From 65afb3f6c33adbcd3cbf5209a301b9be9b0e645d Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 24 Jun 2019 22:04:24 -0700 Subject: [PATCH 07/22] use json instead of shitty hack --- src/cargo/ops/fix.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index e939dba31f0..f3e259ddd6e 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -65,7 +65,6 @@ const EDITION_ENV: &str = "__CARGO_FIX_EDITION"; const IDIOMS_ENV: &str = "__CARGO_FIX_IDIOMS"; const CLIPPY_FIX_ENV: &str = "__CARGO_FIX_CLIPPY_PLZ"; const CLIPPY_FIX_ARGS: &str = "__CARGO_FIX_CLIPPY_ARGS"; -const CLIPPY_PASSHTHROUGH_SEP: &str = "__CLIPPYSEP__"; pub struct FixOptions<'a> { pub edition: bool, @@ -109,7 +108,8 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { } if let Some(clippy_args) = &opts.clippy_args { - wrapper.env(CLIPPY_FIX_ARGS, clippy_args.join(CLIPPY_PASSHTHROUGH_SEP)); + let clippy_args = serde_json::to_string(&clippy_args).unwrap(); + wrapper.env(CLIPPY_FIX_ARGS, clippy_args); } *opts @@ -604,10 +604,7 @@ impl FixArgs { } if let Ok(clippy_args) = env::var(CLIPPY_FIX_ARGS) { - ret.clippy_args = clippy_args - .split(CLIPPY_PASSHTHROUGH_SEP) - .map(ToString::to_string) - .collect(); + ret.clippy_args = serde_json::from_str(&clippy_args).unwrap(); } for arg in env::args_os().skip(2) { From ad909aca502572d9566f940205d4f776ee00b6f8 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 25 Jun 2019 14:44:09 -0700 Subject: [PATCH 08/22] add warning to cargo incase clippy is missing This is only a warning because rustup(?) already warns and is much more verbose about how to recover from the error, so I would like to call into the clippy-driver shim or w/e is outputting nice error messages. But incase rustup is not available or some other shenanigans occur, cargo will also output the warning so the error isn't a mystery. --- src/bin/cargo/commands/fix.rs | 1 + src/cargo/ops/fix.rs | 3 +++ 2 files changed, 4 insertions(+) diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index e3c3f1ae7d9..28a627f032e 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -159,6 +159,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { tests: FilterRule::All, } } + ops::fix( &ws, &mut ops::FixOptions { diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index f3e259ddd6e..56edc487150 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -104,6 +104,9 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { } if opts.use_clippy { + if let Err(e) = util::process("clippy-driver").arg("-V").exec_with_output() { + eprintln!("Warning: clippy-driver not found: {:?}", e); + } wrapper.env(CLIPPY_FIX_ENV, "1"); } From d076f44d73658e310faa33f65ac5c23ea49acce3 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 25 Jun 2019 14:46:06 -0700 Subject: [PATCH 09/22] allow clippy path override via env variable --- src/bin/cargo/commands/clippy.rs | 2 +- src/cargo/ops/fix.rs | 2 +- src/cargo/util/config.rs | 10 ++++++++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs index 44054ae9c37..18a98cb34f5 100644 --- a/src/bin/cargo/commands/clippy.rs +++ b/src/bin/cargo/commands/clippy.rs @@ -69,7 +69,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { .into()); } - let wrapper = util::process("clippy-driver"); + let wrapper = util::process(util::config::clippy_driver()); compile_opts.build_config.rustc_wrapper = Some(wrapper); ops::compile(&ws, &compile_opts)?; diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 56edc487150..4755c8144bf 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -601,7 +601,7 @@ impl FixArgs { fn get() -> FixArgs { let mut ret = FixArgs::default(); if env::var(CLIPPY_FIX_ENV).is_ok() { - ret.rustc = Some(PathBuf::from("clippy-driver")); + ret.rustc = Some(util::config::clippy_driver()); } else { ret.rustc = env::args_os().nth(1).map(PathBuf::from); } diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 54ca624c805..38b538dbda3 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -1784,3 +1784,13 @@ impl Drop for PackageCacheLock<'_> { } } } + +/// returns path to clippy-driver binary +/// +/// Allows override of the path via `CARGO_CLIPPY_DRIVER` env variable +pub fn clippy_driver() -> PathBuf { + env::var("CARGO_CLIPPY_DRIVER") + .ok() + .unwrap_or_else(|| "clippy_driver".into()) + .into() +} From 4ff4c1f810694093bb666afcbcc4908dd5c19863 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 25 Jun 2019 18:40:35 -0700 Subject: [PATCH 10/22] Update src/bin/cargo/commands/fix.rs Co-Authored-By: Eric Huss --- src/bin/cargo/commands/fix.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index 28a627f032e..d0d7813b339 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -75,7 +75,7 @@ pub fn cli() -> App { .arg( Arg::with_name("clippy") .long("clippy") - .help("Get fix suggestions from clippy instead of check") + .help("Get fix suggestions from clippy instead of rustc") .hidden(true), ) .arg( From 85ba8e99b0d45e9f14f6d0357cb9ba287cb0b523 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 25 Jun 2019 18:42:26 -0700 Subject: [PATCH 11/22] Update src/cargo/util/config.rs Co-Authored-By: Eric Huss --- src/cargo/util/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 38b538dbda3..a9b1fc35027 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -1791,6 +1791,6 @@ impl Drop for PackageCacheLock<'_> { pub fn clippy_driver() -> PathBuf { env::var("CARGO_CLIPPY_DRIVER") .ok() - .unwrap_or_else(|| "clippy_driver".into()) + .unwrap_or_else(|| "clippy-driver".into()) .into() } From 6fb65f1e4f653ccbbc38bede4e21c122de2d426d Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Wed, 26 Jun 2019 10:30:24 -0700 Subject: [PATCH 12/22] some changes, ill clean this up in a hot sec --- src/cargo/ops/fix.rs | 4 +-- src/cargo/util/config.rs | 3 +-- tests/testsuite/clippy.rs | 46 +++------------------------------- tests/testsuite/fix.rs | 38 ++++++++++++++++++++++++++++ tests/testsuite/support/mod.rs | 10 ++++++++ 5 files changed, 53 insertions(+), 48 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 4755c8144bf..6ff7d0a66ef 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -75,7 +75,6 @@ pub struct FixOptions<'a> { pub allow_no_vcs: bool, pub allow_staged: bool, pub broken_code: bool, - pub use_clippy: bool, pub clippy_args: Option>, } @@ -103,7 +102,7 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { wrapper.env(IDIOMS_ENV, "1"); } - if opts.use_clippy { + if opts.clippy_args.is_some() { if let Err(e) = util::process("clippy-driver").arg("-V").exec_with_output() { eprintln!("Warning: clippy-driver not found: {:?}", e); } @@ -401,7 +400,6 @@ fn rustfix_and_fix( let mut cmd = Command::new(rustc); cmd.arg("--error-format=json"); - args.apply(&mut cmd); let output = cmd .output() diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index a9b1fc35027..a4eea73aefc 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -1790,7 +1790,6 @@ impl Drop for PackageCacheLock<'_> { /// Allows override of the path via `CARGO_CLIPPY_DRIVER` env variable pub fn clippy_driver() -> PathBuf { env::var("CARGO_CLIPPY_DRIVER") - .ok() - .unwrap_or_else(|| "clippy-driver".into()) + .unwrap_or_else(|_| "clippy_driver".into()) .into() } diff --git a/tests/testsuite/clippy.rs b/tests/testsuite/clippy.rs index d4d935176e3..9e0879668cc 100644 --- a/tests/testsuite/clippy.rs +++ b/tests/testsuite/clippy.rs @@ -1,4 +1,4 @@ -use crate::support::{is_nightly, process, project}; +use crate::support::{clippy_is_available, is_nightly, process, project}; #[cargo_test] fn clippy() { @@ -7,9 +7,8 @@ fn clippy() { eprintln!("skipping test: requires nightly"); return; } - if let Err(e) = process("clippy-driver").arg("-V").exec_with_output() { - eprintln!("clippy-driver not available, skipping clippy test"); - eprintln!("{:?}", e); + + if !clippy_is_available() { return; } @@ -38,42 +37,3 @@ fn clippy() { .with_stderr_contains("[..]assert!(true)[..]") // This should not be here. .run(); } - -#[cargo_test] -fn fix_with_clippy() { - if !is_nightly() { - // fix --clippy is unstable - eprintln!("skipping test: requires nightly"); - return; - } - - if let Err(e) = process("clippy-driver").arg("-V").exec_with_output() { - eprintln!("clippy-driver not available, skipping clippy test"); - eprintln!("{:?}", e); - return; - } - - let p = project() - .file( - "src/lib.rs", - " - pub fn foo() { - let mut v = Vec::::new(); - let _ = v.iter_mut().filter(|&ref a| a.is_empty()); - } - ", - ) - .build(); - - let stderr = "\ -[CHECKING] foo v0.0.1 ([..]) -[FIXING] src/lib.rs (1 fix) -[FINISHED] [..] -"; - - p.cargo("fix -Zunstable-options --clippy --allow-no-vcs") - .masquerade_as_nightly_cargo() - .with_stderr(stderr) - .with_stdout("") - .run(); -} diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 748d1df9b8d..a475494c9fd 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -1285,3 +1285,41 @@ fn fix_in_existing_repo_weird_ignore() { .run(); p.cargo("fix").cwd("src").run(); } + +#[cargo_test] +fn fix_with_clippy() { + if !is_nightly() { + // fix --clippy is unstable + eprintln!("skipping test: requires nightly"); + return; + } + + if !clippy_is_available() { + return; + } + + let p = project() + .file( + "src/lib.rs", + " + pub fn foo() { + let mut v = Vec::::new(); + let _ = v.iter_mut().filter(|&ref a| a.is_empty()); + } + ", + ) + .build(); + + let stderr = "\ +[CHECKING] foo v0.0.1 ([..]) +[FIXING] src/lib.rs (1 fix) +[FINISHED] [..] +"; + + p.cargo("fix -Zunstable-options --clippy --allow-no-vcs") + .masquerade_as_nightly_cargo() + .diff_lines("", "", false) + .with_stderr(stderr) + .with_stdout("") + .run(); +} diff --git a/tests/testsuite/support/mod.rs b/tests/testsuite/support/mod.rs index cd09ea240ad..b695bc06650 100644 --- a/tests/testsuite/support/mod.rs +++ b/tests/testsuite/support/mod.rs @@ -1753,3 +1753,13 @@ pub fn slow_cpu_multiplier(main: u64) -> Duration { } Duration::from_secs(*SLOW_CPU_MULTIPLIER * main) } + +pub fn clippy_is_available() -> bool { + if let Err(e) = process("clippy-driver").arg("-V").exec_with_output() { + eprintln!("clippy-driver not available, skipping clippy test"); + eprintln!("{:?}", e); + false + } else { + true + } +} From 8b1f599dfde9776c0404785d5b4c85d43ab33630 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Wed, 26 Jun 2019 11:27:54 -0700 Subject: [PATCH 13/22] back in tip top shape --- src/bin/cargo/commands/fix.rs | 15 +++++---------- src/cargo/util/config.rs | 2 +- tests/testsuite/clippy.rs | 2 +- tests/testsuite/fix.rs | 13 +++++++++++-- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index d0d7813b339..22ec06395ba 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -78,14 +78,6 @@ pub fn cli() -> App { .help("Get fix suggestions from clippy instead of rustc") .hidden(true), ) - .arg( - Arg::with_name("clippy-arg") - .long("clippy-arg") - .help("Args to pass through to clippy, implies --clippy") - .hidden(true) - .multiple(true) - .number_of_values(1), - ) .after_help( "\ This Cargo subcommand will automatically take rustc's suggestions from @@ -139,7 +131,11 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { // code as we can. let mut opts = args.compile_options(config, mode, Some(&ws))?; - let clippy_args = args.values_of_lossy("clippy-args"); + let clippy_args = args + .value_of("clippy") // always yields None + .map(|s| s.split(' ').map(|s| s.to_string()).collect()) + .or_else(|| Some(vec![])); + let use_clippy = args.is_present("clippy") || clippy_args.is_some(); if use_clippy && !config.cli_unstable().unstable_options { @@ -171,7 +167,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { allow_no_vcs: args.is_present("allow-no-vcs"), allow_staged: args.is_present("allow-staged"), broken_code: args.is_present("broken-code"), - use_clippy, clippy_args, }, )?; diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index a4eea73aefc..e31a2caae27 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -1790,6 +1790,6 @@ impl Drop for PackageCacheLock<'_> { /// Allows override of the path via `CARGO_CLIPPY_DRIVER` env variable pub fn clippy_driver() -> PathBuf { env::var("CARGO_CLIPPY_DRIVER") - .unwrap_or_else(|_| "clippy_driver".into()) + .unwrap_or_else(|_| "clippy-driver".into()) .into() } diff --git a/tests/testsuite/clippy.rs b/tests/testsuite/clippy.rs index 9e0879668cc..9bc7e1710c3 100644 --- a/tests/testsuite/clippy.rs +++ b/tests/testsuite/clippy.rs @@ -1,4 +1,4 @@ -use crate::support::{clippy_is_available, is_nightly, process, project}; +use crate::support::{clippy_is_available, is_nightly, project}; #[cargo_test] fn clippy() { diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index a475494c9fd..9ecca5e4526 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -3,7 +3,7 @@ use std::fs::File; use git2; use crate::support::git; -use crate::support::{basic_manifest, project}; +use crate::support::{basic_manifest, clippy_is_available, is_nightly, project}; use std::io::Write; @@ -1318,8 +1318,17 @@ fn fix_with_clippy() { p.cargo("fix -Zunstable-options --clippy --allow-no-vcs") .masquerade_as_nightly_cargo() - .diff_lines("", "", false) .with_stderr(stderr) .with_stdout("") .run(); + + assert_eq!( + p.read_file("src/lib.rs"), + " + pub fn foo() { + let mut v = Vec::::new(); + let _ = v.iter_mut().filter(|a| a.is_empty()); + } + " + ); } From fefcca196bfa2a308c7d894a447fee5cd50e69fd Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Wed, 26 Jun 2019 15:15:31 -0700 Subject: [PATCH 14/22] it was still a little broken --- src/bin/cargo/commands/fix.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index 22ec06395ba..e4a5a3521bc 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -131,12 +131,13 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { // code as we can. let mut opts = args.compile_options(config, mode, Some(&ws))?; + let use_clippy = args.is_present("clippy"); + let clippy_args = args .value_of("clippy") // always yields None .map(|s| s.split(' ').map(|s| s.to_string()).collect()) - .or_else(|| Some(vec![])); - - let use_clippy = args.is_present("clippy") || clippy_args.is_some(); + .or_else(|| Some(vec![])) + .filter(|_| use_clippy); if use_clippy && !config.cli_unstable().unstable_options { return Err(failure::format_err!( From b9f3c16407d0d46cf382bf50660b0d86a8a6c158 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 9 Jul 2019 18:34:02 -0700 Subject: [PATCH 15/22] Fix --clippy arg for fix --- src/bin/cargo/commands/fix.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index e4a5a3521bc..051148e8d3e 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -76,7 +76,10 @@ pub fn cli() -> App { Arg::with_name("clippy") .long("clippy") .help("Get fix suggestions from clippy instead of rustc") - .hidden(true), + .hidden(true) + .multiple(true) + .min_values(0) + .number_of_values(1), ) .after_help( "\ @@ -134,11 +137,13 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { let use_clippy = args.is_present("clippy"); let clippy_args = args - .value_of("clippy") // always yields None + .value_of("clippy") .map(|s| s.split(' ').map(|s| s.to_string()).collect()) .or_else(|| Some(vec![])) .filter(|_| use_clippy); + // dbg!(&clippy_args, use_clippy); + if use_clippy && !config.cli_unstable().unstable_options { return Err(failure::format_err!( "`cargo fix --clippy` is unstable, pass `-Z unstable-options` to enable it" From abd91493e06fc8253f2f09540d2ee171e4dd4f2d Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 16 Jul 2019 10:52:40 -0700 Subject: [PATCH 16/22] centralize diagnostic server stuff --- src/cargo/core/compiler/compilation.rs | 5 +---- src/cargo/ops/fix.rs | 11 +++++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 99fab990ae8..439bef30a0b 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -80,10 +80,7 @@ impl<'cfg> Compilation<'cfg> { if bcx.config.extra_verbose() { rustc.display_env_vars(); } - let srv = bcx.build_config.rustfix_diagnostic_server.borrow(); - if let Some(server) = &*srv { - server.configure(&mut rustc); - } + Ok(Compilation { // TODO: deprecated; remove. native_dirs: BTreeSet::new(), diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 6ff7d0a66ef..c9b68aa26c0 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -119,6 +119,17 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { .build_config .rustfix_diagnostic_server .borrow_mut() = Some(RustfixDiagnosticServer::new()?); + + if let Some(server) = opts + .compile_opts + .build_config + .rustfix_diagnostic_server + .borrow() + .as_ref() + { + server.configure(&mut wrapper); + } + opts.compile_opts.build_config.rustc_wrapper = Some(wrapper); ops::compile(ws, &opts.compile_opts)?; From 46b48f6e1af04ab5697a84501682d6e3cdb9aa27 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 16 Jul 2019 11:13:54 -0700 Subject: [PATCH 17/22] implement primary unit rustc --- src/cargo/core/compiler/build_config.rs | 5 ++++- src/cargo/core/compiler/compilation.rs | 28 +++++++++++++++++++++++-- src/cargo/core/compiler/mod.rs | 13 ++++++++---- src/cargo/util/rustc.rs | 12 +++++++++++ 4 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index f5fbe119354..f5033bfcd39 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -1,5 +1,5 @@ use std::cell::RefCell; -use std::path::Path; +use std::path::{Path, PathBuf}; use serde::ser; @@ -26,6 +26,8 @@ pub struct BuildConfig { pub build_plan: bool, /// An optional wrapper, if any, used to wrap rustc invocations pub rustc_wrapper: Option, + /// An optional override of the rustc path for primary units only + pub primary_unit_rustc: Option, pub rustfix_diagnostic_server: RefCell>, /// Whether or not Cargo should cache compiler output on disk. cache_messages: bool, @@ -99,6 +101,7 @@ impl BuildConfig { force_rebuild: false, build_plan: false, rustc_wrapper: None, + primary_unit_rustc: None, rustfix_diagnostic_server: RefCell::new(None), cache_messages: config.cli_unstable().cache_messages, }) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 439bef30a0b..43ea31894e7 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -69,6 +69,7 @@ pub struct Compilation<'cfg> { config: &'cfg Config, rustc_process: ProcessBuilder, + primary_unit_rustc_process: Option, target_runner: Option<(PathBuf, Vec)>, } @@ -77,8 +78,17 @@ 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 + .as_ref() + .map(|primary_unit_rustc| bcx.rustc.process_with(primary_unit_rustc)); + if bcx.config.extra_verbose() { rustc.display_env_vars(); + primary_unit_rustc_process.as_mut().map(|rustc| { + rustc.display_env_vars(); + }); } Ok(Compilation { @@ -97,6 +107,7 @@ impl<'cfg> Compilation<'cfg> { rustdocflags: HashMap::new(), config: bcx.config, rustc_process: rustc, + primary_unit_rustc_process, host: bcx.host_triple().to_string(), target: bcx.target_triple().to_string(), target_runner: target_runner(bcx)?, @@ -104,8 +115,21 @@ impl<'cfg> Compilation<'cfg> { } /// See `process`. - pub fn rustc_process(&self, pkg: &Package, target: &Target) -> CargoResult { - let mut p = self.fill_env(self.rustc_process.clone(), pkg, true)?; + pub fn rustc_process( + &self, + pkg: &Package, + target: &Target, + is_primary: bool, + ) -> CargoResult { + let rustc = if is_primary { + self.primary_unit_rustc_process + .clone() + .unwrap_or_else(|| self.rustc_process.clone()) + } else { + self.rustc_process.clone() + }; + + let mut p = self.fill_env(rustc, pkg, true)?; if target.edition() != Edition::Edition2015 { p.arg(format!("--edition={}", target.edition())); } diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 628ac8bbff8..0065468f7ba 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -180,9 +180,6 @@ fn rustc<'a, 'cfg>( exec: &Arc, ) -> CargoResult { let mut rustc = prepare_rustc(cx, &unit.target.rustc_crate_types(), unit)?; - if cx.is_primary_package(unit) { - rustc.env("CARGO_PRIMARY_PACKAGE", "1"); - } let build_plan = cx.bcx.build_config.build_plan; let name = unit.pkg.name().to_string(); @@ -593,7 +590,15 @@ fn prepare_rustc<'a, 'cfg>( crate_types: &[&str], unit: &Unit<'a>, ) -> CargoResult { - let mut base = cx.compilation.rustc_process(unit.pkg, unit.target)?; + let is_primary = cx.is_primary_package(unit); + let mut base = cx + .compilation + .rustc_process(unit.pkg, unit.target, is_primary)?; + + if is_primary { + base.env("CARGO_PRIMARY_PACKAGE", "1"); + } + base.inherit_jobserver(&cx.jobserver); build_base_args(cx, &mut base, unit, crate_types)?; build_deps_args(&mut base, cx, unit)?; diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index db5abeed415..28cb1ac7ec6 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -66,6 +66,18 @@ impl Rustc { }) } + /// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`. + pub fn process_with(&self, path: impl AsRef) -> ProcessBuilder { + match self.wrapper { + Some(ref wrapper) if !wrapper.get_program().is_empty() => { + let mut cmd = wrapper.clone(); + cmd.arg(path.as_ref()); + cmd + } + _ => util::process(path.as_ref()), + } + } + /// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`. pub fn process(&self) -> ProcessBuilder { match self.wrapper { From 18180963417aa347a625b4315ce360cb2544de02 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 16 Jul 2019 11:17:45 -0700 Subject: [PATCH 18/22] deduplicate code --- src/cargo/util/rustc.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index 28cb1ac7ec6..060517f2c16 100644 --- a/src/cargo/util/rustc.rs +++ b/src/cargo/util/rustc.rs @@ -80,14 +80,7 @@ impl Rustc { /// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`. pub fn process(&self) -> ProcessBuilder { - match self.wrapper { - Some(ref wrapper) if !wrapper.get_program().is_empty() => { - let mut cmd = wrapper.clone(); - cmd.arg(&self.path); - cmd - } - _ => self.process_no_wrapper(), - } + self.process_with(&self.path) } pub fn process_no_wrapper(&self) -> ProcessBuilder { From c0c672924a3365d61645839ce0cd67ee10308644 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 16 Jul 2019 11:25:19 -0700 Subject: [PATCH 19/22] missing wiring --- src/cargo/ops/fix.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index c9b68aa26c0..81a70c63ece 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -107,6 +107,7 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { eprintln!("Warning: clippy-driver not found: {:?}", e); } wrapper.env(CLIPPY_FIX_ENV, "1"); + opts.compile_opts.build_config.primary_unit_rustc = Some(util::config::clippy_driver()); } if let Some(clippy_args) = &opts.clippy_args { @@ -609,11 +610,7 @@ impl Default for PrepareFor { impl FixArgs { fn get() -> FixArgs { let mut ret = FixArgs::default(); - if env::var(CLIPPY_FIX_ENV).is_ok() { - ret.rustc = Some(util::config::clippy_driver()); - } else { - ret.rustc = env::args_os().nth(1).map(PathBuf::from); - } + ret.rustc = env::args_os().nth(1).map(PathBuf::from); if let Ok(clippy_args) = env::var(CLIPPY_FIX_ARGS) { ret.clippy_args = serde_json::from_str(&clippy_args).unwrap(); From f3d3318f9f5f66b09e7ebc3b0634893c21d099e9 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Thu, 18 Jul 2019 13:52:28 -0700 Subject: [PATCH 20/22] minor fixes --- src/bin/cargo/commands/fix.rs | 2 -- src/cargo/ops/fix.rs | 11 ++++----- tests/testsuite/cache_messages.rs | 40 ++++++++++++++++++++++++++++++- tests/testsuite/clippy.rs | 39 ------------------------------ tests/testsuite/main.rs | 1 - 5 files changed, 44 insertions(+), 49 deletions(-) delete mode 100644 tests/testsuite/clippy.rs diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs index 051148e8d3e..471d05a4c95 100644 --- a/src/bin/cargo/commands/fix.rs +++ b/src/bin/cargo/commands/fix.rs @@ -142,8 +142,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { .or_else(|| Some(vec![])) .filter(|_| use_clippy); - // dbg!(&clippy_args, use_clippy); - if use_clippy && !config.cli_unstable().unstable_options { return Err(failure::format_err!( "`cargo fix --clippy` is unstable, pass `-Z unstable-options` to enable it" diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 81a70c63ece..6643eba6930 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -63,7 +63,6 @@ const BROKEN_CODE_ENV: &str = "__CARGO_FIX_BROKEN_CODE"; const PREPARE_FOR_ENV: &str = "__CARGO_FIX_PREPARE_FOR"; const EDITION_ENV: &str = "__CARGO_FIX_EDITION"; const IDIOMS_ENV: &str = "__CARGO_FIX_IDIOMS"; -const CLIPPY_FIX_ENV: &str = "__CARGO_FIX_CLIPPY_PLZ"; const CLIPPY_FIX_ARGS: &str = "__CARGO_FIX_CLIPPY_ARGS"; pub struct FixOptions<'a> { @@ -106,13 +105,13 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { if let Err(e) = util::process("clippy-driver").arg("-V").exec_with_output() { eprintln!("Warning: clippy-driver not found: {:?}", e); } - wrapper.env(CLIPPY_FIX_ENV, "1"); + opts.compile_opts.build_config.primary_unit_rustc = Some(util::config::clippy_driver()); - } - if let Some(clippy_args) = &opts.clippy_args { - let clippy_args = serde_json::to_string(&clippy_args).unwrap(); - wrapper.env(CLIPPY_FIX_ARGS, clippy_args); + if let Some(clippy_args) = &opts.clippy_args { + let clippy_args = serde_json::to_string(&clippy_args).unwrap(); + wrapper.env(CLIPPY_FIX_ARGS, clippy_args); + } } *opts diff --git a/tests/testsuite/cache_messages.rs b/tests/testsuite/cache_messages.rs index 36072867ffe..b283a285f4e 100644 --- a/tests/testsuite/cache_messages.rs +++ b/tests/testsuite/cache_messages.rs @@ -1,4 +1,4 @@ -use crate::support::{is_nightly, process, project, registry::Package}; +use crate::support::{clippy_is_available, is_nightly, process, project, registry::Package}; use std::path::Path; fn as_str(bytes: &[u8]) -> &str { @@ -255,6 +255,44 @@ fn fix() { assert_eq!(p.read_file("src/lib.rs"), "pub fn r#try() {}"); } +#[cargo_test] +fn clippy() { + if !is_nightly() { + // --json-rendered is unstable + eprintln!("skipping test: requires nightly"); + return; + } + + if !clippy_is_available() { + return; + } + + // Caching clippy output. + // This is just a random clippy lint (assertions_on_constants) that + // hopefully won't change much in the future. + let p = project() + .file("src/lib.rs", "pub fn f() { assert!(true); }") + .build(); + + p.cargo("clippy-preview -Zunstable-options -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") + .run(); + + // Again, reading from the cache. + p.cargo("clippy-preview -Zunstable-options -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") + .run(); + + // FIXME: Unfortunately clippy is sharing the same hash with check. This + // causes the cache to be reused when it shouldn't. + p.cargo("check -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") // This should not be here. + .run(); +} + #[cargo_test] fn very_verbose() { if !is_nightly() { diff --git a/tests/testsuite/clippy.rs b/tests/testsuite/clippy.rs deleted file mode 100644 index 9bc7e1710c3..00000000000 --- a/tests/testsuite/clippy.rs +++ /dev/null @@ -1,39 +0,0 @@ -use crate::support::{clippy_is_available, is_nightly, project}; - -#[cargo_test] -fn clippy() { - if !is_nightly() { - // --json-rendered is unstable - eprintln!("skipping test: requires nightly"); - return; - } - - if !clippy_is_available() { - return; - } - - // Caching clippy output. - // This is just a random clippy lint (assertions_on_constants) that - // hopefully won't change much in the future. - let p = project() - .file("src/lib.rs", "pub fn f() { assert!(true); }") - .build(); - - p.cargo("clippy-preview -Zunstable-options -Zcache-messages") - .masquerade_as_nightly_cargo() - .with_stderr_contains("[..]assert!(true)[..]") - .run(); - - // Again, reading from the cache. - p.cargo("clippy-preview -Zunstable-options -Zcache-messages") - .masquerade_as_nightly_cargo() - .with_stderr_contains("[..]assert!(true)[..]") - .run(); - - // FIXME: Unfortunately clippy is sharing the same hash with check. This - // causes the cache to be reused when it shouldn't. - p.cargo("check -Zcache-messages") - .masquerade_as_nightly_cargo() - .with_stderr_contains("[..]assert!(true)[..]") // This should not be here. - .run(); -} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 618c92ceb23..c1a7f06f0dd 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -29,7 +29,6 @@ mod cargo_features; mod cfg; mod check; mod clean; -mod clippy; mod collisions; mod concurrent; mod config; From 03feb61f7567fe8905d1d3247fb12369f3d80b59 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Thu, 18 Jul 2019 15:52:05 -0700 Subject: [PATCH 21/22] apply the correct changes --- src/bin/cargo/commands/clippy.rs | 2 +- src/cargo/core/compiler/build_config.rs | 7 +--- src/cargo/core/compiler/build_context/mod.rs | 2 +- src/cargo/core/compiler/compilation.rs | 15 +++---- src/cargo/core/compiler/mod.rs | 6 +-- src/cargo/ops/fix.rs | 42 ++++++++------------ 6 files changed, 30 insertions(+), 44 deletions(-) diff --git a/src/bin/cargo/commands/clippy.rs b/src/bin/cargo/commands/clippy.rs index 18a98cb34f5..8e838389b0f 100644 --- a/src/bin/cargo/commands/clippy.rs +++ b/src/bin/cargo/commands/clippy.rs @@ -70,7 +70,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { } let wrapper = util::process(util::config::clippy_driver()); - compile_opts.build_config.rustc_wrapper = Some(wrapper); + compile_opts.build_config.primary_unit_rustc = Some(wrapper); ops::compile(&ws, &compile_opts)?; Ok(()) diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index f5033bfcd39..7f795c442cd 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -1,5 +1,5 @@ use std::cell::RefCell; -use std::path::{Path, PathBuf}; +use std::path::Path; use serde::ser; @@ -24,10 +24,8 @@ pub struct BuildConfig { pub force_rebuild: bool, /// Output a build plan to stdout instead of actually compiling. pub build_plan: bool, - /// An optional wrapper, if any, used to wrap rustc invocations - pub rustc_wrapper: Option, /// 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, @@ -100,7 +98,6 @@ impl BuildConfig { message_format: MessageFormat::Human, force_rebuild: false, build_plan: false, - rustc_wrapper: None, primary_unit_rustc: None, rustfix_diagnostic_server: RefCell::new(None), cache_messages: config.cli_unstable().cache_messages, diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index 46c6b4b2cc1..1a5c660605e 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -52,7 +52,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { extra_compiler_args: HashMap, Vec>, ) -> CargoResult> { let mut rustc = config.load_global_rustc(Some(ws))?; - if let Some(wrapper) = &build_config.rustc_wrapper { + if let Some(wrapper) = &build_config.primary_unit_rustc { rustc.set_wrapper(wrapper.clone()); } diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 43ea31894e7..5f54818b6d9 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -78,17 +78,18 @@ 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 - .as_ref() - .map(|primary_unit_rustc| bcx.rustc.process_with(primary_unit_rustc)); + let mut primary_unit_rustc_process = + bcx.build_config.primary_unit_rustc.clone().map(|mut r| { + r.arg(&bcx.rustc.path); + r + }); if bcx.config.extra_verbose() { rustc.display_env_vars(); - primary_unit_rustc_process.as_mut().map(|rustc| { + + if let Some(rustc) = primary_unit_rustc_process.as_mut() { rustc.display_env_vars(); - }); + } } Ok(Compilation { diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 0065468f7ba..eb55e0659af 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -591,14 +591,10 @@ fn prepare_rustc<'a, 'cfg>( unit: &Unit<'a>, ) -> CargoResult { let is_primary = cx.is_primary_package(unit); + let mut base = cx .compilation .rustc_process(unit.pkg, unit.target, is_primary)?; - - if is_primary { - base.env("CARGO_PRIMARY_PACKAGE", "1"); - } - base.inherit_jobserver(&cx.jobserver); build_base_args(cx, &mut base, unit, crate_types)?; build_deps_args(&mut base, cx, unit)?; diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 6643eba6930..1ac31974b72 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -106,12 +106,12 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { eprintln!("Warning: clippy-driver not found: {:?}", e); } - opts.compile_opts.build_config.primary_unit_rustc = Some(util::config::clippy_driver()); + let clippy_args = opts + .clippy_args + .as_ref() + .map_or_else(String::new, |args| serde_json::to_string(&args).unwrap()); - if let Some(clippy_args) = &opts.clippy_args { - let clippy_args = serde_json::to_string(&clippy_args).unwrap(); - wrapper.env(CLIPPY_FIX_ARGS, clippy_args); - } + wrapper.env(CLIPPY_FIX_ARGS, clippy_args); } *opts @@ -130,7 +130,9 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> { server.configure(&mut wrapper); } - opts.compile_opts.build_config.rustc_wrapper = Some(wrapper); + // 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); ops::compile(ws, &opts.compile_opts)?; Ok(()) @@ -219,18 +221,10 @@ pub fn fix_maybe_exec_rustc() -> CargoResult { trace!("cargo-fix as rustc got file {:?}", args.file); let rustc = args.rustc.as_ref().expect("fix wrapper rustc was not set"); - // Our goal is to fix only the crates that the end user is interested in. - // That's very likely to only mean the crates in the workspace the user is - // working on, not random crates.io crates. - // - // The master cargo process tells us whether or not this is a "primary" - // crate via the CARGO_PRIMARY_PACKAGE environment variable. let mut fixes = FixedCrate::default(); if let Some(path) = &args.file { - if args.primary_package { - trace!("start rustfixing {:?}", path); - fixes = rustfix_crate(&lock_addr, rustc.as_ref(), path, &args)?; - } + trace!("start rustfixing {:?}", path); + fixes = rustfix_crate(&lock_addr, rustc.as_ref(), path, &args)?; } // Ok now we have our final goal of testing out the changes that we applied. @@ -279,7 +273,6 @@ pub fn fix_maybe_exec_rustc() -> CargoResult { } // This final fall-through handles multiple cases; - // - Non-primary crates, which need to be built. // - If the fix failed, show the original warnings and suggestions. // - If `--broken-code`, show the error messages. // - If the fix succeeded, show any remaining warnings. @@ -589,7 +582,6 @@ struct FixArgs { idioms: bool, enabled_edition: Option, other: Vec, - primary_package: bool, rustc: Option, clippy_args: Vec, } @@ -609,10 +601,12 @@ impl Default for PrepareFor { impl FixArgs { fn get() -> FixArgs { let mut ret = FixArgs::default(); - ret.rustc = env::args_os().nth(1).map(PathBuf::from); if let Ok(clippy_args) = env::var(CLIPPY_FIX_ARGS) { ret.clippy_args = serde_json::from_str(&clippy_args).unwrap(); + ret.rustc = Some(util::config::clippy_driver()); + } else { + ret.rustc = env::args_os().nth(1).map(PathBuf::from); } for arg in env::args_os().skip(2) { @@ -642,7 +636,6 @@ impl FixArgs { } ret.idioms = env::var(IDIOMS_ENV).is_ok(); - ret.primary_package = env::var("CARGO_PRIMARY_PACKAGE").is_ok(); ret } @@ -658,14 +651,13 @@ impl FixArgs { cmd.args(&self.other).arg("--cap-lints=warn"); if let Some(edition) = &self.enabled_edition { cmd.arg("--edition").arg(edition); - if self.idioms && self.primary_package && edition == "2018" { + if self.idioms && edition == "2018" { cmd.arg("-Wrust-2018-idioms"); } } - if self.primary_package { - if let Some(edition) = self.prepare_for_edition_resolve() { - cmd.arg("-W").arg(format!("rust-{}-compatibility", edition)); - } + + if let Some(edition) = self.prepare_for_edition_resolve() { + cmd.arg("-W").arg(format!("rust-{}-compatibility", edition)); } } From 22b430d4561d6a9fdb58b82cc70a6f677287dc6f Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Fri, 19 Jul 2019 10:48:33 -0700 Subject: [PATCH 22/22] dont overuse the wrapper --- src/cargo/core/compiler/build_context/mod.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index 1a5c660605e..324c052ad24 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -51,10 +51,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { units: &'a UnitInterner<'a>, extra_compiler_args: HashMap, Vec>, ) -> CargoResult> { - let mut rustc = config.load_global_rustc(Some(ws))?; - if let Some(wrapper) = &build_config.primary_unit_rustc { - rustc.set_wrapper(wrapper.clone()); - } + let rustc = config.load_global_rustc(Some(ws))?; let host_config = TargetConfig::new(config, &rustc.host)?; let target_config = match build_config.requested_target.as_ref() {