From 04b9038610606313cf023e59fc6a0ef4d5d22340 Mon Sep 17 00:00:00 2001 From: Ezra Shaw Date: Thu, 29 Dec 2022 19:08:59 +1300 Subject: [PATCH 1/8] refactor: clean up `errors.rs` and `error_codes_check.rs` Move them into new `error_codes.rs` tidy check. --- compiler/rustc_error_codes/src/error_codes.rs | 1 - .../src/error_codes/E0729.md | 30 -- src/tools/tidy/src/error_codes.rs | 397 ++++++++++++++++++ src/tools/tidy/src/error_codes_check.rs | 305 -------------- src/tools/tidy/src/errors.rs | 77 ---- src/tools/tidy/src/lib.rs | 3 +- src/tools/tidy/src/main.rs | 4 +- 7 files changed, 400 insertions(+), 417 deletions(-) delete mode 100644 compiler/rustc_error_codes/src/error_codes/E0729.md create mode 100644 src/tools/tidy/src/error_codes.rs delete mode 100644 src/tools/tidy/src/error_codes_check.rs delete mode 100644 src/tools/tidy/src/errors.rs diff --git a/compiler/rustc_error_codes/src/error_codes.rs b/compiler/rustc_error_codes/src/error_codes.rs index 3fba2cf57494d..137325e6fabfd 100644 --- a/compiler/rustc_error_codes/src/error_codes.rs +++ b/compiler/rustc_error_codes/src/error_codes.rs @@ -443,7 +443,6 @@ E0725: include_str!("./error_codes/E0725.md"), E0726: include_str!("./error_codes/E0726.md"), E0727: include_str!("./error_codes/E0727.md"), E0728: include_str!("./error_codes/E0728.md"), -E0729: include_str!("./error_codes/E0729.md"), E0730: include_str!("./error_codes/E0730.md"), E0731: include_str!("./error_codes/E0731.md"), E0732: include_str!("./error_codes/E0732.md"), diff --git a/compiler/rustc_error_codes/src/error_codes/E0729.md b/compiler/rustc_error_codes/src/error_codes/E0729.md deleted file mode 100644 index 74f89080b91a1..0000000000000 --- a/compiler/rustc_error_codes/src/error_codes/E0729.md +++ /dev/null @@ -1,30 +0,0 @@ -Support for Non-Lexical Lifetimes (NLL) has been included in the Rust compiler -since 1.31, and has been enabled on the 2015 edition since 1.36. The new borrow -checker for NLL uncovered some bugs in the old borrow checker, which in some -cases allowed unsound code to compile, resulting in memory safety issues. - -### What do I do? - -Change your code so the warning does no longer trigger. For backwards -compatibility, this unsound code may still compile (with a warning) right now. -However, at some point in the future, the compiler will no longer accept this -code and will throw a hard error. - -### Shouldn't you fix the old borrow checker? - -The old borrow checker has known soundness issues that are basically impossible -to fix. The new NLL-based borrow checker is the fix. - -### Can I turn these warnings into errors by denying a lint? - -No. - -### When are these warnings going to turn into errors? - -No formal timeline for turning the warnings into errors has been set. See -[GitHub issue 58781](https://github.com/rust-lang/rust/issues/58781) for more -information. - -### Why do I get this message with code that doesn't involve borrowing? - -There are some known bugs that trigger this message. diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs new file mode 100644 index 0000000000000..f4aebbf227759 --- /dev/null +++ b/src/tools/tidy/src/error_codes.rs @@ -0,0 +1,397 @@ +//! 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", "E0729"]; + +// 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", +]; + +pub fn check(root_path: &Path, search_paths: &[&Path], bad: &mut bool) { + let mut errors = Vec::new(); + + // Stage 1: create list + let error_codes = extract_error_codes(root_path, &mut errors); + println!("Found {} error codes", error_codes.len()); + + // Stage 2: check list has docs + let no_longer_emitted = check_error_codes_docs(root_path, &error_codes, &mut errors); + + // Stage 3: check list has UI tests + check_error_codes_tests(root_path, &error_codes, &mut errors); + + // Stage 4: check list is emitted by compiler + check_error_codes_used(search_paths, &error_codes, &mut errors, &no_longer_emitted); + + // 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) -> Vec { + let file = fs::read_to_string(root_path.join(Path::new(ERROR_CODES_PATH))) + .unwrap_or_else(|e| panic!("failed to read `error_codes.rs`: {e}")); + + let mut error_codes = Vec::new(); + let mut reached_undocumented_codes = false; + + let mut undocumented_count = 0; + + 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 `{}`", + 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(); + + undocumented_count += 1; + + 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; + } + } + + println!( + "WARNING: {} error codes are undocumented. This *will* become a hard error.", + undocumented_count + ); + + 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, +) -> Vec { + let docs_path = root_path.join(Path::new(ERROR_DOCS_PATH)); + + let mut emit_ignore_warning = 0; + let mut emit_no_longer_warning = 0; + let mut emit_no_code_warning = 0; + + 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; + } + + // `has_test.0` checks whether the error code has any (potentially untested) code example. + // `has_test.1` checks whether the error code has a proper (definitely tested) doctest. + let has_test = check_explanation_has_doctest(&contents, &err_code); + if has_test.2 { + emit_ignore_warning += 1; + } + if has_test.3 { + no_longer_emitted_codes.push(err_code.to_owned()); + emit_no_longer_warning += 1; + } + if !has_test.0 { + emit_no_code_warning += 1; + } + + 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 !has_test.1 && !test_ignored { + errors.push(format!( + "`{}` doesn't use its own error code in compile_fail example", + path.display(), + )); + } else if has_test.1 && 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(), + )); + } + }); + + if emit_ignore_warning > 0 { + println!( + "WARNING: {emit_ignore_warning} error codes use the ignore header. This should not be used, add the error codes to the \ + `IGNORE_DOCTEST_CHECK` constant instead. This *will* become a hard error." + ); + } + if emit_no_code_warning > 0 { + println!( + "WARNING: {emit_ignore_warning} error codes don't have a code example, all error codes are expected \ + to have one (even if untested). This *will* become a hard error." + ); + } + if emit_no_longer_warning > 0 { + println!( + "WARNING: {emit_no_longer_warning} error codes are no longer emitted and should be removed entirely. \ + This *will* become a hard error." + ); + } + + 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) { + 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) { + let tests_path = root_path.join(Path::new(ERROR_TESTS_PATH)); + + // Some warning counters, this whole thing is clunky but'll be removed eventually. + let mut no_ui_test = 0; + let mut no_error_code_in_test = 0; + + 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()) { + no_ui_test += 1; + continue; + } + if IGNORE_UI_TEST_CHECK.contains(&code.as_str()) { + if test_path.exists() { + errors.push(format!( + "Error code `{code}` has a UI test, it shouldn't be listed in `EXEMPTED_FROM_TEST`!" + )); + } + continue; + } + + let file = match fs::read_to_string(test_path) { + Ok(file) => file, + Err(err) => { + println!( + "WARNING: Failed to read UI test file for `{code}` but the file exists. The test is assumed to work:\n{err}" + ); + 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 { + no_error_code_in_test += 1; + } + } + + if no_error_code_in_test > 0 { + println!( + "WARNING: {no_error_code_in_test} error codes have a UI test file, but don't contain their own error code!" + ); + } + + if no_ui_test > 0 { + println!( + "WARNING: {no_ui_test} error codes need to have at least one UI test in the `src/test/ui/error-codes/` directory`! \ + This *will* become a hard error." + ); + } +} + +/// 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, + no_longer_emitted: &[String], +) { + // 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(); + + 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); + } + } + } + }); + + let mut used_when_shouldnt = 0; + + 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) { + used_when_shouldnt += 1; + } + } + + if used_when_shouldnt > 0 { + println!( + "WARNING: {used_when_shouldnt} error codes are used when they are marked as \"no longer emitted\"" + ); + } +} diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs deleted file mode 100644 index 3f060e437aca7..0000000000000 --- a/src/tools/tidy/src/error_codes_check.rs +++ /dev/null @@ -1,305 +0,0 @@ -//! 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 crate::walk::{filter_dirs, walk}; -use std::collections::{HashMap, HashSet}; -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] = &[ - "E0313", "E0461", "E0476", "E0490", "E0514", "E0523", "E0554", "E0640", "E0717", "E0729", - "E0789", -]; - -// Some error codes don't have any tests apparently... -const IGNORE_EXPLANATION_CHECK: &[&str] = &["E0464", "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, - err_code: String, -) -> bool { - let mut invalid_compile_fail_format = false; - let mut found_error_code = false; - - for line in f.lines() { - let s = line.trim(); - if s.starts_with("```") { - if s.contains("compile_fail") && s.contains('E') { - if !found_error_code { - 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.has_test = true); - found_error_code = true; - } - } - } - invalid_compile_fail_format -} - -fn check_if_error_code_is_test_in_explanation(f: &str, err_code: &str) -> bool { - let mut ignore_found = false; - - for line in f.lines() { - let s = line.trim(); - if s.starts_with("#### Note: this error code is no longer emitted by the compiler") { - return true; - } - if s.starts_with("```") { - if s.contains("compile_fail") && s.contains(err_code) { - return true; - } else if s.contains("ignore") { - // It's very likely that we can't actually make it fail compilation... - ignore_found = true; - } - } - } - ignore_found -} - -fn extract_error_codes( - f: &str, - error_codes: &mut HashMap, - path: &Path, - errors: &mut Vec, -) { - let mut reached_no_explanation = false; - - for line in f.lines() { - let s = line.trim(); - if !reached_no_explanation && s.starts_with('E') && s.contains("include_str!(\"") { - let err_code = s - .split_once(':') - .expect( - format!( - "Expected a line with the format `E0xxx: include_str!(\"..\")`, but got {} \ - without a `:` delimiter", - s, - ) - .as_str(), - ) - .0 - .to_owned(); - 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, - Some((_, md)) => match md.split_once("\")") { - None => continue, - Some((file_name, _)) => file_name, - }, - }; - - let Some(parent) = path.parent() else { - continue; - }; - - let path = parent - .join(md_file_name) - .canonicalize() - .expect("failed to canonicalize error explanation file path"); - - match read_to_string(&path) { - Ok(content) => { - let has_test = check_if_error_code_is_test_in_explanation(&content, &err_code); - if !has_test && !IGNORE_EXPLANATION_CHECK.contains(&err_code.as_str()) { - errors.push(format!( - "`{}` doesn't use its own error code in compile_fail example", - path.display(), - )); - } else if has_test && IGNORE_EXPLANATION_CHECK.contains(&err_code.as_str()) { - errors.push(format!( - "`{}` has a compile_fail example with its own error code, it shouldn't \ - be listed in IGNORE_EXPLANATION_CHECK!", - path.display(), - )); - } - if check_error_code_explanation(&content, error_codes, err_code) { - errors.push(format!( - "`{}` uses invalid tag `compile-fail` instead of `compile_fail`", - path.display(), - )); - } - } - Err(e) => { - eprintln!("Couldn't read `{}`: {}", path.display(), e); - } - } - } else if reached_no_explanation && s.starts_with('E') { - let err_code = match s.split_once(',') { - None => s, - Some((err_code, _)) => err_code, - } - .to_string(); - if !error_codes.contains_key(&err_code) { - // this check should *never* fail! - 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) { - for line in f.lines() { - let s = line.trim(); - if s.starts_with("error[E") || s.starts_with("warning[E") { - let err_code = match s.split_once(']') { - None => continue, - Some((err_code, _)) => match err_code.split_once('[') { - None => continue, - Some((_, err_code)) => err_code, - }, - }; - error_codes.entry(err_code.to_owned()).or_default().has_test = true; - } - } -} - -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) { - error_codes.entry(error_code.as_str().to_owned()).or_default().is_used = true; - } - } - } -} - -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(); - let mut explanations: HashSet = HashSet::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(); - - for path in paths { - walk(path, &mut filter_dirs, &mut |entry, contents| { - let file_name = entry.file_name(); - let entry_path = entry.path(); - - if file_name == "error_codes.rs" { - extract_error_codes(contents, &mut error_codes, entry.path(), &mut errors); - found_explanations += 1; - } 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, ®ex); - } - } else if entry_path - .parent() - .and_then(|p| p.file_name()) - .map(|p| p == "error_codes") - .unwrap_or(false) - && entry_path.extension() == Some(OsStr::new("md")) - { - explanations.insert(file_name.to_str().unwrap().replace(".md", "")); - } - }); - } - if found_explanations == 0 { - tidy_error!(bad, "No error code explanation was tested!"); - } - if found_tests == 0 { - tidy_error!(bad, "No error code was found in compilation errors!"); - } - if explanations.is_empty() { - tidy_error!(bad, "No error code explanation was found!"); - } - if errors.is_empty() { - 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 {err_code} needs to have at least one UI test!")); - } 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 - )), - } - } - } - if errors.is_empty() { - for explanation in explanations { - if !error_codes.contains_key(&explanation) { - errors.push(format!( - "{} error code explanation should be listed in `error_codes.rs`", - explanation - )); - } - } - } - errors.sort(); - for err in &errors { - tidy_error!(bad, "{err}"); - } -} diff --git a/src/tools/tidy/src/errors.rs b/src/tools/tidy/src/errors.rs deleted file mode 100644 index fe5fd72b91a49..0000000000000 --- a/src/tools/tidy/src/errors.rs +++ /dev/null @@ -1,77 +0,0 @@ -//! Tidy check to verify the validity of long error diagnostic codes. -//! -//! This ensures that error codes are used at most once and also prints out some -//! statistics about the error codes. - -use crate::walk::{filter_dirs, walk}; -use std::collections::HashMap; -use std::path::Path; - -pub fn check(path: &Path, bad: &mut bool) { - let mut map: HashMap<_, Vec<_>> = HashMap::new(); - walk( - path, - &mut |path| filter_dirs(path) || path.ends_with("src/test"), - &mut |entry, contents| { - let file = entry.path(); - let filename = file.file_name().unwrap().to_string_lossy(); - if filename != "error_codes.rs" { - return; - } - - // In the `register_long_diagnostics!` macro, entries look like this: - // - // ``` - // EXXXX: r##" - // - // "##, - // ``` - // - // and these long messages often have error codes themselves inside - // them, but we don't want to report duplicates in these cases. This - // variable keeps track of whether we're currently inside one of these - // long diagnostic messages. - let mut inside_long_diag = false; - for (num, line) in contents.lines().enumerate() { - if inside_long_diag { - inside_long_diag = !line.contains("\"##"); - continue; - } - - let mut search = line; - while let Some(i) = search.find('E') { - search = &search[i + 1..]; - let code = if search.len() > 4 { search[..4].parse::() } else { continue }; - let code = match code { - Ok(n) => n, - Err(..) => continue, - }; - map.entry(code).or_default().push((file.to_owned(), num + 1, line.to_owned())); - break; - } - - inside_long_diag = line.contains("r##\""); - } - }, - ); - - let mut max = 0; - for (&code, entries) in map.iter() { - if code > max { - max = code; - } - if entries.len() == 1 { - continue; - } - - tidy_error!(bad, "duplicate error code: {}", code); - for &(ref file, line_num, ref line) in entries.iter() { - tidy_error!(bad, "{}:{}: {}", file.display(), line_num, line); - } - } - - if !*bad { - println!("* {} error codes", map.len()); - println!("* highest error code: E{:04}", max); - } -} diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index ce7e7ac5cd4ca..bf6e2cc250f3d 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -56,8 +56,7 @@ pub mod bins; pub mod debug_artifacts; pub mod deps; pub mod edition; -pub mod error_codes_check; -pub mod errors; +pub mod error_codes; pub mod extdeps; pub mod features; pub mod mir_opt_tests; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 6714c63ee62a1..8bb79bab1470b 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -27,6 +27,7 @@ fn main() { let src_path = root_path.join("src"); let library_path = root_path.join("library"); let compiler_path = root_path.join("compiler"); + let librustdoc_path = src_path.join("librustdoc"); let args: Vec = env::args().skip(1).collect(); @@ -79,8 +80,7 @@ fn main() { check!(mir_opt_tests, &src_path, bless); // Checks that only make sense for the compiler. - check!(errors, &compiler_path); - check!(error_codes_check, &[&src_path, &compiler_path]); + check!(error_codes, &root_path, &[&compiler_path, &librustdoc_path]); // Checks that only make sense for the std libs. check!(pal, &library_path); From 9618f646b314f934fe4ff92d6424255573c65a1c Mon Sep 17 00:00:00 2001 From: Ezra Shaw Date: Mon, 2 Jan 2023 09:11:36 +1300 Subject: [PATCH 2/8] docs: revert removal of `E0729` --- compiler/rustc_error_codes/src/error_codes.rs | 1 + .../src/error_codes/E0729.md | 32 +++++++++++++++++++ src/tools/tidy/src/error_codes.rs | 2 +- 3 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 compiler/rustc_error_codes/src/error_codes/E0729.md diff --git a/compiler/rustc_error_codes/src/error_codes.rs b/compiler/rustc_error_codes/src/error_codes.rs index 137325e6fabfd..3fba2cf57494d 100644 --- a/compiler/rustc_error_codes/src/error_codes.rs +++ b/compiler/rustc_error_codes/src/error_codes.rs @@ -443,6 +443,7 @@ E0725: include_str!("./error_codes/E0725.md"), E0726: include_str!("./error_codes/E0726.md"), E0727: include_str!("./error_codes/E0727.md"), E0728: include_str!("./error_codes/E0728.md"), +E0729: include_str!("./error_codes/E0729.md"), E0730: include_str!("./error_codes/E0730.md"), E0731: include_str!("./error_codes/E0731.md"), E0732: include_str!("./error_codes/E0732.md"), diff --git a/compiler/rustc_error_codes/src/error_codes/E0729.md b/compiler/rustc_error_codes/src/error_codes/E0729.md new file mode 100644 index 0000000000000..3891745b5008e --- /dev/null +++ b/compiler/rustc_error_codes/src/error_codes/E0729.md @@ -0,0 +1,32 @@ +#### Note: this error code is no longer emitted by the compiler + +Support for Non-Lexical Lifetimes (NLL) has been included in the Rust compiler +since 1.31, and has been enabled on the 2015 edition since 1.36. The new borrow +checker for NLL uncovered some bugs in the old borrow checker, which in some +cases allowed unsound code to compile, resulting in memory safety issues. + +### What do I do? + +Change your code so the warning does no longer trigger. For backwards +compatibility, this unsound code may still compile (with a warning) right now. +However, at some point in the future, the compiler will no longer accept this +code and will throw a hard error. + +### Shouldn't you fix the old borrow checker? + +The old borrow checker has known soundness issues that are basically impossible +to fix. The new NLL-based borrow checker is the fix. + +### Can I turn these warnings into errors by denying a lint? + +No. + +### When are these warnings going to turn into errors? + +No formal timeline for turning the warnings into errors has been set. See +[GitHub issue 58781](https://github.com/rust-lang/rust/issues/58781) for more +information. + +### Why do I get this message with code that doesn't involve borrowing? + +There are some known bugs that trigger this message. diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index f4aebbf227759..b766f16671815 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -29,7 +29,7 @@ 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", "E0729"]; +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] = &[ From 24671b7fd5daf16a39d6c9c62a457ace0b9c3ff6 Mon Sep 17 00:00:00 2001 From: Ezra Shaw Date: Mon, 2 Jan 2023 09:15:36 +1300 Subject: [PATCH 3/8] use more paths in error codes --- src/tools/tidy/src/error_codes.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index b766f16671815..026f7d87418a3 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -61,8 +61,9 @@ pub fn check(root_path: &Path, search_paths: &[&Path], bad: &mut bool) { /// Stage 1: Parses a list of error codes from `error_codes.rs`. fn extract_error_codes(root_path: &Path, errors: &mut Vec) -> Vec { - let file = fs::read_to_string(root_path.join(Path::new(ERROR_CODES_PATH))) - .unwrap_or_else(|e| panic!("failed to read `error_codes.rs`: {e}")); + 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; @@ -97,7 +98,8 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec) -> Vec file, Err(err) => { println!( - "WARNING: Failed to read UI test file for `{code}` but the file exists. The test is assumed to work:\n{err}" + "WARNING: Failed to read UI test file (`{test_path}`) for `{code}` but the file exists. The test is assumed to work:\n{err}" ); continue; } From def1b7cb9ab99565cb3dc08d88035fd9cdb3c22a Mon Sep 17 00:00:00 2001 From: Ezra Shaw Date: Mon, 2 Jan 2023 09:20:41 +1300 Subject: [PATCH 4/8] fix CI error --- src/tools/tidy/src/error_codes.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index 026f7d87418a3..4498666da1e77 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -289,11 +289,12 @@ fn check_error_codes_tests(root_path: &Path, error_codes: &[String], errors: &mu continue; } - let file = match fs::read_to_string(test_path) { + let file = match fs::read_to_string(&test_path) { Ok(file) => file, Err(err) => { println!( - "WARNING: Failed to read UI test file (`{test_path}`) for `{code}` but the file exists. The test is assumed to work:\n{err}" + "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; } From fafb18e0c45b60bacd08e2d8c8932401181dafbc Mon Sep 17 00:00:00 2001 From: Ezra Shaw Date: Mon, 2 Jan 2023 09:44:10 +1300 Subject: [PATCH 5/8] fixup warnings --- src/tools/tidy/src/error_codes.rs | 123 +++++++++++++----------------- src/tools/tidy/src/main.rs | 2 +- 2 files changed, 53 insertions(+), 72 deletions(-) diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index 4498666da1e77..ac87e4797c595 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -37,21 +37,29 @@ const IGNORE_UI_TEST_CHECK: &[&str] = &[ "E0729", "E0789", ]; -pub fn check(root_path: &Path, search_paths: &[&Path], bad: &mut bool) { +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); + let error_codes = extract_error_codes(root_path, &mut errors, verbose); println!("Found {} error codes", error_codes.len()); // Stage 2: check list has docs - let no_longer_emitted = check_error_codes_docs(root_path, &error_codes, &mut errors); + 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); + 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); + check_error_codes_used(search_paths, &error_codes, &mut errors, &no_longer_emitted, verbose); // Print any errors. for error in errors { @@ -60,7 +68,7 @@ pub fn check(root_path: &Path, search_paths: &[&Path], bad: &mut bool) { } /// Stage 1: Parses a list of error codes from `error_codes.rs`. -fn extract_error_codes(root_path: &Path, errors: &mut Vec) -> Vec { +fn extract_error_codes(root_path: &Path, errors: &mut Vec, verbose: bool) -> Vec { 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}")); @@ -68,8 +76,6 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec) -> Vec) -> Vec) -> Vec, + verbose: bool, ) -> Vec { let docs_path = root_path.join(Path::new(ERROR_DOCS_PATH)); - let mut emit_ignore_warning = 0; - let mut emit_no_longer_warning = 0; - let mut emit_no_code_warning = 0; - let mut no_longer_emitted_codes = Vec::new(); walk(&docs_path, &mut |_| false, &mut |entry, contents| { @@ -179,14 +177,25 @@ fn check_error_codes_docs( // `has_test.1` checks whether the error code has a proper (definitely tested) doctest. let has_test = check_explanation_has_doctest(&contents, &err_code); if has_test.2 { - emit_ignore_warning += 1; + 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 has_test.3 { no_longer_emitted_codes.push(err_code.to_owned()); - emit_no_longer_warning += 1; + verbose_print!( + verbose, + "warning: Error code `{err_code}` is no longer emitted and should be removed entirely." + ); } if !has_test.0 { - emit_no_code_warning += 1; + 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); @@ -206,25 +215,6 @@ fn check_error_codes_docs( } }); - if emit_ignore_warning > 0 { - println!( - "WARNING: {emit_ignore_warning} error codes use the ignore header. This should not be used, add the error codes to the \ - `IGNORE_DOCTEST_CHECK` constant instead. This *will* become a hard error." - ); - } - if emit_no_code_warning > 0 { - println!( - "WARNING: {emit_ignore_warning} error codes don't have a code example, all error codes are expected \ - to have one (even if untested). This *will* become a hard error." - ); - } - if emit_no_longer_warning > 0 { - println!( - "WARNING: {emit_no_longer_warning} error codes are no longer emitted and should be removed entirely. \ - This *will* become a hard error." - ); - } - no_longer_emitted_codes } @@ -266,18 +256,22 @@ fn check_explanation_has_doctest(explanation: &str, err_code: &str) -> (bool, bo } // 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) { +fn check_error_codes_tests( + root_path: &Path, + error_codes: &[String], + errors: &mut Vec, + verbose: bool, +) { let tests_path = root_path.join(Path::new(ERROR_TESTS_PATH)); - // Some warning counters, this whole thing is clunky but'll be removed eventually. - let mut no_ui_test = 0; - let mut no_error_code_in_test = 0; - 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()) { - no_ui_test += 1; + 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()) { @@ -292,8 +286,9 @@ fn check_error_codes_tests(root_path: &Path, error_codes: &[String], errors: &mu let file = match fs::read_to_string(&test_path) { Ok(file) => file, Err(err) => { - println!( - "WARNING: Failed to read UI test file (`{}`) for `{code}` but the file exists. The test is assumed to work:\n{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; @@ -314,22 +309,12 @@ fn check_error_codes_tests(root_path: &Path, error_codes: &[String], errors: &mu } if !found_code { - no_error_code_in_test += 1; + verbose_print!( + verbose, + "warning: Error code {code}`` has a UI test file, but doesn't contain its own error code!" + ); } } - - if no_error_code_in_test > 0 { - println!( - "WARNING: {no_error_code_in_test} error codes have a UI test file, but don't contain their own error code!" - ); - } - - if no_ui_test > 0 { - println!( - "WARNING: {no_ui_test} error codes need to have at least one UI test in the `src/test/ui/error-codes/` directory`! \ - This *will* become a hard error." - ); - } } /// Stage 4: Search `compiler/` and ensure that every error code is actually used by the compiler and that no undocumented error codes exist. @@ -338,6 +323,7 @@ fn check_error_codes_used( error_codes: &[String], errors: &mut Vec, no_longer_emitted: &[String], + verbose: bool, ) { // We want error codes which match the following cases: // @@ -380,21 +366,16 @@ fn check_error_codes_used( } }); - let mut used_when_shouldnt = 0; - 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) { - used_when_shouldnt += 1; + verbose_print!( + verbose, + "warning: Error code `{code}` is used when it's marked as \"no longer emitted\"" + ); } } - - if used_when_shouldnt > 0 { - println!( - "WARNING: {used_when_shouldnt} error codes are used when they are marked as \"no longer emitted\"" - ); - } } diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 8bb79bab1470b..a7b7cc9fa6c59 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -80,7 +80,7 @@ fn main() { check!(mir_opt_tests, &src_path, bless); // Checks that only make sense for the compiler. - check!(error_codes, &root_path, &[&compiler_path, &librustdoc_path]); + check!(error_codes, &root_path, &[&compiler_path, &librustdoc_path], verbose); // Checks that only make sense for the std libs. check!(pal, &library_path); From 1f1dd5f3cc522de86eb0ef5b8df53af0e347f73b Mon Sep 17 00:00:00 2001 From: Ezra Shaw Date: Mon, 2 Jan 2023 16:14:21 +1300 Subject: [PATCH 6/8] pattern destructure `has_test` Co-authored-by: Bruno Kolenbrander <59372212+mejrs@users.noreply.github.com> --- src/tools/tidy/src/error_codes.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index ac87e4797c595..42d94311d2289 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -173,9 +173,7 @@ fn check_error_codes_docs( return; } - // `has_test.0` checks whether the error code has any (potentially untested) code example. - // `has_test.1` checks whether the error code has a proper (definitely tested) doctest. - let has_test = check_explanation_has_doctest(&contents, &err_code); + let (found_code_example, found_proper_doctest, emit_ignore_warning, emit_no_longer_warning) = check_explanation_has_doctest(&contents, &err_code); if has_test.2 { verbose_print!( verbose, From b7341db5d82e93c4257763b33fec853817197078 Mon Sep 17 00:00:00 2001 From: Ezra Shaw Date: Mon, 2 Jan 2023 16:22:18 +1300 Subject: [PATCH 7/8] fix CI --- src/tools/tidy/src/error_codes.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index 42d94311d2289..938e194abdf05 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -173,22 +173,23 @@ fn check_error_codes_docs( return; } - let (found_code_example, found_proper_doctest, emit_ignore_warning, emit_no_longer_warning) = check_explanation_has_doctest(&contents, &err_code); - if has_test.2 { + 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 has_test.3 { + 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 !has_test.0 { + 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 \ @@ -196,15 +197,15 @@ fn check_error_codes_docs( ); } - let test_ignored = IGNORE_DOCTEST_CHECK.contains(&err_code); + 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 !has_test.1 && !test_ignored { + if !found_proper_doctest && !test_ignored { errors.push(format!( "`{}` doesn't use its own error code in compile_fail example", path.display(), )); - } else if has_test.1 && test_ignored { + } 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`", From c00ab4b2de2b5a321476244a9316d4dd3d8f8242 Mon Sep 17 00:00:00 2001 From: Ezra Shaw Date: Tue, 3 Jan 2023 08:44:57 +1300 Subject: [PATCH 8/8] print highest error code --- src/tools/tidy/src/error_codes.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index 938e194abdf05..fdc9d78990590 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -51,6 +51,7 @@ pub fn check(root_path: &Path, search_paths: &[&Path], verbose: bool, bad: &mut // 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);