Skip to content
This repository was archived by the owner on Nov 24, 2023. It is now read-only.

Filter by machine applicability #97

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 1 addition & 2 deletions cargo-fix/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,7 @@ pub fn run() -> Result<(), Error> {
//
// TODO: we probably want to do something fancy here like collect results
// from the client processes and print out a summary of what happened.
let status = cmd
.status()
let status = cmd.status()
.with_context(|e| format!("failed to execute `{}`: {}", cargo.to_string_lossy(), e))?;
exit_with(status);
}
Expand Down
10 changes: 7 additions & 3 deletions cargo-fix/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ use std::str;

use failure::{Error, ResultExt};
use rustfix::diagnostics::Diagnostic;
use termcolor::{ColorSpec, WriteColor, Color};
use termcolor::{Color, ColorSpec, WriteColor};

use diagnostics::{Message, output_stream};
use diagnostics::{output_stream, Message};

mod cli;
mod diagnostics;
Expand Down Expand Up @@ -174,6 +174,10 @@ fn rustfix_crate(rustc: &Path, filename: &str) -> Result<FixedCrate, Error> {
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")?;
Expand All @@ -185,7 +189,7 @@ fn rustfix_crate(rustc: &Path, filename: &str) -> Result<FixedCrate, Error> {
.filter_map(|line| serde_json::from_str::<Diagnostic>(line).ok())

// From each diagnostic try to extract suggestions from rustc
.filter_map(|diag| rustfix::collect_suggestions(&diag, &only));
.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();
Expand Down
6 changes: 5 additions & 1 deletion cargo-fix/tests/all/broken_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ fn fix_broken_if_requested() {
)
.build();

p.expect_cmd("cargo-fix fix --broken-code").status(0).run();
p.expect_cmd("cargo-fix fix --broken-code")
.env("__CARGO_FIX_YOLO", "true")
.status(0)
.run();
}

#[test]
Expand Down Expand Up @@ -111,6 +114,7 @@ fn broken_fixes_backed_out() {

// Attempt to fix code, but our shim will always fail the second compile
p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm does this mean that the lints we're testing against aren't currently classifed as "machine applicable"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm ok, could we perhaps start testing lints that are classified as machine applicable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, like I said, we don't have many -- just unreachable_pub, bare_trait_object, and abs_path_with_module. Do you want to rewrite the tests to always use those? I'd wait a bit for more applicability markers to land and then refactor the tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm ok, the 2018 compatibility lints are at least machine applicable, right? If so can you make sure that they're already tested and/or add a test for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right -- everything but the upgrade_extern_crate case work without the env var. Updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

As one final comment, could this be an extension method for just the test suite to do something like .fix_everything() instead of duplicating the env var? Other than that r=me!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

.cwd("bar")
.env("RUSTC", p.root.join("foo/target/debug/foo"))
.stderr_contains("not rust code")
Expand Down
23 changes: 10 additions & 13 deletions cargo-fix/tests/all/broken_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,18 @@
// .build();

// p.expect_cmd("cargo-fix fix")
// .env("__CARGO_FIX_YOLO", "true")
// .stderr_contains(r"warning: error applying suggestions to `src/lib.rs`")
// .stderr_contains("The full error message was:")
// .stderr_contains(
// "> Could not replace range 56...60 in file -- maybe parts of it were already replaced?",
// )
// .stderr_contains(
// "\
// This likely indicates a bug in either rustc or rustfix itself,\n\
// and we would appreciate a bug report! You're likely to see \n\
// a number of compiler warnings after this message which rustfix\n\
// attempted to fix but failed. If you could open an issue at\n\
// https://github.com/rust-lang-nursery/rustfix/issues\n\
// quoting the full output of this command we'd be very appreciative!\n\n\
// ",
// )
// .stderr_contains("> Could not replace range 56...60 in file -- maybe parts of it were already replaced?")
// .stderr_contains("\
// This likely indicates a bug in either rustc or rustfix itself,\n\
// and we would appreciate a bug report! You're likely to see \n\
// a number of compiler warnings after this message which rustfix\n\
// attempted to fix but failed. If you could open an issue at\n\
// https://github.com/rust-lang-nursery/rustfix/issues\n\
// quoting the full output of this command we'd be very appreciative!\n\n\
// ")
// .status(0)
// .run();
// }
1 change: 1 addition & 0 deletions cargo-fix/tests/all/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ fn fix_path_deps() {
[FINISHED] dev [unoptimized + debuginfo]
";
p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
.stdout("")
.stderr(stderr)
.run();
Expand Down
3 changes: 3 additions & 0 deletions cargo-fix/tests/all/edition_upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ fn local_paths() {
[FINISHED] dev [unoptimized + debuginfo]
";
p.expect_cmd("cargo-fix fix --prepare-for 2018")
.env("__CARGO_FIX_YOLO", "true")
.stdout("")
.stderr(stderr)
.run();
Expand Down Expand Up @@ -104,6 +105,7 @@ fn local_paths_no_fix() {
[FINISHED] dev [unoptimized + debuginfo]
";
p.expect_cmd("cargo-fix fix --prepare-for 2018")
.env("__CARGO_FIX_YOLO", "true")
.stdout("")
.stderr(stderr)
.run();
Expand Down Expand Up @@ -161,6 +163,7 @@ fn upgrade_extern_crate() {
[FINISHED] dev [unoptimized + debuginfo]
";
p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
.stdout("")
.stderr(stderr)
.run();
Expand Down
16 changes: 13 additions & 3 deletions cargo-fix/tests/all/smoke.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ fn fixes_extra_mut() {
[FINISHED] dev [unoptimized + debuginfo]
";
p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
.stdout("")
.stderr(stderr)
.run();
Expand All @@ -60,6 +61,7 @@ fn fixes_two_missing_ampersands() {
[FINISHED] dev [unoptimized + debuginfo]
";
p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
.stdout("")
.stderr(stderr)
.run();
Expand All @@ -85,6 +87,7 @@ fn tricky() {
[FINISHED] dev [unoptimized + debuginfo]
";
p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
.stdout("")
.stderr(stderr)
.run();
Expand All @@ -102,7 +105,9 @@ fn preserve_line_endings() {
)
.build();

p.expect_cmd("cargo-fix fix").run();
p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
.run();
assert!(p.read("src/lib.rs").contains("\r\n"));
}

Expand All @@ -118,7 +123,9 @@ fn fix_deny_warnings() {
)
.build();

p.expect_cmd("cargo-fix fix").run();
p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
.run();
}

#[test]
Expand All @@ -139,7 +146,9 @@ fn fix_deny_warnings_but_not_others() {
)
.build();

p.expect_cmd("cargo-fix fix").run();
p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
.run();
assert!(!p.read("src/lib.rs").contains("let mut x = 3;"));
assert!(p.read("src/lib.rs").contains("fn bar() {}"));
}
Expand Down Expand Up @@ -170,6 +179,7 @@ fn fix_two_files() {
.build();

p.expect_cmd("cargo-fix fix")
.env("__CARGO_FIX_YOLO", "true")
.stderr_contains("[FIXING] src/bar.rs (1 fix)")
.stderr_contains("[FIXING] src/lib.rs (1 fix)")
.run();
Expand Down
1 change: 1 addition & 0 deletions cargo-fix/tests/all/subtargets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ fn fixes_missing_ampersand() {
.build();

p.expect_cmd("cargo fix -- --all-targets")
.env("__CARGO_FIX_YOLO", "true")
.stdout("")
.stderr_contains("[COMPILING] foo v0.1.0 (CWD)")
.stderr_contains("[FIXING] build.rs (1 fix)")
Expand Down
5 changes: 4 additions & 1 deletion cargo-fix/tests/all/vcs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,8 @@ fn does_not_warn_about_clean_working_directory() {
.run();
p.expect_cmd("git config user.name RustFix").run();
p.expect_cmd("git commit -m Initial-commit").run();
p.expect_cmd("cargo-fix fix").check_vcs(true).status(0).run();
p.expect_cmd("cargo-fix fix")
.check_vcs(true)
.status(0)
.run();
}
6 changes: 5 additions & 1 deletion examples/fix-json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ fn main() -> Result<(), Error> {
};

let suggestions = fs::read_to_string(&suggestions_file)?;
let suggestions = rustfix::get_suggestions_from_json(&suggestions, &HashSet::new())?;
let suggestions = rustfix::get_suggestions_from_json(
&suggestions,
&HashSet::new(),
rustfix::Filter::Everything,
)?;

let source = fs::read_to_string(&source_file)?;

Expand Down
9 changes: 9 additions & 0 deletions src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,19 @@ pub struct DiagnosticSpan {
/// load the fully rendered version from the parent `Diagnostic`,
/// however.
pub suggested_replacement: Option<String>,
pub suggestion_applicability: Option<Applicability>,
/// Macro invocations that created the code at this span, if any.
expansion: Option<Box<DiagnosticSpanMacroExpansion>>,
}

#[derive(Copy, Clone, Debug, PartialEq, Deserialize)]
pub enum Applicability {
MachineApplicable,
HasPlaceholders,
MaybeIncorrect,
Unspecified,
}

#[derive(Deserialize, Debug)]
pub struct DiagnosticSpanLine {
pub text: String,
Expand Down
26 changes: 24 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,21 @@ pub mod diagnostics;
use diagnostics::{Diagnostic, DiagnosticSpan};
mod replace;

#[derive(Debug, Clone, Copy)]
pub enum Filter {
MachineApplicableOnly,
Everything,
}

pub fn get_suggestions_from_json<S: ::std::hash::BuildHasher>(
input: &str,
only: &HashSet<String, S>,
filter: Filter,
) -> serde_json::error::Result<Vec<Suggestion>> {
let mut result = Vec::new();
for cargo_msg in serde_json::Deserializer::from_str(input).into_iter::<Diagnostic>() {
// One diagnostic line might have multiple suggestions
result.extend(collect_suggestions(&cargo_msg?, only));
result.extend(collect_suggestions(&cargo_msg?, only, filter));
}
Ok(result)
}
Expand Down Expand Up @@ -142,6 +149,7 @@ fn collect_span(span: &DiagnosticSpan) -> Option<Replacement> {
pub fn collect_suggestions<S: ::std::hash::BuildHasher>(
diagnostic: &Diagnostic,
only: &HashSet<String, S>,
filter: Filter,
) -> Option<Suggestion> {
if !only.is_empty() {
if let Some(ref code) = diagnostic.code {
Expand All @@ -165,7 +173,21 @@ pub fn collect_suggestions<S: ::std::hash::BuildHasher>(
.children
.iter()
.filter_map(|child| {
let replacements: Vec<_> = child.spans.iter().filter_map(collect_span).collect();
let replacements: Vec<_> = child
.spans
.iter()
.filter(|span| {
use Filter::*;
use diagnostics::Applicability::*;

match (filter, &span.suggestion_applicability) {
(MachineApplicableOnly, Some(MachineApplicable)) => true,
(MachineApplicableOnly, _) => false,
(_, _) => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the left hand side of this match be Everything to get exhaustive matches to kick in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, good idea

}
})
.filter_map(collect_span)
.collect();
if replacements.len() == 1 {
Some(Solution {
message: child.message.clone(),
Expand Down
4 changes: 3 additions & 1 deletion tests/edge_cases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use std::fs;
#[test]
fn multiple_fix_options_yield_no_suggestions() {
let json = fs::read_to_string("./tests/edge-cases/skip-multi-option-lints.json").unwrap();
let expected_suggestions = rustfix::get_suggestions_from_json(&json, &HashSet::new()).unwrap();
let expected_suggestions =
rustfix::get_suggestions_from_json(&json, &HashSet::new(), rustfix::Filter::Everything)
.unwrap();
assert!(expected_suggestions.is_empty());
}
14 changes: 10 additions & 4 deletions tests/parse_and_replace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ fn compile(file: &Path, mode: &str) -> Result<Output, Error> {
"-Zunstable-options".into(),
"--emit=metadata".into(),
"--crate-name=rustfix_test".into(),
"-Zsuggestion-applicability".into(),
"--out-dir".into(),
tmp.path().into(),
];
Expand Down Expand Up @@ -142,12 +141,19 @@ fn test_rustfix_with_file<P: AsRef<Path>>(file: P, mode: &str) -> Result<(), Err
let json_file = file.with_extension("json");
let fixed_file = file.with_extension("fixed.rs");

let filter_suggestions = if mode == fixmode::EVERYTHING {
rustfix::Filter::Everything
} else {
rustfix::Filter::MachineApplicableOnly
};

debug!("next up: {:?}", file);
let code = read_file(file).context(format!("could not read {}", file.display()))?;
let errors = compile_and_get_json_errors(file, mode)
.context(format!("could compile {}", file.display()))?;
let suggestions = rustfix::get_suggestions_from_json(&errors, &HashSet::new())
.context("could not load suggestions")?;
let suggestions =
rustfix::get_suggestions_from_json(&errors, &HashSet::new(), filter_suggestions)
.context("could not load suggestions")?;

if std::env::var(settings::RECORD_JSON).is_ok() {
use std::io::Write;
Expand All @@ -163,7 +169,7 @@ fn test_rustfix_with_file<P: AsRef<Path>>(file: P, mode: &str) -> Result<(), Err
file.display()
))?;
let expected_suggestions =
rustfix::get_suggestions_from_json(&expected_json, &HashSet::new())
rustfix::get_suggestions_from_json(&expected_json, &HashSet::new(), filter_suggestions)
.context("could not load expected suggesitons")?;

ensure!(
Expand Down