Skip to content

Commit

Permalink
Rollup merge of #86137 - GuillaumeGomez:error-code-cleanup, r=Mark-Si…
Browse files Browse the repository at this point in the history 
…mulacrum

Error code cleanup and enforce checks

Fixes #86097.

It now checks if an error code is unused, and if so, will report an error if the error code wasn't commented out in the `error_codes.rs` file. It also checks that the constant used in the tidy check is up-to-date.

r? `@Mark-Simulacrum`
  • Loading branch information
@JohnTitor
JohnTitor committed Jun 24, 2021
2 parents f1e691d + 0ab9d01 commit 55fd13b
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 26 deletions.
9 changes: 4 additions & 5 deletions compiler/rustc_error_codes/src/error_codes.rs
View file
Expand Up @@ -609,7 +609,7 @@ E0783: include_str!("./error_codes/E0783.md"),
// E0540, // multiple rustc_deprecated attributes
E0544, // multiple stability levels
// E0548, // replaced with a generic attribute input check
E0553, // multiple rustc_const_unstable attributes
// E0553, // multiple rustc_const_unstable attributes
// E0555, // replaced with a generic attribute input check
// E0558, // replaced with a generic attribute input check
// E0563, // cannot determine a type for this `impl Trait` removed in 6383de15
Expand All @@ -620,10 +620,9 @@ E0783: include_str!("./error_codes/E0783.md"),
// E0612, // merged into E0609
// E0613, // Removed (merged with E0609)
E0625, // thread-local statics cannot be accessed at compile-time
E0629, // missing 'feature' (rustc_const_unstable)
// rustc_const_unstable attribute must be paired with stable/unstable
// attribute
E0630,
// E0629, // missing 'feature' (rustc_const_unstable)
// E0630, // rustc_const_unstable attribute must be paired with stable/unstable
// attribute
E0632, // cannot provide explicit generic arguments when `impl Trait` is
// used in argument position
E0640, // infer outlives requirements
Expand Down
114 changes: 93 additions & 21 deletions src/tools/tidy/src/error_codes_check.rs
View file
Expand Up @@ -6,20 +6,33 @@ use std::ffi::OsStr;
use std::fs::read_to_string;
use std::path::Path;

use regex::Regex;

// A few of those error codes can't be tested but all the others can and *should* be tested!
const EXEMPTED_FROM_TEST: &[&str] = &[
"E0227", "E0279", "E0280", "E0313", "E0314", "E0315", "E0377", "E0461", "E0462", "E0464",
"E0465", "E0473", "E0474", "E0475", "E0476", "E0479", "E0480", "E0481", "E0482", "E0483",
"E0484", "E0485", "E0486", "E0487", "E0488", "E0489", "E0514", "E0519", "E0523", "E0553",
"E0554", "E0570", "E0629", "E0630", "E0640", "E0717", "E0729",
"E0227", "E0279", "E0280", "E0313", "E0377", "E0461", "E0462", "E0464", "E0465", "E0476",
"E0482", "E0514", "E0519", "E0523", "E0554", "E0570", "E0640", "E0717", "E0729",
];

// Some error codes don't have any tests apparently...
const IGNORE_EXPLANATION_CHECK: &[&str] = &["E0570", "E0601", "E0602", "E0729"];

// If the file path contains any of these, we don't want to try to extract error codes from it.
//
// We need to declare each path in the windows version (with backslash).
const PATHS_TO_IGNORE_FOR_EXTRACTION: &[&str] =
&["src/test/", "src\\test\\", "src/doc/", "src\\doc\\", "src/tools/", "src\\tools\\"];

#[derive(Default, Debug)]
struct ErrorCodeStatus {
has_test: bool,
has_explanation: bool,
is_used: bool,
}

