From 3445c2686eed53d204112482e3efe0ff5dd6101e Mon Sep 17 00:00:00 2001 From: est31 Date: Fri, 12 Aug 2022 17:49:58 +0200 Subject: [PATCH 1/3] Add a Paths struct to tidy and use it to pass paths to features::check --- src/tools/tidy/src/features.rs | 20 ++++++----------- src/tools/tidy/src/lib.rs | 20 ++++++++++++++++- src/tools/tidy/src/main.rs | 40 ++++++++++++++++------------------ 3 files changed, 45 insertions(+), 35 deletions(-) diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index 2f22c081a54b5..17df2e2882bcd 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -79,25 +79,19 @@ pub fn collect_lib_features(base_src_path: &Path) -> Features { lib_features } -pub fn check( - src_path: &Path, - compiler_path: &Path, - lib_path: &Path, - bad: &mut bool, - verbose: bool, -) -> CollectedFeatures { - let mut features = collect_lang_features(compiler_path, bad); +pub fn check(paths: &crate::Paths, bad: &mut bool, verbose: bool) -> CollectedFeatures { + let mut features = collect_lang_features(&paths.compiler, bad); assert!(!features.is_empty()); - let lib_features = get_and_check_lib_features(lib_path, bad, &features); + let lib_features = get_and_check_lib_features(&paths.library, bad, &features); assert!(!lib_features.is_empty()); super::walk_many( &[ - &src_path.join("test/ui"), - &src_path.join("test/ui-fulldeps"), - &src_path.join("test/rustdoc-ui"), - &src_path.join("test/rustdoc"), + &paths.src.join("test/ui"), + &paths.src.join("test/ui-fulldeps"), + &paths.src.join("test/rustdoc-ui"), + &paths.src.join("test/rustdoc"), ], &mut |path| super::filter_dirs(path), &mut |entry, contents| { diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 09848462ae207..ce6ba0f81e9ed 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -7,7 +7,7 @@ use std::fs::File; use std::io::Read; use walkdir::{DirEntry, WalkDir}; -use std::path::Path; +use std::path::{Path, PathBuf}; macro_rules! t { ($e:expr, $p:expr) => { @@ -76,6 +76,24 @@ fn filter_dirs(path: &Path) -> bool { skip.iter().any(|p| path.ends_with(p)) } +pub struct Paths { + pub root: PathBuf, + pub src: PathBuf, + pub compiler: PathBuf, + pub library: PathBuf, +} + +impl Paths { + pub fn from_root(root: &Path) -> Self { + Self { + root: root.to_owned(), + src: root.join("src"), + library: root.join("library"), + compiler: root.join("compiler"), + } + } +} + fn walk_many( paths: &[&Path], skip: &mut dyn FnMut(&Path) -> bool, diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index aa8d8b4f64d7d..ee5468a6d5e66 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -24,9 +24,7 @@ fn main() { FromStr::from_str(&env::args().nth(4).expect("need concurrency")) .expect("concurrency must be a number"); - let src_path = root_path.join("src"); - let library_path = root_path.join("library"); - let compiler_path = root_path.join("compiler"); + let paths = Paths::from_root(&root_path); let args: Vec = env::args().skip(1).collect(); @@ -55,53 +53,53 @@ fn main() { } } - check!(target_specific_tests, &src_path); + check!(target_specific_tests, &paths.src); // Checks that are done on the cargo workspace. check!(deps, &root_path, &cargo); check!(extdeps, &root_path); // Checks over tests. - check!(debug_artifacts, &src_path); - check!(ui_tests, &src_path); + check!(debug_artifacts, &paths.src); + check!(ui_tests, &paths.src); // Checks that only make sense for the compiler. - check!(errors, &compiler_path); - check!(error_codes_check, &[&src_path, &compiler_path]); + check!(errors, &paths.compiler); + check!(error_codes_check, &[&paths.src, &paths.compiler]); // Checks that only make sense for the std libs. - check!(pal, &library_path); - check!(primitive_docs, &library_path); + check!(pal, &paths.library); + check!(primitive_docs, &paths.library); // Checks that need to be done for both the compiler and std libraries. - check!(unit_tests, &src_path); - check!(unit_tests, &compiler_path); - check!(unit_tests, &library_path); + check!(unit_tests, &paths.src); + check!(unit_tests, &paths.compiler); + check!(unit_tests, &paths.library); if bins::check_filesystem_support(&[&root_path], &output_directory) { check!(bins, &root_path); } - check!(style, &src_path); - check!(style, &compiler_path); - check!(style, &library_path); + check!(style, &paths.src); + check!(style, &paths.compiler); + check!(style, &paths.library); - check!(edition, &src_path); - check!(edition, &compiler_path); - check!(edition, &library_path); + check!(edition, &paths.src); + check!(edition, &paths.compiler); + check!(edition, &paths.library); let collected = { while handles.len() >= concurrency.get() { handles.pop_front().unwrap().join().unwrap(); } let mut flag = false; - let r = features::check(&src_path, &compiler_path, &library_path, &mut flag, verbose); + let r = features::check(&paths, &mut flag, verbose); if flag { bad.store(true, Ordering::Relaxed); } r }; - check!(unstable_book, &src_path, collected); + check!(unstable_book, &paths.src, collected); }) .unwrap(); From 7dd92730dba0d3380cdb23ec45e2efb659fc7a35 Mon Sep 17 00:00:00 2001 From: est31 Date: Fri, 12 Aug 2022 22:22:33 +0200 Subject: [PATCH 2/3] Check that feature stabilizations contain the current version It's a common phenomenon that feature stabilizations don't make it into a particular release, but the version is still inaccurate. Often this leads to subsequent changes to adjust/correct the version, but it's easy to miss errors. This commit adds checking to ensure that a stabilization is always for the current release. Adjusting versions of already stable features is still supported, as is unstabilizing or other modifications. --- src/tools/tidy/src/features.rs | 197 ++++++++++++++++++++++++++++++--- 1 file changed, 184 insertions(+), 13 deletions(-) diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index 17df2e2882bcd..b61815f288bf9 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -8,12 +8,14 @@ //! * Library features have at most one `since` value. //! * All unstable lang features have tests to ensure they are actually unstable. //! * Language features in a group are sorted by feature name. +#![allow(unused)] use std::collections::HashMap; use std::fmt; use std::fs; use std::num::NonZeroU32; use std::path::Path; +use std::process::{Command, Stdio}; use regex::Regex; @@ -26,7 +28,7 @@ use version::Version; const FEATURE_GROUP_START_PREFIX: &str = "// feature-group-start"; const FEATURE_GROUP_END_PREFIX: &str = "// feature-group-end"; -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] pub enum Status { Stable, Removed, @@ -44,7 +46,7 @@ impl fmt::Display for Status { } } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Feature { pub level: Status, pub since: Option, @@ -169,6 +171,8 @@ pub fn check(paths: &crate::Paths, bad: &mut bool, verbose: bool) -> CollectedFe tidy_error!(bad, "Found {} features without a gate test.", gate_untested.len()); } + check_feature_changes(&paths.root, &lib_features, &features, bad, verbose); + if *bad { return CollectedFeatures { lib: lib_features, lang: features }; } @@ -189,6 +193,137 @@ pub fn check(paths: &crate::Paths, bad: &mut bool, verbose: bool) -> CollectedFe CollectedFeatures { lib: lib_features, lang: features } } +/// Check whether feature changes (stabilizations etc) were done correctly +/// +/// It is common for a stabilization PR to not make it into the release it was +/// originally targetted against, and the stabilization version is not always +/// adjusted accordingly. +fn check_feature_changes( + root: &Path, + lib_features: &Features, + lang_features: &Features, + bad: &mut bool, + verbose: bool, +) { + let base_tree_dir = root.join("build/tmp/base-checkout"); + + let base_commit = if let Some(base_commit) = find_base_commit(root) { + base_commit + } else { + // Not a git repo, don't do the check. + return; + }; + if verbose { + println!("Base commit hash: {base_commit}"); + } + + let base_tree_paths = crate::Paths::from_root(&base_tree_dir); + let current_version = get_version(root); + + // FIXME use git archive --format=tar src/ plus the known grepping to get the lib features + //let base_lib_features = + // get_and_check_lib_features(&base_tree_paths.library, bad, &lib_features); + let base_lang_features = collect_lang_features_at_commit(root, &base_commit, bad); + + //check_changes(&base_lib_features, &lib_features, current_version, bad, verbose); + check_changes(&base_lang_features, &lang_features, current_version, bad, verbose); +} + +/// Returns `Some(c)` if a base commit could be found, and `None` if +/// git is not available, it is not a git repo (say a tarball), or other reasons. +/// +/// "Base commit" in this instance is defined as the bors commit that the change +/// "bases" on, and ideally the most recent one. This needs to support the +/// following scenarios: +/// +/// * In the "bors tests a PR merge commit" scenario, we want bors to ignore the +/// top level merge commit, because that commit is the one being tested, not +/// the one the PR bases on. +/// * In the "PR author runs tidy locally" scenario, they might or might not +/// have committed their changes. So the most recent commit is from bors, and +/// is the base commit. +/// These two requirements in combination mean we can't use a simple logic like +/// "take last bors commit" or "take second last bors commit", because neither +/// are always correct. Instead, we first check if the working directory is +/// clean, and if it isn't, we know that we are not in the first scenario, so +/// we can safely take the last bors commit. +fn find_base_commit(root: &Path) -> Option { + let git_status_output = Command::new("git") + .arg("status") + .arg("--porcelain") + .current_dir(root) + .stderr(Stdio::null()) + .output() + .ok()?; + let git_status_output = String::from_utf8(git_status_output.stdout).ok()?; + let working_dir_is_clean = git_status_output.trim().is_empty(); + + let prior_arg: &[_] = if working_dir_is_clean { &["HEAD~1"] } else { &[] }; + + let base_commit_output = Command::new("git") + .arg("log") + .args(prior_arg) + .arg("--author=\"bors \"") + .arg("--first-parent") + .arg("-1") + .arg("--pretty=format:\"%H\"") + .current_dir(root) + .stderr(Stdio::inherit()) + .output() + .unwrap_or_else(|e| { + panic!("could not run git log: {e}"); + }); + let base_commit_hash = t!(std::str::from_utf8(&base_commit_output.stdout)); + Some(base_commit_hash.to_owned()) +} + +fn check_changes( + base_features: &Features, + features: &Features, + current_version: Version, + bad: &mut bool, + verbose: bool, +) { + let mut features_vec: Vec<_> = features.iter().collect(); + features_vec.sort_by_key(|(name, _feature)| name.to_owned()); + for (name, feature) in features_vec { + let base_feature = base_features.get(name); + if let Some(base_feature) = base_feature && base_feature == feature { + // Most cases: the feature has not been modified + continue; + } + // Whether the feature has been added or stabilized + let was_stabilization = + matches!(base_feature, None | Some(Feature { level: Status::Unstable, .. })) + && feature.level == Status::Stable; + + if was_stabilization { + if let Some(since) = feature.since && current_version != since { + let introducing_or_stabilizing = if base_feature.is_none() { + "introducing and stabilizing" + } else { + "stabilizing" + }; + tidy_error!( + bad, + "Your change is {} feature `{}` for version {} but current version is {}.", + introducing_or_stabilizing, + name, + since, + current_version + ); + } + println!("** 🥳🎉 stabilized feature `{name}` 🎉🥳 **"); + } + } +} + +fn get_version(root: &Path) -> Version { + let version_str = t!(std::fs::read_to_string(root.join("src/version"))); + let version_str = version_str.trim(); + t!(std::str::FromStr::from_str(&version_str).map_err(|e| format!("{e:?}"))) +} + fn format_features<'a>( features: &'a Features, family: &'a str, @@ -236,15 +371,51 @@ 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)); + collect_lang_features_with( + |segments| { + let path = segments.iter().fold(base_compiler_path.to_owned(), |p, q| p.join(q)); + (format!("{}", path.display()), t!(fs::read_to_string(&path))) + }, + bad, + ) +} + +fn collect_lang_features_at_commit(root: &Path, commit_hash: &str, bad: &mut bool) -> Features { + collect_lang_features_with( + |segments| { + let path_arg = format!("{commit_hash}:compiler/{}", segments.join("/")); + let git_show_output = Command::new("git") + .arg("show") + .arg(&path_arg) + .current_dir(root) + .stderr(Stdio::null()) + .output() + .unwrap_or_else(|e| { + panic!("could not run git show: {e}"); + }); + let file_contents = t!(String::from_utf8(git_show_output.stdout)); + (path_arg, file_contents) + }, + bad, + ) +} + +fn collect_lang_features_with( + file_read_fn: impl Fn(&[&str]) -> (String, String), + bad: &mut bool, +) -> Features { + let mut all = collect_lang_features_in(&file_read_fn, "active.rs", bad); + all.extend(collect_lang_features_in(&file_read_fn, "accepted.rs", bad)); + all.extend(collect_lang_features_in(&file_read_fn, "removed.rs", bad)); all } -fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features { - let path = base.join("rustc_feature").join("src").join(file); - let contents = t!(fs::read_to_string(&path)); +fn collect_lang_features_in( + file_read_fn: impl Fn(&[&str]) -> (String, String), + file: &str, + bad: &mut bool, +) -> Features { + let (path, contents) = file_read_fn(&["rustc_feature", "src", file]); // We allow rustc-internal features to omit a tracking issue. // To make tidy accept omitting a tracking issue, group the list of features @@ -279,7 +450,7 @@ fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features bad, "{}:{}: \ new feature group is started without ending the previous one", - path.display(), + path, line_number, ); } @@ -310,7 +481,7 @@ fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features tidy_error!( bad, "{}:{}: failed to parse since: {} ({:?})", - path.display(), + path, line_number, since_str, err, @@ -328,7 +499,7 @@ fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features tidy_error!( bad, "{}:{}: duplicate feature {}", - path.display(), + path, line_number, name, ); @@ -355,7 +526,7 @@ fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features tidy_error!( bad, "{}:{}: feature {} is not sorted by feature name (should be {})", - path.display(), + path, line_number, name, correct_placement, @@ -370,7 +541,7 @@ fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features tidy_error!( bad, "{}:{}: no tracking issue for feature {}", - path.display(), + path, line_number, name, ); From b78006920aca240b3ad60e83ea24699c75f753d8 Mon Sep 17 00:00:00 2001 From: est31 Date: Mon, 15 Aug 2022 13:08:01 +0200 Subject: [PATCH 3/3] Mock stabilization to demonstrate the feature --- compiler/rustc_feature/src/accepted.rs | 2 ++ compiler/rustc_feature/src/active.rs | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_feature/src/accepted.rs b/compiler/rustc_feature/src/accepted.rs index 2d24101b2d59a..5812969fe264e 100644 --- a/compiler/rustc_feature/src/accepted.rs +++ b/compiler/rustc_feature/src/accepted.rs @@ -157,6 +157,8 @@ declare_features! ( (accepted, extern_prelude, "1.30.0", Some(44660), None), /// Allows field shorthands (`x` meaning `x: x`) in struct literal expressions. (accepted, field_init_shorthand, "1.17.0", Some(37340), None), + /// Allows using `#[repr(align(...))]` on function items + (accepted, fn_align, "1.53.0", Some(82232), None), /// Allows `#[must_use]` on functions, and introduces must-use operators (RFC 1940). (accepted, fn_must_use, "1.27.0", Some(43302), None), /// Allows capturing variables in scope using format_args! diff --git a/compiler/rustc_feature/src/active.rs b/compiler/rustc_feature/src/active.rs index 1018facebaed9..c849a29d8784c 100644 --- a/compiler/rustc_feature/src/active.rs +++ b/compiler/rustc_feature/src/active.rs @@ -392,8 +392,6 @@ declare_features! ( (active, ffi_pure, "1.45.0", Some(58329), None), /// Allows using `#[ffi_returns_twice]` on foreign functions. (active, ffi_returns_twice, "1.34.0", Some(58314), None), - /// Allows using `#[repr(align(...))]` on function items - (active, fn_align, "1.53.0", Some(82232), None), /// Allows defining generators. (active, generators, "1.21.0", Some(43122), None), /// Infer generic args for both consts and types.