From a86ce67cb9c062e59430c72b3c106a0fd394d94e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Sat, 5 May 2018 10:31:03 -0700 Subject: [PATCH] Start giving `cargo fix` a dedicated CLI This commit pulls in `clap` to have a location to actually insert a CLI for `cargo fix` itself. This'll hopefully allow us to continue to configure cargo-fix's behavior while also allowing passing arguments down to Cargo. This starts out by adding a `--broken-code` option which can be used to specify that previously broken code should be fixed, despite it being originally broken. Closes #70 Closes #71 Closes #84 --- cargo-fix/Cargo.toml | 1 + cargo-fix/src/cli.rs | 71 +++++++++++++++++++++++++++++ cargo-fix/src/main.rs | 42 ++--------------- cargo-fix/tests/all/broken_build.rs | 19 ++++++++ cargo-fix/tests/all/subtargets.rs | 4 +- 5 files changed, 97 insertions(+), 40 deletions(-) create mode 100644 cargo-fix/src/cli.rs diff --git a/cargo-fix/Cargo.toml b/cargo-fix/Cargo.toml index 82e9b984891..18572dcd09e 100644 --- a/cargo-fix/Cargo.toml +++ b/cargo-fix/Cargo.toml @@ -18,6 +18,7 @@ rustfix = { path = "..", version = "0.2" } serde_json = "1" log = "0.4" env_logger = { version = "0.5", default-features = false } +clap = "2.31" [dev-dependencies] difference = "2" diff --git a/cargo-fix/src/cli.rs b/cargo-fix/src/cli.rs new file mode 100644 index 00000000000..37ec8593304 --- /dev/null +++ b/cargo-fix/src/cli.rs @@ -0,0 +1,71 @@ +use std::env; +use std::process::Command; + +use failure::{Error, ResultExt}; +use clap::{Arg, App, SubCommand, AppSettings}; + +use lock::Server; +use super::exit_with; + +pub fn run() -> Result<(), Error> { + let matches = App::new("Cargo Fix") + .bin_name("cargo") + .subcommand( + SubCommand::with_name("fix") + .version(env!("CARGO_PKG_VERSION")) + .author("The Rust Project Developers") + .about("Automatically apply rustc's suggestions about fixing code") + .setting(AppSettings::TrailingVarArg) + .arg(Arg::with_name("args").multiple(true)) + .arg( + Arg::with_name("broken-code") + .long("broken-code") + .help("Fix code even if it already has compiler errors") + ) + ) + .get_matches(); + let matches = match matches.subcommand() { + ("fix", Some(matches)) => matches, + _ => bail!("unknown cli arguments passed"), + }; + + if matches.is_present("broken-code") { + env::set_var("__CARGO_FIX_BROKEN_CODE", "1"); + } + + // Spin up our lock server which our subprocesses will use to synchronize + // fixes. + let _lockserver = Server::new()?.start()?; + + let cargo = env::var_os("CARGO").unwrap_or("cargo".into()); + let mut cmd = Command::new(&cargo); + // TODO: shouldn't hardcode `check` here, we want to allow things like + // `cargo fix bench` or something like that + // + // TODO: somehow we need to force `check` to actually do something here, if + // `cargo check` was previously run it won't actually do anything again. + cmd.arg("check"); + if let Some(args) = matches.values_of("args") { + cmd.args(args); + } + + // Override the rustc compiler as ourselves. That way whenever rustc would + // run we run instead and have an opportunity to inject fixes. + let me = env::current_exe() + .context("failed to learn about path to current exe")?; + cmd.env("RUSTC", &me) + .env("__CARGO_FIX_NOW_RUSTC", "1"); + if let Some(rustc) = env::var_os("RUSTC") { + cmd.env("RUSTC_ORIGINAL", rustc); + } + + // An now execute all of Cargo! This'll fix everything along the way. + // + // TODO: we probably want to do something fancy here like collect results + // from the client processes and print out a summary of what happened. + let status = cmd.status() + .with_context(|e| { + format!("failed to execute `{}`: {}", cargo.to_string_lossy(), e) + })?; + exit_with(status); +} diff --git a/cargo-fix/src/main.rs b/cargo-fix/src/main.rs index c8f128e3860..e9bd41cb222 100644 --- a/cargo-fix/src/main.rs +++ b/cargo-fix/src/main.rs @@ -1,3 +1,4 @@ +extern crate clap; #[macro_use] extern crate failure; extern crate rustfix; @@ -17,6 +18,7 @@ use std::path::Path; use rustfix::diagnostics::Diagnostic; use failure::{Error, ResultExt}; +mod cli; mod lock; fn main() { @@ -24,7 +26,7 @@ fn main() { let result = if env::var("__CARGO_FIX_NOW_RUSTC").is_ok() { cargo_fix_rustc() } else { - cargo_fix() + cli::run() }; let err = match result { Ok(()) => return, @@ -37,42 +39,6 @@ fn main() { process::exit(102); } -fn cargo_fix() -> Result<(), Error> { - // Spin up our lock server which our subprocesses will use to synchronize - // fixes. - let _lockserver = lock::Server::new()?.start()?; - - let cargo = env::var_os("CARGO").unwrap_or("cargo".into()); - let mut cmd = Command::new(&cargo); - // TODO: shouldn't hardcode `check` here, we want to allow things like - // `cargo fix bench` or something like that - // - // TODO: somehow we need to force `check` to actually do something here, if - // `cargo check` was previously run it won't actually do anything again. - cmd.arg("check"); - cmd.args(env::args().skip(2)); // skip `cmd-fix fix` - - // Override the rustc compiler as ourselves. That way whenever rustc would - // run we run instead and have an opportunity to inject fixes. - let me = env::current_exe() - .context("failed to learn about path to current exe")?; - cmd.env("RUSTC", &me) - .env("__CARGO_FIX_NOW_RUSTC", "1"); - if let Some(rustc) = env::var_os("RUSTC") { - cmd.env("RUSTC_ORIGINAL", rustc); - } - - // An now execute all of Cargo! This'll fix everything along the way. - // - // TODO: we probably want to do something fancy here like collect results - // from the client processes and print out a summary of what happened. - let status = cmd.status() - .with_context(|e| { - format!("failed to execute `{}`: {}", cargo.to_string_lossy(), e) - })?; - exit_with(status); -} - fn cargo_fix_rustc() -> Result<(), Error> { // Try to figure out what we're compiling by looking for a rust-like file // that exists. @@ -163,7 +129,7 @@ fn rustfix_crate(rustc: &Path, filename: &str) -> Result // // TODO: this should be configurable by the CLI to sometimes proceed to // attempt to fix broken code. - if !output.status.success() { + if !output.status.success() && env::var_os("__CARGO_FIX_BROKEN_CODE").is_none() { return Ok(HashMap::new()) } diff --git a/cargo-fix/tests/all/broken_build.rs b/cargo-fix/tests/all/broken_build.rs index 590071eecdc..92295e83495 100644 --- a/cargo-fix/tests/all/broken_build.rs +++ b/cargo-fix/tests/all/broken_build.rs @@ -24,6 +24,25 @@ fn do_not_fix_broken_builds() { assert!(p.read("src/lib.rs").contains("let mut x = 3;")); } +#[test] +fn fix_broken_if_requested() { + let p = project() + .file( + "src/lib.rs", + r#" + fn foo(a: &u32) -> u32 { a + 1 } + pub fn bar() { + foo(1); + } + "# + ) + .build(); + + p.expect_cmd("cargo-fix fix --broken-code") + .status(0) + .run(); +} + #[test] fn broken_fixes_backed_out() { let p = project() diff --git a/cargo-fix/tests/all/subtargets.rs b/cargo-fix/tests/all/subtargets.rs index d86c743d74f..e247299685d 100644 --- a/cargo-fix/tests/all/subtargets.rs +++ b/cargo-fix/tests/all/subtargets.rs @@ -28,7 +28,7 @@ fn fixes_missing_ampersand() { [COMPILING] foo v0.1.0 (CWD) [FINISHED] dev [unoptimized + debuginfo] "; - p.expect_cmd("cargo fix --all-targets").stdout("").stderr(stderr).run(); + p.expect_cmd("cargo fix -- --all-targets").stdout("").stderr(stderr).run(); p.expect_cmd("cargo build").run(); p.expect_cmd("cargo test").run(); } @@ -57,6 +57,6 @@ fn fix_features() { p.expect_cmd("cargo fix").run(); p.expect_cmd("cargo build").run(); - p.expect_cmd("cargo fix --features bar").run(); + p.expect_cmd("cargo fix -- --features bar").run(); p.expect_cmd("cargo build --features bar").run(); }