From 23e5ed1288018a9b17b0b741dd6c36b98c9a415e Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 8 Jun 2021 16:06:33 +0200 Subject: [PATCH 1/5] Check if error code is used --- src/tools/tidy/src/error_codes_check.rs | 115 ++++++++++++++++++++---- 1 file changed, 99 insertions(+), 16 deletions(-) diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index d6e0ebaa541..b8e0042948f 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -1,11 +1,14 @@ //! Checks that all error codes have at least one test to prevent having error //! codes that are silently not thrown by the compiler anymore. +use std::collections::hash_map::Entry; use std::collections::HashMap; 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", @@ -17,9 +20,16 @@ const EXEMPTED_FROM_TEST: &[&str] = &[ // Some error codes don't have any tests apparently... const IGNORE_EXPLANATION_CHECK: &[&str] = &["E0570", "E0601", "E0602", "E0729"]; +#[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, + error_codes: &mut HashMap, err_code: String, ) -> bool { let mut invalid_compile_fail_format = false; @@ -30,7 +40,7 @@ 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") { @@ -38,7 +48,7 @@ fn check_error_code_explanation( } } 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; } } @@ -77,7 +87,7 @@ macro_rules! some_or_continue { fn extract_error_codes( f: &str, - error_codes: &mut HashMap, + error_codes: &mut HashMap, path: &Path, errors: &mut Vec, ) { @@ -90,14 +100,26 @@ 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); + match error_codes.entry(err_code.clone()) { + Entry::Occupied(mut e) => { + let mut entry = e.get_mut(); + entry.has_explanation = true + } + Entry::Vacant(e) => { + e.insert(ErrorCodeStatus { + has_test: false, + is_used: false, + has_explanation: true, + }); + } } // Now we extract the tests from the markdown file! let md_file_name = match s.split_once("include_str!(\"") { @@ -145,7 +167,7 @@ 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; @@ -153,7 +175,7 @@ fn extract_error_codes( } } -fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap) { +fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap) { for line in f.lines() { let s = line.trim(); if s.starts_with("error[E") || s.starts_with("warning[E") { @@ -164,8 +186,48 @@ fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap err_code, }, }; - let nb = error_codes.entry(err_code.to_owned()).or_insert(false); - *nb = true; + match error_codes.entry(err_code.to_owned()) { + Entry::Occupied(mut e) => { + let mut entry = e.get_mut(); + entry.has_test = true + } + Entry::Vacant(e) => { + e.insert(ErrorCodeStatus { + has_test: true, + is_used: false, + has_explanation: false, + }); + } + } + } + } +} + +fn extract_error_codes_from_source( + f: &str, + error_codes: &mut HashMap, + 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) { + match error_codes.entry(error_code.as_str().to_owned()) { + Entry::Occupied(mut e) => { + let mut entry = e.get_mut(); + entry.is_used = true + } + Entry::Vacant(e) => { + e.insert(ErrorCodeStatus { + has_test: false, + is_used: true, + has_explanation: false, + }); + } + } + } } } } @@ -174,8 +236,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 = 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 = HashMap::new(); + for path in paths { super::walk(path, &mut |path| super::filter_dirs(path), &mut |entry, contents| { let file_name = entry.file_name(); @@ -185,6 +256,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 ["src/test/", "src/doc/", "src/tools/"].iter().all(|c| !path.contains(c)) { + extract_error_codes_from_source(contents, &mut error_codes, ®ex); + } } }); } @@ -199,15 +275,22 @@ 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 + )); + } } } errors.sort(); From 22a702d016b25b8c800b9707a5bd416fb6623b79 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 8 Jun 2021 16:49:41 +0200 Subject: [PATCH 2/5] Add check on constant to ensure it's up to date --- src/tools/tidy/src/error_codes_check.rs | 29 +++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index b8e0042948f..b6775589e97 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -11,10 +11,10 @@ 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", "E0315", "E0377", "E0461", "E0462", "E0464", "E0465", + "E0473", "E0474", "E0475", "E0476", "E0479", "E0480", "E0481", "E0482", "E0483", "E0484", + "E0485", "E0486", "E0487", "E0488", "E0489", "E0514", "E0519", "E0523", "E0554", "E0570", + "E0640", "E0717", "E0729", ]; // Some error codes don't have any tests apparently... @@ -293,6 +293,27 @@ pub fn check(paths: &[&Path], bad: &mut bool) { } } } + 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(); for err in &errors { eprintln!("{}", err); From 12b6d32387c397647c9ae4b8880133033f443ab9 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 8 Jun 2021 16:37:59 +0200 Subject: [PATCH 3/5] Remove unused error codes from error_codes.rs and from EXEMPTED_FROM_TEST constant --- compiler/rustc_error_codes/src/error_codes.rs | 9 ++++----- src/tools/tidy/src/error_codes_check.rs | 6 ++---- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_error_codes/src/error_codes.rs b/compiler/rustc_error_codes/src/error_codes.rs index f10efd83236..ff7a2344e69 100644 --- a/compiler/rustc_error_codes/src/error_codes.rs +++ b/compiler/rustc_error_codes/src/error_codes.rs @@ -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 @@ -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 diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index b6775589e97..8d816296675 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -11,10 +11,8 @@ 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", "E0315", "E0377", "E0461", "E0462", "E0464", "E0465", - "E0473", "E0474", "E0475", "E0476", "E0479", "E0480", "E0481", "E0482", "E0483", "E0484", - "E0485", "E0486", "E0487", "E0488", "E0489", "E0514", "E0519", "E0523", "E0554", "E0570", - "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... From 20f1b1cc0a99492b04a6a5994fa5456fd8c54ab9 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 23 Jun 2021 16:42:35 +0200 Subject: [PATCH 4/5] Greatly improve code --- src/tools/tidy/src/error_codes_check.rs | 44 +++---------------------- 1 file changed, 4 insertions(+), 40 deletions(-) diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index 8d816296675..d1d43e9449c 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -1,7 +1,6 @@ //! Checks that all error codes have at least one test to prevent having error //! codes that are silently not thrown by the compiler anymore. -use std::collections::hash_map::Entry; use std::collections::HashMap; use std::ffi::OsStr; use std::fs::read_to_string; @@ -106,19 +105,8 @@ fn extract_error_codes( ) .0 .to_owned(); - match error_codes.entry(err_code.clone()) { - Entry::Occupied(mut e) => { - let mut entry = e.get_mut(); - entry.has_explanation = true - } - Entry::Vacant(e) => { - e.insert(ErrorCodeStatus { - has_test: false, - is_used: false, - has_explanation: true, - }); - } - } + 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, @@ -184,19 +172,7 @@ fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap err_code, }, }; - match error_codes.entry(err_code.to_owned()) { - Entry::Occupied(mut e) => { - let mut entry = e.get_mut(); - entry.has_test = true - } - Entry::Vacant(e) => { - e.insert(ErrorCodeStatus { - has_test: true, - is_used: false, - has_explanation: false, - }); - } - } + error_codes.entry(err_code.to_owned()).or_default().has_test = true; } } } @@ -212,19 +188,7 @@ fn extract_error_codes_from_source( } for cap in regex.captures_iter(line) { if let Some(error_code) = cap.get(1) { - match error_codes.entry(error_code.as_str().to_owned()) { - Entry::Occupied(mut e) => { - let mut entry = e.get_mut(); - entry.is_used = true - } - Entry::Vacant(e) => { - e.insert(ErrorCodeStatus { - has_test: false, - is_used: true, - has_explanation: false, - }); - } - } + error_codes.entry(error_code.as_str().to_owned()).or_default().is_used = true; } } } From 0ab9d01dbdc2f495fc4e62510195d5729fd5d57c Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 23 Jun 2021 23:56:16 +0200 Subject: [PATCH 5/5] Handle windows paths as well --- src/tools/tidy/src/error_codes_check.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index d1d43e9449c..63fbee34bd6 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -17,6 +17,12 @@ const EXEMPTED_FROM_TEST: &[&str] = &[ // 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, @@ -220,7 +226,7 @@ pub fn check(paths: &[&Path], bad: &mut bool) { found_tests += 1; } else if entry.path().extension() == Some(OsStr::new("rs")) { let path = entry.path().to_string_lossy(); - if ["src/test/", "src/doc/", "src/tools/"].iter().all(|c| !path.contains(c)) { + if PATHS_TO_IGNORE_FOR_EXTRACTION.iter().all(|c| !path.contains(c)) { extract_error_codes_from_source(contents, &mut error_codes, ®ex); } }