Skip to content

Commit

Permalink
Auto merge of #8844 - smoelius:fixed-paths, r=Alexendoo,xFrednet
Browse files Browse the repository at this point in the history
Check `.fixed` paths' existence in `run_ui`

This PR adds a test to check that there exists a `.fixed` file for every `.stderr` file in `tests/ui` that mentions a `MachineApplicable` lint. The test leverages `compiletest-rs`'s `rustfix_coverage` option.

I tried to add `.fixed` files where they appeared to be missing. However, 38 exceptional `.rs` files remain. Several of those include comments indicating that they are exceptions, though not all do. Apologies, as I have not tried to associate the 38 files with GH issues. (I think that would be a lot of work, and I worry about linking the wrong issue.)

changelog: none
  • Loading branch information
bors committed May 27, 2022
2 parents a5ece81 + 534fc5d commit 54f237f
Show file tree
Hide file tree
Showing 55 changed files with 1,807 additions and 442 deletions.
85 changes: 84 additions & 1 deletion tests/compile-test.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![feature(test)] // compiletest_rs requires this attribute
#![feature(once_cell)]
#![feature(is_sorted)]
#![cfg_attr(feature = "deny-warnings", deny(warnings))]
#![warn(rust_2018_idioms, unused_lifetimes)]

Expand Down Expand Up @@ -162,7 +163,8 @@ fn base_config(test_dir: &str) -> compiletest::Config {
}

fn run_ui() {
let config = base_config("ui");
let mut config = base_config("ui");
config.rustfix_coverage = true;
// use tests/clippy.toml
let _g = VarGuard::set("CARGO_MANIFEST_DIR", fs::canonicalize("tests").unwrap());
let _threads = VarGuard::set(
Expand All @@ -175,6 +177,7 @@ fn run_ui() {
}),
);
compiletest::run_tests(&config);
check_rustfix_coverage();
}

