Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct the bug report for cargo clippy --fix #11882

Merged
merged 3 commits into from
Apr 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions crates/cargo-test-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/job_queue/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
};
Expand Down
64 changes: 51 additions & 13 deletions src/cargo/util/diagnostic_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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 {
Expand Down Expand Up @@ -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<PathBuf>,
// A set of messages that have already been printed.
dedupe: HashSet<Message>,
}

impl<'a> DiagnosticPrinter<'a> {
pub fn new(config: &'a Config) -> DiagnosticPrinter<'a> {
pub fn new(
config: &'a Config,
workspace_wrapper: &'a Option<PathBuf>,
) -> DiagnosticPrinter<'a> {
DiagnosticPrinter {
config,
workspace_wrapper,
dedupe: HashSet::new(),
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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<PathBuf>) -> &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,
Expand Down
26 changes: 2 additions & 24 deletions tests/testsuite/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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",
Expand All @@ -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();
}
Expand Down
61 changes: 54 additions & 7 deletions tests/testsuite/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -109,7 +108,6 @@ fn broken_fixes_backed_out() {
fs::File::create(&first).unwrap();
}
}

let status = Command::new("rustc")
.args(env::args().skip(1))
.status()
Expand Down Expand Up @@ -142,11 +140,60 @@ 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")
.env("RUSTC", p.root().join("foo/target/debug/foo"))
.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/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() {
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.
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", wrapped_clippy_driver())
.with_stderr_contains(
"warning: failed to automatically apply fixes suggested by rustc \
to crate `bar`\n\
Expand All @@ -160,7 +207,7 @@ 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-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\
Expand Down