-
Notifications
You must be signed in to change notification settings - Fork 13.4k
refactor: clean up errors.rs
and error_codes_check.rs
#106341
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
Merged
bors
merged 8 commits into
rust-lang:master
from
Ezrashaw:refactor-error-code-tidy-check
Jan 7, 2023
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
04b9038
refactor: clean up `errors.rs` and `error_codes_check.rs`
Ezrashaw 9618f64
docs: revert removal of `E0729`
Ezrashaw 24671b7
use more paths in error codes
Ezrashaw def1b7c
fix CI error
Ezrashaw fafb18e
fixup warnings
Ezrashaw 1f1dd5f
pattern destructure `has_test`
Ezrashaw b7341db
fix CI
Ezrashaw c00ab4b
print highest error code
Ezrashaw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,381 @@ | ||
//! Tidy check to ensure error codes are properly documented and tested. | ||
//! | ||
//! Overview of check: | ||
//! | ||
//! 1. We create a list of error codes used by the compiler. Error codes are extracted from `compiler/rustc_error_codes/src/error_codes.rs`. | ||
//! | ||
//! 2. We check that the error code has a long-form explanation in `compiler/rustc_error_codes/src/error_codes/`. | ||
//! - The explanation is expected to contain a `doctest` that fails with the correct error code. (`EXEMPT_FROM_DOCTEST` *currently* bypasses this check) | ||
//! - Note that other stylistic conventions for markdown files are checked in the `style.rs` tidy check. | ||
//! | ||
//! 3. We check that the error code has a UI test in `src/test/ui/error-codes/`. | ||
//! - We ensure that there is both a `Exxxx.rs` file and a corresponding `Exxxx.stderr` file. | ||
//! - We also ensure that the error code is used in the tests. | ||
//! - *Currently*, it is possible to opt-out of this check with the `EXEMPTED_FROM_TEST` constant. | ||
//! | ||
//! 4. We check that the error code is actually emitted by the compiler. | ||
//! - This is done by searching `compiler/` with a regex. | ||
//! | ||
//! This tidy check was merged and refactored from two others. See #PR_NUM for information about linting changes that occurred during this refactor. | ||
|
||
use std::{ffi::OsStr, fs, path::Path}; | ||
|
||
use regex::Regex; | ||
|
||
use crate::walk::{filter_dirs, walk, walk_many}; | ||
|
||
const ERROR_CODES_PATH: &str = "compiler/rustc_error_codes/src/error_codes.rs"; | ||
const ERROR_DOCS_PATH: &str = "compiler/rustc_error_codes/src/error_codes/"; | ||
const ERROR_TESTS_PATH: &str = "src/test/ui/error-codes/"; | ||
|
||
// Error codes that (for some reason) can't have a doctest in their explanation. Error codes are still expected to provide a code example, even if untested. | ||
const IGNORE_DOCTEST_CHECK: &[&str] = &["E0464", "E0570", "E0601", "E0602"]; | ||
|
||
// Error codes that don't yet have a UI test. This list will eventually be removed. | ||
const IGNORE_UI_TEST_CHECK: &[&str] = &[ | ||
"E0313", "E0461", "E0465", "E0476", "E0490", "E0514", "E0523", "E0554", "E0640", "E0717", | ||
"E0729", "E0789", | ||
]; | ||
|
||
macro_rules! verbose_print { | ||
($verbose:expr, $($fmt:tt)*) => { | ||
if $verbose { | ||
println!("{}", format_args!($($fmt)*)); | ||
} | ||
}; | ||
} | ||
|
||
pub fn check(root_path: &Path, search_paths: &[&Path], verbose: bool, bad: &mut bool) { | ||
let mut errors = Vec::new(); | ||
|
||
// Stage 1: create list | ||
let error_codes = extract_error_codes(root_path, &mut errors, verbose); | ||
println!("Found {} error codes", error_codes.len()); | ||
println!("Highest error code: `{}`", error_codes.iter().max().unwrap()); | ||
|
||
// Stage 2: check list has docs | ||
let no_longer_emitted = check_error_codes_docs(root_path, &error_codes, &mut errors, verbose); | ||
|
||
// Stage 3: check list has UI tests | ||
check_error_codes_tests(root_path, &error_codes, &mut errors, verbose); | ||
|
||
// Stage 4: check list is emitted by compiler | ||
check_error_codes_used(search_paths, &error_codes, &mut errors, &no_longer_emitted, verbose); | ||
|
||
// Print any errors. | ||
for error in errors { | ||
tidy_error!(bad, "{}", error); | ||
} | ||
} | ||
|
||
/// Stage 1: Parses a list of error codes from `error_codes.rs`. | ||
fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>, verbose: bool) -> Vec<String> { | ||
let path = root_path.join(Path::new(ERROR_CODES_PATH)); | ||
let file = | ||
fs::read_to_string(&path).unwrap_or_else(|e| panic!("failed to read `{path:?}`: {e}")); | ||
|
||
let mut error_codes = Vec::new(); | ||
let mut reached_undocumented_codes = false; | ||
|
||
for line in file.lines() { | ||
let line = line.trim(); | ||
|
||
if !reached_undocumented_codes && line.starts_with('E') { | ||
let split_line = line.split_once(':'); | ||
|
||
// Extract the error code from the line, emitting a fatal error if it is not in a correct format. | ||
let err_code = if let Some(err_code) = split_line { | ||
err_code.0.to_owned() | ||
} else { | ||
errors.push(format!( | ||
"Expected a line with the format `Exxxx: include_str!(\"..\")`, but got \"{}\" \ | ||
without a `:` delimiter", | ||
line, | ||
)); | ||
continue; | ||
}; | ||
|
||
// If this is a duplicate of another error code, emit a fatal error. | ||
if error_codes.contains(&err_code) { | ||
errors.push(format!("Found duplicate error code: `{}`", err_code)); | ||
continue; | ||
} | ||
|
||
// Ensure that the line references the correct markdown file. | ||
let expected_filename = format!(" include_str!(\"./error_codes/{}.md\"),", err_code); | ||
if expected_filename != split_line.unwrap().1 { | ||
errors.push(format!( | ||
"Error code `{}` expected to reference docs with `{}` but instead found `{}` in \ | ||
`compiler/rustc_error_codes/src/error_codes.rs`", | ||
err_code, | ||
expected_filename, | ||
split_line.unwrap().1, | ||
)); | ||
continue; | ||
} | ||
|
||
error_codes.push(err_code); | ||
} else if reached_undocumented_codes && line.starts_with('E') { | ||
let err_code = match line.split_once(',') { | ||
None => line, | ||
Some((err_code, _)) => err_code, | ||
} | ||
.to_string(); | ||
|
||
verbose_print!(verbose, "warning: Error code `{}` is undocumented.", err_code); | ||
|
||
if error_codes.contains(&err_code) { | ||
errors.push(format!("Found duplicate error code: `{}`", err_code)); | ||
} | ||
|
||
error_codes.push(err_code); | ||
} else if line == ";" { | ||
// Once we reach the undocumented error codes, adapt to different syntax. | ||
reached_undocumented_codes = true; | ||
} | ||
} | ||
|
||
error_codes | ||
} | ||
|
||
/// Stage 2: Checks that long-form error code explanations exist and have doctests. | ||
fn check_error_codes_docs( | ||
root_path: &Path, | ||
error_codes: &[String], | ||
errors: &mut Vec<String>, | ||
verbose: bool, | ||
) -> Vec<String> { | ||
let docs_path = root_path.join(Path::new(ERROR_DOCS_PATH)); | ||
|
||
let mut no_longer_emitted_codes = Vec::new(); | ||
|
||
walk(&docs_path, &mut |_| false, &mut |entry, contents| { | ||
let path = entry.path(); | ||
|
||
// Error if the file isn't markdown. | ||
if path.extension() != Some(OsStr::new("md")) { | ||
errors.push(format!( | ||
"Found unexpected non-markdown file in error code docs directory: {}", | ||
path.display() | ||
)); | ||
return; | ||
} | ||
|
||
// Make sure that the file is referenced in `error_codes.rs` | ||
let filename = path.file_name().unwrap().to_str().unwrap().split_once('.'); | ||
let err_code = filename.unwrap().0; // `unwrap` is ok because we know the filename is in the correct format. | ||
|
||
if error_codes.iter().all(|e| e != err_code) { | ||
errors.push(format!( | ||
"Found valid file `{}` in error code docs directory without corresponding \ | ||
entry in `error_code.rs`", | ||
path.display() | ||
)); | ||
return; | ||
} | ||
|
||
let (found_code_example, found_proper_doctest, emit_ignore_warning, emit_no_longer_warning) = | ||
check_explanation_has_doctest(&contents, &err_code); | ||
if emit_ignore_warning { | ||
verbose_print!( | ||
verbose, | ||
"warning: Error code `{err_code}` uses the ignore header. This should not be used, add the error code to the \ | ||
`IGNORE_DOCTEST_CHECK` constant instead." | ||
); | ||
} | ||
if emit_no_longer_warning { | ||
no_longer_emitted_codes.push(err_code.to_owned()); | ||
verbose_print!( | ||
verbose, | ||
"warning: Error code `{err_code}` is no longer emitted and should be removed entirely." | ||
); | ||
} | ||
if !found_code_example { | ||
verbose_print!( | ||
verbose, | ||
"warning: Error code `{err_code}` doesn't have a code example, all error codes are expected to have one \ | ||
(even if untested)." | ||
); | ||
} | ||
|
||
let test_ignored = IGNORE_DOCTEST_CHECK.contains(&&err_code); | ||
|
||
// Check that the explanation has a doctest, and if it shouldn't, that it doesn't | ||
if !found_proper_doctest && !test_ignored { | ||
errors.push(format!( | ||
"`{}` doesn't use its own error code in compile_fail example", | ||
path.display(), | ||
)); | ||
} else if found_proper_doctest && test_ignored { | ||
errors.push(format!( | ||
"`{}` has a compile_fail doctest with its own error code, it shouldn't \ | ||
be listed in `IGNORE_DOCTEST_CHECK`", | ||
path.display(), | ||
)); | ||
} | ||
}); | ||
|
||
no_longer_emitted_codes | ||
} | ||
|
||
/// This function returns a tuple indicating whether the provided explanation: | ||
/// a) has a code example, tested or not. | ||
/// b) has a valid doctest | ||
fn check_explanation_has_doctest(explanation: &str, err_code: &str) -> (bool, bool, bool, bool) { | ||
Ezrashaw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let mut found_code_example = false; | ||
let mut found_proper_doctest = false; | ||
|
||
let mut emit_ignore_warning = false; | ||
let mut emit_no_longer_warning = false; | ||
|
||
for line in explanation.lines() { | ||
let line = line.trim(); | ||
|
||
if line.starts_with("```") { | ||
found_code_example = true; | ||
|
||
// Check for the `rustdoc` doctest headers. | ||
if line.contains("compile_fail") && line.contains(err_code) { | ||
found_proper_doctest = true; | ||
} | ||
|
||
if line.contains("ignore") { | ||
emit_ignore_warning = true; | ||
found_proper_doctest = true; | ||
} | ||
} else if line | ||
.starts_with("#### Note: this error code is no longer emitted by the compiler") | ||
{ | ||
emit_no_longer_warning = true; | ||
found_code_example = true; | ||
found_proper_doctest = true; | ||
} | ||
} | ||
|
||
(found_code_example, found_proper_doctest, emit_ignore_warning, emit_no_longer_warning) | ||
} | ||
|
||
// Stage 3: Checks that each error code has a UI test in the correct directory | ||
fn check_error_codes_tests( | ||
root_path: &Path, | ||
error_codes: &[String], | ||
errors: &mut Vec<String>, | ||
verbose: bool, | ||
) { | ||
let tests_path = root_path.join(Path::new(ERROR_TESTS_PATH)); | ||
|
||
for code in error_codes { | ||
let test_path = tests_path.join(format!("{}.stderr", code)); | ||
|
||
if !test_path.exists() && !IGNORE_UI_TEST_CHECK.contains(&code.as_str()) { | ||
verbose_print!( | ||
verbose, | ||
"warning: Error code `{code}` needs to have at least one UI test in the `src/test/ui/error-codes/` directory`!" | ||
); | ||
continue; | ||
} | ||
if IGNORE_UI_TEST_CHECK.contains(&code.as_str()) { | ||
if test_path.exists() { | ||
errors.push(format!( | ||
"Error code `{code}` has a UI test in `src/test/ui/error-codes/{code}.rs`, it shouldn't be listed in `EXEMPTED_FROM_TEST`!" | ||
)); | ||
} | ||
continue; | ||
} | ||
|
||
let file = match fs::read_to_string(&test_path) { | ||
Ok(file) => file, | ||
Err(err) => { | ||
verbose_print!( | ||
verbose, | ||
"warning: Failed to read UI test file (`{}`) for `{code}` but the file exists. The test is assumed to work:\n{err}", | ||
test_path.display() | ||
); | ||
continue; | ||
} | ||
}; | ||
|
||
let mut found_code = false; | ||
|
||
for line in file.lines() { | ||
let s = line.trim(); | ||
// Assuming the line starts with `error[E`, we can substring the error code out. | ||
if s.starts_with("error[E") { | ||
if &s[6..11] == code { | ||
found_code = true; | ||
break; | ||
} | ||
}; | ||
} | ||
|
||
if !found_code { | ||
verbose_print!( | ||
verbose, | ||
"warning: Error code {code}`` has a UI test file, but doesn't contain its own error code!" | ||
); | ||
} | ||
} | ||
} | ||
|
||
/// Stage 4: Search `compiler/` and ensure that every error code is actually used by the compiler and that no undocumented error codes exist. | ||
fn check_error_codes_used( | ||
search_paths: &[&Path], | ||
error_codes: &[String], | ||
errors: &mut Vec<String>, | ||
no_longer_emitted: &[String], | ||
verbose: bool, | ||
) { | ||
// 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(); | ||
Ezrashaw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
let mut found_codes = Vec::new(); | ||
|
||
walk_many(search_paths, &mut filter_dirs, &mut |entry, contents| { | ||
let path = entry.path(); | ||
|
||
// Return early if we aren't looking at a source file. | ||
if path.extension() != Some(OsStr::new("rs")) { | ||
return; | ||
} | ||
|
||
for line in contents.lines() { | ||
// We want to avoid parsing error codes in comments. | ||
if line.trim_start().starts_with("//") { | ||
continue; | ||
} | ||
|
||
for cap in regex.captures_iter(line) { | ||
if let Some(error_code) = cap.get(1) { | ||
let error_code = error_code.as_str().to_owned(); | ||
|
||
if !error_codes.contains(&error_code) { | ||
// This error code isn't properly defined, we must error. | ||
errors.push(format!("Error code `{}` is used in the compiler but not defined and documented in `compiler/rustc_error_codes/src/error_codes.rs`.", error_code)); | ||
continue; | ||
} | ||
|
||
// This error code can now be marked as used. | ||
found_codes.push(error_code); | ||
} | ||
} | ||
} | ||
}); | ||
|
||
for code in error_codes { | ||
if !found_codes.contains(code) && !no_longer_emitted.contains(code) { | ||
errors.push(format!("Error code `{code}` exists, but is not emitted by the compiler!")) | ||
} | ||
|
||
if found_codes.contains(code) && no_longer_emitted.contains(code) { | ||
verbose_print!( | ||
verbose, | ||
"warning: Error code `{code}` is used when it's marked as \"no longer emitted\"" | ||
); | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.