fn check_error_code_explanation(
f: &str,
error_codes: &mut HashMap<String, bool>,
error_codes: &mut HashMap<String, ErrorCodeStatus>,
err_code: String,
) -> bool {
let mut invalid_compile_fail_format = false;
Expand All @@ -30,15 +43,15 @@ fn check_error_code_explanation(
if s.starts_with("```") {
if s.contains("compile_fail") && s.contains('E') {
if !found_error_code {
error_codes.insert(err_code.clone(), true);
error_codes.get_mut(&err_code).map(|x| x.has_test = true);
found_error_code = true;
}
} else if s.contains("compile-fail") {
invalid_compile_fail_format = true;
}
} else if s.starts_with("#### Note: this error code is no longer emitted by the compiler") {
if !found_error_code {
error_codes.get_mut(&err_code).map(|x| *x = true);
error_codes.get_mut(&err_code).map(|x| x.has_test = true);
found_error_code = true;
}
}
Expand Down Expand Up @@ -77,7 +90,7 @@ macro_rules! some_or_continue {

fn extract_error_codes(
f: &str,
error_codes: &mut HashMap<String, bool>,
error_codes: &mut HashMap<String, ErrorCodeStatus>,
path: &Path,
errors: &mut Vec<String>,
) {
Expand All @@ -90,15 +103,16 @@ fn extract_error_codes(
.split_once(':')
.expect(
format!(
"Expected a line with the format `E0xxx: include_str!(\"..\")`, but got {} without a `:` delimiter",
"Expected a line with the format `E0xxx: include_str!(\"..\")`, but got {} \
without a `:` delimiter",
s,
).as_str()
)
.as_str(),
)
.0
.to_owned();
if !error_codes.contains_key(&err_code) {
error_codes.insert(err_code.clone(), false);
}
error_codes.entry(err_code.clone()).or_default().has_explanation = true;

// Now we extract the tests from the markdown file!
let md_file_name = match s.split_once("include_str!(\"") {
None => continue,
Expand Down Expand Up @@ -145,15 +159,15 @@ fn extract_error_codes(
.to_string();
if !error_codes.contains_key(&err_code) {
// this check should *never* fail!
error_codes.insert(err_code, false);
error_codes.insert(err_code, ErrorCodeStatus::default());
}
} else if s == ";" {
reached_no_explanation = true;
}
}
}

fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap<String, bool>) {
fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap<String, ErrorCodeStatus>) {
for line in f.lines() {
let s = line.trim();
if s.starts_with("error[E") || s.starts_with("warning[E") {
Expand All @@ -164,8 +178,24 @@ fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap<String, boo
Some((_, err_code)) => err_code,
},
};
let nb = error_codes.entry(err_code.to_owned()).or_insert(false);
*nb = true;
error_codes.entry(err_code.to_owned()).or_default().has_test = true;
}
}
}

fn extract_error_codes_from_source(
f: &str,
error_codes: &mut HashMap<String, ErrorCodeStatus>,
regex: &Regex,
) {
for line in f.lines() {
if line.trim_start().starts_with("//") {
continue;
}
for cap in regex.captures_iter(line) {
if let Some(error_code) = cap.get(1) {
error_codes.entry(error_code.as_str().to_owned()).or_default().is_used = true;
}
}
}
}
Expand All @@ -174,8 +204,17 @@ pub fn check(paths: &[&Path], bad: &mut bool) {
let mut errors = Vec::new();
let mut found_explanations = 0;
let mut found_tests = 0;
let mut error_codes: HashMap<String, ErrorCodeStatus> = HashMap::new();
// We want error codes which match the following cases:
//
// * foo(a, E0111, a)
// * foo(a, E0111)
// * foo(E0111, a)
// * #[error = "E0111"]
let regex = Regex::new(r#"[(,"\s](E\d{4})[,)"]"#).unwrap();

println!("Checking which error codes lack tests...");
let mut error_codes: HashMap<String, bool> = HashMap::new();

for path in paths {
super::walk(path, &mut |path| super::filter_dirs(path), &mut |entry, contents| {
let file_name = entry.file_name();
Expand All @@ -185,6 +224,11 @@ pub fn check(paths: &[&Path], bad: &mut bool) {
} else if entry.path().extension() == Some(OsStr::new("stderr")) {
extract_error_codes_from_tests(contents, &mut error_codes);
found_tests += 1;
} else if entry.path().extension() == Some(OsStr::new("rs")) {
let path = entry.path().to_string_lossy();
if PATHS_TO_IGNORE_FOR_EXTRACTION.iter().all(|c| !path.contains(c)) {
extract_error_codes_from_source(contents, &mut error_codes, &regex);
}
}
});
}
Expand All @@ -199,15 +243,43 @@ pub fn check(paths: &[&Path], bad: &mut bool) {
if errors.is_empty() {
println!("Found {} error codes", error_codes.len());

for (err_code, nb) in &error_codes {
if !*nb && !EXEMPTED_FROM_TEST.contains(&err_code.as_str()) {
for (err_code, error_status) in &error_codes {
if !error_status.has_test && !EXEMPTED_FROM_TEST.contains(&err_code.as_str()) {
errors.push(format!("Error code {} needs to have at least one UI test!", err_code));
} else if *nb && EXEMPTED_FROM_TEST.contains(&err_code.as_str()) {
} else if error_status.has_test && EXEMPTED_FROM_TEST.contains(&err_code.as_str()) {
errors.push(format!(
"Error code {} has a UI test, it shouldn't be listed into EXEMPTED_FROM_TEST!",
err_code
));
}
if !error_status.is_used && !error_status.has_explanation {
errors.push(format!(
"Error code {} isn't used and doesn't have an error explanation, it should be \
commented in error_codes.rs file",
err_code
));
}
}
}
if errors.is_empty() {
// Checking if local constants need to be cleaned.
for err_code in EXEMPTED_FROM_TEST {
match error_codes.get(err_code.to_owned()) {
Some(status) => {
if status.has_test {
errors.push(format!(
"{} error code has a test and therefore should be \
removed from the `EXEMPTED_FROM_TEST` constant",
err_code
));
}
}
None => errors.push(format!(
"{} error code isn't used anymore and therefore should be removed \
from `EXEMPTED_FROM_TEST` constant",
err_code
)),
}
}
}
errors.sort();
Expand Down

0 comments on commit 55fd13b

Please sign in to comment.