From b02ba3771e6b4b5f5a7cb1563a94724a720ae792 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 13 Jul 2018 18:49:26 -0700 Subject: [PATCH] Import `cargo fix` directly in to Cargo This commit imports the `cargo fix` subcommand in rust-lang-nursery/rustfix directly into Cargo as a subcommand. This should allow us to ease our distribution story of `cargo fix` as we prepare for the upcoming 2018 edition release. It's been attempted here to make the code as idiomatic as possible for Cargo's own codebase. Additionally all tests from cargo-fix were imported into Cargo's test suite as well. After this lands and is published in nightly the `cargo-fix` command in rust-lang-nursery/rustfix will likely be removed. cc rust-lang/rust#52272 --- Cargo.toml | 1 + src/bin/cargo/commands/fix.rs | 117 +++ src/bin/cargo/commands/mod.rs | 3 + src/bin/cargo/main.rs | 12 +- src/cargo/core/compiler/build_config.rs | 15 +- src/cargo/core/compiler/compilation.rs | 25 +- src/cargo/core/compiler/context/mod.rs | 2 +- src/cargo/core/compiler/job_queue.rs | 19 +- src/cargo/lib.rs | 1 + src/cargo/ops/cargo_new.rs | 2 +- src/cargo/ops/fix.rs | 371 ++++++++ src/cargo/ops/mod.rs | 2 + src/cargo/util/diagnostic_server.rs | 189 ++++ src/cargo/util/lockserver.rs | 172 ++++ src/cargo/util/mod.rs | 4 + tests/testsuite/build.rs | 4 +- tests/testsuite/cargotest/support/mod.rs | 51 +- tests/testsuite/fix.rs | 1025 ++++++++++++++++++++++ tests/testsuite/generate_lockfile.rs | 8 +- tests/testsuite/main.rs | 1 + 20 files changed, 1984 insertions(+), 40 deletions(-) create mode 100644 src/bin/cargo/commands/fix.rs create mode 100644 src/cargo/ops/fix.rs create mode 100644 src/cargo/util/diagnostic_server.rs create mode 100644 src/cargo/util/lockserver.rs create mode 100644 tests/testsuite/fix.rs diff --git a/Cargo.toml b/Cargo.toml index e9d45290314..f0f5d44f96f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,6 +40,7 @@ libc = "0.2" libgit2-sys = "0.7.1" log = "0.4" num_cpus = "1.0" +rustfix = "0.4" same-file = "1" semver = { version = "0.9.0", features = ["serde"] } serde = "1.0" diff --git a/src/bin/cargo/commands/fix.rs b/src/bin/cargo/commands/fix.rs new file mode 100644 index 00000000000..f552b7d024c --- /dev/null +++ b/src/bin/cargo/commands/fix.rs @@ -0,0 +1,117 @@ +use command_prelude::*; + +use cargo::ops; + +pub fn cli() -> App { + subcommand("fix") + .about("Automatically fix lint warnings reported by rustc") + .arg_package_spec( + "Package(s) to fix", + "Fix all packages in the workspace", + "Exclude packages from the fixes", + ) + .arg_jobs() + .arg_targets_all( + "Fix only this package's library", + "Fix only the specified binary", + "Fix all binaries", + "Fix only the specified example", + "Fix all examples", + "Fix only the specified test target", + "Fix all tests", + "Fix only the specified bench target", + "Fix all benches", + "Fix all targets (lib and bin targets by default)", + ) + .arg_release("Fix artifacts in release mode, with optimizations") + .arg(opt("profile", "Profile to build the selected target for").value_name("PROFILE")) + .arg_features() + .arg_target_triple("Fix for the target triple") + .arg_target_dir() + .arg_manifest_path() + .arg_message_format() + .arg( + Arg::with_name("broken-code") + .long("broken-code") + .help("Fix code even if it already has compiler errors"), + ) + .arg( + Arg::with_name("edition") + .long("prepare-for") + .help("Fix warnings in preparation of an edition upgrade") + .takes_value(true) + .possible_values(&["2018"]), + ) + .arg( + Arg::with_name("allow-no-vcs") + .long("allow-no-vcs") + .help("Fix code even if a VCS was not detected"), + ) + .arg( + Arg::with_name("allow-dirty") + .long("allow-dirty") + .help("Fix code even if the working directory is dirty"), + ) + .after_help( + "\ +This Cargo subcommmand will automatically take rustc's suggestions from +diagnostics like warnings and apply them to your source code. This is intended +to help automate tasks that rustc itself already knows how to tell you to fix! +The `cargo fix` subcommand is also being developed for the Rust 2018 edition +to provide code the ability to easily opt-in to the new edition without having +to worry about any breakage. + +Executing `cargo fix` will under the hood execute `cargo check`. Any warnings +applicable to your crate will be automatically fixed (if possible) and all +remaining warnings will be displayed when the check process is finished. For +example if you'd like to prepare for the 2018 edition, you can do so by +executing: + + cargo fix --prepare-for 2018 + +Note that this is not guaranteed to fix all your code as it only fixes code that +`cargo check` would otherwise compile. For example unit tests are left out +from this command, but they can be checked with: + + cargo fix --prepare-for 2018 --all-targets + +which behaves the same as `cargo check --all-targets`. Similarly if you'd like +to fix code for different platforms you can do: + + cargo fix --prepare-for 2018 --target x86_64-pc-windows-gnu + +or if your crate has optional features: + + cargo fix --prepare-for 2018 --no-default-features --features foo + +If you encounter any problems with `cargo fix` or otherwise have any questions +or feature requests please don't hesitate to file an issue at +https://github.com/rust-lang/cargo +", + ) +} + +pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { + let ws = args.workspace(config)?; + let test = match args.value_of("profile") { + Some("test") => true, + None => false, + Some(profile) => { + let err = format_err!( + "unknown profile: `{}`, only `test` is \ + currently supported", + profile + ); + return Err(CliError::new(err, 101)); + } + }; + let mode = CompileMode::Check { test }; + ops::fix(&ws, &mut ops::FixOptions { + edition: args.value_of("edition"), + compile_opts: args.compile_options(config, mode)?, + allow_dirty: args.is_present("allow-dirty"), + allow_no_vcs: args.is_present("allow-no-vcs"), + broken_code: args.is_present("broken-code"), + })?; + Ok(()) +} diff --git a/src/bin/cargo/commands/mod.rs b/src/bin/cargo/commands/mod.rs index fc829a85543..057dc2f07b4 100644 --- a/src/bin/cargo/commands/mod.rs +++ b/src/bin/cargo/commands/mod.rs @@ -8,6 +8,7 @@ pub fn builtin() -> Vec { clean::cli(), doc::cli(), fetch::cli(), + fix::cli(), generate_lockfile::cli(), git_checkout::cli(), init::cli(), @@ -42,6 +43,7 @@ pub fn builtin_exec(cmd: &str) -> Option CliResu "clean" => clean::exec, "doc" => doc::exec, "fetch" => fetch::exec, + "fix" => fix::exec, "generate-lockfile" => generate_lockfile::exec, "git-checkout" => git_checkout::exec, "init" => init::exec, @@ -76,6 +78,7 @@ pub mod check; pub mod clean; pub mod doc; pub mod fetch; +pub mod fix; pub mod generate_lockfile; pub mod git_checkout; pub mod init; diff --git a/src/bin/cargo/main.rs b/src/bin/cargo/main.rs index e5338e833d6..81fdd8f8e0f 100644 --- a/src/bin/cargo/main.rs +++ b/src/bin/cargo/main.rs @@ -35,10 +35,14 @@ fn main() { } }; - let result = { - init_git_transports(&mut config); - let _token = cargo::util::job::setup(); - cli::main(&mut config) + let result = match cargo::ops::fix_maybe_exec_rustc() { + Ok(true) => Ok(()), + Ok(false) => { + init_git_transports(&mut config); + let _token = cargo::util::job::setup(); + cli::main(&mut config) + } + Err(e) => Err(CliError::from(e)), }; match result { diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index eb2a0b414a0..73b8d761af0 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -1,5 +1,7 @@ use std::path::Path; -use util::{CargoResult, CargoResultExt, Config}; +use std::cell::RefCell; + +use util::{CargoResult, CargoResultExt, Config, RustfixDiagnosticServer}; /// Configuration information for a rustc build. #[derive(Debug)] @@ -16,6 +18,13 @@ pub struct BuildConfig { pub message_format: MessageFormat, /// Output a build plan to stdout instead of actually compiling. pub build_plan: bool, + /// Use Cargo itself as the wrapper around rustc, only used for `cargo fix` + pub cargo_as_rustc_wrapper: bool, + /// Extra env vars to inject into rustc commands + pub extra_rustc_env: Vec<(String, String)>, + /// Extra args to inject into rustc commands + pub extra_rustc_args: Vec, + pub rustfix_diagnostic_server: RefCell>, } impl BuildConfig { @@ -71,6 +80,10 @@ impl BuildConfig { mode, message_format: MessageFormat::Human, build_plan: false, + cargo_as_rustc_wrapper: false, + extra_rustc_env: Vec::new(), + extra_rustc_args: Vec::new(), + rustfix_diagnostic_server: RefCell::new(None), }) } diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 28b2a6b17f2..4dfe9a99ded 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -1,4 +1,5 @@ use std::collections::{BTreeSet, HashMap, HashSet}; +use std::env; use std::ffi::OsStr; use std::path::PathBuf; @@ -80,8 +81,24 @@ pub struct Compilation<'cfg> { } impl<'cfg> Compilation<'cfg> { - pub fn new<'a>(bcx: &BuildContext<'a, 'cfg>) -> Compilation<'cfg> { - Compilation { + pub fn new<'a>(bcx: &BuildContext<'a, 'cfg>) -> CargoResult> { + let mut rustc = bcx.rustc.process(); + for (k, v) in bcx.build_config.extra_rustc_env.iter() { + rustc.env(k, v); + } + for arg in bcx.build_config.extra_rustc_args.iter() { + rustc.arg(arg); + } + if bcx.build_config.cargo_as_rustc_wrapper { + let prog = rustc.get_program().to_owned(); + rustc.env("RUSTC", prog); + rustc.program(env::current_exe()?); + } + let srv = bcx.build_config.rustfix_diagnostic_server.borrow(); + if let Some(server) = &*srv { + server.configure(&mut rustc); + } + Ok(Compilation { libraries: HashMap::new(), native_dirs: BTreeSet::new(), // TODO: deprecated, remove root_output: PathBuf::from("/"), @@ -96,11 +113,11 @@ impl<'cfg> Compilation<'cfg> { cfgs: HashMap::new(), rustdocflags: HashMap::new(), config: bcx.config, - rustc_process: bcx.rustc.process(), + rustc_process: rustc, host: bcx.host_triple().to_string(), target: bcx.target_triple().to_string(), target_runner: LazyCell::new(), - } + }) } /// See `process`. diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 0b5c77d2277..0ac5f8c9978 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -118,7 +118,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { Ok(Self { bcx, - compilation: Compilation::new(bcx), + compilation: Compilation::new(bcx)?, build_state: Arc::new(BuildState::new(&bcx.host_config, &bcx.target_config)), fingerprints: HashMap::new(), compiled: HashSet::new(), diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index 98e0ca0f75e..9c95e15959c 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -15,6 +15,7 @@ use handle_error; use util::{internal, profile, CargoResult, CargoResultExt, ProcessBuilder}; use util::{Config, DependencyQueue, Dirty, Fresh, Freshness}; use util::{Progress, ProgressStyle}; +use util::diagnostic_server; use super::job::Job; use super::{BuildContext, BuildPlan, CompileMode, Context, Kind, Unit}; @@ -64,6 +65,7 @@ enum Message<'a> { BuildPlanMsg(String, ProcessBuilder, Arc>), Stdout(String), Stderr(String), + FixDiagnostic(diagnostic_server::Message), Token(io::Result), Finish(Key<'a>, CargoResult<()>), } @@ -134,9 +136,9 @@ impl<'a> JobQueue<'a> { self.queue.queue_finished(); // We need to give a handle to the send half of our message queue to the - // jobserver helper thread. Unfortunately though we need the handle to be - // `'static` as that's typically what's required when spawning a - // thread! + // jobserver and (optionally) diagnostic helper thread. Unfortunately + // though we need the handle to be `'static` as that's typically what's + // required when spawning a thread! // // To work around this we transmute the `Sender` to a static lifetime. // we're only sending "longer living" messages and we should also @@ -148,12 +150,20 @@ impl<'a> JobQueue<'a> { // practice. let tx = self.tx.clone(); let tx = unsafe { mem::transmute::>, Sender>>(tx) }; + let tx2 = tx.clone(); let helper = cx.jobserver .clone() .into_helper_thread(move |token| { drop(tx.send(Message::Token(token))); }) .chain_err(|| "failed to create helper thread for jobserver management")?; + let _diagnostic_server = cx.bcx.build_config + .rustfix_diagnostic_server + .borrow_mut() + .take() + .map(move |srv| { + srv.start(move |msg| drop(tx2.send(Message::FixDiagnostic(msg)))) + }); crossbeam::scope(|scope| self.drain_the_queue(cx, plan, scope, &helper)) } @@ -279,6 +289,9 @@ impl<'a> JobQueue<'a> { writeln!(cx.bcx.config.shell().err(), "{}", err)?; } } + Message::FixDiagnostic(msg) => { + msg.print_to(cx.bcx.config)?; + } Message::Finish(key, result) => { info!("end: {:?}", key); diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index c81f594f857..823da348845 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -30,6 +30,7 @@ extern crate libgit2_sys; #[macro_use] extern crate log; extern crate num_cpus; +extern crate rustfix; extern crate same_file; extern crate semver; #[macro_use] diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs index 8bd99ca344b..2e6b39c344a 100644 --- a/src/cargo/ops/cargo_new.rs +++ b/src/cargo/ops/cargo_new.rs @@ -409,7 +409,7 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> { Ok(()) } -fn existing_vcs_repo(path: &Path, cwd: &Path) -> bool { +pub fn existing_vcs_repo(path: &Path, cwd: &Path) -> bool { GitRepo::discover(path, cwd).is_ok() || HgRepo::discover(path, cwd).is_ok() } diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs new file mode 100644 index 00000000000..d7701f7582b --- /dev/null +++ b/src/cargo/ops/fix.rs @@ -0,0 +1,371 @@ +use std::collections::{HashMap, HashSet, BTreeSet}; +use std::env; +use std::fs::File; +use std::io::Write; +use std::path::Path; +use std::process::{self, Command, ExitStatus}; +use std::str; + +use failure::{Error, ResultExt}; +use git2; +use rustfix::diagnostics::Diagnostic; +use rustfix; +use serde_json; + +use core::Workspace; +use ops::{self, CompileOptions}; +use ops::cargo_new::existing_vcs_repo; +use util::errors::CargoResult; +use util::{LockServer, LockServerClient}; +use util::diagnostic_server::{Message, RustfixDiagnosticServer}; +use util::paths; + +const FIX_ENV: &str = "__CARGO_FIX_PLZ"; +const BROKEN_CODE_ENV: &str = "__CARGO_FIX_BROKEN_CODE"; + +pub struct FixOptions<'a> { + pub edition: Option<&'a str>, + pub compile_opts: CompileOptions<'a>, + pub allow_dirty: bool, + pub allow_no_vcs: bool, + pub broken_code: bool, +} + +pub fn fix(ws: &Workspace, opts: &mut FixOptions) -> CargoResult<()> { + check_version_control(opts)?; + + // Spin up our lock server which our subprocesses will use to synchronize + // fixes. + let lock_server = LockServer::new()?; + opts.compile_opts.build_config.extra_rustc_env.push(( + FIX_ENV.to_string(), + lock_server.addr().to_string(), + )); + let _started = lock_server.start()?; + + if opts.broken_code { + let key = BROKEN_CODE_ENV.to_string(); + opts.compile_opts.build_config.extra_rustc_env.push((key, "1".to_string())); + } + + if let Some(edition) = opts.edition { + opts.compile_opts.build_config.extra_rustc_args.push("-W".to_string()); + let lint_name = format!("rust-{}-compatibility", edition); + opts.compile_opts.build_config.extra_rustc_args.push(lint_name); + } + opts.compile_opts.build_config.cargo_as_rustc_wrapper = true; + *opts.compile_opts.build_config.rustfix_diagnostic_server.borrow_mut() = + Some(RustfixDiagnosticServer::new()?); + + ops::compile(ws, &opts.compile_opts)?; + Ok(()) +} + +fn check_version_control(opts: &FixOptions) -> CargoResult<()> { + if opts.allow_no_vcs { + return Ok(()) + } + let config = opts.compile_opts.config; + if !existing_vcs_repo(config.cwd(), config.cwd()) { + bail!("no VCS found for this project and `cargo fix` can potentially \ + perform destructive changes; if you'd like to suppress this \ + error pass `--allow-no-vcs`") + } + + if opts.allow_dirty { + return Ok(()) + } + + let mut dirty_files = Vec::new(); + if let Ok(repo) = git2::Repository::discover(config.cwd()) { + for status in repo.statuses(None)?.iter() { + if status.status() != git2::Status::CURRENT { + if let Some(path) = status.path() { + dirty_files.push(path.to_string()); + } + } + + } + } + + if dirty_files.len() == 0 { + return Ok(()) + } + + let mut files_list = String::new(); + for file in dirty_files { + files_list.push_str(" * "); + files_list.push_str(&file); + files_list.push_str("\n"); + } + + bail!("the working directory of this project is detected as dirty, and \ + `cargo fix` can potentially perform destructive changes; if you'd \ + like to suppress this error pass `--allow-dirty`, or commit the \ + changes to these files:\n\ + \n\ + {}\n\ + ", files_list); +} + +pub fn fix_maybe_exec_rustc() -> CargoResult { + let lock_addr = match env::var(FIX_ENV) { + Ok(s) => s, + Err(_) => return Ok(false), + }; + + // Try to figure out what we're compiling by looking for a rust-like file + // that exists. + let filename = env::args() + .skip(1) + .filter(|s| s.ends_with(".rs")) + .filter(|s| Path::new(s).exists()) + .next(); + + trace!("cargo-fix as rustc got file {:?}", filename); + let rustc = env::var_os("RUSTC").expect("failed to find RUSTC env var"); + + // 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. + // + // To that end we only actually try to fix things if it looks like we're + // compiling a Rust file and it *doesn't* have an absolute filename. That's + // not the best heuristic but matches what Cargo does today at least. + let mut fixes = FixedCrate::default(); + if let Some(path) = filename { + if !Path::new(&path).is_absolute() { + trace!("start rustfixing {:?}", path); + fixes = rustfix_crate(&lock_addr, rustc.as_ref(), &path)?; + } + } + + // Ok now we have our final goal of testing out the changes that we applied. + // If these changes went awry and actually started to cause the crate to + // *stop* compiling then we want to back them out and continue to print + // warnings to the user. + // + // If we didn't actually make any changes then we can immediately exec the + // new rustc, and otherwise we capture the output to hide it in the scenario + // that we have to back it all out. + let mut cmd = Command::new(&rustc); + cmd.args(env::args().skip(1)); + cmd.arg("--cap-lints=warn"); + cmd.arg("--error-format=json"); + if fixes.original_files.len() > 0 { + let output = cmd.output().context("failed to spawn rustc")?; + + if output.status.success() { + for message in fixes.messages.drain(..) { + message.post()?; + } + } + + // If we succeeded then we'll want to commit to the changes we made, if + // any. If stderr is empty then there's no need for the final exec at + // the end, we just bail out here. + if output.status.success() && output.stderr.len() == 0 { + return Ok(true); + } + + // Otherwise if our rustc just failed then that means that we broke the + // user's code with our changes. Back out everything and fall through + // below to recompile again. + if !output.status.success() { + for (k, v) in fixes.original_files { + File::create(&k) + .and_then(|mut f| f.write_all(v.as_bytes())) + .with_context(|_| format!("failed to write file `{}`", k))?; + } + log_failed_fix(&output.stderr)?; + } + } + + let mut cmd = Command::new(&rustc); + cmd.args(env::args().skip(1)); + cmd.arg("--cap-lints=warn"); + exit_with(cmd.status().context("failed to spawn rustc")?); +} + +#[derive(Default)] +struct FixedCrate { + messages: Vec, + original_files: HashMap, +} + +fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &str) + -> Result +{ + // If not empty, filter by these lints + // + // TODO: Implement a way to specify this + let only = HashSet::new(); + + // First up we want to make sure that each crate is only checked by one + // process at a time. If two invocations concurrently check a crate then + // it's likely to corrupt it. + // + // Currently we do this by assigning the name on our lock to the first + // argument that looks like a Rust file. + let _lock = LockServerClient::lock(&lock_addr.parse()?, filename)?; + + let mut cmd = Command::new(&rustc); + cmd.args(env::args().skip(1)); + cmd.arg("--error-format=json").arg("--cap-lints=warn"); + let output = cmd.output() + .with_context(|_| format!("failed to execute `{}`", rustc.display()))?; + + // If rustc didn't succeed for whatever reasons then we're very likely to be + // looking at otherwise broken code. Let's not make things accidentally + // worse by applying fixes where a bug could cause *more* broken code. + // Instead, punt upwards which will reexec rustc over the original code, + // displaying pretty versions of the diagnostics we just read out. + if !output.status.success() && env::var_os(BROKEN_CODE_ENV).is_none() { + debug!( + "rustfixing `{:?}` failed, rustc exited with {:?}", + filename, + output.status.code() + ); + return Ok(Default::default()); + } + + let fix_mode = env::var_os("__CARGO_FIX_YOLO") + .map(|_| rustfix::Filter::Everything) + .unwrap_or(rustfix::Filter::MachineApplicableOnly); + + // Sift through the output of the compiler to look for JSON messages + // indicating fixes that we can apply. + let stderr = str::from_utf8(&output.stderr).context("failed to parse rustc stderr as utf-8")?; + + let suggestions = stderr.lines() + .filter(|x| !x.is_empty()) + .inspect(|y| trace!("line: {}", y)) + + // Parse each line of stderr ignoring errors as they may not all be json + .filter_map(|line| serde_json::from_str::(line).ok()) + + // From each diagnostic try to extract suggestions from rustc + .filter_map(|diag| rustfix::collect_suggestions(&diag, &only, fix_mode)); + + // Collect suggestions by file so we can apply them one at a time later. + let mut file_map = HashMap::new(); + let mut num_suggestion = 0; + for suggestion in suggestions { + trace!("suggestion"); + // Make sure we've got a file associated with this suggestion and all + // snippets point to the same location. Right now it's not clear what + // we would do with multiple locations. + let (file_name, range) = match suggestion.snippets.get(0) { + Some(s) => (s.file_name.clone(), s.line_range), + None => { + trace!("rejecting as it has no snippets {:?}", suggestion); + continue; + } + }; + if !suggestion + .snippets + .iter() + .all(|s| s.file_name == file_name && s.line_range == range) + { + trace!("rejecting as it spans multiple files {:?}", suggestion); + continue; + } + + file_map + .entry(file_name) + .or_insert_with(Vec::new) + .push(suggestion); + num_suggestion += 1; + } + + debug!( + "collected {} suggestions for `{}`", + num_suggestion, filename + ); + + let mut original_files = HashMap::with_capacity(file_map.len()); + let mut messages = Vec::new(); + for (file, suggestions) in file_map { + // Attempt to read the source code for this file. If this fails then + // that'd be pretty surprising, so log a message and otherwise keep + // going. + let code = match paths::read(file.as_ref()) { + Ok(s) => s, + Err(e) => { + warn!("failed to read `{}`: {}", file, e); + continue; + } + }; + let num_suggestions = suggestions.len(); + debug!("applying {} fixes to {}", num_suggestions, file); + + messages.push(Message::fixing(&file, num_suggestions)); + + match rustfix::apply_suggestions(&code, &suggestions) { + Err(e) => { + Message::ReplaceFailed { + file: file, + message: e.to_string(), + }.post()?; + // TODO: Add flag to decide if we want to continue or bail out + continue; + } + Ok(new_code) => { + File::create(&file) + .and_then(|mut f| f.write_all(new_code.as_bytes())) + .with_context(|_| format!("failed to write file `{}`", file))?; + original_files.insert(file, code); + } + } + } + + Ok(FixedCrate { + messages, + original_files, + }) +} + +fn exit_with(status: ExitStatus) -> ! { + #[cfg(unix)] + { + use std::os::unix::prelude::*; + if let Some(signal) = status.signal() { + eprintln!("child failed with signal `{}`", signal); + process::exit(2); + } + } + process::exit(status.code().unwrap_or(3)); +} + +fn log_failed_fix(stderr: &[u8]) -> Result<(), Error> { + let stderr = str::from_utf8(stderr).context("failed to parse rustc stderr as utf-8")?; + + let diagnostics = stderr + .lines() + .filter(|x| !x.is_empty()) + .filter_map(|line| serde_json::from_str::(line).ok()); + let mut files = BTreeSet::new(); + for diagnostic in diagnostics { + for span in diagnostic.spans.into_iter() { + files.insert(span.file_name); + } + } + let mut krate = None; + let mut prev_dash_dash_krate_name = false; + for arg in env::args() { + if prev_dash_dash_krate_name { + krate = Some(arg.clone()); + } + + if arg == "--crate-name" { + prev_dash_dash_krate_name = true; + } else { + prev_dash_dash_krate_name = false; + } + } + + let files = files.into_iter().collect(); + Message::FixFailed { files, krate }.post()?; + + Ok(()) +} diff --git a/src/cargo/ops/mod.rs b/src/cargo/ops/mod.rs index f106896d01e..9c09f14f5ed 100644 --- a/src/cargo/ops/mod.rs +++ b/src/cargo/ops/mod.rs @@ -21,6 +21,7 @@ pub use self::cargo_pkgid::pkgid; pub use self::resolve::{add_overrides, get_resolved_packages, resolve_with_previous, resolve_ws, resolve_ws_precisely, resolve_ws_with_method}; pub use self::cargo_output_metadata::{output_metadata, ExportInfo, OutputMetadataOptions}; +pub use self::fix::{fix, FixOptions, fix_maybe_exec_rustc}; mod cargo_clean; mod cargo_compile; @@ -38,3 +39,4 @@ mod cargo_test; mod lockfile; mod registry; mod resolve; +mod fix; diff --git a/src/cargo/util/diagnostic_server.rs b/src/cargo/util/diagnostic_server.rs new file mode 100644 index 00000000000..78ef3e72f71 --- /dev/null +++ b/src/cargo/util/diagnostic_server.rs @@ -0,0 +1,189 @@ +//! A small TCP server to handle collection of diagnostics information in a +//! cross-platform way for the `cargo fix` command. + +use std::env; +use std::io::{BufReader, Read, Write}; +use std::net::{Shutdown, SocketAddr, TcpListener, TcpStream}; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::thread::{self, JoinHandle}; + +use failure::{Error, ResultExt}; +use serde_json; + +use util::{Config, ProcessBuilder}; +use util::errors::CargoResult; + +const DIAGNOSICS_SERVER_VAR: &str = "__CARGO_FIX_DIAGNOSTICS_SERVER"; +const PLEASE_REPORT_THIS_BUG: &str = + "\ + This likely indicates a bug in either rustc or cargo itself,\n\ + and we would appreciate a bug report! You're likely to see \n\ + a number of compiler warnings after this message which cargo\n\ + attempted to fix but failed. If you could open an issue at\n\ + https://github.com/rust-lang/cargo/issues\n\ + quoting the full output of this command we'd be very appreciative!\n\n\ + "; + +#[derive(Deserialize, Serialize)] +pub enum Message { + Fixing { + file: String, + fixes: usize, + }, + FixFailed { + files: Vec, + krate: Option, + }, + ReplaceFailed { + file: String, + message: String, + }, +} + +impl Message { + pub fn fixing(file: &str, num: usize) -> Message { + Message::Fixing { + file: file.into(), + fixes: num, + } + } + + pub fn post(&self) -> Result<(), Error> { + let addr = env::var(DIAGNOSICS_SERVER_VAR) + .context("diagnostics collector misconfigured")?; + let mut client = + TcpStream::connect(&addr).context("failed to connect to parent diagnostics target")?; + + let s = serde_json::to_string(self).context("failed to serialize message")?; + client + .write_all(s.as_bytes()) + .context("failed to write message to diagnostics target")?; + client + .shutdown(Shutdown::Write) + .context("failed to shutdown")?; + + let mut tmp = Vec::new(); + client + .read_to_end(&mut tmp) + .context("failed to receive a disconnect")?; + + Ok(()) + } + + pub fn print_to(&self, config: &Config) -> CargoResult<()> { + match self { + Message::Fixing { file, fixes } => { + let msg = if *fixes == 1 { "fix" } else { "fixes" }; + let msg = format!("{} ({} {})", file, fixes, msg); + config.shell().status("Fixing", msg) + } + Message::ReplaceFailed { file, message } => { + let msg = format!("error applying suggestions to `{}`\n", file); + config.shell().warn(&msg)?; + write!( + config.shell().err(), + "The full error message was:\n\n> {}", + message, + )?; + write!(config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?; + Ok(()) + } + Message::FixFailed { files, krate } => { + if let Some(ref krate) = *krate { + config.shell().warn(&format!( + "failed to automatically apply fixes suggested by rustc \ + to crate `{}`", + krate, + ))?; + } else { + config.shell().warn( + "failed to automatically apply fixes suggested by rustc" + )?; + } + if files.len() > 0 { + write!( + config.shell().err(), + "\nafter fixes were automatically applied the compiler \ + reported errors within these files:\n\n" + )?; + for file in files { + write!(config.shell().err(), " * {}\n", file)?; + } + write!(config.shell().err(), "\n")?; + } + write!(config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?; + Ok(()) + } + } + + } +} + +#[derive(Debug)] +pub struct RustfixDiagnosticServer { + listener: TcpListener, + addr: SocketAddr, +} + +pub struct StartedServer { + addr: SocketAddr, + done: Arc, + thread: Option>, +} + +impl RustfixDiagnosticServer { + pub fn new() -> Result { + let listener = TcpListener::bind("127.0.0.1:0") + .with_context(|_| "failed to bind TCP listener to manage locking")?; + let addr = listener.local_addr()?; + + Ok(RustfixDiagnosticServer { listener, addr }) + } + + pub fn configure(&self, process: &mut ProcessBuilder) { + process.env(DIAGNOSICS_SERVER_VAR, self.addr.to_string()); + } + + pub fn start(self, on_message: F) -> Result + where + F: Fn(Message) + Send + 'static, + { + let addr = self.addr; + let done = Arc::new(AtomicBool::new(false)); + let done2 = done.clone(); + let thread = thread::spawn(move || { + self.run(&on_message, &done2); + }); + + Ok(StartedServer { + addr, + thread: Some(thread), + done, + }) + } + + fn run(self, on_message: &Fn(Message), done: &AtomicBool) { + while let Ok((client, _)) = self.listener.accept() { + let mut client = BufReader::new(client); + match serde_json::from_reader(client) { + Ok(message) => on_message(message), + Err(e) => warn!("invalid diagnostics message: {}", e), + } + if done.load(Ordering::SeqCst) { + break + } + } + } +} + +impl Drop for StartedServer { + fn drop(&mut self) { + self.done.store(true, Ordering::SeqCst); + // Ignore errors here as this is largely best-effort + if TcpStream::connect(&self.addr).is_err() { + return; + } + drop(self.thread.take().unwrap().join()); + } +} diff --git a/src/cargo/util/lockserver.rs b/src/cargo/util/lockserver.rs new file mode 100644 index 00000000000..699ceca7387 --- /dev/null +++ b/src/cargo/util/lockserver.rs @@ -0,0 +1,172 @@ +//! An implementation of IPC locks, guaranteed to be released if a process dies +//! +//! This module implements a locking server/client where the main `cargo fix` +//! process will start up a server and then all the client processes will +//! connect to it. The main purpose of this file is to enusre that each crate +//! (aka file entry point) is only fixed by one process at a time, currently +//! concurrent fixes can't happen. +//! +//! The basic design here is to use a TCP server which is pretty portable across +//! platforms. For simplicity it just uses threads as well. Clients connect to +//! the main server, inform the server what its name is, and then wait for the +//! server to give it the lock (aka write a byte). + +use std::collections::HashMap; +use std::io::{BufRead, BufReader, Read, Write}; +use std::net::{SocketAddr, TcpListener, TcpStream}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; +use std::thread::{self, JoinHandle}; + +use failure::{Error, ResultExt}; + +pub struct LockServer { + listener: TcpListener, + addr: SocketAddr, + threads: HashMap, + done: Arc, +} + +pub struct LockServerStarted { + done: Arc, + addr: SocketAddr, + thread: Option>, +} + +pub struct LockServerClient { + _socket: TcpStream, +} + +struct ServerClient { + thread: Option>, + lock: Arc)>>, +} + +impl LockServer { + pub fn new() -> Result { + let listener = TcpListener::bind("127.0.0.1:0") + .with_context(|_| "failed to bind TCP listener to manage locking")?; + let addr = listener.local_addr()?; + Ok(LockServer { + listener, + addr, + threads: HashMap::new(), + done: Arc::new(AtomicBool::new(false)), + }) + } + + pub fn addr(&self) -> &SocketAddr { + &self.addr + } + + pub fn start(self) -> Result { + let addr = self.addr; + let done = self.done.clone(); + let thread = thread::spawn(|| { + self.run(); + }); + Ok(LockServerStarted { + addr, + thread: Some(thread), + done, + }) + } + + fn run(mut self) { + while let Ok((client, _)) = self.listener.accept() { + if self.done.load(Ordering::SeqCst) { + break; + } + + // Learn the name of our connected client to figure out if it needs + // to wait for another process to release the lock. + let mut client = BufReader::new(client); + let mut name = String::new(); + if client.read_line(&mut name).is_err() { + continue; + } + let client = client.into_inner(); + + // If this "named mutex" is already registered and the thread is + // still going, put it on the queue. Otherwise wait on the previous + // thread and we'll replace it just below. + if let Some(t) = self.threads.get_mut(&name) { + let mut state = t.lock.lock().unwrap(); + if state.0 { + state.1.push(client); + continue; + } + drop(t.thread.take().unwrap().join()); + } + + let lock = Arc::new(Mutex::new((true, vec![client]))); + let lock2 = lock.clone(); + let thread = thread::spawn(move || { + loop { + let mut client = { + let mut state = lock2.lock().unwrap(); + if state.1.len() == 0 { + state.0 = false; + break; + } else { + state.1.remove(0) + } + }; + // Inform this client that it now has the lock and wait for + // it to disconnect by waiting for EOF. + if client.write_all(&[1]).is_err() { + continue; + } + let mut dst = Vec::new(); + drop(client.read_to_end(&mut dst)); + } + }); + + self.threads.insert( + name, + ServerClient { + thread: Some(thread), + lock, + }, + ); + } + } +} + +impl Drop for LockServer { + fn drop(&mut self) { + for (_, mut client) in self.threads.drain() { + if let Some(thread) = client.thread.take() { + drop(thread.join()); + } + } + } +} + +impl Drop for LockServerStarted { + fn drop(&mut self) { + self.done.store(true, Ordering::SeqCst); + // Ignore errors here as this is largely best-effort + if TcpStream::connect(&self.addr).is_err() { + return; + } + drop(self.thread.take().unwrap().join()); + } +} + +impl LockServerClient { + pub fn lock(addr: &SocketAddr, name: &str) -> Result { + let mut client = + TcpStream::connect(&addr).with_context(|_| "failed to connect to parent lock server")?; + client + .write_all(name.as_bytes()) + .and_then(|_| client.write_all(b"\n")) + .with_context(|_| "failed to write to lock server")?; + let mut buf = [0]; + client + .read_exact(&mut buf) + .with_context(|_| "failed to acquire lock")?; + Ok(LockServerClient { _socket: client }) + } +} + diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index a553601f676..426e79fe5cf 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -18,6 +18,8 @@ pub use self::to_url::ToUrl; pub use self::vcs::{FossilRepo, GitRepo, HgRepo, PijulRepo}; pub use self::read2::read2; pub use self::progress::{Progress, ProgressStyle}; +pub use self::lockserver::{LockServer, LockServerStarted, LockServerClient}; +pub use self::diagnostic_server::RustfixDiagnosticServer; pub mod config; pub mod errors; @@ -42,3 +44,5 @@ mod vcs; mod flock; mod read2; mod progress; +mod lockserver; +pub mod diagnostic_server; diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 3c02873d8ac..493521747c0 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -535,7 +535,7 @@ Caused by: #[test] fn cargo_compile_without_manifest() { let tmpdir = tempfile::Builder::new().prefix("cargo").tempdir().unwrap(); - let p = ProjectBuilder::new("foo", tmpdir.path().to_path_buf()).build(); + let p = ProjectBuilder::new(tmpdir.path().to_path_buf()).build(); assert_that( p.cargo("build"), @@ -3587,7 +3587,7 @@ fn ignore_dotdirs() { #[test] fn dotdir_root() { - let p = ProjectBuilder::new("foo", root().join(".foo")) + let p = ProjectBuilder::new(root().join(".foo")) .file( "Cargo.toml", r#" diff --git a/tests/testsuite/cargotest/support/mod.rs b/tests/testsuite/cargotest/support/mod.rs index 677f90a316f..7f7196ffe00 100644 --- a/tests/testsuite/cargotest/support/mod.rs +++ b/tests/testsuite/cargotest/support/mod.rs @@ -14,6 +14,7 @@ use cargo::util::ProcessError; use hamcrest as ham; use serde_json::{self, Value}; use url::Url; +use tempfile::TempDir; use cargotest::support::paths::CargoPathExt; @@ -94,15 +95,13 @@ impl SymlinkBuilder { } } -#[derive(PartialEq, Clone)] -pub struct Project { - root: PathBuf, +pub enum Project { + Rooted(PathBuf), + Temp(TempDir), } #[must_use] -#[derive(PartialEq, Clone)] pub struct ProjectBuilder { - name: String, root: Project, files: Vec, symlinks: Vec, @@ -119,10 +118,9 @@ impl ProjectBuilder { self.root.target_debug_dir() } - pub fn new(name: &str, root: PathBuf) -> ProjectBuilder { + pub fn new(root: PathBuf) -> ProjectBuilder { ProjectBuilder { - name: name.to_string(), - root: Project { root }, + root: Project::Rooted(root), files: vec![], symlinks: vec![], } @@ -136,25 +134,30 @@ impl ProjectBuilder { fn _file(&mut self, path: &Path, body: &str) { self.files - .push(FileBuilder::new(self.root.root.join(path), body)); + .push(FileBuilder::new(self.root.root().join(path), body)); } /// Add a symlink to the project. pub fn symlink>(mut self, dst: T, src: T) -> Self { self.symlinks.push(SymlinkBuilder::new( - self.root.root.join(dst), - self.root.root.join(src), + self.root.root().join(dst), + self.root.root().join(src), )); self } + pub fn use_temp_dir(mut self) -> Self { + self.root = Project::Temp(TempDir::new().unwrap()); + self + } + /// Create the project. pub fn build(self) -> Project { // First, clean the directory if it already exists self.rm_root(); // Create the empty directory - self.root.root.mkdir_p(); + self.root.root().mkdir_p(); for file in self.files.iter() { file.mk(); @@ -165,7 +168,6 @@ impl ProjectBuilder { } let ProjectBuilder { - name: _, root, files: _, symlinks: _, @@ -175,19 +177,22 @@ impl ProjectBuilder { } fn rm_root(&self) { - self.root.root.rm_rf() + self.root.root().rm_rf() } } impl Project { /// Root of the project, ex: `/path/to/cargo/target/cit/t0/foo` pub fn root(&self) -> PathBuf { - self.root.clone() + match self { + Project::Rooted(p) => p.clone(), + Project::Temp(t) => t.path().to_path_buf(), + } } /// Project's target dir, ex: `/path/to/cargo/target/cit/t0/foo/target` pub fn build_dir(&self) -> PathBuf { - self.root.join("target") + self.root().join("target") } /// Project's debug dir, ex: `/path/to/cargo/target/cit/t0/foo/target/debug` @@ -243,7 +248,7 @@ impl Project { /// Change the contents of an existing file. pub fn change_file(&self, path: &str, body: &str) { - FileBuilder::new(self.root.join(path), body).mk() + FileBuilder::new(self.root().join(path), body).mk() } /// Create a `ProcessBuilder` to run a program in the project. @@ -275,8 +280,13 @@ impl Project { /// Returns the contents of `Cargo.lock`. pub fn read_lockfile(&self) -> String { + self.read_file("Cargo.lock") + } + + /// Returns the contents of a path in the project root + pub fn read_file(&self, path: &str) -> String { let mut buffer = String::new(); - fs::File::open(self.root().join("Cargo.lock")) + fs::File::open(self.root().join(path)) .unwrap() .read_to_string(&mut buffer) .unwrap(); @@ -336,12 +346,12 @@ impl Project { // Generates a project layout pub fn project(name: &str) -> ProjectBuilder { - ProjectBuilder::new(name, paths::root().join(name)) + ProjectBuilder::new(paths::root().join(name)) } // Generates a project layout inside our fake home dir pub fn project_in_home(name: &str) -> ProjectBuilder { - ProjectBuilder::new(name, paths::home().join(name)) + ProjectBuilder::new(paths::home().join(name)) } // === Helpers === @@ -1174,6 +1184,7 @@ fn substitute_macros(input: &str) -> String { ("[REPLACING]", " Replacing"), ("[UNPACKING]", " Unpacking"), ("[SUMMARY]", " Summary"), + ("[FIXING]", " Fixing"), ("[EXE]", if cfg!(windows) { ".exe" } else { "" }), ("[/]", if cfg!(windows) { "\\" } else { "/" }), ]; diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs new file mode 100644 index 00000000000..d56f6f7c59b --- /dev/null +++ b/tests/testsuite/fix.rs @@ -0,0 +1,1025 @@ +use cargotest::support::git; +use cargotest::support::{execs, project}; +use cargotest::{is_nightly, ChannelChanger}; +use git2; +use hamcrest::assert_that; + +#[test] +fn do_not_fix_broken_builds() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + r#" + pub fn foo() { + let mut x = 3; + drop(x); + } + + pub fn foo2() { + let _x: u32 = "a"; + } + "#, + ) + .build(); + + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs().with_status(101), + ); + assert!(p.read_file("src/lib.rs").contains("let mut x = 3;")); +} + +#[test] +fn fix_broken_if_requested() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + r#" + fn foo(a: &u32) -> u32 { a + 1 } + pub fn bar() { + foo(1); + } + "#, + ) + .build(); + + assert_that( + p.cargo("fix --allow-no-vcs --broken-code") + .env("__CARGO_FIX_YOLO", "1"), + execs().with_status(0), + ); +} + +#[test] +fn broken_fixes_backed_out() { + let p = project("foo") + .file( + "foo/Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + [workspace] + "#, + ) + .file( + "foo/src/main.rs", + r##" + use std::env; + use std::fs; + use std::io::Write; + use std::path::{Path, PathBuf}; + use std::process::{self, Command}; + + fn main() { + let is_lib_rs = env::args_os() + .map(PathBuf::from) + .any(|l| l == Path::new("src/lib.rs")); + if is_lib_rs { + let path = PathBuf::from(env::var_os("OUT_DIR").unwrap()); + let path = path.join("foo"); + if path.exists() { + fs::File::create("src/lib.rs") + .unwrap() + .write_all(b"not rust code") + .unwrap(); + } else { + fs::File::create(&path).unwrap(); + } + } + + let status = Command::new("rustc") + .args(env::args().skip(1)) + .status() + .expect("failed to run rustc"); + process::exit(status.code().unwrap_or(2)); + } + "##, + ) + .file( + "bar/Cargo.toml", + r#" + [package] + name = 'bar' + version = '0.1.0' + [workspace] + "#, + ) + .file("bar/build.rs", "fn main() {}") + .file( + "bar/src/lib.rs", + r#" + pub fn foo() { + let mut x = 3; + drop(x); + } + "#, + ) + .build(); + + // Build our rustc shim + assert_that( + p.cargo("build").cwd(p.root().join("foo")), + execs().with_status(0), + ); + + // Attempt to fix code, but our shim will always fail the second compile + assert_that( + p.cargo("fix --allow-no-vcs") + .cwd(p.root().join("bar")) + .env("__CARGO_FIX_YOLO", "1") + .env("RUSTC", p.root().join("foo/target/debug/foo")), + execs() + .with_status(101) + .with_stderr_contains("[..]not rust code[..]") + .with_stderr_contains("\ + warning: failed to automatically apply fixes suggested by rustc \ + to crate `bar`\n\ + \n\ + after fixes were automatically applied the compiler reported \ + errors within these files:\n\ + \n \ + * src[/]lib.rs\n\ + \n\ + This likely indicates a bug in either rustc or cargo itself,\n\ + and we would appreciate a bug report! You're likely to see \n\ + a number of compiler warnings after this message which cargo\n\ + attempted to fix but failed. If you could open an issue at\n\ + https://github.com/rust-lang/cargo/issues\n\ + quoting the full output of this command we'd be very appreciative!\ + ") + .with_stderr_does_not_contain("[..][FIXING][..]"), + ); +} + +#[test] +fn fix_path_deps() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { path = 'bar' } + + [workspace] + "#, + ) + .file( + "src/lib.rs", + r#" + extern crate bar; + + pub fn foo() -> u32 { + let mut x = 3; + x + } + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.1.0" + "#, + ) + .file( + "bar/src/lib.rs", + r#" + pub fn foo() -> u32 { + let mut x = 3; + x + } + "#, + ) + .build(); + + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs() + .with_status(0) + .with_stdout("") + .with_stderr("\ +[CHECKING] bar v0.1.0 ([..]) +[FIXING] bar[/]src[/]lib.rs (1 fix) +[CHECKING] foo v0.1.0 ([..]) +[FIXING] src[/]lib.rs (1 fix) +[FINISHED] [..] +") + ); +} + +#[test] +fn do_not_fix_non_relevant_deps() { + let p = project("foo") + .file( + "foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { path = '../bar' } + + [workspace] + "#, + ) + .file("foo/src/lib.rs", "") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.1.0" + "#, + ) + .file( + "bar/src/lib.rs", + r#" + pub fn foo() -> u32 { + let mut x = 3; + x + } + "#, + ) + .build(); + + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1") + .cwd(p.root().join("foo")), + execs().with_status(0) + ); + + assert!(p.read_file("bar/src/lib.rs").contains("mut")); +} + +#[test] +fn prepare_for_2018() { + if !is_nightly() { + return + } + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + r#" + #![allow(unused)] + #![feature(rust_2018_preview)] + + mod foo { + pub const FOO: &str = "fooo"; + } + + mod bar { + use ::foo::FOO; + } + + fn main() { + let x = ::foo::FOO; + } + "#, + ) + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 ([..]) +[FIXING] src[/]lib.rs (2 fixes) +[FINISHED] [..] +"; + assert_that( + p.cargo("fix --prepare-for 2018 --allow-no-vcs"), + execs().with_status(0).with_stderr(stderr).with_stdout(""), + ); + + println!("{}", p.read_file("src/lib.rs")); + assert!(p.read_file("src/lib.rs").contains("use crate::foo::FOO;")); + assert!(p.read_file("src/lib.rs").contains("let x = crate::foo::FOO;")); +} + +#[test] +fn local_paths() { + if !is_nightly() { + return + } + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + r#" + #![feature(rust_2018_preview)] + + use test::foo; + + mod test { + pub fn foo() {} + } + + pub fn f() { + foo(); + } + "#, + ) + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 ([..]) +[FIXING] src[/]lib.rs (1 fix) +[FINISHED] [..] +"; + + assert_that( + p.cargo("fix --prepare-for 2018 --allow-no-vcs"), + execs().with_status(0).with_stderr(stderr).with_stdout(""), + ); + + println!("{}", p.read_file("src/lib.rs")); + assert!(p.read_file("src/lib.rs").contains("use crate::test::foo;")); +} + +#[test] +fn local_paths_no_fix() { + if !is_nightly() { + return + } + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + r#" + use test::foo; + + mod test { + pub fn foo() {} + } + + pub fn f() { + foo(); + } + "#, + ) + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 ([..]) +[FINISHED] [..] +"; + assert_that( + p.cargo("fix --prepare-for 2018 --allow-no-vcs"), + execs().with_status(0).with_stderr(stderr).with_stdout(""), + ); +} + +#[test] +fn upgrade_extern_crate() { + if !is_nightly() { + return + } + let p = project("foo") + .file( + "Cargo.toml", + r#" + cargo-features = ["edition"] + + [package] + name = "foo" + version = "0.1.0" + edition = '2018' + + [workspace] + + [dependencies] + bar = { path = 'bar' } + "#, + ) + .file( + "src/lib.rs", + r#" + #![warn(rust_2018_idioms)] + extern crate bar; + + use bar::bar; + + pub fn foo() { + ::bar::bar(); + bar(); + } + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.1.0" + "#, + ) + .file("bar/src/lib.rs", "pub fn bar() {}") + .build(); + + let stderr = "\ +[CHECKING] bar v0.1.0 ([..]) +[CHECKING] foo v0.1.0 ([..]) +[FIXING] src[/]lib.rs (1 fix) +[FINISHED] [..] +"; + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1") + .masquerade_as_nightly_cargo(), + execs().with_status(0).with_stderr(stderr).with_stdout(""), + ); + println!("{}", p.read_file("src/lib.rs")); + assert!(!p.read_file("src/lib.rs").contains("extern crate")); +} + +#[test] +fn specify_rustflags() { + if !is_nightly() { + return + } + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + r#" + #![allow(unused)] + #![feature(rust_2018_preview)] + + mod foo { + pub const FOO: &str = "fooo"; + } + + fn main() { + let x = ::foo::FOO; + } + "#, + ) + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 ([..]) +[FIXING] src[/]lib.rs (1 fix) +[FINISHED] [..] +"; + assert_that( + p.cargo("fix --prepare-for 2018 --allow-no-vcs") + .env("RUSTFLAGS", "-C target-cpu=native"), + execs().with_status(0).with_stderr(stderr).with_stdout(""), + ); +} + +#[test] +fn no_changes_necessary() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file("src/lib.rs", "") + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 ([..]) +[FINISHED] [..] +"; + assert_that( + p.cargo("fix --allow-no-vcs"), + execs().with_status(0).with_stderr(stderr).with_stdout(""), + ); +} + +#[test] +fn fixes_extra_mut() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + r#" + pub fn foo() -> u32 { + let mut x = 3; + x + } + "#, + ) + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 ([..]) +[FIXING] src[/]lib.rs (1 fix) +[FINISHED] [..] +"; + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs().with_status(0).with_stderr(stderr).with_stdout(""), + ); +} + +#[test] +fn fixes_two_missing_ampersands() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + r#" + pub fn foo() -> u32 { + let mut x = 3; + let mut y = 3; + x + y + } + "#, + ) + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 ([..]) +[FIXING] src[/]lib.rs (2 fixes) +[FINISHED] [..] +"; + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs().with_status(0).with_stderr(stderr).with_stdout(""), + ); +} + +#[test] +fn tricky() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + r#" + pub fn foo() -> u32 { + let mut x = 3; let mut y = 3; + x + y + } + "#, + ) + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 ([..]) +[FIXING] src[/]lib.rs (2 fixes) +[FINISHED] [..] +"; + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs().with_status(0).with_stderr(stderr).with_stdout(""), + ); +} + +#[test] +fn preserve_line_endings() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + "\ + fn add(a: &u32) -> u32 { a + 1 }\r\n\ + pub fn foo() -> u32 { let mut x = 3; add(&x) }\r\n\ + ", + ) + .build(); + + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs().with_status(0), + ); + assert!(p.read_file("src/lib.rs").contains("\r\n")); +} + +#[test] +fn fix_deny_warnings() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + "\ + #![deny(warnings)] + pub fn foo() { let mut x = 3; drop(x); } + ", + ) + .build(); + + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs().with_status(0), + ); +} + +#[test] +fn fix_deny_warnings_but_not_others() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + " + #![deny(warnings)] + + pub fn foo() -> u32 { + let mut x = 3; + x + } + + fn bar() {} + ", + ) + .build(); + + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs().with_status(0), + ); + assert!(!p.read_file("src/lib.rs").contains("let mut x = 3;")); + assert!(p.read_file("src/lib.rs").contains("fn bar() {}")); +} + +#[test] +fn fix_two_files() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/lib.rs", + " + pub mod bar; + + pub fn foo() -> u32 { + let mut x = 3; + x + } + ", + ) + .file( + "src/bar.rs", + " + pub fn foo() -> u32 { + let mut x = 3; + x + } + + ", + ) + .build(); + + assert_that( + p.cargo("fix --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs() + .with_status(0) + .with_stderr_contains("[FIXING] src[/]bar.rs (1 fix)") + .with_stderr_contains("[FIXING] src[/]lib.rs (1 fix)"), + ); + assert!(!p.read_file("src/lib.rs").contains("let mut x = 3;")); + assert!(!p.read_file("src/bar.rs").contains("let mut x = 3;")); +} + +#[test] +fn fixes_missing_ampersand() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + "# + ) + .file( + "src/main.rs", + "fn main() { let mut x = 3; drop(x); }", + ) + .file( + "src/lib.rs", + r#" + pub fn foo() { let mut x = 3; drop(x); } + + #[test] + pub fn foo2() { let mut x = 3; drop(x); } + "#, + ) + .file( + "tests/a.rs", + r#" + #[test] + pub fn foo() { let mut x = 3; drop(x); } + "#, + ) + .file( + "examples/foo.rs", + r#" + fn main() { let mut x = 3; drop(x); } + "#, + ) + .file( + "build.rs", + r#" + fn main() { let mut x = 3; drop(x); } + "#, + ) + .build(); + + assert_that( + p.cargo("fix --all-targets --allow-no-vcs") + .env("__CARGO_FIX_YOLO", "1"), + execs() + .with_status(0) + .with_stdout("") + .with_stderr_contains("[COMPILING] foo v0.1.0 ([..])") + .with_stderr_contains("[FIXING] build.rs (1 fix)") + // Don't assert number of fixes for this one, as we don't know if we're + // fixing it once or twice! We run this all concurrently, and if we + // compile (and fix) in `--test` mode first, we get two fixes. Otherwise + // we'll fix one non-test thing, and then fix another one later in + // test mode. + .with_stderr_contains("[FIXING] src[/]lib.rs[..]") + .with_stderr_contains("[FIXING] src[/]main.rs (1 fix)") + .with_stderr_contains("[FIXING] examples[/]foo.rs (1 fix)") + .with_stderr_contains("[FIXING] tests[/]a.rs (1 fix)") + .with_stderr_contains("[FINISHED] [..]"), + ); + assert_that(p.cargo("build"), execs().with_status(0)); + assert_that(p.cargo("test"), execs().with_status(0)); +} + +#[test] +fn fix_features() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [features] + bar = [] + + [workspace] + "#, + ) + .file( + "src/lib.rs", + r#" + #[cfg(feature = "bar")] + pub fn foo() -> u32 { let mut x = 3; x } + "#, + ) + .build(); + + assert_that(p.cargo("fix --allow-no-vcs"), execs().with_status(0)); + assert_that(p.cargo("build"), execs().with_status(0)); + assert_that(p.cargo("fix --features bar --allow-no-vcs"), execs().with_status(0)); + assert_that(p.cargo("build --features bar"), execs().with_status(0)); +} + +#[test] +fn shows_warnings() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + "#, + ) + .file( + "src/lib.rs", + r#" + use std::default::Default; + + pub fn foo() { + } + "#, + ) + .build(); + + assert_that( + p.cargo("fix --allow-no-vcs"), + execs().with_status(0).with_stderr_contains("[..]warning: unused import[..]"), + ); +} + +#[test] +fn warns_if_no_vcs_detected() { + let p = project("foo") + .use_temp_dir() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn foo() { + } + "#, + ) + .build(); + + assert_that( + p.cargo("fix"), + execs() + .with_status(101) + .with_stderr("\ +error: no VCS found for this project and `cargo fix` can potentially perform \ +destructive changes; if you'd like to suppress this error pass `--allow-no-vcs`\ +") + ); + assert_that( + p.cargo("fix --allow-no-vcs"), + execs().with_status(0), + ); +} + +#[test] +fn warns_about_dirty_working_directory() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn foo() { + } + "#, + ) + .build(); + + git2::Repository::init(&p.root()).unwrap(); + + assert_that( + p.cargo("fix"), + execs() + .with_status(101) + .with_stderr("\ +error: the working directory of this project is detected as dirty, and `cargo \ +fix` can potentially perform destructive changes; if you'd like to \ +suppress this error pass `--allow-dirty`, or commit the changes to \ +these files: + + * Cargo.toml + * src/lib.rs + + +") + ); + assert_that( + p.cargo("fix --allow-dirty"), + execs().with_status(0), + ); +} + +#[test] +fn does_not_warn_about_clean_working_directory() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn foo() { + } + "#, + ) + .build(); + + let repo = git2::Repository::init(&p.root()).unwrap(); + let mut cfg = t!(repo.config()); + t!(cfg.set_str("user.email", "foo@bar.com")); + t!(cfg.set_str("user.name", "Foo Bar")); + drop(cfg); + git::add(&repo); + git::commit(&repo); + + assert_that( + p.cargo("fix"), + execs().with_status(0), + ); +} diff --git a/tests/testsuite/generate_lockfile.rs b/tests/testsuite/generate_lockfile.rs index 2a6d09faa86..78d1af96006 100644 --- a/tests/testsuite/generate_lockfile.rs +++ b/tests/testsuite/generate_lockfile.rs @@ -253,7 +253,7 @@ fn cargo_update_generate_lockfile() { #[test] fn duplicate_entries_in_lockfile() { - let _a = ProjectBuilder::new("a", paths::root().join("a")) + let _a = ProjectBuilder::new(paths::root().join("a")) .file( "Cargo.toml", r#" @@ -276,12 +276,12 @@ fn duplicate_entries_in_lockfile() { version = "0.0.1" "#; - let _common_in_a = ProjectBuilder::new("common", paths::root().join("a/common")) + let _common_in_a = ProjectBuilder::new(paths::root().join("a/common")) .file("Cargo.toml", common_toml) .file("src/lib.rs", "") .build(); - let b = ProjectBuilder::new("common", paths::root().join("b")) + let b = ProjectBuilder::new(paths::root().join("b")) .file( "Cargo.toml", r#" @@ -298,7 +298,7 @@ fn duplicate_entries_in_lockfile() { .file("src/lib.rs", "") .build(); - let _common_in_b = ProjectBuilder::new("common", paths::root().join("b/common")) + let _common_in_b = ProjectBuilder::new(paths::root().join("b/common")) .file("Cargo.toml", common_toml) .file("src/lib.rs", "") .build(); diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index f57ecf25368..22747b65de3 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -51,6 +51,7 @@ mod directory; mod doc; mod features; mod fetch; +mod fix; mod freshness; mod generate_lockfile; mod git;