Auto merge of #7069 - yaahallo:master, r=ehuss

initial working version of cargo fix --clippy

closes #7006

Author: bors

src/bin/cargo/commands/clippy.rs

@@ -69,8 +69,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
         .into());
     }
 
-    let wrapper = util::process("clippy-driver");
-    compile_opts.build_config.rustc_wrapper = Some(wrapper);
+    let wrapper = util::process(util::config::clippy_driver());
+    compile_opts.build_config.primary_unit_rustc = Some(wrapper);
 
     ops::compile(&ws, &compile_opts)?;
     Ok(())

src/bin/cargo/commands/fix.rs

@@ -72,6 +72,15 @@ 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("Get fix suggestions from clippy instead of rustc")
+                .hidden(true)
+                .multiple(true)
+                .min_values(0)
+                .number_of_values(1),
+        )
         .after_help(
             "\
 This Cargo subcommand will automatically take rustc's suggestions from
@@ -125,6 +134,21 @@ 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")
+        .map(|s| s.split(' ').map(|s| s.to_string()).collect())
+        .or_else(|| Some(vec![]))
+        .filter(|_| 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"
+        )
+        .into());
+    }
+
     if let CompileFilter::Default { .. } = opts.filter {
         opts.filter = CompileFilter::Only {
             all_targets: true,
@@ -135,6 +159,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
             tests: FilterRule::All,
         }
     }
+
     ops::fix(
         &ws,
         &mut ops::FixOptions {
@@ -146,6 +171,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"),
+            clippy_args,
         },
     )?;
     Ok(())

src/cargo/core/compiler/build_config.rs

@@ -24,8 +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<ProcessBuilder>,
+    /// An optional override of the rustc path for primary units only
+    pub primary_unit_rustc: Option<ProcessBuilder>,
     pub rustfix_diagnostic_server: RefCell<Option<RustfixDiagnosticServer>>,
     /// Whether or not Cargo should cache compiler output on disk.
     cache_messages: bool,
@@ -98,7 +98,7 @@ 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,
         })

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<Unit<'a>, Vec<String>>,
     ) -> CargoResult<BuildContext<'a, 'cfg>> {
-        let mut rustc = config.load_global_rustc(Some(ws))?;
-        if let Some(wrapper) = &build_config.rustc_wrapper {
-            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() {

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<ProcessBuilder>,
 
     target_runner: Option<(PathBuf, Vec<String>)>,
 }
@@ -77,13 +78,20 @@ impl<'cfg> Compilation<'cfg> {
     pub fn new<'a>(bcx: &BuildContext<'a, 'cfg>) -> CargoResult<Compilation<'cfg>> {
         let mut rustc = bcx.rustc.process();
 
+        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();
+
+            if let Some(rustc) = primary_unit_rustc_process.as_mut() {
+                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(),
@@ -100,15 +108,29 @@ 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)?,
         })
     }
 
     /// See `process`.
-    pub fn rustc_process(&self, pkg: &Package, target: &Target) -> CargoResult<ProcessBuilder> {
-        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<ProcessBuilder> {
+        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()));
         }

src/cargo/core/compiler/mod.rs

@@ -180,9 +180,6 @@ fn rustc<'a, 'cfg>(
     exec: &Arc<dyn Executor>,
 ) -> CargoResult<Work> {
     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,11 @@ fn prepare_rustc<'a, 'cfg>(
     crate_types: &[&str],
     unit: &Unit<'a>,
 ) -> CargoResult<ProcessBuilder> {
-    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)?;
     base.inherit_jobserver(&cx.jobserver);
     build_base_args(cx, &mut base, unit, crate_types)?;
     build_deps_args(&mut base, cx, unit)?;

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_ARGS: &str = "__CARGO_FIX_CLIPPY_ARGS";
 
 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 clippy_args: Option<Vec<String>>,
 }
 
 pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> {
@@ -99,12 +101,38 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> {
         wrapper.env(IDIOMS_ENV, "1");
     }
 
+    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);
+        }
+
+        let clippy_args = opts
+            .clippy_args
+            .as_ref()
+            .map_or_else(String::new, |args| serde_json::to_string(&args).unwrap());
+
+        wrapper.env(CLIPPY_FIX_ARGS, clippy_args);
+    }
+
     *opts
         .compile_opts
         .build_config
         .rustfix_diagnostic_server
         .borrow_mut() = Some(RustfixDiagnosticServer::new()?);
-    opts.compile_opts.build_config.rustc_wrapper = Some(wrapper);
+
+    if let Some(server) = opts
+        .compile_opts
+        .build_config
+        .rustfix_diagnostic_server
+        .borrow()
+        .as_ref()
+    {
+        server.configure(&mut 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(())
@@ -193,18 +221,10 @@ pub fn fix_maybe_exec_rustc() -> CargoResult<bool> {
     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.
@@ -253,7 +273,6 @@ pub fn fix_maybe_exec_rustc() -> CargoResult<bool> {
     }
 
     // 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.
@@ -563,8 +582,8 @@ struct FixArgs {
     idioms: bool,
     enabled_edition: Option<String>,
     other: Vec<OsString>,
-    primary_package: bool,
     rustc: Option<PathBuf>,
+    clippy_args: Vec<String>,
 }
 
 enum PrepareFor {
@@ -582,7 +601,14 @@ 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) {
             let path = PathBuf::from(arg);
             if path.extension().and_then(|s| s.to_str()) == Some("rs") && path.exists() {
@@ -608,26 +634,30 @@ 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
     }
 
     fn apply(&self, cmd: &mut Command) {
         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);
-            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));
         }
     }
 

src/cargo/util/config.rs

@@ -1782,3 +1782,12 @@ 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")
+        .unwrap_or_else(|_| "clippy-driver".into())
+        .into()
+}

src/cargo/util/rustc.rs

@@ -67,17 +67,22 @@ impl Rustc {
     }
 
     /// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`.
-    pub fn process(&self) -> ProcessBuilder {
+    pub fn process_with(&self, path: impl AsRef<Path>) -> ProcessBuilder {
         match self.wrapper {
             Some(ref wrapper) if !wrapper.get_program().is_empty() => {
                 let mut cmd = wrapper.clone();
-                cmd.arg(&self.path);
+                cmd.arg(path.as_ref());
                 cmd
             }
-            _ => self.process_no_wrapper(),
+            _ => 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 {
+        self.process_with(&self.path)
+    }
+
     pub fn process_no_wrapper(&self) -> ProcessBuilder {
         util::process(&self.path)
     }