From 0d6dc2f52de5bd7900f36c3e12234ee5d8fa7f91 Mon Sep 17 00:00:00 2001 From: Marijn Schouten Date: Mon, 14 Jul 2025 09:08:29 +0000 Subject: [PATCH 1/3] tidy: more clippy fixes --- src/tools/tidy/src/features.rs | 35 ++++++++-------- src/tools/tidy/src/fluent_period.rs | 29 ++++++------- src/tools/tidy/src/lib.rs | 2 +- src/tools/tidy/src/mir_opt_tests.rs | 31 +++++++------- src/tools/tidy/src/rustdoc_templates.rs | 2 +- src/tools/tidy/src/style.rs | 46 ++++++++++----------- src/tools/tidy/src/target_specific_tests.rs | 22 +++++----- src/tools/tidy/src/ui_tests.rs | 43 ++++++++++--------- src/tools/tidy/src/x_version.rs | 12 +++--- 9 files changed, 110 insertions(+), 112 deletions(-) diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index e83b47e138064..fb00b3a943f87 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -331,11 +331,9 @@ fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, ba continue; } - if in_feature_group { - if let Some(doc_comment) = line.strip_prefix("///") { - doc_comments.push(doc_comment.trim().to_string()); - continue; - } + if in_feature_group && let Some(doc_comment) = line.strip_prefix("///") { + doc_comments.push(doc_comment.trim().to_string()); + continue; } let mut parts = line.split(','); @@ -465,19 +463,20 @@ 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(s) = list.get(name) { - if f.tracking_issue != s.tracking_issue && f.level != Status::Accepted { - tidy_error!( - bad, - "{}:{}: feature gate {} has inconsistent `issue`: \"{}\" mismatches the {} `issue` of \"{}\"", - file.display(), - line, - name, - f.tracking_issue_display(), - display, - s.tracking_issue_display(), - ); - } + if let Some(s) = list.get(name) + && f.tracking_issue != s.tracking_issue + && f.level != Status::Accepted + { + tidy_error!( + bad, + "{}:{}: feature gate {} has inconsistent `issue`: \"{}\" mismatches the {} `issue` of \"{}\"", + file.display(), + line, + name, + f.tracking_issue_display(), + display, + s.tracking_issue_display(), + ); } }; check_features(&f, lang_features, "corresponding lang feature"); diff --git a/src/tools/tidy/src/fluent_period.rs b/src/tools/tidy/src/fluent_period.rs index 85c1ef6166a4b..836b5699289f2 100644 --- a/src/tools/tidy/src/fluent_period.rs +++ b/src/tools/tidy/src/fluent_period.rs @@ -33,14 +33,14 @@ fn check_period(filename: &str, contents: &str, bad: &mut bool) { continue; } - if let Some(pat) = &m.value { - 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 name = m.id.name; - tidy_error!(bad, "{filename}:{ll}: message `{name}` ends in a period"); - } + if let Some(pat) = &m.value + && 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 name = m.id.name; + tidy_error!(bad, "{filename}:{ll}: message `{name}` ends in a period"); } } @@ -50,12 +50,13 @@ fn check_period(filename: &str, contents: &str, bad: &mut bool) { continue; } - if let Some(PatternElement::TextElement { value }) = attr.value.elements.last() { - if value.ends_with(".") && !value.ends_with("...") { - let ll = find_line(contents, value); - let name = attr.id.name; - tidy_error!(bad, "{filename}:{ll}: attr `{name}` ends in a period"); - } + if let Some(PatternElement::TextElement { value }) = attr.value.elements.last() + && value.ends_with(".") + && !value.ends_with("...") + { + 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/lib.rs b/src/tools/tidy/src/lib.rs index 77855392b4dac..bd8660797a8fb 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -134,7 +134,7 @@ pub fn files_modified(ci_info: &CiInfo, pred: impl Fn(&str) -> bool) -> bool { eprintln!("No base commit, assuming all files are modified"); return true; }; - match crate::git_diff(&base_commit, "--name-status") { + match crate::git_diff(base_commit, "--name-status") { Some(output) => { let modified_files = output.lines().filter_map(|ln| { let (status, name) = ln diff --git a/src/tools/tidy/src/mir_opt_tests.rs b/src/tools/tidy/src/mir_opt_tests.rs index 1efe71b1687e3..6119eb58383e3 100644 --- a/src/tools/tidy/src/mir_opt_tests.rs +++ b/src/tools/tidy/src/mir_opt_tests.rs @@ -55,22 +55,21 @@ fn check_dash_files(path: &Path, bless: bool, bad: &mut bool) { .filter(|e| e.file_type().is_file()) { let path = file.path(); - if path.extension() == Some("rs".as_ref()) { - if let Some(name) = path.file_name().and_then(|s| s.to_str()) { - if name.contains('-') { - if !bless { - tidy_error!( - bad, - "mir-opt test files should not have dashes in them: {}", - path.display() - ); - } else { - let new_name = name.replace('-', "_"); - let mut new_path = path.to_owned(); - new_path.set_file_name(new_name); - let _ = std::fs::rename(path, new_path); - } - } + if path.extension() == Some("rs".as_ref()) + && let Some(name) = path.file_name().and_then(|s| s.to_str()) + && name.contains('-') + { + if !bless { + tidy_error!( + bad, + "mir-opt test files should not have dashes in them: {}", + path.display() + ); + } else { + let new_name = name.replace('-', "_"); + let mut new_path = path.to_owned(); + new_path.set_file_name(new_name); + let _ = std::fs::rename(path, new_path); } } } diff --git a/src/tools/tidy/src/rustdoc_templates.rs b/src/tools/tidy/src/rustdoc_templates.rs index dca3e8d9d25f8..597290a6a9a8d 100644 --- a/src/tools/tidy/src/rustdoc_templates.rs +++ b/src/tools/tidy/src/rustdoc_templates.rs @@ -26,7 +26,7 @@ pub fn check(librustdoc_path: &Path, bad: &mut bool) { None // Then we check if this a comment tag. } else if *tag != "{#" { - return Some(false); + Some(false) // And finally we check if the comment is empty (ie, only there to strip // extra whitespace characters). } else if let Some(start_pos) = line.rfind(tag) { diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index 8dde4618ce528..35ed61eacc73f 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -417,10 +417,10 @@ pub fn check(path: &Path, bad: &mut bool) { return; } // Shell completions are automatically generated - if let Some(p) = file.parent() { - if p.ends_with(Path::new("src/etc/completions")) { - return; - } + if let Some(p) = file.parent() + && p.ends_with(Path::new("src/etc/completions")) + { + return; } let [ mut skip_cr, @@ -604,25 +604,25 @@ pub fn check(path: &Path, bad: &mut bool) { backtick_count += comment_text.chars().filter(|ch| *ch == '`').count(); } comment_block = Some((start_line, backtick_count)); - } else if let Some((start_line, backtick_count)) = comment_block.take() { - if backtick_count % 2 == 1 { - let mut err = |msg: &str| { - tidy_error!(bad, "{}:{start_line}: {msg}", file.display()); - }; - let block_len = (i + 1) - start_line; - if block_len == 1 { - suppressible_tidy_err!( - err, - skip_odd_backticks, - "comment with odd number of backticks" - ); - } else { - suppressible_tidy_err!( - err, - skip_odd_backticks, - "{block_len}-line comment block with odd number of backticks" - ); - } + } else if let Some((start_line, backtick_count)) = comment_block.take() + && backtick_count % 2 == 1 + { + let mut err = |msg: &str| { + tidy_error!(bad, "{}:{start_line}: {msg}", file.display()); + }; + let block_len = (i + 1) - start_line; + if block_len == 1 { + suppressible_tidy_err!( + err, + skip_odd_backticks, + "comment with odd number of backticks" + ); + } else { + suppressible_tidy_err!( + err, + skip_odd_backticks, + "{block_len}-line comment block with odd number of backticks" + ); } } } diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs index 1a6fd3eaf2dde..f4a6783abb679 100644 --- a/src/tools/tidy/src/target_specific_tests.rs +++ b/src/tools/tidy/src/target_specific_tests.rs @@ -30,17 +30,17 @@ pub fn check(tests_path: &Path, bad: &mut bool) { comp_vec.push(component); } } - } 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([' ', '=']); - 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()); - info.target_arch.replace(arch); - } else { - eprintln!("{file}: seems to have a malformed --target value"); - *bad = true; - } + } else if let Some(compile_flags) = directive.strip_prefix(COMPILE_FLAGS_HEADER) + && let Some((_, v)) = compile_flags.split_once("--target") + { + 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()); + info.target_arch.replace(arch); + } else { + eprintln!("{file}: seems to have a malformed --target value"); + *bad = true; } } }); diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index b1ace74e5bd28..adc0018287237 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -161,31 +161,30 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { tidy_error!(bad, "Stray file with UI testing output: {:?}", file_path); } - if let Ok(metadata) = fs::metadata(file_path) { - if metadata.len() == 0 { - tidy_error!(bad, "Empty file with UI testing output: {:?}", file_path); - } + if let Ok(metadata) = fs::metadata(file_path) + && metadata.len() == 0 + { + tidy_error!(bad, "Empty file with UI testing output: {:?}", file_path); } } - if ext == "rs" { - if let Some(test_name) = static_regex!(r"^issues?[-_]?(\d{3,})").captures(testname) - { - // these paths are always relative to the passed `path` and always UTF8 - let stripped_path = file_path - .strip_prefix(path) - .unwrap() - .to_str() - .unwrap() - .replace(std::path::MAIN_SEPARATOR_STR, "/"); - - if !remaining_issue_names.remove(stripped_path.as_str()) { - tidy_error!( - bad, - "file `tests/{stripped_path}` must begin with a descriptive name, consider `{{reason}}-issue-{issue_n}.rs`", - issue_n = &test_name[1], - ); - } + if ext == "rs" + && let Some(test_name) = static_regex!(r"^issues?[-_]?(\d{3,})").captures(testname) + { + // these paths are always relative to the passed `path` and always UTF8 + let stripped_path = file_path + .strip_prefix(path) + .unwrap() + .to_str() + .unwrap() + .replace(std::path::MAIN_SEPARATOR_STR, "/"); + + if !remaining_issue_names.remove(stripped_path.as_str()) { + tidy_error!( + bad, + "file `tests/{stripped_path}` must begin with a descriptive name, consider `{{reason}}-issue-{issue_n}.rs`", + issue_n = &test_name[1], + ); } } } diff --git a/src/tools/tidy/src/x_version.rs b/src/tools/tidy/src/x_version.rs index 6a5e9eca81328..9f7f43c4000b1 100644 --- a/src/tools/tidy/src/x_version.rs +++ b/src/tools/tidy/src/x_version.rs @@ -25,12 +25,12 @@ pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { if let Some(version) = iter.next() { // 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") { - let version = version.strip_prefix("v").unwrap(); - installed = Some(Version::parse(version).unwrap()); - break; - } + if let Some(path) = iter.next() + && path.contains("src/tools/x") + { + let version = version.strip_prefix("v").unwrap(); + installed = Some(Version::parse(version).unwrap()); + break; }; } } else { From da6d7ca1be6586ba8e0d2b89f1d60ff256dfc7ea Mon Sep 17 00:00:00 2001 From: Marijn Schouten Date: Tue, 15 Jul 2025 08:59:41 +0000 Subject: [PATCH 2/3] tidy static regex: OnceLock -> LazyLock --- src/tools/tidy/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index bd8660797a8fb..a301781fac86e 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -13,8 +13,9 @@ use termcolor::WriteColor; macro_rules! static_regex { ($re:literal) => {{ - static RE: ::std::sync::OnceLock<::regex::Regex> = ::std::sync::OnceLock::new(); - RE.get_or_init(|| ::regex::Regex::new($re).unwrap()) + static RE: ::std::sync::LazyLock<::regex::Regex> = + ::std::sync::LazyLock::new(|| ::regex::Regex::new($re).unwrap()); + &*RE }}; } From 32391f71598f637887abcb7614ad287763f8275d Mon Sep 17 00:00:00 2001 From: Marijn Schouten Date: Tue, 15 Jul 2025 09:07:07 +0000 Subject: [PATCH 3/3] tidy: improve comment --- src/tools/tidy/src/fluent_used.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/tidy/src/fluent_used.rs b/src/tools/tidy/src/fluent_used.rs index 12fafd9a7ffc2..909bf482ddfce 100644 --- a/src/tools/tidy/src/fluent_used.rs +++ b/src/tools/tidy/src/fluent_used.rs @@ -12,7 +12,7 @@ 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. + // so we check messages appear only once in all Rust and Fluent files. 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()) {