fn run_internal_tests() {
Expand Down Expand Up @@ -337,6 +340,86 @@ fn compile_test() {
run_internal_tests();
}

const RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS: &[&str] = &[
"assign_ops2.rs",
"cast_size_32bit.rs",
"char_lit_as_u8.rs",
"cmp_owned/without_suggestion.rs",
"crashes/ice-6250.rs",
"crashes/ice-6251.rs",
"dbg_macro.rs",
"deref_addrof_double_trigger.rs",
"doc/unbalanced_ticks.rs",
"eprint_with_newline.rs",
"explicit_counter_loop.rs",
"iter_skip_next_unfixable.rs",
"let_and_return.rs",
"literals.rs",
"map_flatten.rs",
"map_unwrap_or.rs",
"match_bool.rs",
"mem_replace_macro.rs",
"needless_arbitrary_self_type_unfixable.rs",
"needless_borrow_pat.rs",
"needless_for_each_unfixable.rs",
"nonminimal_bool.rs",
"print_literal.rs",
"print_with_newline.rs",
"redundant_static_lifetimes_multiple.rs",
"ref_binding_to_reference.rs",
"repl_uninit.rs",
"result_map_unit_fn_unfixable.rs",
"search_is_some.rs",
"single_component_path_imports_nested_first.rs",
"string_add.rs",
"toplevel_ref_arg_non_rustfix.rs",
"unit_arg.rs",
"unnecessary_clone.rs",
"unnecessary_lazy_eval_unfixable.rs",
"write_literal.rs",
"write_literal_2.rs",
"write_with_newline.rs",
];

fn check_rustfix_coverage() {
let missing_coverage_path = Path::new("target/debug/test/ui/rustfix_missing_coverage.txt");

if let Ok(missing_coverage_contents) = std::fs::read_to_string(missing_coverage_path) {
assert!(RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS.iter().is_sorted_by_key(Path::new));

for rs_path in missing_coverage_contents.lines() {
let filename = Path::new(rs_path).strip_prefix("tests/ui/").unwrap();
assert!(
RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS
.binary_search_by_key(&filename, Path::new)
.is_ok(),
"`{}` runs `MachineApplicable` diagnostics but is missing a `run-rustfix` annotation. \
Please either add `// run-rustfix` at the top of the file or add the file to \
`RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS` in `tests/compile-test.rs`.",
rs_path,
);
}
}
}

#[test]
fn rustfix_coverage_known_exceptions_accuracy() {
for filename in RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS {
let rs_path = Path::new("tests/ui").join(filename);
assert!(
rs_path.exists(),
"`{}` does not exists",
rs_path.strip_prefix(env!("CARGO_MANIFEST_DIR")).unwrap().display()
);
let fixed_path = rs_path.with_extension("fixed");
assert!(
!fixed_path.exists(),
"`{}` exists",
fixed_path.strip_prefix(env!("CARGO_MANIFEST_DIR")).unwrap().display()
);
}
}

/// Restores an env var on drop
#[must_use]
struct VarGuard {
Expand Down
62 changes: 62 additions & 0 deletions tests/ui/bind_instead_of_map_multipart.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// run-rustfix
#![deny(clippy::bind_instead_of_map)]
#![allow(clippy::blocks_in_if_conditions)]

pub fn main() {
let _ = Some("42").map(|s| if s.len() < 42 { 0 } else { s.len() });
let _ = Some("42").and_then(|s| if s.len() < 42 { None } else { Some(s.len()) });

let _ = Ok::<_, ()>("42").map(|s| if s.len() < 42 { 0 } else { s.len() });
let _ = Ok::<_, ()>("42").and_then(|s| if s.len() < 42 { Err(()) } else { Ok(s.len()) });

let _ = Err::<(), _>("42").map_err(|s| if s.len() < 42 { s.len() + 20 } else { s.len() });
let _ = Err::<(), _>("42").or_else(|s| if s.len() < 42 { Ok(()) } else { Err(s.len()) });

hard_example();
macro_example();
}

fn hard_example() {
Some("42").map(|s| {
if {
if s == "43" {
return 43;
}
s == "42"
} {
return 45;
}
match s.len() {
10 => 2,
20 => {
if foo() {
return {
if foo() {
return 20;
}
println!("foo");
3
};
}
20
},
40 => 30,
_ => 1,
}
});
}

fn foo() -> bool {
true
}

macro_rules! m {
() => {
Some(10)
};
}

fn macro_example() {
let _ = Some("").and_then(|s| if s.len() == 20 { m!() } else { Some(20) });
let _ = Some("").map(|s| if s.len() == 20 { m!() } else { Some(20) });
}
1 change: 1 addition & 0 deletions tests/ui/bind_instead_of_map_multipart.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// run-rustfix
#![deny(clippy::bind_instead_of_map)]
#![allow(clippy::blocks_in_if_conditions)]

Expand Down
12 changes: 6 additions & 6 deletions tests/ui/bind_instead_of_map_multipart.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:5:13
--> $DIR/bind_instead_of_map_multipart.rs:6:13
|
LL | let _ = Some("42").and_then(|s| if s.len() < 42 { Some(0) } else { Some(s.len()) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/bind_instead_of_map_multipart.rs:1:9
--> $DIR/bind_instead_of_map_multipart.rs:2:9
|
LL | #![deny(clippy::bind_instead_of_map)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -15,7 +15,7 @@ LL | let _ = Some("42").map(|s| if s.len() < 42 { 0 } else { s.len() });
| ~~~ ~ ~~~~~~~

error: using `Result.and_then(|x| Ok(y))`, which is more succinctly expressed as `map(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:8:13
--> $DIR/bind_instead_of_map_multipart.rs:9:13
|
LL | let _ = Ok::<_, ()>("42").and_then(|s| if s.len() < 42 { Ok(0) } else { Ok(s.len()) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -26,7 +26,7 @@ LL | let _ = Ok::<_, ()>("42").map(|s| if s.len() < 42 { 0 } else { s.len()
| ~~~ ~ ~~~~~~~

error: using `Result.or_else(|x| Err(y))`, which is more succinctly expressed as `map_err(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:11:13
--> $DIR/bind_instead_of_map_multipart.rs:12:13
|
LL | let _ = Err::<(), _>("42").or_else(|s| if s.len() < 42 { Err(s.len() + 20) } else { Err(s.len()) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -37,7 +37,7 @@ LL | let _ = Err::<(), _>("42").map_err(|s| if s.len() < 42 { s.len() + 20 }
| ~~~~~~~ ~~~~~~~~~~~~ ~~~~~~~

error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:19:5
--> $DIR/bind_instead_of_map_multipart.rs:20:5
|
LL | / Some("42").and_then(|s| {
LL | | if {
Expand All @@ -59,7 +59,7 @@ LL | s == "42"
...

error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:60:13
--> $DIR/bind_instead_of_map_multipart.rs:61:13
|
LL | let _ = Some("").and_then(|s| if s.len() == 20 { Some(m!()) } else { Some(Some(20)) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
12 changes: 12 additions & 0 deletions tests/ui/crashes/ice-7169.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// run-rustfix
#![allow(dead_code)]

#[derive(Default)]
struct A<T> {
a: Vec<A<T>>,
b: T,
}

fn main() {
if Ok::<_, ()>(A::<String>::default()).is_ok() {}
}
3 changes: 3 additions & 0 deletions tests/ui/crashes/ice-7169.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// run-rustfix
#![allow(dead_code)]

#[derive(Default)]
struct A<T> {
a: Vec<A<T>>,
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/crashes/ice-7169.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: redundant pattern matching, consider using `is_ok()`
--> $DIR/ice-7169.rs:8:12
--> $DIR/ice-7169.rs:11:12
|
LL | if let Ok(_) = Ok::<_, ()>(A::<String>::default()) {}
| -------^^^^^-------------------------------------- help: try this: `if Ok::<_, ()>(A::<String>::default()).is_ok()`
Expand Down
7 changes: 7 additions & 0 deletions tests/ui/crashes/ice-8250.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// run-rustfix
fn _f(s: &str) -> Option<()> {
let _ = s[1..].split('.').next()?;
Some(())
}

fn main() {}
1 change: 1 addition & 0 deletions tests/ui/crashes/ice-8250.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// run-rustfix
fn _f(s: &str) -> Option<()> {
let _ = s[1..].splitn(2, '.').next()?;
Some(())
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/crashes/ice-8250.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: unnecessary use of `splitn`
--> $DIR/ice-8250.rs:2:13
--> $DIR/ice-8250.rs:3:13
|
LL | let _ = s[1..].splitn(2, '.').next()?;
| ^^^^^^^^^^^^^^^^^^^^^ help: try this: `s[1..].split('.')`
Expand Down
9 changes: 9 additions & 0 deletions tests/ui/crashes/ice-8821.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// run-rustfix
#![warn(clippy::let_unit_value)]

fn f() {}
static FN: fn() = f;

fn main() {
FN();
}
1 change: 1 addition & 0 deletions tests/ui/crashes/ice-8821.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// run-rustfix
#![warn(clippy::let_unit_value)]

fn f() {}
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/crashes/ice-8821.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: this let-binding has unit value
--> $DIR/ice-8821.rs:7:5
--> $DIR/ice-8821.rs:8:5
|
LL | let _: () = FN();
| ^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `FN();`
Expand Down
Loading

0 comments on commit 54f237f

Please sign in to comment.