diff options
Diffstat (limited to '')
| -rw-r--r-- | src/tools/tidy/Cargo.toml | 1 | ||||
| -rw-r--r-- | src/tools/tidy/src/alphabetical.rs | 111 | ||||
| -rw-r--r-- | src/tools/tidy/src/bins.rs | 15 | ||||
| -rw-r--r-- | src/tools/tidy/src/debug_artifacts.rs | 3 | ||||
| -rw-r--r-- | src/tools/tidy/src/deps.rs | 61 | ||||
| -rw-r--r-- | src/tools/tidy/src/edition.rs | 5 | ||||
| -rw-r--r-- | src/tools/tidy/src/error_codes_check.rs | 3 | ||||
| -rw-r--r-- | src/tools/tidy/src/errors.rs | 5 | ||||
| -rw-r--r-- | src/tools/tidy/src/features.rs | 313 | ||||
| -rw-r--r-- | src/tools/tidy/src/features/version.rs | 19 | ||||
| -rw-r--r-- | src/tools/tidy/src/lib.rs | 73 | ||||
| -rw-r--r-- | src/tools/tidy/src/main.rs | 11 | ||||
| -rw-r--r-- | src/tools/tidy/src/pal.rs | 5 | ||||
| -rw-r--r-- | src/tools/tidy/src/style.rs | 20 | ||||
| -rw-r--r-- | src/tools/tidy/src/target_specific_tests.rs | 2 | ||||
| -rw-r--r-- | src/tools/tidy/src/ui_tests.rs | 6 | ||||
| -rw-r--r-- | src/tools/tidy/src/unit_tests.rs | 5 | ||||
| -rw-r--r-- | src/tools/tidy/src/walk.rs | 72 |
18 files changed, 469 insertions, 261 deletions
diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml index 96ab42b47..471d78a29 100644 --- a/src/tools/tidy/Cargo.toml +++ b/src/tools/tidy/Cargo.toml @@ -9,7 +9,6 @@ cargo_metadata = "0.14" regex = "1" lazy_static = "1" walkdir = "2" -crossbeam-utils = "0.8.0" [[bin]] name = "rust-tidy" diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs new file mode 100644 index 000000000..f913f6cde --- /dev/null +++ b/src/tools/tidy/src/alphabetical.rs @@ -0,0 +1,111 @@ +//! Checks that a list of items is in alphabetical order +//! +//! To use, use the following annotation in the code: +//! ```rust +//! // tidy-alphabetical-start +//! fn aaa() {} +//! fn eee() {} +//! fn z() {} +//! // tidy-alphabetical-end +//! ``` +//! +//! The following lines are ignored: +//! - Lines that are indented with more or less spaces than the first line +//! - Lines starting with `//`, `#[`, `)`, `]`, `}` if the comment has the same indentation as +//! the first line +//! +//! If a line ends with an opening bracket, the line is ignored and the next line will have +//! its extra indentation ignored. + +use std::{fmt::Display, path::Path}; + +use crate::walk::{filter_dirs, walk}; + +fn indentation(line: &str) -> usize { + line.find(|c| c != ' ').unwrap_or(0) +} + +fn is_close_bracket(c: char) -> bool { + matches!(c, ')' | ']' | '}') +} + +// Don't let tidy check this here :D +const START_COMMENT: &str = concat!("// tidy-alphabetical", "-start"); +const END_COMMENT: &str = "// tidy-alphabetical-end"; + +fn check_section<'a>( + file: impl Display, + lines: impl Iterator<Item = (usize, &'a str)>, + bad: &mut bool, +) { + let content_lines = lines.take_while(|(_, line)| !line.contains(END_COMMENT)); + + let mut prev_line = String::new(); + let mut first_indent = None; + let mut in_split_line = None; + + for (line_idx, line) in content_lines { + if line.contains(START_COMMENT) { + tidy_error!( + bad, + "{file}:{} found `{START_COMMENT}` expecting `{END_COMMENT}`", + line_idx + ) + } + + let indent = first_indent.unwrap_or_else(|| { + let indent = indentation(line); + first_indent = Some(indent); + indent + }); + + let line = if let Some(prev_split_line) = in_split_line { + in_split_line = None; + format!("{prev_split_line}{}", line.trim_start()) + } else { + line.to_string() + }; + + if indentation(&line) != indent { + continue; + } + + let trimmed_line = line.trim_start_matches(' '); + + if trimmed_line.starts_with("//") + || trimmed_line.starts_with("#[") + || trimmed_line.starts_with(is_close_bracket) + { + continue; + } + + if line.trim_end().ends_with('(') { + in_split_line = Some(line); + continue; + } + + let prev_line_trimmed_lowercase = prev_line.trim_start_matches(' ').to_lowercase(); + + if trimmed_line.to_lowercase() < prev_line_trimmed_lowercase { + tidy_error!(bad, "{file}:{}: line not in alphabetical order", line_idx + 1,); + } + + prev_line = line; + } +} + +pub fn check(path: &Path, bad: &mut bool) { + walk(path, &mut filter_dirs, &mut |entry, contents| { + let file = &entry.path().display(); + + let mut lines = contents.lines().enumerate().peekable(); + while let Some((_, line)) = lines.next() { + if line.contains(START_COMMENT) { + check_section(file, &mut lines, bad); + if lines.peek().is_none() { + tidy_error!(bad, "{file}: reached end of file expecting `{END_COMMENT}`") + } + } + } + }); +} diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index 9615c4db6..b898f20a5 100644 --- a/src/tools/tidy/src/bins.rs +++ b/src/tools/tidy/src/bins.rs @@ -21,6 +21,7 @@ mod os_impl { #[cfg(unix)] mod os_impl { + use crate::walk::{filter_dirs, walk_no_read}; use std::fs; use std::os::unix::prelude::*; use std::path::Path; @@ -96,12 +97,14 @@ mod os_impl { #[cfg(unix)] pub fn check(path: &Path, bad: &mut bool) { - const ALLOWED: &[&str] = &["configure"]; + use std::ffi::OsStr; - crate::walk_no_read( + const ALLOWED: &[&str] = &["configure", "x"]; + + walk_no_read( path, &mut |path| { - crate::filter_dirs(path) + filter_dirs(path) || path.ends_with("src/etc") // This is a list of directories that we almost certainly // don't need to walk. A future PR will likely want to @@ -117,9 +120,9 @@ mod os_impl { }, &mut |entry| { let file = entry.path(); - let filename = file.file_name().unwrap().to_string_lossy(); - let extensions = [".py", ".sh"]; - if extensions.iter().any(|e| filename.ends_with(e)) { + let extension = file.extension(); + let scripts = ["py", "sh", "ps1"]; + if scripts.into_iter().any(|e| extension == Some(OsStr::new(e))) { return; } diff --git a/src/tools/tidy/src/debug_artifacts.rs b/src/tools/tidy/src/debug_artifacts.rs index ab87230f8..9880a32ad 100644 --- a/src/tools/tidy/src/debug_artifacts.rs +++ b/src/tools/tidy/src/debug_artifacts.rs @@ -1,5 +1,6 @@ //! Tidy check to prevent creation of unnecessary debug artifacts while running tests. +use crate::walk::{filter_dirs, walk}; use std::path::{Path, PathBuf}; const GRAPHVIZ_POSTFLOW_MSG: &str = "`borrowck_graphviz_postflow` attribute in test"; @@ -7,7 +8,7 @@ const GRAPHVIZ_POSTFLOW_MSG: &str = "`borrowck_graphviz_postflow` attribute in t pub fn check(path: &Path, bad: &mut bool) { let test_dir: PathBuf = path.join("test"); - super::walk(&test_dir, &mut super::filter_dirs, &mut |entry, contents| { + walk(&test_dir, &mut filter_dirs, &mut |entry, contents| { let filename = entry.path(); let is_rust = filename.extension().map_or(false, |ext| ext == "rs"); if !is_rust { diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index 333f85f6d..8a0239ece 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -18,9 +18,11 @@ const LICENSES: &[&str] = &[ "ISC", "Unlicense/MIT", "Unlicense OR MIT", - "0BSD OR MIT OR Apache-2.0", // adler license - "Zlib OR Apache-2.0 OR MIT", // tinyvec - "MIT OR Zlib OR Apache-2.0", // miniz_oxide + "0BSD OR MIT OR Apache-2.0", // adler license + "Zlib OR Apache-2.0 OR MIT", // tinyvec + "MIT OR Apache-2.0 OR Zlib", // tinyvec_macros + "MIT OR Zlib OR Apache-2.0", // miniz_oxide + "(MIT OR Apache-2.0) AND Unicode-DFS-2016", // unicode_ident ]; /// These are exceptions to Rust's permissive licensing policy, and @@ -31,8 +33,7 @@ const EXCEPTIONS: &[(&str, &str)] = &[ ("mdbook", "MPL-2.0"), // mdbook ("openssl", "Apache-2.0"), // cargo, mdbook ("colored", "MPL-2.0"), // rustfmt - ("ordslice", "Apache-2.0"), // rls - ("ryu", "Apache-2.0 OR BSL-1.0"), // rls/cargo/... (because of serde) + ("ryu", "Apache-2.0 OR BSL-1.0"), // cargo/... (because of serde) ("bytesize", "Apache-2.0"), // cargo ("im-rc", "MPL-2.0+"), // cargo ("sized-chunks", "MPL-2.0+"), // cargo via im-rc @@ -73,14 +74,11 @@ const EXCEPTIONS_BOOTSTRAP: &[(&str, &str)] = &[ /// these and all their dependencies *must not* be in the exception list. const RUNTIME_CRATES: &[&str] = &["std", "core", "alloc", "test", "panic_abort", "panic_unwind"]; -/// Crates whose dependencies must be explicitly permitted. -const RESTRICTED_DEPENDENCY_CRATES: &[&str] = &["rustc_driver", "rustc_codegen_llvm"]; - /// Crates rustc is allowed to depend on. Avoid adding to the list if possible. /// /// This list is here to provide a speed-bump to adding a new dependency to /// rustc. Please check with the compiler team before adding an entry. -const PERMITTED_DEPENDENCIES: &[&str] = &[ +const PERMITTED_RUSTC_DEPENDENCIES: &[&str] = &[ "addr2line", "adler", "ahash", @@ -92,9 +90,6 @@ const PERMITTED_DEPENDENCIES: &[&str] = &[ "autocfg", "bitflags", "block-buffer", - "block-padding", - "byte-tools", - "byteorder", "cc", "cfg-if", "chalk-derive", @@ -119,7 +114,6 @@ const PERMITTED_DEPENDENCIES: &[&str] = &[ "ena", "env_logger", "expect-test", - "fake-simd", "fallible-iterator", // dependency of `thorin` "filetime", "fixedbitset", @@ -163,7 +157,6 @@ const PERMITTED_DEPENDENCIES: &[&str] = &[ "object", "odht", "once_cell", - "opaque-debug", "parking_lot", "parking_lot_core", "pathdiff", @@ -224,6 +217,8 @@ const PERMITTED_DEPENDENCIES: &[&str] = &[ "time", "tinystr", "tinyvec", + "tinyvec_macros", + "thin-vec", "tracing", "tracing-attributes", "tracing-core", @@ -241,12 +236,14 @@ const PERMITTED_DEPENDENCIES: &[&str] = &[ "unic-langid-macros", "unic-langid-macros-impl", "unic-ucd-version", + "unicode-ident", "unicode-normalization", "unicode-script", "unicode-security", "unicode-width", "unicode-xid", "vcpkg", + "valuable", "version_check", "wasi", "winapi", @@ -262,7 +259,9 @@ const PERMITTED_CRANELIFT_DEPENDENCIES: &[&str] = &[ "ahash", "anyhow", "ar", + "arrayvec", "autocfg", + "bumpalo", "bitflags", "byteorder", "cfg-if", @@ -300,10 +299,16 @@ const PERMITTED_CRANELIFT_DEPENDENCIES: &[&str] = &[ "winapi", "winapi-i686-pc-windows-gnu", "winapi-x86_64-pc-windows-gnu", + "windows-sys", + "windows_aarch64_msvc", + "windows_i686_gnu", + "windows_i686_msvc", + "windows_x86_64_gnu", + "windows_x86_64_msvc", ]; const FORBIDDEN_TO_HAVE_DUPLICATES: &[&str] = &[ - // These two crates take quite a long time to build, so don't allow two versions of them + // This crate takes quite a long time to build, so don't allow two versions of them // to accidentally sneak into our dependency graph, in order to ensure we keep our CI times // under control. "cargo", @@ -320,12 +325,12 @@ pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { .features(cargo_metadata::CargoOpt::AllFeatures); let metadata = t!(cmd.exec()); let runtime_ids = compute_runtime_crates(&metadata); - check_exceptions(&metadata, EXCEPTIONS, runtime_ids, bad); - check_dependencies( + check_license_exceptions(&metadata, EXCEPTIONS, runtime_ids, bad); + check_permitted_dependencies( &metadata, - "main workspace", - PERMITTED_DEPENDENCIES, - RESTRICTED_DEPENDENCY_CRATES, + "rustc", + PERMITTED_RUSTC_DEPENDENCIES, + &["rustc_driver", "rustc_codegen_llvm"], bad, ); check_crate_duplicate(&metadata, FORBIDDEN_TO_HAVE_DUPLICATES, bad); @@ -338,8 +343,8 @@ pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { .features(cargo_metadata::CargoOpt::AllFeatures); let metadata = t!(cmd.exec()); let runtime_ids = HashSet::new(); - check_exceptions(&metadata, EXCEPTIONS_CRANELIFT, runtime_ids, bad); - check_dependencies( + check_license_exceptions(&metadata, EXCEPTIONS_CRANELIFT, runtime_ids, bad); + check_permitted_dependencies( &metadata, "cranelift", PERMITTED_CRANELIFT_DEPENDENCIES, @@ -354,13 +359,13 @@ pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { .features(cargo_metadata::CargoOpt::AllFeatures); let metadata = t!(cmd.exec()); let runtime_ids = HashSet::new(); - check_exceptions(&metadata, EXCEPTIONS_BOOTSTRAP, runtime_ids, bad); + check_license_exceptions(&metadata, EXCEPTIONS_BOOTSTRAP, runtime_ids, bad); } /// Check that all licenses are in the valid list in `LICENSES`. /// -/// Packages listed in `EXCEPTIONS` are allowed for tools. -fn check_exceptions( +/// Packages listed in `exceptions` are allowed for tools. +fn check_license_exceptions( metadata: &Metadata, exceptions: &[(&str, &str)], runtime_ids: HashSet<&PackageId>, @@ -430,11 +435,11 @@ fn check_exceptions( } } -/// Checks the dependency of `RESTRICTED_DEPENDENCY_CRATES` at the given path. Changes `bad` to +/// Checks the dependency of `restricted_dependency_crates` at the given path. Changes `bad` to /// `true` if a check failed. /// -/// Specifically, this checks that the dependencies are on the `PERMITTED_DEPENDENCIES`. -fn check_dependencies( +/// Specifically, this checks that the dependencies are on the `permitted_dependencies`. +fn check_permitted_dependencies( metadata: &Metadata, descr: &str, permitted_dependencies: &[&'static str], diff --git a/src/tools/tidy/src/edition.rs b/src/tools/tidy/src/edition.rs index b0abee459..8a7c4460d 100644 --- a/src/tools/tidy/src/edition.rs +++ b/src/tools/tidy/src/edition.rs @@ -1,5 +1,6 @@ //! Tidy check to ensure that crate `edition` is '2018' or '2021'. +use crate::walk::{filter_dirs, walk}; use std::path::Path; fn is_edition_2021(mut line: &str) -> bool { @@ -8,9 +9,9 @@ fn is_edition_2021(mut line: &str) -> bool { } pub fn check(path: &Path, bad: &mut bool) { - super::walk( + walk( path, - &mut |path| super::filter_dirs(path) || path.ends_with("src/test"), + &mut |path| filter_dirs(path) || path.ends_with("src/test"), &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap(); diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index f0054a1c1..610e322e1 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -1,6 +1,7 @@ //! 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; @@ -217,7 +218,7 @@ pub fn check(paths: &[&Path], bad: &mut bool) { println!("Checking which error codes lack tests..."); for path in paths { - super::walk(path, &mut |path| super::filter_dirs(path), &mut |entry, contents| { + walk(path, &mut filter_dirs, &mut |entry, contents| { let file_name = entry.file_name(); let entry_path = entry.path(); diff --git a/src/tools/tidy/src/errors.rs b/src/tools/tidy/src/errors.rs index dbcc9341a..fe5fd72b9 100644 --- a/src/tools/tidy/src/errors.rs +++ b/src/tools/tidy/src/errors.rs @@ -3,14 +3,15 @@ //! 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(); - super::walk( + walk( path, - &mut |path| super::filter_dirs(path) || path.ends_with("src/test"), + &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(); diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index 2f22c081a..f10ecf5f2 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -9,7 +9,8 @@ //! * All unstable lang features have tests to ensure they are actually unstable. //! * Language features in a group are sorted by feature name. -use std::collections::HashMap; +use crate::walk::{filter_dirs, walk, walk_many}; +use std::collections::hash_map::{Entry, HashMap}; use std::fmt; use std::fs; use std::num::NonZeroU32; @@ -92,14 +93,14 @@ pub fn check( let lib_features = get_and_check_lib_features(lib_path, bad, &features); assert!(!lib_features.is_empty()); - super::walk_many( + walk_many( &[ &src_path.join("test/ui"), &src_path.join("test/ui-fulldeps"), &src_path.join("test/rustdoc-ui"), &src_path.join("test/rustdoc"), ], - &mut |path| super::filter_dirs(path), + &mut filter_dirs, &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); @@ -175,6 +176,35 @@ pub fn check( tidy_error!(bad, "Found {} features without a gate test.", gate_untested.len()); } + let (version, channel) = get_version_and_channel(src_path); + + let all_features_iter = features + .iter() + .map(|feat| (feat, "lang")) + .chain(lib_features.iter().map(|feat| (feat, "lib"))); + for ((feature_name, feature), kind) in all_features_iter { + let since = if let Some(since) = feature.since { since } else { continue }; + if since > version && since != Version::CurrentPlaceholder { + tidy_error!( + bad, + "The stabilization version {since} of {kind} feature `{feature_name}` is newer than the current {version}" + ); + } + if channel == "nightly" && since == version { + tidy_error!( + bad, + "The stabilization version {since} of {kind} feature `{feature_name}` is written out but should be {}", + version::VERSION_PLACEHOLDER + ); + } + if channel != "nightly" && since == Version::CurrentPlaceholder { + tidy_error!( + bad, + "The placeholder use of {kind} feature `{feature_name}` is not allowed on the {channel} channel", + ); + } + } + if *bad { return CollectedFeatures { lib: lib_features, lang: features }; } @@ -195,6 +225,14 @@ pub fn check( CollectedFeatures { lib: lib_features, lang: features } } +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 channel_str = t!(std::fs::read_to_string(src_path.join("ci").join("channel"))); + (version, channel_str.trim().to_owned()) +} + fn format_features<'a>( features: &'a Features, family: &'a str, @@ -242,13 +280,14 @@ fn test_filen_gate(filen_underscore: &str, features: &mut Features) -> bool { } pub fn collect_lang_features(base_compiler_path: &Path, bad: &mut bool) -> Features { - let mut all = collect_lang_features_in(base_compiler_path, "active.rs", bad); - all.extend(collect_lang_features_in(base_compiler_path, "accepted.rs", bad)); - all.extend(collect_lang_features_in(base_compiler_path, "removed.rs", bad)); - all + let mut features = Features::new(); + collect_lang_features_in(&mut features, base_compiler_path, "active.rs", bad); + collect_lang_features_in(&mut features, base_compiler_path, "accepted.rs", bad); + collect_lang_features_in(&mut features, base_compiler_path, "removed.rs", bad); + features } -fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features { +fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, bad: &mut bool) { let path = base.join("rustc_feature").join("src").join(file); let contents = t!(fs::read_to_string(&path)); @@ -260,135 +299,145 @@ fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features let mut in_feature_group = false; let mut prev_names = vec![]; - contents - .lines() - .zip(1..) - .filter_map(|(line, line_number)| { - let line = line.trim(); - - // Within -start and -end, the tracking issue can be omitted. - match line { - "// no-tracking-issue-start" => { - next_feature_omits_tracking_issue = true; - return None; - } - "// no-tracking-issue-end" => { - next_feature_omits_tracking_issue = false; - return None; - } - _ => {} + let lines = contents.lines().zip(1..); + for (line, line_number) in lines { + let line = line.trim(); + + // Within -start and -end, the tracking issue can be omitted. + match line { + "// no-tracking-issue-start" => { + next_feature_omits_tracking_issue = true; + continue; + } + "// no-tracking-issue-end" => { + next_feature_omits_tracking_issue = false; + continue; } + _ => {} + } - if line.starts_with(FEATURE_GROUP_START_PREFIX) { - if in_feature_group { - tidy_error!( - bad, - "{}:{}: \ + if line.starts_with(FEATURE_GROUP_START_PREFIX) { + if in_feature_group { + tidy_error!( + bad, + "{}:{}: \ new feature group is started without ending the previous one", - path.display(), - line_number, - ); - } - - in_feature_group = true; - prev_names = vec![]; - return None; - } else if line.starts_with(FEATURE_GROUP_END_PREFIX) { - in_feature_group = false; - prev_names = vec![]; - return None; + path.display(), + line_number, + ); } - let mut parts = line.split(','); - let level = match parts.next().map(|l| l.trim().trim_start_matches('(')) { - Some("active") => Status::Unstable, - Some("incomplete") => Status::Unstable, - Some("removed") => Status::Removed, - Some("accepted") => Status::Stable, - _ => return None, - }; - let name = parts.next().unwrap().trim(); - - let since_str = parts.next().unwrap().trim().trim_matches('"'); - let since = match since_str.parse() { - Ok(since) => Some(since), - Err(err) => { - tidy_error!( - bad, - "{}:{}: failed to parse since: {} ({:?})", - path.display(), - line_number, - since_str, - err, - ); - None - } - }; - if in_feature_group { - if prev_names.last() > Some(&name) { - // This assumes the user adds the feature name at the end of the list, as we're - // not looking ahead. - let correct_index = match prev_names.binary_search(&name) { - Ok(_) => { - // This only occurs when the feature name has already been declared. - tidy_error!( - bad, - "{}:{}: duplicate feature {}", - path.display(), - line_number, - name, - ); - // skip any additional checks for this line - return None; - } - Err(index) => index, - }; + in_feature_group = true; + prev_names = vec![]; + continue; + } else if line.starts_with(FEATURE_GROUP_END_PREFIX) { + in_feature_group = false; + prev_names = vec![]; + continue; + } - let correct_placement = if correct_index == 0 { - "at the beginning of the feature group".to_owned() - } else if correct_index == prev_names.len() { - // I don't believe this is reachable given the above assumption, but it - // doesn't hurt to be safe. - "at the end of the feature group".to_owned() - } else { - format!( - "between {} and {}", - prev_names[correct_index - 1], - prev_names[correct_index], - ) - }; + let mut parts = line.split(','); + let level = match parts.next().map(|l| l.trim().trim_start_matches('(')) { + Some("active") => Status::Unstable, + Some("incomplete") => Status::Unstable, + Some("removed") => Status::Removed, + Some("accepted") => Status::Stable, + _ => continue, + }; + let name = parts.next().unwrap().trim(); + + let since_str = parts.next().unwrap().trim().trim_matches('"'); + let since = match since_str.parse() { + Ok(since) => Some(since), + Err(err) => { + tidy_error!( + bad, + "{}:{}: failed to parse since: {} ({:?})", + path.display(), + line_number, + since_str, + err, + ); + None + } + }; + if in_feature_group { + if prev_names.last() > Some(&name) { + // This assumes the user adds the feature name at the end of the list, as we're + // not looking ahead. + let correct_index = match prev_names.binary_search(&name) { + Ok(_) => { + // This only occurs when the feature name has already been declared. + tidy_error!( + bad, + "{}:{}: duplicate feature {}", + path.display(), + line_number, + name, + ); + // skip any additional checks for this line + continue; + } + Err(index) => index, + }; - tidy_error!( - bad, - "{}:{}: feature {} is not sorted by feature name (should be {})", - path.display(), - line_number, - name, - correct_placement, - ); - } - prev_names.push(name); + let correct_placement = if correct_index == 0 { + "at the beginning of the feature group".to_owned() + } else if correct_index == prev_names.len() { + // I don't believe this is reachable given the above assumption, but it + // doesn't hurt to be safe. + "at the end of the feature group".to_owned() + } else { + format!( + "between {} and {}", + prev_names[correct_index - 1], + prev_names[correct_index], + ) + }; + + tidy_error!( + bad, + "{}:{}: feature {} is not sorted by feature name (should be {})", + path.display(), + line_number, + name, + correct_placement, + ); } + prev_names.push(name); + } - let issue_str = parts.next().unwrap().trim(); - let tracking_issue = if issue_str.starts_with("None") { - if level == Status::Unstable && !next_feature_omits_tracking_issue { - tidy_error!( - bad, - "{}:{}: no tracking issue for feature {}", - path.display(), - line_number, - name, - ); - } - None - } else { - let s = issue_str.split('(').nth(1).unwrap().split(')').next().unwrap(); - Some(s.parse().unwrap()) - }; - Some((name.to_owned(), Feature { level, since, has_gate_test: false, tracking_issue })) - }) - .collect() + let issue_str = parts.next().unwrap().trim(); + let tracking_issue = if issue_str.starts_with("None") { + if level == Status::Unstable && !next_feature_omits_tracking_issue { + tidy_error!( + bad, + "{}:{}: no tracking issue for feature {}", + path.display(), + line_number, + name, + ); + } + None + } else { + let s = issue_str.split('(').nth(1).unwrap().split(')').next().unwrap(); + Some(s.parse().unwrap()) + }; + match features.entry(name.to_owned()) { + Entry::Occupied(e) => { + tidy_error!( + bad, + "{}:{} feature {name} already specified with status '{}'", + path.display(), + line_number, + e.get().level, + ); + } + Entry::Vacant(e) => { + e.insert(Feature { level, since, has_gate_test: false, tracking_issue }); + } + } + } } fn get_and_check_lib_features( @@ -429,9 +478,9 @@ fn map_lib_features( base_src_path: &Path, mf: &mut dyn FnMut(Result<(&str, Feature), &str>, &Path, usize), ) { - super::walk( + walk( base_src_path, - &mut |path| super::filter_dirs(path) || path.ends_with("src/test"), + &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(); @@ -500,7 +549,9 @@ fn map_lib_features( becoming_feature = None; if line.contains("rustc_const_unstable(") { // `const fn` features are handled specially. - let feature_name = match find_attr_val(line, "feature") { + let feature_name = match find_attr_val(line, "feature").or_else(|| { + iter_lines.peek().and_then(|next| find_attr_val(next.1, "feature")) + }) { Some(name) => name, None => err!("malformed stability attribute: missing `feature` key"), }; diff --git a/src/tools/tidy/src/features/version.rs b/src/tools/tidy/src/features/version.rs index 620be2f98..0830c226c 100644 --- a/src/tools/tidy/src/features/version.rs +++ b/src/tools/tidy/src/features/version.rs @@ -5,14 +5,22 @@ use std::str::FromStr; #[cfg(test)] mod tests; +pub const VERSION_PLACEHOLDER: &str = "CURRENT_RUSTC_VERSION"; + #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] -pub struct Version { - parts: [u32; 3], +pub enum Version { + Explicit { parts: [u32; 3] }, + CurrentPlaceholder, } impl fmt::Display for Version { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.pad(&format!("{}.{}.{}", self.parts[0], self.parts[1], self.parts[2])) + match self { + Version::Explicit { parts } => { + f.pad(&format!("{}.{}.{}", parts[0], parts[1], parts[2])) + } + Version::CurrentPlaceholder => f.pad(&format!("CURRENT")), + } } } @@ -32,6 +40,9 @@ impl FromStr for Version { type Err = ParseVersionError; fn from_str(s: &str) -> Result<Self, Self::Err> { + if s == VERSION_PLACEHOLDER { + return Ok(Version::CurrentPlaceholder); + } let mut iter = s.split('.').map(|part| Ok(part.parse()?)); let mut part = || iter.next().unwrap_or(Err(ParseVersionError::WrongNumberOfParts)); @@ -43,6 +54,6 @@ impl FromStr for Version { return Err(ParseVersionError::WrongNumberOfParts); } - Ok(Self { parts }) + Ok(Version::Explicit { parts }) } } diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 09848462a..fc0bce585 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -3,12 +3,12 @@ //! This library contains the tidy lints and exposes it //! to be used by tools. -use std::fs::File; -use std::io::Read; -use walkdir::{DirEntry, WalkDir}; - -use std::path::Path; - +/// A helper macro to `unwrap` a result except also print out details like: +/// +/// * The expression that failed +/// * The error itself +/// * (optionally) a path connected to the error (e.g. failure to open a file) +#[macro_export] macro_rules! t { ($e:expr, $p:expr) => { match $e { @@ -28,7 +28,8 @@ macro_rules! t { macro_rules! tidy_error { ($bad:expr, $fmt:expr) => ({ *$bad = true; - eprintln!("tidy error: {}", $fmt); + eprint!("tidy error: "); + eprintln!($fmt); }); ($bad:expr, $fmt:expr, $($arg:tt)*) => ({ *$bad = true; @@ -37,6 +38,7 @@ macro_rules! tidy_error { }); } +pub mod alphabetical; pub mod bins; pub mod debug_artifacts; pub mod deps; @@ -52,59 +54,4 @@ pub mod target_specific_tests; pub mod ui_tests; pub mod unit_tests; pub mod unstable_book; - -fn filter_dirs(path: &Path) -> bool { - let skip = [ - "tidy-test-file", - "compiler/rustc_codegen_cranelift", - "compiler/rustc_codegen_gcc", - "src/llvm-project", - "library/backtrace", - "library/portable-simd", - "library/stdarch", - "src/tools/cargo", - "src/tools/clippy", - "src/tools/miri", - "src/tools/rls", - "src/tools/rust-analyzer", - "src/tools/rust-installer", - "src/tools/rustfmt", - "src/doc/book", - // Filter RLS output directories - "target/rls", - ]; - skip.iter().any(|p| path.ends_with(p)) -} - -fn walk_many( - paths: &[&Path], - skip: &mut dyn FnMut(&Path) -> bool, - f: &mut dyn FnMut(&DirEntry, &str), -) { - for path in paths { - walk(path, skip, f); - } -} - -fn walk(path: &Path, skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&DirEntry, &str)) { - let mut contents = String::new(); - walk_no_read(path, skip, &mut |entry| { - contents.clear(); - if t!(File::open(entry.path()), entry.path()).read_to_string(&mut contents).is_err() { - contents.clear(); - } - f(&entry, &contents); - }); -} - -fn walk_no_read(path: &Path, skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&DirEntry)) { - let walker = WalkDir::new(path).into_iter().filter_entry(|e| !skip(e.path())); - for entry in walker { - if let Ok(entry) = entry { - if entry.file_type().is_dir() { - continue; - } - f(&entry); - } - } -} +pub mod walk; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index aa8d8b4f6..ca785042a 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -6,7 +6,6 @@ use tidy::*; -use crossbeam_utils::thread::{scope, ScopedJoinHandle}; use std::collections::VecDeque; use std::env; use std::num::NonZeroUsize; @@ -14,6 +13,7 @@ use std::path::PathBuf; use std::process; use std::str::FromStr; use std::sync::atomic::{AtomicBool, Ordering}; +use std::thread::{scope, ScopedJoinHandle}; fn main() { let root_path: PathBuf = env::args_os().nth(1).expect("need path to root of repo").into(); @@ -44,7 +44,7 @@ fn main() { handles.pop_front().unwrap().join().unwrap(); } - let handle = s.spawn(|_| { + let handle = s.spawn(|| { let mut flag = false; $p::check($($args),* , &mut flag); if (flag) { @@ -90,6 +90,10 @@ fn main() { check!(edition, &compiler_path); check!(edition, &library_path); + check!(alphabetical, &src_path); + check!(alphabetical, &compiler_path); + check!(alphabetical, &library_path); + let collected = { while handles.len() >= concurrency.get() { handles.pop_front().unwrap().join().unwrap(); @@ -102,8 +106,7 @@ fn main() { r }; check!(unstable_book, &src_path, collected); - }) - .unwrap(); + }); if bad.load(Ordering::Relaxed) { eprintln!("some tidy checks failed"); diff --git a/src/tools/tidy/src/pal.rs b/src/tools/tidy/src/pal.rs index c5414233f..f4592fdcf 100644 --- a/src/tools/tidy/src/pal.rs +++ b/src/tools/tidy/src/pal.rs @@ -30,6 +30,7 @@ //! platform-specific cfgs are allowed. Not sure yet how to deal with //! this in the long term. +use crate::walk::{filter_dirs, walk}; use std::iter::Iterator; use std::path::Path; @@ -59,13 +60,15 @@ const EXCEPTION_PATHS: &[&str] = &[ "library/std/src/sys_common", // Should only contain abstractions over platforms "library/std/src/net/test.rs", // Utility helpers for tests "library/std/src/panic.rs", // fuchsia-specific panic backtrace handling + "library/std/src/personality.rs", + "library/std/src/personality/", ]; pub fn check(path: &Path, bad: &mut bool) { // Sanity check that the complex parsing here works. let mut saw_target_arch = false; let mut saw_cfg_bang = false; - super::walk(path, &mut super::filter_dirs, &mut |entry, contents| { + walk(path, &mut filter_dirs, &mut |entry, contents| { let file = entry.path(); let filestr = file.to_string_lossy().replace("\\", "/"); if !filestr.ends_with(".rs") { diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index 3cf44a2d7..541380ceb 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -16,6 +16,7 @@ //! A number of these checks can be opted-out of with various directives of the form: //! `// ignore-tidy-CHECK-NAME`. +use crate::walk::{filter_dirs, walk}; use regex::Regex; use std::path::Path; @@ -125,16 +126,13 @@ fn should_ignore(line: &str) -> bool { /// Returns `true` if `line` is allowed to be longer than the normal limit. fn long_line_is_ok(extension: &str, is_error_code: bool, max_columns: usize, line: &str) -> bool { - if extension != "md" || is_error_code { - if line_is_url(is_error_code, max_columns, line) || should_ignore(line) { - return true; - } - } else if extension == "md" { + match extension { + // fluent files are allowed to be any length + "ftl" => true, // non-error code markdown is allowed to be any length - return true; + "md" if !is_error_code => true, + _ => line_is_url(is_error_code, max_columns, line) || should_ignore(line), } - - false } enum Directive { @@ -221,16 +219,16 @@ fn is_unexplained_ignore(extension: &str, line: &str) -> bool { pub fn check(path: &Path, bad: &mut bool) { fn skip(path: &Path) -> bool { - super::filter_dirs(path) || skip_markdown_path(path) + filter_dirs(path) || skip_markdown_path(path) } let problematic_consts_strings: Vec<String> = (PROBLEMATIC_CONSTS.iter().map(u32::to_string)) .chain(PROBLEMATIC_CONSTS.iter().map(|v| format!("{:x}", v))) .chain(PROBLEMATIC_CONSTS.iter().map(|v| format!("{:X}", v))) .collect(); - super::walk(path, &mut skip, &mut |entry, contents| { + walk(path, &mut skip, &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); - let extensions = [".rs", ".py", ".js", ".sh", ".c", ".cpp", ".h", ".md", ".css"]; + let extensions = [".rs", ".py", ".js", ".sh", ".c", ".cpp", ".h", ".md", ".css", ".ftl"]; if extensions.iter().all(|e| !filename.ends_with(e)) || filename.starts_with(".#") { return; } diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs index 723684bfa..8ba257056 100644 --- a/src/tools/tidy/src/target_specific_tests.rs +++ b/src/tools/tidy/src/target_specific_tests.rs @@ -36,7 +36,7 @@ struct RevisionInfo<'a> { pub fn check(path: &Path, bad: &mut bool) { let tests = path.join("test"); - super::walk( + crate::walk::walk( &tests, &mut |path| path.extension().map(|p| p == "rs") == Some(false), &mut |entry, content| { diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 8ec5c3324..c600f99c2 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -7,8 +7,8 @@ use std::path::Path; const ENTRY_LIMIT: usize = 1000; // FIXME: The following limits should be reduced eventually. -const ROOT_ENTRY_LIMIT: usize = 968; -const ISSUES_ENTRY_LIMIT: usize = 2147; +const ROOT_ENTRY_LIMIT: usize = 948; +const ISSUES_ENTRY_LIMIT: usize = 2117; fn check_entries(path: &Path, bad: &mut bool) { let dirs = walkdir::WalkDir::new(&path.join("test/ui")) @@ -47,7 +47,7 @@ fn check_entries(path: &Path, bad: &mut bool) { pub fn check(path: &Path, bad: &mut bool) { check_entries(&path, bad); for path in &[&path.join("test/ui"), &path.join("test/ui-fulldeps")] { - super::walk_no_read(path, &mut |_| false, &mut |entry| { + crate::walk::walk_no_read(path, &mut |_| false, &mut |entry| { let file_path = entry.path(); if let Some(ext) = file_path.extension() { if ext == "stderr" || ext == "stdout" { diff --git a/src/tools/tidy/src/unit_tests.rs b/src/tools/tidy/src/unit_tests.rs index f675b7865..2c23b6ebc 100644 --- a/src/tools/tidy/src/unit_tests.rs +++ b/src/tools/tidy/src/unit_tests.rs @@ -7,6 +7,7 @@ //! named `tests.rs` or `benches.rs`, or directories named `tests` or `benches` unconfigured //! during normal build. +use crate::walk::{filter_dirs, walk}; use std::path::Path; pub fn check(root_path: &Path, bad: &mut bool) { @@ -20,7 +21,7 @@ pub fn check(root_path: &Path, bad: &mut bool) { let mut skip = |path: &Path| { let file_name = path.file_name().unwrap_or_default(); if path.is_dir() { - super::filter_dirs(path) + filter_dirs(path) || path.ends_with("src/test") || path.ends_with("src/doc") || (file_name == "tests" || file_name == "benches") && !is_core(path) @@ -34,7 +35,7 @@ pub fn check(root_path: &Path, bad: &mut bool) { } }; - super::walk(root_path, &mut skip, &mut |entry, contents| { + walk(root_path, &mut skip, &mut |entry, contents| { let path = entry.path(); let is_core = path.starts_with(core); for (i, line) in contents.lines().enumerate() { diff --git a/src/tools/tidy/src/walk.rs b/src/tools/tidy/src/walk.rs new file mode 100644 index 000000000..4cfb70fa3 --- /dev/null +++ b/src/tools/tidy/src/walk.rs @@ -0,0 +1,72 @@ +use std::fs::File; +use std::io::Read; +use walkdir::{DirEntry, WalkDir}; + +use std::path::Path; + +pub fn filter_dirs(path: &Path) -> bool { + let skip = [ + "tidy-test-file", + "compiler/rustc_codegen_cranelift", + "compiler/rustc_codegen_gcc", + "src/llvm-project", + "library/backtrace", + "library/portable-simd", + "library/stdarch", + "src/tools/cargo", + "src/tools/clippy", + "src/tools/miri", + "src/tools/rls", + "src/tools/rust-analyzer", + "src/tools/rust-installer", + "src/tools/rustfmt", + "src/doc/book", + "src/doc/edition-guide", + "src/doc/embedded-book", + "src/doc/nomicon", + "src/doc/rust-by-example", + "src/doc/rustc-dev-guide", + "src/doc/reference", + // Filter RLS output directories + "target/rls", + "src/bootstrap/target", + ]; + skip.iter().any(|p| path.ends_with(p)) +} + +pub fn walk_many( + paths: &[&Path], + skip: &mut dyn FnMut(&Path) -> bool, + f: &mut dyn FnMut(&DirEntry, &str), +) { + for path in paths { + walk(path, skip, f); + } +} + +pub fn walk(path: &Path, skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&DirEntry, &str)) { + let mut contents = String::new(); + walk_no_read(path, skip, &mut |entry| { + contents.clear(); + if t!(File::open(entry.path()), entry.path()).read_to_string(&mut contents).is_err() { + contents.clear(); + } + f(&entry, &contents); + }); +} + +pub(crate) fn walk_no_read( + path: &Path, + skip: &mut dyn FnMut(&Path) -> bool, + f: &mut dyn FnMut(&DirEntry), +) { + let walker = WalkDir::new(path).into_iter().filter_entry(|e| !skip(e.path())); + for entry in walker { + if let Ok(entry) = entry { + if entry.file_type().is_dir() { + continue; + } + f(&entry); + } + } +} |