diff options
Diffstat (limited to 'src/tools/tidy/src')
30 files changed, 360 insertions, 150 deletions
diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index 141083290c6..9ddce725106 100644 --- a/src/tools/tidy/src/alphabetical.rs +++ b/src/tools/tidy/src/alphabetical.rs @@ -103,7 +103,7 @@ fn check_section<'a>( let prev_line_trimmed_lowercase = prev_line.trim_start_matches(' '); - if version_sort(&trimmed_line, &prev_line_trimmed_lowercase).is_lt() { + if version_sort(trimmed_line, prev_line_trimmed_lowercase).is_lt() { tidy_error_ext!(err, bad, "{file}:{}: line not in alphabetical order", idx + 1); } diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index 9b78ba75a05..a18f549844b 100644 --- a/src/tools/tidy/src/bins.rs +++ b/src/tools/tidy/src/bins.rs @@ -75,7 +75,7 @@ mod os_impl { return ReadOnlyFs; } - panic!("unable to create temporary file `{:?}`: {:?}", path, e); + panic!("unable to create temporary file `{path:?}`: {e:?}"); } } } @@ -83,12 +83,7 @@ mod os_impl { for &source_dir in sources { match check_dir(source_dir) { Unsupported => return false, - ReadOnlyFs => { - return match check_dir(output) { - Supported => true, - _ => false, - }; - } + ReadOnlyFs => return matches!(check_dir(output), Supported), _ => {} } } @@ -139,7 +134,7 @@ mod os_impl { return; } - if t!(is_executable(&file), file) { + if t!(is_executable(file), file) { let rel_path = file.strip_prefix(path).unwrap(); let git_friendly_path = rel_path.to_str().unwrap().replace("\\", "/"); diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index bf813d2131e..f43f5eae9a5 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -624,7 +624,7 @@ fn check_proc_macro_dep_list(root: &Path, cargo: &Path, bless: bool, bad: &mut b let is_proc_macro_pkg = |pkg: &Package| pkg.targets.iter().any(|target| target.is_proc_macro()); let mut proc_macro_deps = HashSet::new(); - for pkg in metadata.packages.iter().filter(|pkg| is_proc_macro_pkg(*pkg)) { + for pkg in metadata.packages.iter().filter(|pkg| is_proc_macro_pkg(pkg)) { deps_of(&metadata, &pkg.id, &mut proc_macro_deps); } // Remove the proc-macro crates themselves diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index bb61412f678..65aa89fe801 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -119,8 +119,7 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>) -> Vec<String let Some(split_line) = split_line else { errors.push(format!( "{path}:{line_index}: Expected a line with the format `Eabcd: abcd, \ - but got \"{}\" without a `:` delimiter", - line, + but got \"{line}\" without a `:` delimiter", )); continue; }; @@ -129,10 +128,8 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>) -> Vec<String // If this is a duplicate of another error code, emit a fatal error. if error_codes.contains(&err_code) { - errors.push(format!( - "{path}:{line_index}: Found duplicate error code: `{}`", - err_code - )); + errors + .push(format!("{path}:{line_index}: Found duplicate error code: `{err_code}`")); continue; } @@ -145,8 +142,7 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>) -> Vec<String let Some(rest) = rest else { errors.push(format!( "{path}:{line_index}: Expected a line with the format `Eabcd: abcd, \ - but got \"{}\" without a `,` delimiter", - line, + but got \"{line}\" without a `,` delimiter", )); continue; }; @@ -209,7 +205,7 @@ fn check_error_codes_docs( } let (found_code_example, found_proper_doctest, emit_ignore_warning, no_longer_emitted) = - check_explanation_has_doctest(&contents, &err_code); + check_explanation_has_doctest(contents, err_code); if emit_ignore_warning { verbose_print!( @@ -232,7 +228,7 @@ fn check_error_codes_docs( return; } - 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 !found_proper_doctest && !test_ignored { @@ -300,7 +296,7 @@ fn check_error_codes_tests( 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)); + let test_path = tests_path.join(format!("{code}.stderr")); if !test_path.exists() && !IGNORE_UI_TEST_CHECK.contains(&code.as_str()) { verbose_print!( @@ -388,7 +384,7 @@ fn check_error_codes_used( 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/lib.rs`.", error_code)); + errors.push(format!("Error code `{error_code}` is used in the compiler but not defined and documented in `compiler/rustc_error_codes/src/lib.rs`.")); continue; } diff --git a/src/tools/tidy/src/ext_tool_checks.rs b/src/tools/tidy/src/ext_tool_checks.rs index d2da63a9703..381ea44fd46 100644 --- a/src/tools/tidy/src/ext_tool_checks.rs +++ b/src/tools/tidy/src/ext_tool_checks.rs @@ -20,8 +20,11 @@ use std::ffi::OsStr; use std::path::{Path, PathBuf}; use std::process::Command; +use std::str::FromStr; use std::{fmt, fs, io}; +use crate::CiInfo; + const MIN_PY_REV: (u32, u32) = (3, 9); const MIN_PY_REV_STR: &str = "≥3.9"; @@ -36,15 +39,19 @@ const RUFF_CONFIG_PATH: &[&str] = &["src", "tools", "tidy", "config", "ruff.toml const RUFF_CACHE_PATH: &[&str] = &["cache", "ruff_cache"]; const PIP_REQ_PATH: &[&str] = &["src", "tools", "tidy", "config", "requirements.txt"]; +// this must be kept in sync with with .github/workflows/spellcheck.yml +const SPELLCHECK_DIRS: &[&str] = &["compiler", "library", "src/bootstrap", "src/librustdoc"]; + pub fn check( root_path: &Path, outdir: &Path, + ci_info: &CiInfo, bless: bool, extra_checks: Option<&str>, pos_args: &[String], bad: &mut bool, ) { - if let Err(e) = check_impl(root_path, outdir, bless, extra_checks, pos_args) { + if let Err(e) = check_impl(root_path, outdir, ci_info, bless, extra_checks, pos_args) { tidy_error!(bad, "{e}"); } } @@ -52,34 +59,55 @@ pub fn check( fn check_impl( root_path: &Path, outdir: &Path, + ci_info: &CiInfo, bless: bool, extra_checks: Option<&str>, pos_args: &[String], ) -> Result<(), Error> { - let show_diff = std::env::var("TIDY_PRINT_DIFF") - .map_or(false, |v| v.eq_ignore_ascii_case("true") || v == "1"); + let show_diff = + std::env::var("TIDY_PRINT_DIFF").is_ok_and(|v| v.eq_ignore_ascii_case("true") || v == "1"); // Split comma-separated args up let lint_args = match extra_checks { - Some(s) => s.strip_prefix("--extra-checks=").unwrap().split(',').collect(), + Some(s) => s + .strip_prefix("--extra-checks=") + .unwrap() + .split(',') + .map(|s| { + if s == "spellcheck:fix" { + eprintln!("warning: `spellcheck:fix` is no longer valid, use `--extra-checks=spellcheck --bless`"); + } + (ExtraCheckArg::from_str(s), s) + }) + .filter_map(|(res, src)| match res { + Ok(arg) => { + if arg.is_inactive_auto(ci_info) { + None + } else { + Some(arg) + } + } + Err(err) => { + // only warn because before bad extra checks would be silently ignored. + eprintln!("warning: bad extra check argument {src:?}: {err:?}"); + None + } + }) + .collect(), None => vec![], }; - if lint_args.contains(&"spellcheck:fix") { - return Err(Error::Generic( - "`spellcheck:fix` is no longer valid, use `--extra=check=spellcheck --bless`" - .to_string(), - )); + macro_rules! extra_check { + ($lang:ident, $kind:ident) => { + lint_args.iter().any(|arg| arg.matches(ExtraCheckLang::$lang, ExtraCheckKind::$kind)) + }; } - let python_all = lint_args.contains(&"py"); - let python_lint = lint_args.contains(&"py:lint") || python_all; - let python_fmt = lint_args.contains(&"py:fmt") || python_all; - let shell_all = lint_args.contains(&"shell"); - let shell_lint = lint_args.contains(&"shell:lint") || shell_all; - let cpp_all = lint_args.contains(&"cpp"); - let cpp_fmt = lint_args.contains(&"cpp:fmt") || cpp_all; - let spellcheck = lint_args.contains(&"spellcheck"); + let python_lint = extra_check!(Py, Lint); + let python_fmt = extra_check!(Py, Fmt); + let shell_lint = extra_check!(Shell, Lint); + let cpp_fmt = extra_check!(Cpp, Fmt); + let spellcheck = extra_check!(Spellcheck, None); let mut py_path = None; @@ -113,7 +141,7 @@ fn check_impl( ); } // Rethrow error - let _ = res?; + res?; } if python_fmt { @@ -145,7 +173,7 @@ fn check_impl( } // Rethrow error - let _ = res?; + res?; } if cpp_fmt { @@ -216,7 +244,7 @@ fn check_impl( } } // Rethrow error - let _ = res?; + res?; } if shell_lint { @@ -234,15 +262,9 @@ fn check_impl( if spellcheck { let config_path = root_path.join("typos.toml"); - // sync target files with .github/workflows/spellcheck.yml - let mut args = vec![ - "-c", - config_path.as_os_str().to_str().unwrap(), - "./compiler", - "./library", - "./src/bootstrap", - "./src/librustdoc", - ]; + let mut args = vec!["-c", config_path.as_os_str().to_str().unwrap()]; + + args.extend_from_slice(SPELLCHECK_DIRS); if bless { eprintln!("spellcheck files and fix"); @@ -264,8 +286,8 @@ fn run_ruff( file_args: &[&OsStr], ruff_args: &[&OsStr], ) -> Result<(), Error> { - let mut cfg_args_ruff = cfg_args.into_iter().copied().collect::<Vec<_>>(); - let mut file_args_ruff = file_args.into_iter().copied().collect::<Vec<_>>(); + let mut cfg_args_ruff = cfg_args.to_vec(); + let mut file_args_ruff = file_args.to_vec(); let mut cfg_path = root_path.to_owned(); cfg_path.extend(RUFF_CONFIG_PATH); @@ -283,7 +305,7 @@ fn run_ruff( file_args_ruff.push(root_path.as_os_str()); } - let mut args: Vec<&OsStr> = ruff_args.into_iter().copied().collect(); + let mut args: Vec<&OsStr> = ruff_args.to_vec(); args.extend(merge_args(&cfg_args_ruff, &file_args_ruff)); py_runner(py_path, true, None, "ruff", &args) } @@ -638,3 +660,136 @@ impl From<io::Error> for Error { Self::Io(value) } } + +#[derive(Debug)] +enum ExtraCheckParseError { + #[allow(dead_code, reason = "shown through Debug")] + UnknownKind(String), + #[allow(dead_code)] + UnknownLang(String), + UnsupportedKindForLang, + /// Too many `:` + TooManyParts, + /// Tried to parse the empty string + Empty, + /// `auto` specified without lang part. + AutoRequiresLang, +} + +struct ExtraCheckArg { + auto: bool, + lang: ExtraCheckLang, + /// None = run all extra checks for the given lang + kind: Option<ExtraCheckKind>, +} + +impl ExtraCheckArg { + fn matches(&self, lang: ExtraCheckLang, kind: ExtraCheckKind) -> bool { + self.lang == lang && self.kind.map(|k| k == kind).unwrap_or(true) + } + + /// Returns `true` if this is an auto arg and the relevant files are not modified. + fn is_inactive_auto(&self, ci_info: &CiInfo) -> bool { + if !self.auto { + return false; + } + let ext = match self.lang { + ExtraCheckLang::Py => ".py", + ExtraCheckLang::Cpp => ".cpp", + ExtraCheckLang::Shell => ".sh", + ExtraCheckLang::Spellcheck => { + return !crate::files_modified(ci_info, |s| { + SPELLCHECK_DIRS.iter().any(|dir| Path::new(s).starts_with(dir)) + }); + } + }; + !crate::files_modified(ci_info, |s| s.ends_with(ext)) + } + + fn has_supported_kind(&self) -> bool { + let Some(kind) = self.kind else { + // "run all extra checks" mode is supported for all languages. + return true; + }; + use ExtraCheckKind::*; + let supported_kinds: &[_] = match self.lang { + ExtraCheckLang::Py => &[Fmt, Lint], + ExtraCheckLang::Cpp => &[Fmt], + ExtraCheckLang::Shell => &[Lint], + ExtraCheckLang::Spellcheck => &[], + }; + supported_kinds.contains(&kind) + } +} + +impl FromStr for ExtraCheckArg { + type Err = ExtraCheckParseError; + + fn from_str(s: &str) -> Result<Self, Self::Err> { + let mut auto = false; + let mut parts = s.split(':'); + let Some(mut first) = parts.next() else { + return Err(ExtraCheckParseError::Empty); + }; + if first == "auto" { + let Some(part) = parts.next() else { + return Err(ExtraCheckParseError::AutoRequiresLang); + }; + auto = true; + first = part; + } + let second = parts.next(); + if parts.next().is_some() { + return Err(ExtraCheckParseError::TooManyParts); + } + let arg = Self { auto, lang: first.parse()?, kind: second.map(|s| s.parse()).transpose()? }; + if !arg.has_supported_kind() { + return Err(ExtraCheckParseError::UnsupportedKindForLang); + } + + Ok(arg) + } +} + +#[derive(PartialEq, Copy, Clone)] +enum ExtraCheckLang { + Py, + Shell, + Cpp, + Spellcheck, +} + +impl FromStr for ExtraCheckLang { + type Err = ExtraCheckParseError; + + fn from_str(s: &str) -> Result<Self, Self::Err> { + Ok(match s { + "py" => Self::Py, + "shell" => Self::Shell, + "cpp" => Self::Cpp, + "spellcheck" => Self::Spellcheck, + _ => return Err(ExtraCheckParseError::UnknownLang(s.to_string())), + }) + } +} + +#[derive(PartialEq, Copy, Clone)] +enum ExtraCheckKind { + Lint, + Fmt, + /// Never parsed, but used as a placeholder for + /// langs that never have a specific kind. + None, +} + +impl FromStr for ExtraCheckKind { + type Err = ExtraCheckParseError; + + fn from_str(s: &str) -> Result<Self, Self::Err> { + Ok(match s { + "lint" => Self::Lint, + "fmt" => Self::Fmt, + _ => return Err(ExtraCheckParseError::UnknownKind(s.to_string())), + }) + } +} diff --git a/src/tools/tidy/src/extdeps.rs b/src/tools/tidy/src/extdeps.rs index 55f937aeacf..bc217a55cc1 100644 --- a/src/tools/tidy/src/extdeps.rs +++ b/src/tools/tidy/src/extdeps.rs @@ -41,7 +41,7 @@ pub fn check(root: &Path, bad: &mut bool) { let source = line.split_once('=').unwrap().1.trim(); // Ensure source is allowed. - if !ALLOWED_SOURCES.contains(&&*source) { + if !ALLOWED_SOURCES.contains(&source) { tidy_error!(bad, "invalid source: {}", source); } } diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index 6093e7fd263..e83b47e1380 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -125,8 +125,8 @@ pub fn check( let gate_test_str = "gate-test-"; let feature_name = match line.find(gate_test_str) { - // NB: the `splitn` always succeeds, even if the delimiter is not present. - Some(i) => line[i + gate_test_str.len()..].splitn(2, ' ').next().unwrap(), + // `split` always contains at least 1 element, even if the delimiter is not present. + Some(i) => line[i + gate_test_str.len()..].split(' ').next().unwrap(), None => continue, }; match features.get_mut(feature_name) { @@ -135,16 +135,14 @@ pub fn check( err(&format!( "The file is already marked as gate test \ through its name, no need for a \ - 'gate-test-{}' comment", - feature_name + 'gate-test-{feature_name}' comment" )); } f.has_gate_test = true; } None => { err(&format!( - "gate-test test found referencing a nonexistent feature '{}'", - feature_name + "gate-test test found referencing a nonexistent feature '{feature_name}'" )); } } @@ -170,8 +168,7 @@ pub fn check( ); println!( "Hint: If you already have such a test and don't want to rename it,\ - \n you can also add a // gate-test-{} line to the test file.", - name + \n you can also add a // gate-test-{name} line to the test file." ); } @@ -231,7 +228,7 @@ pub fn check( fn get_version_and_channel(src_path: &Path) -> (Version, String) { let version_str = t!(std::fs::read_to_string(src_path.join("version"))); let version_str = version_str.trim(); - let version = t!(std::str::FromStr::from_str(&version_str).map_err(|e| format!("{e:?}"))); + let version = t!(std::str::FromStr::from_str(version_str).map_err(|e| format!("{e:?}"))); let channel_str = t!(std::fs::read_to_string(src_path.join("ci").join("channel"))); (version, channel_str.trim().to_owned()) } @@ -468,7 +465,7 @@ fn get_and_check_lib_features( map_lib_features(base_src_path, &mut |res, file, line| match res { Ok((name, f)) => { let mut check_features = |f: &Feature, list: &Features, display: &str| { - if let Some(ref s) = list.get(name) { + if let Some(s) = list.get(name) { if f.tracking_issue != s.tracking_issue && f.level != Status::Accepted { tidy_error!( bad, @@ -483,7 +480,7 @@ fn get_and_check_lib_features( } } }; - check_features(&f, &lang_features, "corresponding lang feature"); + check_features(&f, lang_features, "corresponding lang feature"); check_features(&f, &lib_features, "previous"); lib_features.insert(name.to_owned(), f); } @@ -543,7 +540,7 @@ fn map_lib_features( continue; } - if let Some((ref name, ref mut f)) = becoming_feature { + if let Some((name, ref mut f)) = becoming_feature { if f.tracking_issue.is_none() { f.tracking_issue = find_attr_val(line, "issue").and_then(handle_issue_none); } diff --git a/src/tools/tidy/src/features/version.rs b/src/tools/tidy/src/features/version.rs index 6a902e80f8e..0e0629a48e2 100644 --- a/src/tools/tidy/src/features/version.rs +++ b/src/tools/tidy/src/features/version.rs @@ -20,7 +20,7 @@ impl fmt::Display for Version { Version::Explicit { parts } => { f.pad(&format!("{}.{}.{}", parts[0], parts[1], parts[2])) } - Version::CurrentPlaceholder => f.pad(&format!("CURRENT")), + Version::CurrentPlaceholder => f.pad("CURRENT"), } } } diff --git a/src/tools/tidy/src/features/version/tests.rs b/src/tools/tidy/src/features/version/tests.rs index 7701dce2df1..453ba40586e 100644 --- a/src/tools/tidy/src/features/version/tests.rs +++ b/src/tools/tidy/src/features/version/tests.rs @@ -33,6 +33,6 @@ fn test_to_string() { assert_eq!(v_1_0_0.to_string(), "1.0.0"); assert_eq!(v_1_32_1.to_string(), "1.32.1"); - assert_eq!(format!("{:<8}", v_1_32_1), "1.32.1 "); - assert_eq!(format!("{:>8}", v_1_32_1), " 1.32.1"); + assert_eq!(format!("{v_1_32_1:<8}"), "1.32.1 "); + assert_eq!(format!("{v_1_32_1:>8}"), " 1.32.1"); } diff --git a/src/tools/tidy/src/filenames.rs b/src/tools/tidy/src/filenames.rs new file mode 100644 index 00000000000..53115f4eaa4 --- /dev/null +++ b/src/tools/tidy/src/filenames.rs @@ -0,0 +1,40 @@ +//! Tidy check to ensure that there are no filenames containing forbidden characters +//! checked into the source tree by accident: +//! - Non-UTF8 filenames +//! - Control characters such as CR or TAB +//! - Filenames containing ":" as they are not supported on Windows +//! +//! Only files added to git are checked, as it may be acceptable to have temporary +//! invalid filenames in the local directory during development. + +use std::path::Path; +use std::process::Command; + +pub fn check(root_path: &Path, bad: &mut bool) { + let stat_output = Command::new("git") + .arg("-C") + .arg(root_path) + .args(["ls-files", "-z"]) + .output() + .unwrap() + .stdout; + for filename in stat_output.split(|&b| b == 0) { + match str::from_utf8(filename) { + Err(_) => tidy_error!( + bad, + r#"non-UTF8 file names are not supported: "{}""#, + String::from_utf8_lossy(filename), + ), + Ok(name) if name.chars().any(|c| c.is_control()) => tidy_error!( + bad, + r#"control characters are not supported in file names: "{}""#, + String::from_utf8_lossy(filename), + ), + Ok(name) if name.contains(':') => tidy_error!( + bad, + r#"":" is not supported in file names because of Windows compatibility: "{name}""#, + ), + _ => (), + } + } +} diff --git a/src/tools/tidy/src/fluent_alphabetical.rs b/src/tools/tidy/src/fluent_alphabetical.rs index 6f154b92eff..48d14a37514 100644 --- a/src/tools/tidy/src/fluent_alphabetical.rs +++ b/src/tools/tidy/src/fluent_alphabetical.rs @@ -13,8 +13,8 @@ fn message() -> &'static Regex { static_regex!(r#"(?m)^([a-zA-Z0-9_]+)\s*=\s*"#) } -fn filter_fluent(path: &Path) -> bool { - if let Some(ext) = path.extension() { ext.to_str() != Some("ftl") } else { true } +fn is_fluent(path: &Path) -> bool { + path.extension().is_some_and(|ext| ext == "flt") } fn check_alphabetic( @@ -92,7 +92,7 @@ pub fn check(path: &Path, bless: bool, bad: &mut bool) { let mut all_defined_msgs = HashMap::new(); walk( path, - |path, is_dir| filter_dirs(path) || (!is_dir && filter_fluent(path)), + |path, is_dir| filter_dirs(path) || (!is_dir && !is_fluent(path)), &mut |ent, contents| { if bless { let sorted = sort_messages( @@ -104,7 +104,7 @@ pub fn check(path: &Path, bless: bool, bad: &mut bool) { if sorted != contents { let mut f = OpenOptions::new().write(true).truncate(true).open(ent.path()).unwrap(); - f.write(sorted.as_bytes()).unwrap(); + f.write_all(sorted.as_bytes()).unwrap(); } } else { check_alphabetic( diff --git a/src/tools/tidy/src/fluent_period.rs b/src/tools/tidy/src/fluent_period.rs index 6a136e5aec6..85c1ef6166a 100644 --- a/src/tools/tidy/src/fluent_period.rs +++ b/src/tools/tidy/src/fluent_period.rs @@ -37,7 +37,7 @@ fn check_period(filename: &str, contents: &str, bad: &mut bool) { if let Some(PatternElement::TextElement { value }) = pat.elements.last() { // We don't care about ellipses. if value.ends_with(".") && !value.ends_with("...") { - let ll = find_line(contents, *value); + let ll = find_line(contents, value); let name = m.id.name; tidy_error!(bad, "{filename}:{ll}: message `{name}` ends in a period"); } @@ -52,7 +52,7 @@ fn check_period(filename: &str, contents: &str, bad: &mut bool) { if let Some(PatternElement::TextElement { value }) = attr.value.elements.last() { if value.ends_with(".") && !value.ends_with("...") { - let ll = find_line(contents, *value); + let ll = find_line(contents, value); let name = attr.id.name; tidy_error!(bad, "{filename}:{ll}: attr `{name}` ends in a period"); } diff --git a/src/tools/tidy/src/fluent_used.rs b/src/tools/tidy/src/fluent_used.rs index ab56b5f0b0e..12fafd9a7ff 100644 --- a/src/tools/tidy/src/fluent_used.rs +++ b/src/tools/tidy/src/fluent_used.rs @@ -13,8 +13,8 @@ fn filter_used_messages( // we don't just check messages never appear in Rust files, // because messages can be used as parts of other fluent messages in Fluent files, // so we do checking messages appear only once in all Rust and Fluent files. - let mut matches = static_regex!(r"\w+").find_iter(contents); - while let Some(name) = matches.next() { + let matches = static_regex!(r"\w+").find_iter(contents); + for name in matches { if let Some((name, filename)) = msgs_not_appeared_yet.remove_entry(name.as_str()) { // if one msg appears for the first time, // remove it from `msgs_not_appeared_yet` and insert it into `msgs_appeared_only_once`. diff --git a/src/tools/tidy/src/gcc_submodule.rs b/src/tools/tidy/src/gcc_submodule.rs index 952ebe9e0cf..5d726c3ea48 100644 --- a/src/tools/tidy/src/gcc_submodule.rs +++ b/src/tools/tidy/src/gcc_submodule.rs @@ -7,7 +7,9 @@ use std::process::Command; pub fn check(root_path: &Path, compiler_path: &Path, bad: &mut bool) { let cg_gcc_version_path = compiler_path.join("rustc_codegen_gcc/libgccjit.version"); let cg_gcc_version = std::fs::read_to_string(&cg_gcc_version_path) - .expect(&format!("Cannot read GCC version from {}", cg_gcc_version_path.display())) + .unwrap_or_else(|_| { + panic!("Cannot read GCC version from {}", cg_gcc_version_path.display()) + }) .trim() .to_string(); @@ -27,14 +29,13 @@ pub fn check(root_path: &Path, compiler_path: &Path, bad: &mut bool) { // e607be166673a8de9fc07f6f02c60426e556c5f2 src/gcc (master-e607be166673a8de9fc07f6f02c60426e556c5f2.e607be) // +e607be166673a8de9fc07f6f02c60426e556c5f2 src/gcc (master-e607be166673a8de9fc07f6f02c60426e556c5f2.e607be) let git_output = String::from_utf8_lossy(&git_output.stdout) - .trim() .split_whitespace() .next() .unwrap_or_default() .to_string(); // The SHA can start with + if the submodule is modified or - if it is not checked out. - let gcc_submodule_sha = git_output.trim_start_matches(&['+', '-']); + let gcc_submodule_sha = git_output.trim_start_matches(['+', '-']); if gcc_submodule_sha != cg_gcc_version { *bad = true; eprintln!( diff --git a/src/tools/tidy/src/issues.txt b/src/tools/tidy/src/issues.txt index cac4dba2b49..8b57db23d01 100644 --- a/src/tools/tidy/src/issues.txt +++ b/src/tools/tidy/src/issues.txt @@ -2338,7 +2338,6 @@ ui/issues/issue-50415.rs ui/issues/issue-50442.rs ui/issues/issue-50471.rs ui/issues/issue-50518.rs -ui/issues/issue-50571.rs ui/issues/issue-50581.rs ui/issues/issue-50582.rs ui/issues/issue-50585.rs diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 237737f0f16..ade4055b5bd 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -124,6 +124,40 @@ pub fn git_diff<S: AsRef<OsStr>>(base_commit: &str, extra_arg: S) -> Option<Stri Some(String::from_utf8_lossy(&output.stdout).into()) } +/// Returns true if any modified file matches the predicate, if we are in CI, or if unable to list modified files. +pub fn files_modified(ci_info: &CiInfo, pred: impl Fn(&str) -> bool) -> bool { + if CiEnv::is_ci() { + // assume everything is modified on CI because we really don't want false positives there. + return true; + } + let Some(base_commit) = &ci_info.base_commit else { + eprintln!("No base commit, assuming all files are modified"); + return true; + }; + match crate::git_diff(&base_commit, "--name-status") { + Some(output) => { + let modified_files = output.lines().filter_map(|ln| { + let (status, name) = ln + .trim_end() + .split_once('\t') + .expect("bad format from `git diff --name-status`"); + if status == "M" { Some(name) } else { None } + }); + for modified_file in modified_files { + if pred(modified_file) { + return true; + } + } + false + } + None => { + eprintln!("warning: failed to run `git diff` to check for changes"); + eprintln!("warning: assuming all files are modified"); + true + } + } +} + pub mod alphabetical; pub mod bins; pub mod debug_artifacts; @@ -133,6 +167,7 @@ pub mod error_codes; pub mod ext_tool_checks; pub mod extdeps; pub mod features; +pub mod filenames; pub mod fluent_alphabetical; pub mod fluent_period; mod fluent_used; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index ef6ff5c9277..0f1116a632e 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -15,11 +15,12 @@ use std::{env, process}; use tidy::*; fn main() { - // Running Cargo will read the libstd Cargo.toml + // Enable nightly, because Cargo will read the libstd Cargo.toml // which uses the unstable `public-dependency` feature. - // - // `setenv` might not be thread safe, so run it before using multiple threads. - env::set_var("RUSTC_BOOTSTRAP", "1"); + // SAFETY: no other threads have been spawned + unsafe { + env::set_var("RUSTC_BOOTSTRAP", "1"); + } let root_path: PathBuf = env::args_os().nth(1).expect("need path to root of repo").into(); let cargo: PathBuf = env::args_os().nth(2).expect("need path to cargo").into(); @@ -154,6 +155,8 @@ fn main() { check!(triagebot, &root_path); + check!(filenames, &root_path); + let collected = { drain_handles(&mut handles); @@ -173,7 +176,15 @@ fn main() { }; check!(unstable_book, &src_path, collected); - check!(ext_tool_checks, &root_path, &output_directory, bless, extra_checks, pos_args); + check!( + ext_tool_checks, + &root_path, + &output_directory, + &ci_info, + bless, + extra_checks, + pos_args + ); }); if bad.load(Ordering::Relaxed) { diff --git a/src/tools/tidy/src/mir_opt_tests.rs b/src/tools/tidy/src/mir_opt_tests.rs index bb0d8150be4..1efe71b1687 100644 --- a/src/tools/tidy/src/mir_opt_tests.rs +++ b/src/tools/tidy/src/mir_opt_tests.rs @@ -49,7 +49,7 @@ fn check_unused_files(path: &Path, bless: bool, bad: &mut bool) { } fn check_dash_files(path: &Path, bless: bool, bad: &mut bool) { - for file in walkdir::WalkDir::new(&path.join("mir-opt")) + for file in walkdir::WalkDir::new(path.join("mir-opt")) .into_iter() .filter_map(Result::ok) .filter(|e| e.file_type().is_file()) diff --git a/src/tools/tidy/src/pal.rs b/src/tools/tidy/src/pal.rs index 9b915e0f737..b7d4a331891 100644 --- a/src/tools/tidy/src/pal.rs +++ b/src/tools/tidy/src/pal.rs @@ -86,7 +86,7 @@ pub fn check(path: &Path, bad: &mut bool) { return; } - check_cfgs(contents, &file, bad, &mut saw_target_arch, &mut saw_cfg_bang); + check_cfgs(contents, file, bad, &mut saw_target_arch, &mut saw_cfg_bang); }); assert!(saw_target_arch); diff --git a/src/tools/tidy/src/rustdoc_gui_tests.rs b/src/tools/tidy/src/rustdoc_gui_tests.rs index 91776bc989e..3b995f219d2 100644 --- a/src/tools/tidy/src/rustdoc_gui_tests.rs +++ b/src/tools/tidy/src/rustdoc_gui_tests.rs @@ -5,7 +5,7 @@ use std::path::Path; pub fn check(path: &Path, bad: &mut bool) { crate::walk::walk( &path.join("rustdoc-gui"), - |p, is_dir| !is_dir && p.extension().map_or(true, |e| e != "goml"), + |p, is_dir| !is_dir && p.extension().is_none_or(|e| e != "goml"), &mut |entry, content| { for line in content.lines() { if !line.starts_with("// ") { diff --git a/src/tools/tidy/src/rustdoc_js.rs b/src/tools/tidy/src/rustdoc_js.rs index 5e924544f0d..5737fcbafc0 100644 --- a/src/tools/tidy/src/rustdoc_js.rs +++ b/src/tools/tidy/src/rustdoc_js.rs @@ -88,7 +88,7 @@ pub fn check(librustdoc_path: &Path, tools_path: &Path, src_path: &Path, bad: &m let mut files_to_check = Vec::new(); walk_no_read( &[&librustdoc_path.join("html/static/js")], - |path, is_dir| is_dir || !path.extension().is_some_and(|ext| ext == OsStr::new("js")), + |path, is_dir| is_dir || path.extension().is_none_or(|ext| ext != OsStr::new("js")), &mut |path: &DirEntry| { files_to_check.push(path.path().into()); }, diff --git a/src/tools/tidy/src/rustdoc_json.rs b/src/tools/tidy/src/rustdoc_json.rs index dfbb35d69f1..722e1ebd0ca 100644 --- a/src/tools/tidy/src/rustdoc_json.rs +++ b/src/tools/tidy/src/rustdoc_json.rs @@ -14,25 +14,13 @@ pub fn check(src_path: &Path, ci_info: &crate::CiInfo, bad: &mut bool) { }; // First we check that `src/rustdoc-json-types` was modified. - match crate::git_diff(&base_commit, "--name-status") { - Some(output) => { - if !output - .lines() - .any(|line| line.starts_with("M") && line.contains(RUSTDOC_JSON_TYPES)) - { - // `rustdoc-json-types` was not modified so nothing more to check here. - println!("`rustdoc-json-types` was not modified."); - return; - } - } - None => { - *bad = true; - eprintln!("error: failed to run `git diff` in rustdoc_json check"); - return; - } + if !crate::files_modified(ci_info, |p| p == RUSTDOC_JSON_TYPES) { + // `rustdoc-json-types` was not modified so nothing more to check here. + println!("`rustdoc-json-types` was not modified."); + return; } // Then we check that if `FORMAT_VERSION` was updated, the `Latest feature:` was also updated. - match crate::git_diff(&base_commit, src_path.join("rustdoc-json-types")) { + match crate::git_diff(base_commit, src_path.join("rustdoc-json-types")) { Some(output) => { let mut format_version_updated = false; let mut latest_feature_comment_updated = false; diff --git a/src/tools/tidy/src/rustdoc_templates.rs b/src/tools/tidy/src/rustdoc_templates.rs index 2173dbf7e74..dca3e8d9d25 100644 --- a/src/tools/tidy/src/rustdoc_templates.rs +++ b/src/tools/tidy/src/rustdoc_templates.rs @@ -14,7 +14,7 @@ const TAGS: &[(&str, &str)] = &[("{#", "#}"), ("{%", "%}"), ("{{", "}}")]; pub fn check(librustdoc_path: &Path, bad: &mut bool) { walk( &librustdoc_path.join("html/templates"), - |path, is_dir| is_dir || !path.extension().is_some_and(|ext| ext == OsStr::new("html")), + |path, is_dir| is_dir || path.extension().is_none_or(|ext| ext != OsStr::new("html")), &mut |path: &DirEntry, file_content: &str| { let mut lines = file_content.lines().enumerate().peekable(); @@ -23,7 +23,7 @@ pub fn check(librustdoc_path: &Path, bad: &mut bool) { if let Some(need_next_line_check) = TAGS.iter().find_map(|(tag, end_tag)| { // We first check if the line ends with a jinja tag. if !line.ends_with(end_tag) { - return None; + None // Then we check if this a comment tag. } else if *tag != "{#" { return Some(false); diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index 2237eac200d..8dde4618ce5 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -94,10 +94,9 @@ fn generate_problems<'a>( letter_digit: &'a FxHashMap<char, char>, ) -> impl Iterator<Item = u32> + 'a { consts.iter().flat_map(move |const_value| { - let problem = - letter_digit.iter().fold(format!("{:X}", const_value), |acc, (key, value)| { - acc.replace(&value.to_string(), &key.to_string()) - }); + let problem = letter_digit.iter().fold(format!("{const_value:X}"), |acc, (key, value)| { + acc.replace(&value.to_string(), &key.to_string()) + }); let indexes: Vec<usize> = problem .chars() .enumerate() @@ -341,7 +340,7 @@ fn is_unexplained_ignore(extension: &str, line: &str) -> bool { pub fn check(path: &Path, bad: &mut bool) { fn skip(path: &Path, is_dir: bool) -> bool { - if path.file_name().map_or(false, |name| name.to_string_lossy().starts_with(".#")) { + if path.file_name().is_some_and(|name| name.to_string_lossy().starts_with(".#")) { // vim or emacs temporary file return true; } @@ -358,12 +357,12 @@ pub fn check(path: &Path, bad: &mut bool) { let extensions = ["rs", "py", "js", "sh", "c", "cpp", "h", "md", "css", "ftl", "goml"]; // NB: don't skip paths without extensions (or else we'll skip all directories and will only check top level files) - if path.extension().map_or(true, |ext| !extensions.iter().any(|e| ext == OsStr::new(e))) { + if path.extension().is_none_or(|ext| !extensions.iter().any(|e| ext == OsStr::new(e))) { return true; } // We only check CSS files in rustdoc. - path.extension().map_or(false, |e| e == "css") && !is_in(path, "src", "librustdoc") + path.extension().is_some_and(|e| e == "css") && !is_in(path, "src", "librustdoc") } // This creates a RegexSet as regex contains performance optimizations to be able to deal with these over @@ -435,7 +434,7 @@ pub fn check(path: &Path, bad: &mut bool) { mut skip_copyright, mut skip_dbg, mut skip_odd_backticks, - ] = contains_ignore_directives(&path_str, can_contain, &contents, CONFIGURABLE_CHECKS); + ] = contains_ignore_directives(&path_str, can_contain, contents, CONFIGURABLE_CHECKS); let mut leading_new_lines = false; let mut trailing_new_lines = 0; let mut lines = 0; diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs index a66ccd37070..1a6fd3eaf2d 100644 --- a/src/tools/tidy/src/target_specific_tests.rs +++ b/src/tools/tidy/src/target_specific_tests.rs @@ -30,10 +30,9 @@ pub fn check(tests_path: &Path, bad: &mut bool) { comp_vec.push(component); } } - } else if directive.starts_with(COMPILE_FLAGS_HEADER) { - let compile_flags = &directive[COMPILE_FLAGS_HEADER.len()..]; + } else if let Some(compile_flags) = directive.strip_prefix(COMPILE_FLAGS_HEADER) { if let Some((_, v)) = compile_flags.split_once("--target") { - let v = v.trim_start_matches(|c| c == ' ' || c == '='); + let v = v.trim_start_matches([' ', '=']); let v = if v == "{{target}}" { Some((v, v)) } else { v.split_once("-") }; if let Some((arch, _)) = v { let info = header_map.entry(revision).or_insert(RevisionInfo::default()); @@ -57,15 +56,13 @@ pub fn check(tests_path: &Path, bad: &mut bool) { (None, None) => {} (Some(_), None) => { eprintln!( - "{}: revision {} should specify `{}` as it has `--target` set", - file, rev, LLVM_COMPONENTS_HEADER + "{file}: revision {rev} should specify `{LLVM_COMPONENTS_HEADER}` as it has `--target` set" ); *bad = true; } (None, Some(_)) => { eprintln!( - "{}: revision {} should not specify `{}` as it doesn't need `--target`", - file, rev, LLVM_COMPONENTS_HEADER + "{file}: revision {rev} should not specify `{LLVM_COMPONENTS_HEADER}` as it doesn't need `--target`" ); *bad = true; } diff --git a/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs b/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs index ee92d302db3..02412b6f190 100644 --- a/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs +++ b/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs @@ -38,7 +38,7 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { let sibling_path = sibling.path(); - let Some(ext) = sibling_path.extension().map(OsStr::to_str).flatten() else { + let Some(ext) = sibling_path.extension().and_then(OsStr::to_str) else { continue; }; @@ -84,7 +84,7 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { } }); - let Some(test_name) = test.file_stem().map(OsStr::to_str).flatten() else { + let Some(test_name) = test.file_stem().and_then(OsStr::to_str) else { continue; }; @@ -102,9 +102,9 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { // of the form: `test-name.revision.compare_mode.extension`, but our only concern is // `test-name.revision` and `extension`. for sibling in files_under_inspection.iter().filter(|f| { - f.extension().map(OsStr::to_str).flatten().is_some_and(|ext| EXTENSIONS.contains(&ext)) + f.extension().and_then(OsStr::to_str).is_some_and(|ext| EXTENSIONS.contains(&ext)) }) { - let Some(filename) = sibling.file_name().map(OsStr::to_str).flatten() else { + let Some(filename) = sibling.file_name().and_then(OsStr::to_str) else { continue; }; @@ -131,10 +131,10 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { } [_, _] => return, [_, found_revision, .., extension] => { - if !IGNORES.contains(&found_revision) + if !IGNORES.contains(found_revision) && !expected_revisions.contains(*found_revision) // This is from `//@ stderr-per-bitwidth` - && !(*extension == "stderr" && ["32bit", "64bit"].contains(&found_revision)) + && !(*extension == "stderr" && ["32bit", "64bit"].contains(found_revision)) { // Found some unexpected revision-esque component that is not a known // compare-mode or expected revision. diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 53226fcb80e..7e295731c56 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -17,7 +17,7 @@ use ignore::Walk; const ENTRY_LIMIT: u32 = 901; // FIXME: The following limits should be reduced eventually. -const ISSUES_ENTRY_LIMIT: u32 = 1619; +const ISSUES_ENTRY_LIMIT: u32 = 1616; const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[ "rs", // test source files @@ -57,11 +57,9 @@ const EXTENSION_EXCEPTION_PATHS: &[&str] = &[ fn check_entries(tests_path: &Path, bad: &mut bool) { let mut directories: HashMap<PathBuf, u32> = HashMap::new(); - for dir in Walk::new(&tests_path.join("ui")) { - if let Ok(entry) = dir { - let parent = entry.path().parent().unwrap().to_path_buf(); - *directories.entry(parent).or_default() += 1; - } + for entry in Walk::new(tests_path.join("ui")).flatten() { + let parent = entry.path().parent().unwrap().to_path_buf(); + *directories.entry(parent).or_default() += 1; } let (mut max, mut max_issues) = (0, 0); @@ -99,7 +97,7 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { "#; let path = &root_path.join("tests"); - check_entries(&path, bad); + check_entries(path, bad); // the list of files in ui tests that are allowed to start with `issue-XXXX` // BTreeSet because we would like a stable ordering so --bless works @@ -109,13 +107,12 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { .strip_prefix(issues_txt_header) .unwrap() .lines() - .map(|line| { + .inspect(|&line| { if prev_line > line { is_sorted = false; } prev_line = line; - line }) .collect(); @@ -203,7 +200,7 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { // so we don't bork things on panic or a contributor using Ctrl+C let blessed_issues_path = tidy_src.join("issues_blessed.txt"); let mut blessed_issues_txt = fs::File::create(&blessed_issues_path).unwrap(); - blessed_issues_txt.write(issues_txt_header.as_bytes()).unwrap(); + blessed_issues_txt.write_all(issues_txt_header.as_bytes()).unwrap(); // If we changed paths to use the OS separator, reassert Unix chauvinism for blessing. for filename in allowed_issue_names.difference(&remaining_issue_names) { writeln!(blessed_issues_txt, "{filename}").unwrap(); diff --git a/src/tools/tidy/src/unstable_book.rs b/src/tools/tidy/src/unstable_book.rs index a2453a6c960..9dc9d42d466 100644 --- a/src/tools/tidy/src/unstable_book.rs +++ b/src/tools/tidy/src/unstable_book.rs @@ -38,7 +38,7 @@ fn dir_entry_is_file(dir_entry: &fs::DirEntry) -> bool { pub fn collect_unstable_feature_names(features: &Features) -> BTreeSet<String> { features .iter() - .filter(|&(_, ref f)| f.level == Status::Unstable) + .filter(|&(_, f)| f.level == Status::Unstable) .map(|(name, _)| name.replace('_', "-")) .collect() } @@ -90,7 +90,7 @@ pub fn check(path: &Path, features: CollectedFeatures, bad: &mut bool) { let lib_features = features .lib .into_iter() - .filter(|&(ref name, _)| !lang_features.contains_key(name)) + .filter(|(name, _)| !lang_features.contains_key(name)) .collect::<Features>(); // Library features diff --git a/src/tools/tidy/src/walk.rs b/src/tools/tidy/src/walk.rs index c2c9eb8d250..7825ebeb5ba 100644 --- a/src/tools/tidy/src/walk.rs +++ b/src/tools/tidy/src/walk.rs @@ -66,7 +66,7 @@ pub fn walk_many( Ok(s) => s, Err(_) => return, // skip this file }; - f(&entry, &contents_str); + f(entry, contents_str); }); } @@ -83,7 +83,7 @@ pub(crate) fn walk_no_read( !skip(e.path(), e.file_type().map(|ft| ft.is_dir()).unwrap_or(false)) }); for entry in walker.build().flatten() { - if entry.file_type().map_or(true, |kind| kind.is_dir() || kind.is_symlink()) { + if entry.file_type().is_none_or(|kind| kind.is_dir() || kind.is_symlink()) { continue; } f(&entry); diff --git a/src/tools/tidy/src/x_version.rs b/src/tools/tidy/src/x_version.rs index 489343561e1..6a5e9eca813 100644 --- a/src/tools/tidy/src/x_version.rs +++ b/src/tools/tidy/src/x_version.rs @@ -26,7 +26,7 @@ pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { // Check this is the rust-lang/rust x tool installation since it should be // installed at a path containing `src/tools/x`. if let Some(path) = iter.next() { - if path.contains(&"src/tools/x") { + if path.contains("src/tools/x") { let version = version.strip_prefix("v").unwrap(); installed = Some(Version::parse(version).unwrap()); break; @@ -42,15 +42,15 @@ pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { if let Some(expected) = get_x_wrapper_version(root, cargo) { if installed < expected { - return println!( + println!( "Current version of x is {installed}, but the latest version is {expected}\nConsider updating to the newer version of x by running `cargo install --path src/tools/x`" - ); + ) } } else { - return tidy_error!( + tidy_error!( bad, "Unable to parse the latest version of `x` at `src/tools/x/Cargo.toml`" - ); + ) } } else { tidy_error!(bad, "failed to check version of `x`: {}", cargo_list.status) |