Merge pull request #1887 from klensy/clippy-list

collector: beautify warning when include\exclude filters wrong

Author: Kobzol

Diff for: collector/benchlib/src/benchmark.rs

@@ -83,9 +83,7 @@ impl<'a> BenchmarkGroup<'a> {
         let mut items: Vec<(&'static str, BenchmarkProfileFns)> = self
             .benchmarks
             .into_iter()
-            .filter(|(name, _)| {
-                passes_filter(name, args.exclude.as_deref(), args.include.as_deref())
-            })
+            .filter(|(name, _)| passes_filter(name, &args.exclude, &args.include))
             .collect();
         items.sort_unstable_by_key(|item| item.0);
 
@@ -144,16 +142,21 @@ impl<'a> BenchmarkGroup<'a> {
 
 /// Tests if the name of the benchmark passes through the include and exclude filters.
 /// Both filters can contain multiple comma-separated prefixes.
-pub fn passes_filter(name: &str, exclude: Option<&str>, include: Option<&str>) -> bool {
+pub fn passes_filter(name: &str, exclude: &[String], include: &[String]) -> bool {
     match (exclude, include) {
-        (Some(exclude), Some(include)) => {
-            let included = include.split(',').any(|filter| name.starts_with(filter));
-            let excluded = exclude.split(',').any(|filter| name.starts_with(filter));
+        (exclude, include) if !exclude.is_empty() && !include.is_empty() => {
+            let included = include.iter().any(|filter| name.starts_with(filter));
+            let excluded = exclude.iter().any(|filter| name.starts_with(filter));
             included && !excluded
         }
-        (None, Some(include)) => include.split(',').any(|filter| name.starts_with(filter)),
-        (Some(exclude), None) => !exclude.split(',').any(|filter| name.starts_with(filter)),
-        (None, None) => true,
+        ([], include) if !include.is_empty() => {
+            include.iter().any(|filter| name.starts_with(filter))
+        }
+        (exclude, []) if !exclude.is_empty() => {
+            !exclude.iter().any(|filter| name.starts_with(filter))
+        }
+        ([], []) => true,
+        (_, _) => unreachable!(),
     }
 }
 
@@ -172,34 +175,58 @@ mod tests {
 
     #[test]
     fn test_passes_filter_no_filter() {
-        assert!(passes_filter("foo", None, None));
+        assert!(passes_filter("foo", &[], &[]));
     }
 
     #[test]
     fn test_passes_filter_include() {
-        assert!(!passes_filter("foo", None, Some("bar")));
-        assert!(!passes_filter("foo", None, Some("foobar")));
-
-        assert!(passes_filter("foo", None, Some("f")));
-        assert!(passes_filter("foo", None, Some("foo")));
-        assert!(passes_filter("foo", None, Some("bar,baz,foo")));
+        assert!(!passes_filter("foo", &[], &["bar".to_string()]));
+        assert!(!passes_filter("foo", &[], &["foobar".to_string()]));
+
+        assert!(passes_filter("foo", &[], &["f".to_string()]));
+        assert!(passes_filter("foo", &[], &["foo".to_string()]));
+        assert!(passes_filter(
+            "foo",
+            &[],
+            &["bar".to_string(), "baz".to_string(), "foo".to_string()]
+        ));
     }
 
     #[test]
     fn test_passes_filter_exclude() {
-        assert!(passes_filter("foo", Some("bar"), None));
-        assert!(passes_filter("foo", Some("foobar"), None));
-
-        assert!(!passes_filter("foo", Some("f"), None));
-        assert!(!passes_filter("foo", Some("foo"), None));
-        assert!(!passes_filter("foo", Some("bar,baz,foo"), None));
+        assert!(passes_filter("foo", &["bar".to_string()], &[]));
+        assert!(passes_filter("foo", &["foobar".to_string()], &[]));
+
+        assert!(!passes_filter("foo", &["f".to_string()], &[]));
+        assert!(!passes_filter("foo", &["foo".to_string()], &[]));
+        assert!(!passes_filter(
+            "foo",
+            &["bar".to_string(), "baz".to_string(), "foo".to_string()],
+            &[]
+        ));
     }
 
     #[test]
     fn test_passes_filter_include_exclude() {
-        assert!(!passes_filter("foo", Some("bar"), Some("baz")));
-        assert!(passes_filter("foo", Some("bar"), Some("foo")));
-        assert!(!passes_filter("foo", Some("foo"), Some("bar")));
-        assert!(!passes_filter("foo", Some("foo"), Some("foo")));
+        assert!(!passes_filter(
+            "foo",
+            &["bar".to_string()],
+            &["baz".to_string()]
+        ));
+        assert!(passes_filter(
+            "foo",
+            &["bar".to_string()],
+            &["foo".to_string()]
+        ));
+        assert!(!passes_filter(
+            "foo",
+            &["foo".to_string()],
+            &["bar".to_string()]
+        ));
+        assert!(!passes_filter(
+            "foo",
+            &["foo".to_string()],
+            &["foo".to_string()]
+        ));
     }
 }

Diff for: collector/benchlib/src/cli.rs

@@ -17,12 +17,12 @@ pub struct BenchmarkArgs {
     pub iterations: u32,
 
     /// Exclude all benchmarks matching a prefix in this comma-separated list
-    #[arg(long)]
-    pub exclude: Option<String>,
+    #[arg(long, value_delimiter = ',')]
+    pub exclude: Vec<String>,
 
     /// Include only benchmarks matching a prefix in this comma-separated list
-    #[arg(long)]
-    pub include: Option<String>,
+    #[arg(long, value_delimiter = ',')]
+    pub include: Vec<String>,
 }
 
 #[derive(clap::Parser, Debug)]

Diff for: collector/src/bin/collector.rs

@@ -312,16 +312,16 @@ struct LocalOptions {
     cargo: Option<PathBuf>,
 
     /// Exclude all benchmarks matching a prefix in this comma-separated list
-    #[arg(long)]
-    exclude: Option<String>,
+    #[arg(long, value_delimiter = ',')]
+    exclude: Vec<String>,
 
     /// Exclude all benchmarks matching a suffix in this comma-separated list
-    #[arg(long)]
-    exclude_suffix: Option<String>,
+    #[arg(long, value_delimiter = ',')]
+    exclude_suffix: Vec<String>,
 
     /// Include only benchmarks matching a prefix in this comma-separated list
-    #[arg(long)]
-    include: Option<String>,
+    #[arg(long, value_delimiter = ',')]
+    include: Vec<String>,
 
     /// Include only benchmarks belonging to the given categories.
     #[arg(long, value_parser = EnumArgParser::<Category>::default(), default_value = "Primary,Secondary")]
@@ -689,9 +689,9 @@ fn main_result() -> anyhow::Result<i32> {
             };
             let benchmarks = get_compile_benchmarks(
                 &compile_benchmark_dir,
-                local.include.as_deref(),
-                local.exclude.as_deref(),
-                local.exclude_suffix.as_deref(),
+                &local.include,
+                &local.exclude,
+                &local.exclude_suffix,
             )?;
             for benchmark in benchmarks {
                 println!("Stats for benchmark `{}`", benchmark.name);
@@ -901,9 +901,9 @@ fn main_result() -> anyhow::Result<i32> {
 
             let mut benchmarks = get_compile_benchmarks(
                 &compile_benchmark_dir,
-                local.include.as_deref(),
-                local.exclude.as_deref(),
-                local.exclude_suffix.as_deref(),
+                &local.include,
+                &local.exclude,
+                &local.exclude_suffix,
             )?;
             benchmarks.retain(|b| local.category.0.contains(&b.category()));
 
@@ -978,6 +978,16 @@ fn main_result() -> anyhow::Result<i32> {
                         exclude,
                         runs,
                     } => {
+                        // FIXME: remove this when/if NextArtifact::Commit's include/exclude
+                        // changed from Option<String> to Vec<String>
+                        // to not to manually parse args
+                        let split_args = |l: Option<String>| -> Vec<String> {
+                            if let Some(l) = l {
+                                l.split(',').map(|arg| arg.trim().to_owned()).collect()
+                            } else {
+                                vec![]
+                            }
+                        };
                         let sha = commit.sha.to_string();
                         let sysroot = Sysroot::install(
                             sha.clone(),
@@ -988,9 +998,9 @@ fn main_result() -> anyhow::Result<i32> {
 
                         let mut benchmarks = get_compile_benchmarks(
                             &compile_benchmark_dir,
-                            include.as_deref(),
-                            exclude.as_deref(),
-                            None,
+                            &split_args(include),
+                            &split_args(exclude),
+                            &[],
                         )?;
                         benchmarks.retain(|b| b.category().is_primary_or_secondary());
 
@@ -1085,9 +1095,9 @@ fn main_result() -> anyhow::Result<i32> {
 
             let mut benchmarks = get_compile_benchmarks(
                 &compile_benchmark_dir,
-                local.include.as_deref(),
-                local.exclude.as_deref(),
-                local.exclude_suffix.as_deref(),
+                &local.include,
+                &local.exclude,
+                &local.exclude_suffix,
             )?;
             benchmarks.retain(|b| local.category.0.contains(&b.category()));
 
@@ -1569,7 +1579,7 @@ fn bench_published_artifact(
     };
 
     // Exclude benchmarks that don't work with a stable compiler.
-    let mut compile_benchmarks = get_compile_benchmarks(dirs.compile, None, None, None)?;
+    let mut compile_benchmarks = get_compile_benchmarks(dirs.compile, &[], &[], &[])?;
     compile_benchmarks.retain(|b| b.category().is_stable());
 
     let runtime_suite = rt.block_on(load_runtime_benchmarks(

Diff for: collector/src/compile/benchmark/mod.rs

@@ -440,9 +440,9 @@ pub fn compile_benchmark_dir() -> PathBuf {
 
 pub fn get_compile_benchmarks(
     benchmark_dir: &Path,
-    include: Option<&str>,
-    exclude: Option<&str>,
-    exclude_suffix: Option<&str>,
+    include: &[String],
+    exclude: &[String],
+    exclude_suffix: &[String],
 ) -> anyhow::Result<Vec<Benchmark>> {
     let mut benchmarks = Vec::new();
 
@@ -467,33 +467,29 @@ pub fn get_compile_benchmarks(
 
     // For each --include/--exclude entry, we count how many times it's used,
     // to enable `check_for_unused` below.
-    fn to_hashmap(xyz: Option<&str>) -> Option<HashMap<&str, usize>> {
-        xyz.map(|list| {
-            list.split(',')
-                .map(|x| (x, 0))
-                .collect::<HashMap<&str, usize>>()
-        })
+    fn to_hashmap(xyz: &[String]) -> HashMap<&str, usize> {
+        xyz.iter().map(|x| (x.as_ref(), 0)).collect()
     }
 
     let mut includes = to_hashmap(include);
     let mut excludes = to_hashmap(exclude);
     let mut exclude_suffixes = to_hashmap(exclude_suffix);
 
-    for (path, name) in paths {
+    for (path, name) in paths.clone() {
         let mut skip = false;
 
         let name_matches_prefix = |prefixes: &mut HashMap<&str, usize>| {
             substring_matches(prefixes, |prefix| name.starts_with(prefix))
         };
 
-        if let Some(includes) = includes.as_mut() {
-            skip |= !name_matches_prefix(includes);
+        if !includes.is_empty() {
+            skip |= !name_matches_prefix(&mut includes);
         }
-        if let Some(excludes) = excludes.as_mut() {
-            skip |= name_matches_prefix(excludes);
+        if !excludes.is_empty() {
+            skip |= name_matches_prefix(&mut excludes);
         }
-        if let Some(exclude_suffixes) = exclude_suffixes.as_mut() {
-            skip |= substring_matches(exclude_suffixes, |suffix| name.ends_with(suffix));
+        if !exclude_suffixes.is_empty() {
+            skip |= substring_matches(&mut exclude_suffixes, |suffix| name.ends_with(suffix));
         }
         if skip {
             continue;
@@ -504,17 +500,19 @@ pub fn get_compile_benchmarks(
     }
 
     // All prefixes/suffixes must be used at least once. This is to catch typos.
-    let check_for_unused = |option, substrings: Option<HashMap<&str, usize>>| {
-        if let Some(substrings) = substrings {
+    let check_for_unused = |option, substrings: HashMap<&str, usize>| {
+        if !substrings.is_empty() {
             let unused: Vec<_> = substrings
                 .into_iter()
                 .filter_map(|(i, n)| if n == 0 { Some(i) } else { None })
                 .collect();
             if !unused.is_empty() {
                 bail!(
-                    "Warning: one or more unused --{} entries: {:?}",
+                    r#"Warning: one or more unused --{} entries: {:?} found.
+Expected zero or more entries or substrings from list: {:?}."#,
                     option,
-                    unused
+                    unused,
+                    &paths.iter().map(|(_, name)| name).collect::<Vec<_>>(),
                 );
             }
         }
@@ -560,13 +558,9 @@ mod tests {
         // Check that we can deserialize all perf-config.json files in the compile benchmark
         // directory.
         let root = env!("CARGO_MANIFEST_DIR");
-        let benchmarks = get_compile_benchmarks(
-            &Path::new(root).join("compile-benchmarks"),
-            None,
-            None,
-            None,
-        )
-        .unwrap();
+        let benchmarks =
+            get_compile_benchmarks(&Path::new(root).join("compile-benchmarks"), &[], &[], &[])
+                .unwrap();
         assert!(!benchmarks.is_empty());
     }
 }

Diff for: collector/src/runtime/benchmark.rs

@@ -54,13 +54,10 @@ impl BenchmarkSuite {
             groups: groups
                 .into_iter()
                 .filter(|group| {
-                    group.benchmark_names.iter().any(|benchmark| {
-                        passes_filter(
-                            benchmark,
-                            filter.exclude.as_deref(),
-                            filter.include.as_deref(),
-                        )
-                    })
+                    group
+                        .benchmark_names
+                        .iter()
+                        .any(|benchmark| passes_filter(benchmark, &filter.exclude, &filter.include))
                 })
                 .collect(),
             _tmp_artifacts_dir,
@@ -69,13 +66,7 @@ impl BenchmarkSuite {
 
     pub fn filtered_benchmark_count(&self, filter: &BenchmarkFilter) -> u64 {
         self.benchmark_names()
-            .filter(|benchmark| {
-                passes_filter(
-                    benchmark,
-                    filter.exclude.as_deref(),
-                    filter.include.as_deref(),
-                )
-            })
+            .filter(|benchmark| passes_filter(benchmark, &filter.exclude, &filter.include))
             .count() as u64
     }
 
@@ -96,19 +87,19 @@ impl BenchmarkSuite {
 }
 
 pub struct BenchmarkFilter {
-    pub exclude: Option<String>,
-    pub include: Option<String>,
+    pub exclude: Vec<String>,
+    pub include: Vec<String>,
 }
 
 impl BenchmarkFilter {
     pub fn keep_all() -> Self {
         Self {
-            exclude: None,
-            include: None,
+            exclude: vec![],
+            include: vec![],
         }
     }
 
-    pub fn new(exclude: Option<String>, include: Option<String>) -> Self {
+    pub fn new(exclude: Vec<String>, include: Vec<String>) -> Self {
         Self { exclude, include }
     }
 }