From 3a880a2b39ccdabf821a60831beecf855a00c252 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 24 Mar 2023 12:22:15 +0800 Subject: [PATCH 1/3] Correct the bug report for `cargo clippy --fix` Signed-off-by: hi-rustin --- src/cargo/core/compiler/job_queue/mod.rs | 2 +- src/cargo/util/diagnostic_server.rs | 64 +++++++++++++++++++----- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/cargo/core/compiler/job_queue/mod.rs b/src/cargo/core/compiler/job_queue/mod.rs index 38ab0fe49a2..abae9bf516b 100644 --- a/src/cargo/core/compiler/job_queue/mod.rs +++ b/src/cargo/core/compiler/job_queue/mod.rs @@ -487,7 +487,7 @@ impl<'cfg> JobQueue<'cfg> { timings: self.timings, tokens: Vec::new(), pending_queue: Vec::new(), - print: DiagnosticPrinter::new(cx.bcx.config), + print: DiagnosticPrinter::new(cx.bcx.config, &cx.bcx.rustc().workspace_wrapper), finished: 0, per_package_future_incompat_reports: Vec::new(), }; diff --git a/src/cargo/util/diagnostic_server.rs b/src/cargo/util/diagnostic_server.rs index cc531426090..36215735be2 100644 --- a/src/cargo/util/diagnostic_server.rs +++ b/src/cargo/util/diagnostic_server.rs @@ -4,6 +4,7 @@ use std::collections::HashSet; use std::io::{BufReader, Read, Write}; use std::net::{Shutdown, SocketAddr, TcpListener, TcpStream}; +use std::path::PathBuf; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; use std::thread::{self, JoinHandle}; @@ -18,16 +19,6 @@ use crate::util::errors::CargoResult; use crate::util::Config; const DIAGNOSTICS_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/rust/issues\n\ - quoting the full output of this command we'd be very appreciative!\n\ - Note that you may be able to make some more progress in the near-term\n\ - fixing code with the `--broken-code` flag\n\n\ - "; #[derive(Deserialize, Serialize, Hash, Eq, PartialEq, Clone)] pub enum Message { @@ -83,15 +74,27 @@ impl Message { } } +/// A printer that will print diagnostics messages to the shell. pub struct DiagnosticPrinter<'a> { + /// The config to get the shell to print to. config: &'a Config, + /// An optional wrapper to be used in addition to `rustc.wrapper` for workspace crates. + /// This is used to get the correct bug report URL. For instance, + /// if `clippy-driver` is set as the value for the wrapper, + /// then the correct bug report URL for `clippy` can be obtained. + workspace_wrapper: &'a Option, + // A set of messages that have already been printed. dedupe: HashSet, } impl<'a> DiagnosticPrinter<'a> { - pub fn new(config: &'a Config) -> DiagnosticPrinter<'a> { + pub fn new( + config: &'a Config, + workspace_wrapper: &'a Option, + ) -> DiagnosticPrinter<'a> { DiagnosticPrinter { config, + workspace_wrapper, dedupe: HashSet::new(), } } @@ -128,7 +131,12 @@ impl<'a> DiagnosticPrinter<'a> { "The full error message was:\n\n> {}\n\n", message, )?; - write!(self.config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?; + let issue_link = get_bug_report_url(self.workspace_wrapper); + write!( + self.config.shell().err(), + "{}", + gen_please_report_this_bug_text(issue_link) + )?; Ok(()) } Message::FixFailed { @@ -159,7 +167,12 @@ impl<'a> DiagnosticPrinter<'a> { } writeln!(self.config.shell().err())?; } - write!(self.config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?; + let issue_link = get_bug_report_url(self.workspace_wrapper); + write!( + self.config.shell().err(), + "{}", + gen_please_report_this_bug_text(issue_link) + )?; if !errors.is_empty() { writeln!( self.config.shell().err(), @@ -218,6 +231,31 @@ https://doc.rust-lang.org/edition-guide/editions/transitioning-an-existing-proje } } +fn gen_please_report_this_bug_text(url: &str) -> String { + format!( + "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\ + {}\n\ + quoting the full output of this command we'd be very appreciative!\n\ + Note that you may be able to make some more progress in the near-term\n\ + fixing code with the `--broken-code` flag\n\n\ + ", + url + ) +} + +fn get_bug_report_url(rustc_workspace_wrapper: &Option) -> &str { + let clippy = std::ffi::OsStr::new("clippy-driver"); + let issue_link = match rustc_workspace_wrapper.as_ref().and_then(|x| x.file_stem()) { + Some(wrapper) if wrapper == clippy => "https://github.com/rust-lang/rust-clippy/issues", + _ => "https://github.com/rust-lang/rust/issues", + }; + + issue_link +} + #[derive(Debug)] pub struct RustfixDiagnosticServer { listener: TcpListener, From 47f6e2ddc940bf996c205e85be3bd6dbd4ebca2d Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sat, 25 Mar 2023 21:13:08 +0800 Subject: [PATCH 2/3] Add broken_clippy_fixes_backed_out Signed-off-by: hi-rustin --- tests/testsuite/fix.rs | 153 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 151 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 54a021c03c9..eb22e19328b 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -109,7 +109,6 @@ fn broken_fixes_backed_out() { fs::File::create(&first).unwrap(); } } - let status = Command::new("rustc") .args(env::args().skip(1)) .status() @@ -160,7 +159,157 @@ fn broken_fixes_backed_out() { 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\ - [..]\n\ + https://github.com/rust-lang/rust/issues\n\ + quoting the full output of this command we'd be very appreciative!\n\ + Note that you may be able to make some more progress in the near-term\n\ + fixing code with the `--broken-code` flag\n\ + \n\ + The following errors were reported:\n\ + error: expected one of `!` or `::`, found `rust`\n\ + ", + ) + .with_stderr_contains("Original diagnostics will follow.") + .with_stderr_contains("[WARNING] variable does not need to be mutable") + .with_stderr_does_not_contain("[..][FIXED][..]") + .run(); + + // Make sure the fix which should have been applied was backed out + assert!(p.read_file("bar/src/lib.rs").contains("let mut x = 3;")); +} + +#[cargo_test] +fn broken_clippy_fixes_backed_out() { + // A wrapper around `rustc` instead of calling `clippy` + let clippy_driver = project() + .at(cargo_test_support::paths::global_root().join("clippy-driver")) + .file("Cargo.toml", &basic_manifest("clippy-driver", "0.0.1")) + .file( + "src/main.rs", + r#" + fn main() { + let mut args = std::env::args_os(); + let _me = args.next().unwrap(); + let rustc = args.next().unwrap(); + let status = std::process::Command::new(rustc).args(args).status().unwrap(); + std::process::exit(status.code().unwrap_or(1)); + } + "#, + ) + .build(); + clippy_driver.cargo("build").run(); + + // This works as follows: + // - Create a `rustc` shim (the "foo" project) which will pretend that the + // verification step fails. + // - There is an empty build script so `foo` has `OUT_DIR` to track the steps. + // - The first "check", `foo` creates a file in OUT_DIR, and it completes + // successfully with a warning diagnostic to remove unused `mut`. + // - rustfix removes the `mut`. + // - The second "check" to verify the changes, `foo` swaps out the content + // with something that fails to compile. It creates a second file so it + // won't do anything in the third check. + // - cargo fix discovers that the fix failed, and it backs out the changes. + // - The third "check" is done to display the original diagnostics of the + // original code. + let p = project() + .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() { + // Ignore calls to things like --print=file-names and compiling build.rs. + // Also compatible for rustc invocations with `@path` argfile. + let is_lib_rs = env::args_os() + .map(PathBuf::from) + .flat_map(|p| if let Some(p) = p.to_str().unwrap_or_default().strip_prefix("@") { + fs::read_to_string(p).unwrap().lines().map(PathBuf::from).collect() + } else { + vec![p] + }) + .any(|l| l == Path::new("src/lib.rs")); + if is_lib_rs { + let path = PathBuf::from(env::var_os("OUT_DIR").unwrap()); + let first = path.join("first"); + let second = path.join("second"); + if first.exists() && !second.exists() { + fs::write("src/lib.rs", b"not rust code").unwrap(); + fs::File::create(&second).unwrap(); + } else { + fs::File::create(&first).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 + p.cargo("build").cwd("foo").run(); + + // Attempt to fix code, but our shim will always fail the second compile. + // Also, we use `clippy` as a workspace wrapper to make sure that we properly + // generate the report bug text. + p.cargo("fix --allow-no-vcs --lib") + .cwd("bar") + .env("__CARGO_FIX_YOLO", "1") + .env("RUSTC", p.root().join("foo/target/debug/foo")) + // We can't use `clippy` so we use a `rustc` workspace wrapper instead + .env( + "RUSTC_WORKSPACE_WRAPPER", + clippy_driver.bin("clippy-driver"), + ) + .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/rust-clippy/issues\n\ quoting the full output of this command we'd be very appreciative!\n\ Note that you may be able to make some more progress in the near-term\n\ fixing code with the `--broken-code` flag\n\ From 08169fd015c4b4aeff2118753c7adb5d873f908b Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 14 Apr 2023 11:23:41 +0800 Subject: [PATCH 3/3] Add `rustc_shim_for_cargo_fix` and `wrapped_clippy_driver` to reuse code Signed-off-by: hi-rustin --- crates/cargo-test-support/src/lib.rs | 23 +++++ tests/testsuite/check.rs | 26 +----- tests/testsuite/fix.rs | 126 +++------------------------ 3 files changed, 37 insertions(+), 138 deletions(-) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 04d6ce9f816..7003dd06aa6 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -517,6 +517,29 @@ pub fn cargo_exe() -> PathBuf { snapbox::cmd::cargo_bin("cargo") } +/// A wrapper around `rustc` instead of calling `clippy`. +pub fn wrapped_clippy_driver() -> PathBuf { + let clippy_driver = project() + .at(paths::global_root().join("clippy-driver")) + .file("Cargo.toml", &basic_manifest("clippy-driver", "0.0.1")) + .file( + "src/main.rs", + r#" + fn main() { + let mut args = std::env::args_os(); + let _me = args.next().unwrap(); + let rustc = args.next().unwrap(); + let status = std::process::Command::new(rustc).args(args).status().unwrap(); + std::process::exit(status.code().unwrap_or(1)); + } + "#, + ) + .build(); + clippy_driver.cargo("build").run(); + + clippy_driver.bin("clippy-driver") +} + /// This is the raw output from the process. /// /// This is similar to `std::process::Output`, however the `status` is diff --git a/tests/testsuite/check.rs b/tests/testsuite/check.rs index bbcf750fb2e..7bc9a38a3ad 100644 --- a/tests/testsuite/check.rs +++ b/tests/testsuite/check.rs @@ -6,8 +6,8 @@ use crate::messages::raw_rustc_output; use cargo_test_support::install::exe; use cargo_test_support::paths::CargoPathExt; use cargo_test_support::registry::Package; -use cargo_test_support::tools; use cargo_test_support::{basic_bin_manifest, basic_manifest, git, project}; +use cargo_test_support::{tools, wrapped_clippy_driver}; #[cargo_test] fn check_success() { @@ -1416,25 +1416,6 @@ fn check_fixable_mixed() { #[cargo_test] fn check_fixable_warning_for_clippy() { - // A wrapper around `rustc` instead of calling `clippy` - let clippy_driver = project() - .at(cargo_test_support::paths::global_root().join("clippy-driver")) - .file("Cargo.toml", &basic_manifest("clippy-driver", "0.0.1")) - .file( - "src/main.rs", - r#" - fn main() { - let mut args = std::env::args_os(); - let _me = args.next().unwrap(); - let rustc = args.next().unwrap(); - let status = std::process::Command::new(rustc).args(args).status().unwrap(); - std::process::exit(status.code().unwrap_or(1)); - } - "#, - ) - .build(); - clippy_driver.cargo("build").run(); - let foo = project() .file( "Cargo.toml", @@ -1452,10 +1433,7 @@ fn check_fixable_warning_for_clippy() { foo.cargo("check") // We can't use `clippy` so we use a `rustc` workspace wrapper instead - .env( - "RUSTC_WORKSPACE_WRAPPER", - clippy_driver.bin("clippy-driver"), - ) + .env("RUSTC_WORKSPACE_WRAPPER", wrapped_clippy_driver()) .with_stderr_contains("[..] (run `cargo clippy --fix --lib -p foo` to apply 1 suggestion)") .run(); } diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index eb22e19328b..33de721cd5b 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -5,8 +5,8 @@ use cargo_test_support::compare::assert_match_exact; use cargo_test_support::git::{self, init}; use cargo_test_support::paths::{self, CargoPathExt}; use cargo_test_support::registry::{Dependency, Package}; -use cargo_test_support::tools; -use cargo_test_support::{basic_manifest, is_nightly, project}; +use cargo_test_support::{basic_manifest, is_nightly, project, Project}; +use cargo_test_support::{tools, wrapped_clippy_driver}; #[cargo_test] fn do_not_fix_broken_builds() { @@ -53,8 +53,7 @@ fn fix_broken_if_requested() { .run(); } -#[cargo_test] -fn broken_fixes_backed_out() { +fn rustc_shim_for_cargo_fix() -> Project { // This works as follows: // - Create a `rustc` shim (the "foo" project) which will pretend that the // verification step fails. @@ -141,7 +140,13 @@ fn broken_fixes_backed_out() { // Build our rustc shim p.cargo("build").cwd("foo").run(); - // Attempt to fix code, but our shim will always fail the second compile + p +} + +#[cargo_test] +fn broken_fixes_backed_out() { + let p = rustc_shim_for_cargo_fix(); + // Attempt to fix code, but our shim will always fail the second compile. p.cargo("fix --allow-no-vcs --lib") .cwd("bar") .env("__CARGO_FIX_YOLO", "1") @@ -179,111 +184,7 @@ fn broken_fixes_backed_out() { #[cargo_test] fn broken_clippy_fixes_backed_out() { - // A wrapper around `rustc` instead of calling `clippy` - let clippy_driver = project() - .at(cargo_test_support::paths::global_root().join("clippy-driver")) - .file("Cargo.toml", &basic_manifest("clippy-driver", "0.0.1")) - .file( - "src/main.rs", - r#" - fn main() { - let mut args = std::env::args_os(); - let _me = args.next().unwrap(); - let rustc = args.next().unwrap(); - let status = std::process::Command::new(rustc).args(args).status().unwrap(); - std::process::exit(status.code().unwrap_or(1)); - } - "#, - ) - .build(); - clippy_driver.cargo("build").run(); - - // This works as follows: - // - Create a `rustc` shim (the "foo" project) which will pretend that the - // verification step fails. - // - There is an empty build script so `foo` has `OUT_DIR` to track the steps. - // - The first "check", `foo` creates a file in OUT_DIR, and it completes - // successfully with a warning diagnostic to remove unused `mut`. - // - rustfix removes the `mut`. - // - The second "check" to verify the changes, `foo` swaps out the content - // with something that fails to compile. It creates a second file so it - // won't do anything in the third check. - // - cargo fix discovers that the fix failed, and it backs out the changes. - // - The third "check" is done to display the original diagnostics of the - // original code. - let p = project() - .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() { - // Ignore calls to things like --print=file-names and compiling build.rs. - // Also compatible for rustc invocations with `@path` argfile. - let is_lib_rs = env::args_os() - .map(PathBuf::from) - .flat_map(|p| if let Some(p) = p.to_str().unwrap_or_default().strip_prefix("@") { - fs::read_to_string(p).unwrap().lines().map(PathBuf::from).collect() - } else { - vec![p] - }) - .any(|l| l == Path::new("src/lib.rs")); - if is_lib_rs { - let path = PathBuf::from(env::var_os("OUT_DIR").unwrap()); - let first = path.join("first"); - let second = path.join("second"); - if first.exists() && !second.exists() { - fs::write("src/lib.rs", b"not rust code").unwrap(); - fs::File::create(&second).unwrap(); - } else { - fs::File::create(&first).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 - p.cargo("build").cwd("foo").run(); - + let p = rustc_shim_for_cargo_fix(); // Attempt to fix code, but our shim will always fail the second compile. // Also, we use `clippy` as a workspace wrapper to make sure that we properly // generate the report bug text. @@ -292,10 +193,7 @@ fn broken_clippy_fixes_backed_out() { .env("__CARGO_FIX_YOLO", "1") .env("RUSTC", p.root().join("foo/target/debug/foo")) // We can't use `clippy` so we use a `rustc` workspace wrapper instead - .env( - "RUSTC_WORKSPACE_WRAPPER", - clippy_driver.bin("clippy-driver"), - ) + .env("RUSTC_WORKSPACE_WRAPPER", wrapped_clippy_driver()) .with_stderr_contains( "warning: failed to automatically apply fixes suggested by rustc \ to crate `bar`\n\