From 2b6371dbfbf390a57b68e670a416f8a0b3094b07 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 11:04:30 -0400 Subject: [PATCH 01/11] Make tidy quieter by default --- src/bootstrap/test.rs | 4 ++-- src/tools/tidy/src/features.rs | 22 +++++++++++----------- src/tools/tidy/src/main.rs | 4 ++-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 74caaae2840c5..2f9bd067c3115 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -709,8 +709,8 @@ impl Step for Tidy { if !builder.config.vendor { cmd.arg("--no-vendor"); } - if !builder.config.verbose_tests { - cmd.arg("--quiet"); + if builder.is_verbose() { + cmd.arg("--verbose"); } let _folder = builder.fold_output(|| "tidy"); diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index 637f10c5ae745..6aa093c1ef0d1 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -51,7 +51,7 @@ pub struct Feature { pub type Features = HashMap; -pub fn check(path: &Path, bad: &mut bool, quiet: bool) { +pub fn check(path: &Path, bad: &mut bool, verbose: bool) { let mut features = collect_lang_features(path, bad); assert!(!features.is_empty()); @@ -132,18 +132,18 @@ pub fn check(path: &Path, bad: &mut bool, quiet: bool) { if *bad { return; } - if quiet { - println!("* {} features", features.len()); - return; - } - let mut lines = Vec::new(); - lines.extend(format_features(&features, "lang")); - lines.extend(format_features(&lib_features, "lib")); + if verbose { + let mut lines = Vec::new(); + lines.extend(format_features(&features, "lang")); + lines.extend(format_features(&lib_features, "lib")); - lines.sort(); - for line in lines { - println!("* {}", line); + lines.sort(); + for line in lines { + println!("* {}", line); + } + } else { + println!("* {} features", features.len()); } } diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index eef3719043825..fa88b078cd8d4 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -19,12 +19,12 @@ fn main() { let args: Vec = env::args().skip(1).collect(); let mut bad = false; - let quiet = args.iter().any(|s| *s == "--quiet"); + let verbose = args.iter().any(|s| *s == "--verbose"); bins::check(&path, &mut bad); style::check(&path, &mut bad); errors::check(&path, &mut bad); cargo::check(&path, &mut bad); - features::check(&path, &mut bad, quiet); + features::check(&path, &mut bad, verbose); pal::check(&path, &mut bad); unstable_book::check(&path, &mut bad); libcoretest::check(&path, &mut bad); From 5f1da8e5337900b9d94b55ec65618f0223608f6c Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 12:04:27 -0400 Subject: [PATCH 02/11] Cache Regex's --- Cargo.lock | 1 + src/tools/tidy/Cargo.toml | 1 + src/tools/tidy/src/features.rs | 17 ++++++++++++++--- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 40b8cf507e97c..a9f9cd2b97783 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3801,6 +3801,7 @@ dependencies = [ name = "tidy" version = "0.1.0" dependencies = [ + "lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "regex 1.1.6 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.92 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.33 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml index eeac6cfbb30e0..fb58adcf9b282 100644 --- a/src/tools/tidy/Cargo.toml +++ b/src/tools/tidy/Cargo.toml @@ -8,3 +8,4 @@ edition = "2018" regex = "1" serde = { version = "1.0.8", features = ["derive"] } serde_json = "1.0.2" +lazy_static = "1" diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index 6aa093c1ef0d1..ced30eb6d58ad 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -15,7 +15,7 @@ use std::fs::{self, File}; use std::io::prelude::*; use std::path::Path; -use regex::{Regex, escape}; +use regex::Regex; mod version; use version::Version; @@ -159,8 +159,19 @@ fn format_features<'a>(features: &'a Features, family: &'a str) -> impl Iterator } fn find_attr_val<'a>(line: &'a str, attr: &str) -> Option<&'a str> { - let r = Regex::new(&format!(r#"{}\s*=\s*"([^"]*)""#, escape(attr))) - .expect("malformed regex for find_attr_val"); + lazy_static::lazy_static! { + static ref ISSUE: Regex = Regex::new(r#"issue\s*=\s*"([^"]*)""#).unwrap(); + static ref FEATURE: Regex = Regex::new(r#"feature\s*=\s*"([^"]*)""#).unwrap(); + static ref SINCE: Regex = Regex::new(r#"since\s*=\s*"([^"]*)""#).unwrap(); + } + + let r = match attr { + "issue" => &*ISSUE, + "feature" => &*FEATURE, + "since" => &*SINCE, + _ => unimplemented!("{} not handled", attr), + }; + r.captures(line) .and_then(|c| c.get(1)) .map(|m| m.as_str()) From e17b02d9b98dae037df2761a84d9dae066ba7e31 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 12:05:00 -0400 Subject: [PATCH 03/11] Use walkdir crate It's more efficient than the fs::read_dir API --- Cargo.lock | 1 + src/tools/tidy/Cargo.toml | 1 + src/tools/tidy/src/lib.rs | 20 ++++++++------------ 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a9f9cd2b97783..657831be894f8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3805,6 +3805,7 @@ dependencies = [ "regex 1.1.6 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.92 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.33 (registry+https://github.com/rust-lang/crates.io-index)", + "walkdir 2.2.7 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml index fb58adcf9b282..43cae31f33f1f 100644 --- a/src/tools/tidy/Cargo.toml +++ b/src/tools/tidy/Cargo.toml @@ -9,3 +9,4 @@ regex = "1" serde = { version = "1.0.8", features = ["derive"] } serde_json = "1.0.2" lazy_static = "1" +walkdir = "2" diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index d06c99725bc6a..b9d89ad6b756e 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -3,7 +3,7 @@ //! This library contains the tidy lints and exposes it //! to be used by tools. -use std::fs; +use walkdir::WalkDir; use std::path::Path; @@ -72,18 +72,14 @@ fn walk_many(paths: &[&Path], skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn F } fn walk(path: &Path, skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&Path)) { - if let Ok(dir) = fs::read_dir(path) { - for entry in dir { - let entry = t!(entry); - let kind = t!(entry.file_type()); - let path = entry.path(); - if kind.is_dir() { - if !skip(&path) { - walk(&path, skip, f); - } - } else { - f(&path); + 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.path()); } } } From 5c33c3e308a4fb6d93352115ec00e99d1920eff7 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 12:08:26 -0400 Subject: [PATCH 04/11] Stop calling format! to check feature gate --- src/tools/tidy/src/features.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index ced30eb6d58ad..da13879429769 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -186,9 +186,11 @@ fn test_find_attr_val() { } fn test_filen_gate(filen_underscore: &str, features: &mut Features) -> bool { - if filen_underscore.starts_with("feature_gate") { + let prefix = "feature_gate_"; + if filen_underscore.starts_with(prefix) { for (n, f) in features.iter_mut() { - if filen_underscore == format!("feature_gate_{}", n) { + // Equivalent to filen_underscore == format!("feature_gate_{}", n) + if &filen_underscore[prefix.len()..] == n { f.has_gate_test = true; return true; } From c113a3769de9d2a0fd09c6aa8ccd9aa3d516e915 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 12:23:20 -0400 Subject: [PATCH 05/11] Pass contents and DirEntry to walkers --- src/tools/tidy/src/bins.rs | 3 ++- src/tools/tidy/src/errors.rs | 3 ++- src/tools/tidy/src/features.rs | 6 ++++-- src/tools/tidy/src/lib.rs | 17 +++++++++++++---- src/tools/tidy/src/libcoretest.rs | 3 ++- src/tools/tidy/src/pal.rs | 3 ++- src/tools/tidy/src/style.rs | 3 ++- src/tools/tidy/src/ui_tests.rs | 3 ++- 8 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index 610d1d8af3b5e..a837d19f3c133 100644 --- a/src/tools/tidy/src/bins.rs +++ b/src/tools/tidy/src/bins.rs @@ -27,7 +27,8 @@ pub fn check(path: &Path, bad: &mut bool) { super::walk(path, &mut |path| super::filter_dirs(path) || path.ends_with("src/etc"), - &mut |file| { + &mut |entry, _contents| { + 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)) { diff --git a/src/tools/tidy/src/errors.rs b/src/tools/tidy/src/errors.rs index ef1000ee5065a..0d459493938c8 100644 --- a/src/tools/tidy/src/errors.rs +++ b/src/tools/tidy/src/errors.rs @@ -13,7 +13,8 @@ pub fn check(path: &Path, bad: &mut bool) { let mut map: HashMap<_, Vec<_>> = HashMap::new(); super::walk(path, &mut |path| super::filter_dirs(path) || path.ends_with("src/test"), - &mut |file| { + &mut |entry, _contents| { + let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); if filename != "error_codes.rs" { return diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index da13879429769..c19e6c6c4e78f 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -64,7 +64,8 @@ pub fn check(path: &Path, bad: &mut bool, verbose: bool) { &path.join("test/ui-fulldeps"), &path.join("test/compile-fail")], &mut |path| super::filter_dirs(path), - &mut |file| { + &mut |entry, _contents| { + let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); if !filename.ends_with(".rs") || filename == "features.rs" || filename == "diagnostic_list.rs" { @@ -371,7 +372,8 @@ fn map_lib_features(base_src_path: &Path, let mut contents = String::new(); super::walk(base_src_path, &mut |path| super::filter_dirs(path) || path.ends_with("src/test"), - &mut |file| { + &mut |entry, _contents| { + let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); if !filename.ends_with(".rs") || filename == "features.rs" || filename == "diagnostic_list.rs" { diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index b9d89ad6b756e..0f55f297e0e4e 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -3,7 +3,9 @@ //! This library contains the tidy lints and exposes it //! to be used by tools. -use walkdir::WalkDir; +use walkdir::{DirEntry, WalkDir}; +use std::fs::File; +use std::io::Read; use std::path::Path; @@ -65,21 +67,28 @@ fn filter_dirs(path: &Path) -> bool { skip.iter().any(|p| path.ends_with(p)) } -fn walk_many(paths: &[&Path], skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&Path)) { +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(&Path)) { +fn walk(path: &Path, skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&DirEntry, &str)) { let walker = WalkDir::new(path).into_iter() .filter_entry(|e| !skip(e.path())); + let mut contents = String::new(); for entry in walker { if let Ok(entry) = entry { if entry.file_type().is_dir() { continue; } - f(&entry.path()); + contents.clear(); + if t!(File::open(entry.path()), entry.path()).read_to_string(&mut contents).is_err() { + contents.clear(); + } + f(&entry, &contents); } } } diff --git a/src/tools/tidy/src/libcoretest.rs b/src/tools/tidy/src/libcoretest.rs index b15b9c3462f79..b9138517f1e7e 100644 --- a/src/tools/tidy/src/libcoretest.rs +++ b/src/tools/tidy/src/libcoretest.rs @@ -11,7 +11,8 @@ pub fn check(path: &Path, bad: &mut bool) { super::walk( &libcore_path, &mut |subpath| t!(subpath.strip_prefix(&libcore_path)).starts_with("tests"), - &mut |subpath| { + &mut |entry, _contents| { + let subpath = entry.path(); if let Some("rs") = subpath.extension().and_then(|e| e.to_str()) { match read_to_string(subpath) { Ok(contents) => { diff --git a/src/tools/tidy/src/pal.rs b/src/tools/tidy/src/pal.rs index d4a6cf73bf98c..412003c75c3ba 100644 --- a/src/tools/tidy/src/pal.rs +++ b/src/tools/tidy/src/pal.rs @@ -91,7 +91,8 @@ 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 |file| { + super::walk(path, &mut super::filter_dirs, &mut |entry, _contents| { + let file = entry.path(); let filestr = file.to_string_lossy().replace("\\", "/"); if !filestr.ends_with(".rs") { return } diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index e860f2e9df0ad..2db6353358c6b 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -130,7 +130,8 @@ macro_rules! suppressible_tidy_err { pub fn check(path: &Path, bad: &mut bool) { let mut contents = String::new(); - super::walk(path, &mut super::filter_dirs, &mut |file| { + super::walk(path, &mut super::filter_dirs, &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"]; if extensions.iter().all(|e| !filename.ends_with(e)) || diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index b572b52ea8f35..2d7c1df9c235d 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -7,7 +7,8 @@ pub fn check(path: &Path, bad: &mut bool) { super::walk_many( &[&path.join("test/ui"), &path.join("test/ui-fulldeps")], &mut |_| false, - &mut |file_path| { + &mut |entry, _contents| { + let file_path = entry.path(); if let Some(ext) = file_path.extension() { if ext == "stderr" || ext == "stdout" { // Test output filenames have one of the formats: From 38f0b90e45c049145c3e59b3d5555ce8dda678e4 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 12:47:15 -0400 Subject: [PATCH 06/11] Move file-reading into walker loop --- src/tools/tidy/src/bins.rs | 4 ++-- src/tools/tidy/src/errors.rs | 8 +------- src/tools/tidy/src/features.rs | 16 +++------------- src/tools/tidy/src/lib.rs | 19 +++++++++++++------ src/tools/tidy/src/libcoretest.rs | 24 ++++++++---------------- src/tools/tidy/src/pal.rs | 12 +++--------- src/tools/tidy/src/style.rs | 10 ++-------- src/tools/tidy/src/ui_tests.rs | 10 ++++------ 8 files changed, 36 insertions(+), 67 deletions(-) diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index a837d19f3c133..d2f4f07c48537 100644 --- a/src/tools/tidy/src/bins.rs +++ b/src/tools/tidy/src/bins.rs @@ -25,9 +25,9 @@ pub fn check(path: &Path, bad: &mut bool) { } } - super::walk(path, + super::walk_no_read(path, &mut |path| super::filter_dirs(path) || path.ends_with("src/etc"), - &mut |entry, _contents| { + &mut |entry| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); let extensions = [".py", ".sh"]; diff --git a/src/tools/tidy/src/errors.rs b/src/tools/tidy/src/errors.rs index 0d459493938c8..1bc27745376cc 100644 --- a/src/tools/tidy/src/errors.rs +++ b/src/tools/tidy/src/errors.rs @@ -4,25 +4,19 @@ //! statistics about the error codes. use std::collections::HashMap; -use std::fs::File; -use std::io::prelude::*; use std::path::Path; pub fn check(path: &Path, bad: &mut bool) { - let mut contents = String::new(); let mut map: HashMap<_, Vec<_>> = HashMap::new(); super::walk(path, &mut |path| super::filter_dirs(path) || path.ends_with("src/test"), - &mut |entry, _contents| { + &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); if filename != "error_codes.rs" { return } - contents.truncate(0); - t!(t!(File::open(file)).read_to_string(&mut contents)); - // In the `register_long_diagnostics!` macro, entries look like this: // // ``` diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index c19e6c6c4e78f..ba3132845be39 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -11,8 +11,7 @@ use std::collections::HashMap; use std::fmt; -use std::fs::{self, File}; -use std::io::prelude::*; +use std::fs; use std::path::Path; use regex::Regex; @@ -58,13 +57,11 @@ pub fn check(path: &Path, bad: &mut bool, verbose: bool) { let lib_features = get_and_check_lib_features(path, bad, &features); assert!(!lib_features.is_empty()); - let mut contents = String::new(); - super::walk_many(&[&path.join("test/ui"), &path.join("test/ui-fulldeps"), &path.join("test/compile-fail")], &mut |path| super::filter_dirs(path), - &mut |entry, _contents| { + &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); if !filename.ends_with(".rs") || filename == "features.rs" || @@ -75,9 +72,6 @@ pub fn check(path: &Path, bad: &mut bool, verbose: bool) { let filen_underscore = filename.replace('-',"_").replace(".rs",""); let filename_is_gate_test = test_filen_gate(&filen_underscore, &mut features); - contents.truncate(0); - t!(t!(File::open(&file), &file).read_to_string(&mut contents)); - for (i, line) in contents.lines().enumerate() { let mut err = |msg: &str| { tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg); @@ -369,10 +363,9 @@ fn get_and_check_lib_features(base_src_path: &Path, fn map_lib_features(base_src_path: &Path, mf: &mut dyn FnMut(Result<(&str, Feature), &str>, &Path, usize)) { - let mut contents = String::new(); super::walk(base_src_path, &mut |path| super::filter_dirs(path) || path.ends_with("src/test"), - &mut |entry, _contents| { + &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); if !filename.ends_with(".rs") || filename == "features.rs" || @@ -380,9 +373,6 @@ fn map_lib_features(base_src_path: &Path, return; } - contents.truncate(0); - t!(t!(File::open(&file), &file).read_to_string(&mut contents)); - let mut becoming_feature: Option<(String, Feature)> = None; for (i, line) in contents.lines().enumerate() { macro_rules! err { diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 0f55f297e0e4e..a0bf0b0735418 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -67,6 +67,7 @@ fn filter_dirs(path: &Path) -> bool { 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) ) { @@ -76,19 +77,25 @@ fn walk_many( } 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())); - let mut contents = String::new(); for entry in walker { if let Ok(entry) = entry { if entry.file_type().is_dir() { continue; } - contents.clear(); - if t!(File::open(entry.path()), entry.path()).read_to_string(&mut contents).is_err() { - contents.clear(); - } - f(&entry, &contents); + f(&entry); } } } diff --git a/src/tools/tidy/src/libcoretest.rs b/src/tools/tidy/src/libcoretest.rs index b9138517f1e7e..ea92f989ada7d 100644 --- a/src/tools/tidy/src/libcoretest.rs +++ b/src/tools/tidy/src/libcoretest.rs @@ -4,30 +4,22 @@ //! item. All tests must be written externally in `libcore/tests`. use std::path::Path; -use std::fs::read_to_string; pub fn check(path: &Path, bad: &mut bool) { let libcore_path = path.join("libcore"); super::walk( &libcore_path, &mut |subpath| t!(subpath.strip_prefix(&libcore_path)).starts_with("tests"), - &mut |entry, _contents| { + &mut |entry, contents| { let subpath = entry.path(); if let Some("rs") = subpath.extension().and_then(|e| e.to_str()) { - match read_to_string(subpath) { - Ok(contents) => { - if contents.contains("#[test]") { - tidy_error!( - bad, - "{} contains #[test]; libcore tests must be placed inside \ - `src/libcore/tests/`", - subpath.display() - ); - } - } - Err(err) => { - panic!("failed to read file {:?}: {}", subpath, err); - } + if contents.contains("#[test]") { + tidy_error!( + bad, + "{} contains #[test]; libcore tests must be placed inside \ + `src/libcore/tests/`", + subpath.display() + ); } } }, diff --git a/src/tools/tidy/src/pal.rs b/src/tools/tidy/src/pal.rs index 412003c75c3ba..c6bb16318b6ee 100644 --- a/src/tools/tidy/src/pal.rs +++ b/src/tools/tidy/src/pal.rs @@ -31,8 +31,6 @@ //! platform-specific cfgs are allowed. Not sure yet how to deal with //! this in the long term. -use std::fs::File; -use std::io::Read; use std::path::Path; use std::iter::Iterator; @@ -87,11 +85,10 @@ const EXCEPTION_PATHS: &[&str] = &[ ]; pub fn check(path: &Path, bad: &mut bool) { - let mut contents = String::new(); // 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| { + super::walk(path, &mut super::filter_dirs, &mut |entry, contents| { let file = entry.path(); let filestr = file.to_string_lossy().replace("\\", "/"); if !filestr.ends_with(".rs") { return } @@ -99,18 +96,15 @@ pub fn check(path: &Path, bad: &mut bool) { let is_exception_path = EXCEPTION_PATHS.iter().any(|s| filestr.contains(&**s)); if is_exception_path { return } - check_cfgs(&mut 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); assert!(saw_cfg_bang); } -fn check_cfgs(contents: &mut String, file: &Path, +fn check_cfgs(contents: &str, file: &Path, bad: &mut bool, saw_target_arch: &mut bool, saw_cfg_bang: &mut bool) { - contents.truncate(0); - t!(t!(File::open(file), file).read_to_string(contents)); - // For now it's ok to have platform-specific code after 'mod tests'. let mod_tests_idx = find_test_mod(contents); let contents = &contents[..mod_tests_idx]; diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index 2db6353358c6b..d994f60b2bb47 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -13,8 +13,6 @@ //! A number of these checks can be opted-out of with various directives of the form: //! `// ignore-tidy-CHECK-NAME`. -use std::fs::File; -use std::io::prelude::*; use std::path::Path; const COLS: usize = 100; @@ -109,7 +107,7 @@ enum Directive { Ignore(bool), } -fn contains_ignore_directive(contents: &String, check: &str) -> Directive { +fn contains_ignore_directive(contents: &str, check: &str) -> Directive { if contents.contains(&format!("// ignore-tidy-{}", check)) || contents.contains(&format!("# ignore-tidy-{}", check)) { Directive::Ignore(false) @@ -129,8 +127,7 @@ macro_rules! suppressible_tidy_err { } pub fn check(path: &Path, bad: &mut bool) { - let mut contents = String::new(); - super::walk(path, &mut super::filter_dirs, &mut |entry, _contents| { + super::walk(path, &mut super::filter_dirs, &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"]; @@ -139,9 +136,6 @@ pub fn check(path: &Path, bad: &mut bool) { return } - contents.truncate(0); - t!(t!(File::open(file), file).read_to_string(&mut contents)); - if contents.is_empty() { tidy_error!(bad, "{}: empty file", file.display()); } diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 2d7c1df9c235d..2c52cecccb5df 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -4,10 +4,8 @@ use std::fs; use std::path::Path; pub fn check(path: &Path, bad: &mut bool) { - super::walk_many( - &[&path.join("test/ui"), &path.join("test/ui-fulldeps")], - &mut |_| false, - &mut |entry, _contents| { + for path in &[&path.join("test/ui"), &path.join("test/ui-fulldeps")] { + super::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" { @@ -46,6 +44,6 @@ pub fn check(path: &Path, bad: &mut bool) { } } } - }, - ); + }); + } } From d619e44a55bb3f8d1deb5ad125e16becb42da695 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 12:53:32 -0400 Subject: [PATCH 07/11] Utilize entry.metadata over fs::symlink_metadata --- src/tools/tidy/src/bins.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index d2f4f07c48537..680585a6e04a7 100644 --- a/src/tools/tidy/src/bins.rs +++ b/src/tools/tidy/src/bins.rs @@ -35,7 +35,7 @@ pub fn check(path: &Path, bad: &mut bool) { return; } - let metadata = t!(fs::symlink_metadata(&file), &file); + let metadata = t!(entry.metadata(), file); if metadata.mode() & 0o111 != 0 { let rel_path = file.strip_prefix(path).unwrap(); let git_friendly_path = rel_path.to_str().unwrap().replace("\\", "/"); From 7dd7c0ff0a24a82efd49ea2cb24deb13ddde0d16 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 13:16:41 -0400 Subject: [PATCH 08/11] Skip querying each ignore directive if none in file --- src/tools/tidy/src/style.rs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index d994f60b2bb47..4a159d926b7cc 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -107,7 +107,11 @@ enum Directive { Ignore(bool), } -fn contains_ignore_directive(contents: &str, check: &str) -> Directive { +fn contains_ignore_directive(can_contain: bool, contents: &str, check: &str) -> Directive { + if !can_contain { + return Directive::Deny; + } + // Update `can_contain` when changing this if contents.contains(&format!("// ignore-tidy-{}", check)) || contents.contains(&format!("# ignore-tidy-{}", check)) { Directive::Ignore(false) @@ -140,12 +144,15 @@ pub fn check(path: &Path, bad: &mut bool) { tidy_error!(bad, "{}: empty file", file.display()); } - let mut skip_cr = contains_ignore_directive(&contents, "cr"); - let mut skip_tab = contains_ignore_directive(&contents, "tab"); - let mut skip_line_length = contains_ignore_directive(&contents, "linelength"); - let mut skip_file_length = contains_ignore_directive(&contents, "filelength"); - let mut skip_end_whitespace = contains_ignore_directive(&contents, "end-whitespace"); - let mut skip_copyright = contains_ignore_directive(&contents, "copyright"); + let can_contain = contents.contains("// ignore-tidy-") || + contents.contains("# ignore-tidy-"); + let mut skip_cr = contains_ignore_directive(can_contain, &contents, "cr"); + let mut skip_tab = contains_ignore_directive(can_contain, &contents, "tab"); + let mut skip_line_length = contains_ignore_directive(can_contain, &contents, "linelength"); + let mut skip_file_length = contains_ignore_directive(can_contain, &contents, "filelength"); + let mut skip_end_whitespace = + contains_ignore_directive(can_contain, &contents, "end-whitespace"); + let mut skip_copyright = contains_ignore_directive(can_contain, &contents, "copyright"); let mut leading_new_lines = false; let mut trailing_new_lines = 0; let mut lines = 0; From ebbc662f079ab222838e1f2f394b29022f9372a0 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 14:19:05 -0400 Subject: [PATCH 09/11] Use Path/PathBuf directly instead of through path:: --- src/tools/tidy/src/unstable_book.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/tools/tidy/src/unstable_book.rs b/src/tools/tidy/src/unstable_book.rs index f7e40ce4bae36..010253fc35a37 100644 --- a/src/tools/tidy/src/unstable_book.rs +++ b/src/tools/tidy/src/unstable_book.rs @@ -1,6 +1,6 @@ use std::collections::BTreeSet; use std::fs; -use std::path; +use std::path::{PathBuf, Path}; use crate::features::{collect_lang_features, collect_lib_features, Features, Status}; pub const PATH_STR: &str = "doc/unstable-book"; @@ -12,19 +12,19 @@ pub const LANG_FEATURES_DIR: &str = "src/language-features"; pub const LIB_FEATURES_DIR: &str = "src/library-features"; /// Builds the path to the Unstable Book source directory from the Rust 'src' directory. -pub fn unstable_book_path(base_src_path: &path::Path) -> path::PathBuf { +pub fn unstable_book_path(base_src_path: &Path) -> PathBuf { base_src_path.join(PATH_STR) } /// Builds the path to the directory where the features are documented within the Unstable Book /// source directory. -pub fn unstable_book_lang_features_path(base_src_path: &path::Path) -> path::PathBuf { +pub fn unstable_book_lang_features_path(base_src_path: &Path) -> PathBuf { unstable_book_path(base_src_path).join(LANG_FEATURES_DIR) } /// Builds the path to the directory where the features are documented within the Unstable Book /// source directory. -pub fn unstable_book_lib_features_path(base_src_path: &path::Path) -> path::PathBuf { +pub fn unstable_book_lib_features_path(base_src_path: &Path) -> PathBuf { unstable_book_path(base_src_path).join(LIB_FEATURES_DIR) } @@ -45,7 +45,7 @@ pub fn collect_unstable_feature_names(features: &Features) -> BTreeSet { .collect() } -pub fn collect_unstable_book_section_file_names(dir: &path::Path) -> BTreeSet { +pub fn collect_unstable_book_section_file_names(dir: &Path) -> BTreeSet { fs::read_dir(dir) .expect("could not read directory") .map(|entry| entry.expect("could not read directory entry")) @@ -60,7 +60,7 @@ pub fn collect_unstable_book_section_file_names(dir: &path::Path) -> BTreeSet BTreeSet { collect_unstable_book_section_file_names(&unstable_book_lang_features_path(base_src_path)) } @@ -69,12 +69,11 @@ fn collect_unstable_book_lang_features_section_file_names(base_src_path: &path:: /// /// * hyphens replaced by underscores, /// * the markdown suffix ('.md') removed. -fn collect_unstable_book_lib_features_section_file_names(base_src_path: &path::Path) - -> BTreeSet { +fn collect_unstable_book_lib_features_section_file_names(base_src_path: &Path) -> BTreeSet { collect_unstable_book_section_file_names(&unstable_book_lib_features_path(base_src_path)) } -pub fn check(path: &path::Path, bad: &mut bool) { +pub fn check(path: &Path, bad: &mut bool) { // Library features let lang_features = collect_lang_features(path, bad); From 6c5c78d00c809977e3d56e21adce63b9d5532c71 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 14:32:58 -0400 Subject: [PATCH 10/11] Collect features only once --- src/tools/tidy/src/features.rs | 61 ++++++++++++++++------------- src/tools/tidy/src/main.rs | 4 +- src/tools/tidy/src/unstable_book.rs | 23 +++++++---- 3 files changed, 51 insertions(+), 37 deletions(-) diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index ba3132845be39..4bd89f6f0bf22 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -50,7 +50,36 @@ pub struct Feature { pub type Features = HashMap; -pub fn check(path: &Path, bad: &mut bool, verbose: bool) { +pub struct CollectedFeatures { + pub lib: Features, + pub lang: Features, +} + +// Currently only used for unstable book generation +pub fn collect_lib_features(base_src_path: &Path) -> Features { + let mut lib_features = Features::new(); + + // This library feature is defined in the `compiler_builtins` crate, which + // has been moved out-of-tree. Now it can no longer be auto-discovered by + // `tidy`, because we need to filter out its (submodule) directory. Manually + // add it to the set of known library features so we can still generate docs. + lib_features.insert("compiler_builtins_lib".to_owned(), Feature { + level: Status::Unstable, + since: None, + has_gate_test: false, + tracking_issue: None, + }); + + map_lib_features(base_src_path, + &mut |res, _, _| { + if let Ok((name, feature)) = res { + lib_features.insert(name.to_owned(), feature); + } + }); + lib_features +} + +pub fn check(path: &Path, bad: &mut bool, verbose: bool) -> CollectedFeatures { let mut features = collect_lang_features(path, bad); assert!(!features.is_empty()); @@ -125,7 +154,7 @@ pub fn check(path: &Path, bad: &mut bool, verbose: bool) { } if *bad { - return; + return CollectedFeatures { lib: lib_features, lang: features }; } if verbose { @@ -140,6 +169,8 @@ pub fn check(path: &Path, bad: &mut bool, verbose: bool) { } else { println!("* {} features", features.len()); } + + CollectedFeatures { lib: lib_features, lang: features } } fn format_features<'a>(features: &'a Features, family: &'a str) -> impl Iterator + 'a { @@ -303,32 +334,6 @@ pub fn collect_lang_features(base_src_path: &Path, bad: &mut bool) -> Features { .collect() } -pub fn collect_lib_features(base_src_path: &Path) -> Features { - let mut lib_features = Features::new(); - - // This library feature is defined in the `compiler_builtins` crate, which - // has been moved out-of-tree. Now it can no longer be auto-discovered by - // `tidy`, because we need to filter out its (submodule) directory. Manually - // add it to the set of known library features so we can still generate docs. - lib_features.insert("compiler_builtins_lib".to_owned(), Feature { - level: Status::Unstable, - since: None, - has_gate_test: false, - tracking_issue: None, - }); - - map_lib_features(base_src_path, - &mut |res, _, _| { - if let Ok((name, feature)) = res { - if lib_features.contains_key(name) { - return; - } - lib_features.insert(name.to_owned(), feature); - } - }); - lib_features -} - fn get_and_check_lib_features(base_src_path: &Path, bad: &mut bool, lang_features: &Features) -> Features { diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index fa88b078cd8d4..918762ed6e69a 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -24,9 +24,9 @@ fn main() { style::check(&path, &mut bad); errors::check(&path, &mut bad); cargo::check(&path, &mut bad); - features::check(&path, &mut bad, verbose); + let collected = features::check(&path, &mut bad, verbose); pal::check(&path, &mut bad); - unstable_book::check(&path, &mut bad); + unstable_book::check(&path, collected, &mut bad); libcoretest::check(&path, &mut bad); if !args.iter().any(|s| *s == "--no-vendor") { deps::check(&path, &mut bad); diff --git a/src/tools/tidy/src/unstable_book.rs b/src/tools/tidy/src/unstable_book.rs index 010253fc35a37..fb63520f0684a 100644 --- a/src/tools/tidy/src/unstable_book.rs +++ b/src/tools/tidy/src/unstable_book.rs @@ -1,7 +1,7 @@ use std::collections::BTreeSet; use std::fs; use std::path::{PathBuf, Path}; -use crate::features::{collect_lang_features, collect_lib_features, Features, Status}; +use crate::features::{CollectedFeatures, Features, Feature, Status}; pub const PATH_STR: &str = "doc/unstable-book"; @@ -73,13 +73,22 @@ fn collect_unstable_book_lib_features_section_file_names(base_src_path: &Path) - collect_unstable_book_section_file_names(&unstable_book_lib_features_path(base_src_path)) } -pub fn check(path: &Path, bad: &mut bool) { - // Library features - - let lang_features = collect_lang_features(path, bad); - let lib_features = collect_lib_features(path).into_iter().filter(|&(ref name, _)| { +pub fn check(path: &Path, features: CollectedFeatures, bad: &mut bool) { + let lang_features = features.lang; + let mut lib_features = features.lib.into_iter().filter(|&(ref name, _)| { !lang_features.contains_key(name) - }).collect(); + }).collect::(); + + // This library feature is defined in the `compiler_builtins` crate, which + // has been moved out-of-tree. Now it can no longer be auto-discovered by + // `tidy`, because we need to filter out its (submodule) directory. Manually + // add it to the set of known library features so we can still generate docs. + lib_features.insert("compiler_builtins_lib".to_owned(), Feature { + level: Status::Unstable, + since: None, + has_gate_test: false, + tracking_issue: None, + }); // Library features let unstable_lib_feature_names = collect_unstable_feature_names(&lib_features); From 777951c926820cc20f0047d49091c37e0fbff14e Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 21 Jun 2019 14:58:48 -0400 Subject: [PATCH 11/11] Exit early from feature search if no features in file --- src/tools/tidy/src/features.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index 4bd89f6f0bf22..1841beb1fd116 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -378,7 +378,15 @@ fn map_lib_features(base_src_path: &Path, return; } - let mut becoming_feature: Option<(String, Feature)> = None; + // This is an early exit -- all the attributes we're concerned with must contain this: + // * rustc_const_unstable( + // * unstable( + // * stable( + if !contents.contains("stable(") { + return; + } + + let mut becoming_feature: Option<(&str, Feature)> = None; for (i, line) in contents.lines().enumerate() { macro_rules! err { ($msg:expr) => {{ @@ -457,7 +465,7 @@ fn map_lib_features(base_src_path: &Path, if line.contains(']') { mf(Ok((feature_name, feature)), file, i + 1); } else { - becoming_feature = Some((feature_name.to_owned(), feature)); + becoming_feature = Some((feature_name, feature)); } } });