From 7691deb32518fd886794314cb699e29e72f7ab03 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Sat, 28 Jul 2018 10:38:34 -0700 Subject: [PATCH 1/2] fix: Only fix "primary" packages by default The previous heuristic for fixing packages was to fix all packages in a workspace, aka those with path dependencies. Instead this commit switches cargo over to only fixing the "primary" package, or those requested on the command line or implicitly via cwd. This will later help us identify which packages are being targeted so we can provide tailored warnings and errors for mixed up transition steps. --- src/cargo/core/compiler/context/mod.rs | 7 +++++++ src/cargo/core/compiler/mod.rs | 7 +++++-- src/cargo/ops/fix.rs | 2 +- tests/testsuite/fix.rs | 2 +- 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index ff38c4a8c4e..3687ee60ff8 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -96,6 +96,7 @@ pub struct Context<'a, 'cfg: 'a> { pub links: Links<'a>, pub used_in_plugin: HashSet>, pub jobserver: Client, + primary_packages: HashSet<&'a PackageId>, unit_dependencies: HashMap, Vec>>, files: Option>, } @@ -129,6 +130,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { jobserver, build_script_overridden: HashSet::new(), + primary_packages: HashSet::new(), unit_dependencies: HashMap::new(), files: None, }) @@ -321,6 +323,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { Some(target) => Some(Layout::new(self.bcx.ws, Some(target), dest)?), None => None, }; + self.primary_packages.extend(units.iter().map(|u| u.pkg.package_id())); build_unit_dependencies(units, self.bcx, &mut self.unit_dependencies)?; self.build_used_in_plugin_map(units)?; @@ -487,6 +490,10 @@ impl<'a, 'cfg> Context<'a, 'cfg> { let dir = self.files().layout(unit.kind).incremental().display(); Ok(vec!["-C".to_string(), format!("incremental={}", dir)]) } + + pub fn is_primary_package(&self, unit: &Unit<'a>) -> bool { + self.primary_packages.contains(unit.pkg.package_id()) + } } #[derive(Default)] diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 4f971d9d45a..e673e33066a 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -161,11 +161,14 @@ fn compile<'a, 'cfg: 'a>( } fn rustc<'a, 'cfg>( - mut cx: &mut Context<'a, 'cfg>, + cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>, 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(); @@ -195,7 +198,7 @@ fn rustc<'a, 'cfg>( } else { root.join(&cx.files().file_stem(unit)) }.with_extension("d"); - let dep_info_loc = fingerprint::dep_info_loc(&mut cx, unit); + let dep_info_loc = fingerprint::dep_info_loc(cx, unit); rustc.args(&cx.bcx.rustflags_args(unit)?); let json_messages = cx.bcx.build_config.json_messages(); diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index f44e467a42e..0fa16c8158d 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -134,7 +134,7 @@ pub fn fix_maybe_exec_rustc() -> CargoResult { // 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() { + if env::var("CARGO_PRIMARY_PACKAGE").is_ok() { trace!("start rustfixing {:?}", path); fixes = rustfix_crate(&lock_addr, rustc.as_ref(), &path)?; } diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index bf0351efc61..386d0e33836 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -196,7 +196,7 @@ fn fix_path_deps() { .build(); assert_that( - p.cargo("fix --allow-no-vcs") + p.cargo("fix --allow-no-vcs -p foo -p bar") .env("__CARGO_FIX_YOLO", "1"), execs() .with_status(0) From fa7a387740bd760dc3ec041a6e15c1b66c794852 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Sat, 28 Jul 2018 13:22:32 -0700 Subject: [PATCH 2/2] Add more diagnostics to smooth edition transition This commit adds two diagnostics in particular to ease the transition into the 2018 edition. The current transition process is pretty particular and must be done carefully, so let's try to automate things to make it as painless as possible! Notably the new diagnostics are: * If you `cargo fix --prepare-for 2018` a crate which already has the 2018 edition enabled, then an error is generated. This is because the compiler can't prepare for the 2018 edition if you're already in the 2018 edition, the lints won't have a chance to fire. You can only execute `--prepare-for 2018` over crates in the 2015 edition. * If you `cargo fix --prepare-for 2018` and have forgotten the `rust_2018_preview` feature, a warning is issued. The lints don't fire unless the feature is enabled, so this is intended to warn in this situation to ensure that lints fire as much as they can. After this commit if `cargo fix --prepare-for` exits successfully with zero warnings then crates should be guaranteed to be compatible! Closes #5778 --- src/cargo/core/compiler/job_queue.rs | 5 +- src/cargo/ops/fix.rs | 163 ++++++++++++++++++++++----- src/cargo/util/diagnostic_server.rs | 88 +++++++++++++-- src/cargo/util/lockserver.rs | 5 +- tests/testsuite/fix.rs | 70 ++++++++++++ 5 files changed, 285 insertions(+), 46 deletions(-) diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index a50228a8e01..5155a6e632b 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -16,7 +16,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 util::diagnostic_server::{self, DiagnosticPrinter}; use super::job::Job; use super::{BuildContext, BuildPlan, CompileMode, Context, Kind, Unit}; @@ -200,6 +200,7 @@ impl<'a> JobQueue<'a> { let mut tokens = Vec::new(); let mut queue = Vec::new(); let build_plan = cx.bcx.build_config.build_plan; + let mut print = DiagnosticPrinter::new(cx.bcx.config); trace!("queue: {:#?}", self.queue); // Iteratively execute the entire dependency graph. Each turn of the @@ -311,7 +312,7 @@ impl<'a> JobQueue<'a> { } } Message::FixDiagnostic(msg) => { - msg.print_to(cx.bcx.config)?; + print.print(&msg)?; } Message::Finish(key, result) => { info!("end: {:?}", key); diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 0fa16c8158d..5d7ddb80345 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -1,8 +1,8 @@ use std::collections::{HashMap, HashSet, BTreeSet}; use std::env; -use std::fs::File; -use std::io::Write; -use std::path::Path; +use std::ffi::OsString; +use std::fs; +use std::path::{Path, PathBuf}; use std::process::{self, Command, ExitStatus}; use std::str; @@ -21,6 +21,7 @@ use util::paths; const FIX_ENV: &str = "__CARGO_FIX_PLZ"; const BROKEN_CODE_ENV: &str = "__CARGO_FIX_BROKEN_CODE"; +const EDITION_ENV: &str = "__CARGO_FIX_EDITION"; pub struct FixOptions<'a> { pub edition: Option<&'a str>, @@ -48,9 +49,10 @@ pub fn fix(ws: &Workspace, opts: &mut FixOptions) -> CargoResult<()> { } 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.extra_rustc_env.push(( + EDITION_ENV.to_string(), + edition.to_string(), + )); } opts.compile_opts.build_config.cargo_as_rustc_wrapper = true; *opts.compile_opts.build_config.rustfix_diagnostic_server.borrow_mut() = @@ -115,14 +117,8 @@ pub fn fix_maybe_exec_rustc() -> CargoResult { 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")) - .find(|s| Path::new(s).exists()); - - trace!("cargo-fix as rustc got file {:?}", filename); + let args = FixArgs::get(); + trace!("cargo-fix as rustc got file {:?}", args.file); 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. @@ -133,10 +129,10 @@ pub fn fix_maybe_exec_rustc() -> CargoResult { // 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 let Some(path) = &args.file { if env::var("CARGO_PRIMARY_PACKAGE").is_ok() { trace!("start rustfixing {:?}", path); - fixes = rustfix_crate(&lock_addr, rustc.as_ref(), &path)?; + fixes = rustfix_crate(&lock_addr, rustc.as_ref(), path, &args)?; } } @@ -148,11 +144,10 @@ pub fn fix_maybe_exec_rustc() -> CargoResult { // 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.is_empty() { + let mut cmd = Command::new(&rustc); + args.apply(&mut cmd); + cmd.arg("--error-format=json"); let output = cmd.output().context("failed to spawn rustc")?; if output.status.success() { @@ -173,8 +168,7 @@ pub fn fix_maybe_exec_rustc() -> CargoResult { // 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())) + fs::write(&k, v) .with_context(|_| format!("failed to write file `{}`", k))?; } log_failed_fix(&output.stderr)?; @@ -182,8 +176,7 @@ pub fn fix_maybe_exec_rustc() -> CargoResult { } let mut cmd = Command::new(&rustc); - cmd.args(env::args().skip(1)); - cmd.arg("--cap-lints=warn"); + args.apply(&mut cmd); exit_with(cmd.status().context("failed to spawn rustc")?); } @@ -193,9 +186,12 @@ struct FixedCrate { original_files: HashMap, } -fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &str) +fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &Path, args: &FixArgs) -> Result { + args.verify_not_preparing_for_enabled_edition()?; + args.warn_if_preparing_probably_inert()?; + // If not empty, filter by these lints // // TODO: Implement a way to specify this @@ -210,8 +206,8 @@ fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &str) 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"); + cmd.arg("--error-format=json"); + args.apply(&mut cmd); let output = cmd.output() .with_context(|_| format!("failed to execute `{}`", rustc.display()))?; @@ -280,7 +276,8 @@ fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &str) debug!( "collected {} suggestions for `{}`", - num_suggestion, filename + num_suggestion, + filename.display(), ); let mut original_files = HashMap::with_capacity(file_map.len()); @@ -311,8 +308,7 @@ fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &str) continue; } Ok(new_code) => { - File::create(&file) - .and_then(|mut f| f.write_all(new_code.as_bytes())) + fs::write(&file, new_code) .with_context(|_| format!("failed to write file `{}`", file))?; original_files.insert(file, code); } @@ -369,3 +365,110 @@ fn log_failed_fix(stderr: &[u8]) -> Result<(), Error> { Ok(()) } + +#[derive(Default)] +struct FixArgs { + file: Option, + prepare_for_edition: Option, + enabled_edition: Option, + other: Vec, +} + +impl FixArgs { + fn get() -> FixArgs { + let mut ret = FixArgs::default(); + for arg in env::args_os().skip(1) { + let path = PathBuf::from(arg); + if path.extension().and_then(|s| s.to_str()) == Some("rs") { + if path.exists() { + ret.file = Some(path); + continue + } + } + if let Some(s) = path.to_str() { + let prefix = "--edition="; + if s.starts_with(prefix) { + ret.enabled_edition = Some(s[prefix.len()..].to_string()); + continue + } + } + ret.other.push(path.into()); + } + if let Ok(s) = env::var(EDITION_ENV) { + ret.prepare_for_edition = Some(s); + } + return ret + } + + fn apply(&self, cmd: &mut Command) { + if let Some(path) = &self.file { + cmd.arg(path); + } + cmd.args(&self.other) + .arg("--cap-lints=warn"); + if let Some(edition) = &self.enabled_edition { + cmd.arg("--edition").arg(edition); + } + if let Some(prepare_for) = &self.prepare_for_edition { + cmd.arg("-W").arg(format!("rust-{}-compatibility", prepare_for)); + } + } + + /// Verify that we're not both preparing for an enabled edition and enabling + /// the edition. + /// + /// This indicates that `cargo fix --prepare-for` is being executed out of + /// order with enabling the edition itself, meaning that we wouldn't + /// actually be able to fix anything! If it looks like this is happening + /// then yield an error to the user, indicating that this is happening. + fn verify_not_preparing_for_enabled_edition(&self) -> CargoResult<()> { + let edition = match &self.prepare_for_edition { + Some(s) => s, + None => return Ok(()), + }; + let enabled = match &self.enabled_edition { + Some(s) => s, + None => return Ok(()), + }; + if edition != enabled { + return Ok(()) + } + let path = match &self.file { + Some(s) => s, + None => return Ok(()), + }; + + Message::EditionAlreadyEnabled { + file: path.display().to_string(), + edition: edition.to_string(), + }.post()?; + + process::exit(1); + } + + fn warn_if_preparing_probably_inert(&self) -> CargoResult<()> { + let edition = match &self.prepare_for_edition { + Some(s) => s, + None => return Ok(()), + }; + let path = match &self.file { + Some(s) => s, + None => return Ok(()), + }; + let contents = match fs::read_to_string(path) { + Ok(s) => s, + Err(_) => return Ok(()) + }; + + let feature_name = format!("rust_{}_preview", edition); + if contents.contains(&feature_name) { + return Ok(()) + } + Message::PreviewNotFound { + file: path.display().to_string(), + edition: edition.to_string(), + }.post()?; + + Ok(()) + } +} diff --git a/src/cargo/util/diagnostic_server.rs b/src/cargo/util/diagnostic_server.rs index f948da4068b..624762b1cc8 100644 --- a/src/cargo/util/diagnostic_server.rs +++ b/src/cargo/util/diagnostic_server.rs @@ -1,6 +1,7 @@ //! A small TCP server to handle collection of diagnostics information in a //! cross-platform way for the `cargo fix` command. +use std::collections::HashSet; use std::env; use std::io::{BufReader, Read, Write}; use std::net::{Shutdown, SocketAddr, TcpListener, TcpStream}; @@ -39,6 +40,14 @@ pub enum Message { file: String, message: String, }, + PreviewNotFound { + file: String, + edition: String, + }, + EditionAlreadyEnabled { + file: String, + edition: String, + }, } impl Message { @@ -70,49 +79,104 @@ impl Message { Ok(()) } +} - pub fn print_to(&self, config: &Config) -> CargoResult<()> { - match self { +pub struct DiagnosticPrinter<'a> { + config: &'a Config, + preview_not_found: HashSet, + edition_already_enabled: HashSet, +} + +impl<'a> DiagnosticPrinter<'a> { + pub fn new(config: &'a Config) -> DiagnosticPrinter<'a> { + DiagnosticPrinter { + config, + preview_not_found: HashSet::new(), + edition_already_enabled: HashSet::new(), + } + } + + pub fn print(&mut self, msg: &Message) -> CargoResult<()> { + match msg { Message::Fixing { file, fixes } => { let msg = if *fixes == 1 { "fix" } else { "fixes" }; let msg = format!("{} ({} {})", file, fixes, msg); - config.shell().status("Fixing", msg) + self.config.shell().status("Fixing", msg) } Message::ReplaceFailed { file, message } => { let msg = format!("error applying suggestions to `{}`\n", file); - config.shell().warn(&msg)?; + self.config.shell().warn(&msg)?; write!( - config.shell().err(), + self.config.shell().err(), "The full error message was:\n\n> {}\n\n", message, )?; - write!(config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?; + write!(self.config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?; Ok(()) } Message::FixFailed { files, krate } => { if let Some(ref krate) = *krate { - config.shell().warn(&format!( + self.config.shell().warn(&format!( "failed to automatically apply fixes suggested by rustc \ to crate `{}`", krate, ))?; } else { - config.shell().warn( + self.config.shell().warn( "failed to automatically apply fixes suggested by rustc" )?; } if !files.is_empty() { writeln!( - config.shell().err(), + self.config.shell().err(), "\nafter fixes were automatically applied the compiler \ reported errors within these files:\n" )?; for file in files { - writeln!(config.shell().err(), " * {}", file)?; + writeln!(self.config.shell().err(), " * {}", file)?; } - writeln!(config.shell().err())?; + writeln!(self.config.shell().err())?; + } + write!(self.config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?; + Ok(()) + } + Message::PreviewNotFound { file, edition } => { + // By default we're fixing a lot of things concurrently, don't + // warn about the same file multiple times. + if !self.preview_not_found.insert(file.clone()) { + return Ok(()) } - write!(config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?; + self.config.shell().warn(&format!( + "failed to find `#![feature(rust_{}_preview)]` in `{}`\n\ + this may cause `cargo fix` to not be able to fix all\n\ + issues in preparation for the {0} edition", + edition, + file, + ))?; + Ok(()) + } + Message::EditionAlreadyEnabled { file, edition } => { + // Like above, only warn once per file + if !self.edition_already_enabled.insert(file.clone()) { + return Ok(()) + } + + let msg = format!( + "\ +cannot prepare for the {} edition when it is enabled, so cargo cannot +automatically fix errors in `{}` + +To prepare for the {0} edition you should first remove `edition = '{0}'` from +your `Cargo.toml` and then rerun this command. Once all warnings have been fixed +then you can re-enable the `edition` key in `Cargo.toml`. For some more +information about transitioning to the {0} edition see: + + https://rust-lang-nursery.github.io/edition-guide/editions/transitioning.html +", + edition, + file, + ); + self.config.shell().error(&msg)?; Ok(()) } } diff --git a/src/cargo/util/lockserver.rs b/src/cargo/util/lockserver.rs index 7e0176472b6..0e5f524835c 100644 --- a/src/cargo/util/lockserver.rs +++ b/src/cargo/util/lockserver.rs @@ -14,6 +14,7 @@ use std::collections::HashMap; use std::io::{BufRead, BufReader, Read, Write}; use std::net::{SocketAddr, TcpListener, TcpStream}; +use std::path::Path; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use std::thread::{self, JoinHandle}; @@ -155,11 +156,11 @@ impl Drop for LockServerStarted { } impl LockServerClient { - pub fn lock(addr: &SocketAddr, name: &str) -> Result { + pub fn lock(addr: &SocketAddr, name: &Path) -> Result { let mut client = TcpStream::connect(&addr).with_context(|_| "failed to connect to parent lock server")?; client - .write_all(name.as_bytes()) + .write_all(name.display().to_string().as_bytes()) .and_then(|_| client.write_all(b"\n")) .with_context(|_| "failed to write to lock server")?; let mut buf = [0]; diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 386d0e33836..2dcad7934b8 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -356,6 +356,9 @@ fn local_paths_no_fix() { let stderr = "\ [CHECKING] foo v0.0.1 ([..]) +warning: failed to find `#![feature(rust_2018_preview)]` in `src[/]lib.rs` +this may cause `cargo fix` to not be able to fix all +issues in preparation for the 2018 edition [FINISHED] [..] "; assert_that( @@ -857,3 +860,70 @@ fn fix_all_targets_by_default() { assert!(!p.read_file("src/lib.rs").contains("let mut x")); assert!(!p.read_file("tests/foo.rs").contains("let mut x")); } + +#[test] +fn prepare_for_and_enable() { + if !is_nightly() { + return + } + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ['edition'] + + [package] + name = 'foo' + version = '0.1.0' + edition = '2018' + "#, + ) + .file("src/lib.rs", "") + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 ([..]) +error: cannot prepare for the 2018 edition when it is enabled, so cargo cannot +automatically fix errors in `src[/]lib.rs` + +To prepare for the 2018 edition you should first remove `edition = '2018'` from +your `Cargo.toml` and then rerun this command. Once all warnings have been fixed +then you can re-enable the `edition` key in `Cargo.toml`. For some more +information about transitioning to the 2018 edition see: + + https://[..] + +"; + assert_that( + p.cargo("fix --prepare-for 2018 --allow-no-vcs") + .masquerade_as_nightly_cargo(), + execs() + .with_stderr_contains(stderr) + .with_status(101), + ); +} + +#[test] +fn prepare_for_without_feature_issues_warning() { + if !is_nightly() { + return + } + let p = project() + .file("src/lib.rs", "") + .build(); + + let stderr = "\ +[CHECKING] foo v0.0.1 ([..]) +warning: failed to find `#![feature(rust_2018_preview)]` in `src[/]lib.rs` +this may cause `cargo fix` to not be able to fix all +issues in preparation for the 2018 edition +[FINISHED] [..] +"; + assert_that( + p.cargo("fix --prepare-for 2018 --allow-no-vcs") + .masquerade_as_nightly_cargo(), + execs() + .with_stderr(stderr) + .with_status(0), + ); +}