bootstrap: Overhaul matching of command-line selectors to steps

src/bootstrap/src/core/builder/cli_paths.rs

@@ -2,44 +2,21 @@
 //! command-line, extracted from `core/builder/mod.rs` because that file is
 //! large and hard to navigate.
 
-use std::fmt::{self, Debug};
+use std::collections::{HashMap, HashSet};
+use std::hash::Hash;
 use std::path::PathBuf;
 
 use crate::core::builder::{Builder, CommandLineStepDescription, Kind, PathSet, ShouldRun};
 
 #[cfg(test)]
 mod tests;
 
-#[derive(Clone, PartialEq)]
-pub(crate) struct CLIStepPath {
-    pub(crate) path: PathBuf,
-    pub(crate) will_be_executed: bool,
-}
-
-impl Debug for CLIStepPath {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{}", self.path.display())
-    }
-}
-
-impl From<PathBuf> for CLIStepPath {
-    fn from(path: PathBuf) -> Self {
-        Self { path, will_be_executed: false }
-    }
-}
-
 /// Combines a [`CommandLineStepDescription`] with its corresponding [`ShouldRun`].
 struct StepExtra<'a> {
     desc: &'a CommandLineStepDescription,
     should_run: ShouldRun<'a>,
 }
 
-struct StepToRun<'a> {
-    sort_index: usize,
-    desc: &'a CommandLineStepDescription,
-    pathsets: Vec<PathSet>,
-}
-
 pub(crate) fn match_paths_to_steps_and_run(
     builder: &Builder<'_>,
     step_descs: &[CommandLineStepDescription],
@@ -67,6 +44,7 @@ pub(crate) fn match_paths_to_steps_and_run(
         assert!(!should_run.paths.is_empty(), "{:?} should have at least one pathset", desc.name);
     }
 
+    // Run default steps if appropriate.
     if paths.is_empty() || builder.config.include_default_paths {
         for StepExtra { desc, should_run } in &steps {
             if (desc.is_default_step_fn)(builder) {
@@ -86,7 +64,7 @@ pub(crate) fn match_paths_to_steps_and_run(
     //
     // It is also possible that someone passed a relative path starting with . or ..
     // In that case, we have to remove that path prefix.
-    let mut paths = paths
+    let paths = paths
         .iter()
         .map(|path| {
             // Here we "launder" the path through builder.src, to normalize relative path prefixes
@@ -111,7 +89,6 @@ pub(crate) fn match_paths_to_steps_and_run(
                 path
             }
         })
-        .map(|p| p.to_owned())
         .collect::<Vec<_>>();
 
     // If any absolute paths couldn't be made relative, stop now and report them.
@@ -123,66 +100,48 @@ pub(crate) fn match_paths_to_steps_and_run(
         crate::exit!(1);
     }
 
-    // Handle all test suite paths.
-    // (This is separate from the loop below to avoid having to handle multiple paths in `is_suite_path` somehow.)
-    paths.retain(|path| {
-        for StepExtra { desc, should_run } in &steps {
-            if let Some(suite) = should_run.is_suite_path(path) {
-                desc.maybe_run(builder, vec![suite.clone()]);
-                return false;
-            }
-        }
-        true
-    });
-
-    if paths.is_empty() {
-        return;
-    }
-
-    let mut paths: Vec<CLIStepPath> = paths.into_iter().map(|p| p.into()).collect();
-    let mut path_lookup: Vec<(CLIStepPath, bool)> =
-        paths.clone().into_iter().map(|p| (p, false)).collect();
-
-    // Before actually running (non-suite) steps, collect them into a list of structs
-    // so that we can then sort the list to preserve CLI order as much as possible.
-    let mut steps_to_run = vec![];
-
-    for StepExtra { desc, should_run } in &steps {
-        let pathsets = should_run.pathsets_for_paths_flagging_matches(&mut paths);
-
-        // This value is used for sorting the step execution order.
-        // By default, `usize::MAX` is used as the index for steps to assign them the lowest priority.
-        //
-        // If we resolve the step's path from the given CLI input, this value will be updated with
-        // the step's actual index.
-        let mut closest_index = usize::MAX;
-
-        // Find the closest index from the original list of paths given by the CLI input.
-        for (index, (path, is_used)) in path_lookup.iter_mut().enumerate() {
-            if !*is_used && !paths.contains(path) {
-                closest_index = index;
-                *is_used = true;
-                break;
+    // When matching selectors to steps, we want to balance two conflicting goals:
+    // - Ideally, steps should run in the order specified by command-line arguments.
+    // - A selected step should be invoked only once, not multiple times.
+    //
+    // We therefore build up:
+    // - An ordered list of steps to run, each represented by its index in `steps`.
+    // - For each step (by index), the list of its anchors that were matched.
+    let mut step_queue = Vec::<usize>::with_capacity(paths.len());
+    let mut step_anchors = HashMap::<usize, Vec<&PathSet>>::with_capacity(steps.len());
+    let mut unmatched_paths = vec![];
+
+    // For each command-line selector, enqueue the steps that it matches.
+    for path in &paths {
+        let mut path_matched = false;
+
+        for (step_ix, step) in steps.iter().enumerate() {
+            let matched_anchors = step
+                .should_run
+                .paths
+                .iter()
+                .filter(|anchor| {
+                    // The extra `starts_with` here allows an argument like
+                    // `tests/ui/asm/cfg.rs` to select the suite anchor `tests/ui`.
+                    anchor.has(path)
+                        || matches!(anchor, PathSet::Suite(suite) if path.starts_with(&suite.path))
+                })
+                .collect::<Vec<_>>();
+
+            if !matched_anchors.is_empty() {
+                step_queue.push(step_ix);
+                step_anchors.entry(step_ix).or_default().extend(matched_anchors);
+                path_matched = true;
             }
         }
 
-        steps_to_run.push(StepToRun { sort_index: closest_index, desc, pathsets });
-    }
-
-    // Sort the steps before running them to respect the CLI order.
-    steps_to_run.sort_by_key(|step| step.sort_index);
-
-    // Handle all PathSets.
-    for StepToRun { sort_index: _, desc, pathsets } in steps_to_run {
-        if !pathsets.is_empty() {
-            desc.maybe_run(builder, pathsets);
+        if !path_matched {
+            unmatched_paths.push(path);
         }
     }
 
-    paths.retain(|p| !p.will_be_executed);
-
-    if !paths.is_empty() {
-        eprintln!("ERROR: no `{}` rules matched {:?}", builder.kind.as_str(), paths);
+    if !unmatched_paths.is_empty() {
+        eprintln!("ERROR: no `{}` rules matched {unmatched_paths:?}", builder.kind.as_str());
         eprintln!(
             "HELP: run `x.py {} --help --verbose` to show a list of available paths",
             builder.kind.as_str()
@@ -192,4 +151,23 @@ pub(crate) fn match_paths_to_steps_and_run(
         );
         crate::exit!(1);
     }
+
+    fn dedup_vec<T: Copy + Eq + Hash>(vec: &mut Vec<T>) {
+        let mut seen = HashSet::<T>::with_capacity(vec.len());
+        vec.retain(|&x| seen.insert(x));
+    }
+
+    // Deduplicate the queue of steps to run, and the list of anchors to run for each step.
+    dedup_vec(&mut step_queue);
+    for anchors in step_anchors.values_mut() {
+        dedup_vec(anchors);
+    }
+
+    // Run the steps that were selected, in (roughly) command-line order.
+    // For each step, pass all of its matched anchors, regardless of position.
+    for &step_ix in &step_queue {
+        let step = &steps[step_ix];
+        let anchors = step_anchors[&step_ix].iter().map(|p| PathSet::clone(p)).collect::<Vec<_>>();
+        step.desc.maybe_run(builder, anchors);
+    }
 }

src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_library_core_and_alloc_and_stdarch.snap

@@ -4,8 +4,8 @@ expression: test library/core library/alloc library/stdarch
 ---
 [Test] test::Crate
     targets: [aarch64-unknown-linux-gnu]
-    - Set({library/alloc})
     - Set({library/core})
+    - Set({library/alloc})
 [Test] test::StdarchVerify
     targets: [x86_64-unknown-linux-gnu]
     - Set({library/stdarch/crates/stdarch-verify})

src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_tests_coverage_trivial_rs.snap

@@ -0,0 +1,7 @@
+---
+source: src/bootstrap/src/core/builder/cli_paths/tests.rs
+expression: test tests/coverage/trivial.rs
+---
+[Test] test::Coverage
+    targets: [aarch64-unknown-linux-gnu]
+    - Suite(tests/coverage)

src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_tests_coverage_trivial_rs_and_attr_impl_rs.snap

@@ -0,0 +1,7 @@
+---
+source: src/bootstrap/src/core/builder/cli_paths/tests.rs
+expression: test tests/coverage/trivial.rs tests/coverage/attr/impl.rs
+---
+[Test] test::Coverage
+    targets: [aarch64-unknown-linux-gnu]
+    - Suite(tests/coverage)

src/bootstrap/src/core/builder/cli_paths/tests.rs

@@ -197,6 +197,11 @@ declare_tests!(
     (x_test_src_tools_miri, "test src/tools/miri"),
     (x_test_src_tools_miri_and_cargo_miri, "test src/tools/miri src/tools/miri/cargo-miri"),
     (x_test_tests, "test tests"),
+    (x_test_tests_coverage_trivial_rs, "test tests/coverage/trivial.rs"),
+    (
+        x_test_tests_coverage_trivial_rs_and_attr_impl_rs,
+        "test tests/coverage/trivial.rs tests/coverage/attr/impl.rs"
+    ),
     (x_test_tests_skip_coverage, "test tests --skip=coverage"),
     (x_test_tests_ui, "test tests/ui"),
     (x_test_tests_ui_dot_prefix, "test ./tests/ui"),

src/bootstrap/src/core/builder/mod.rs

@@ -20,7 +20,6 @@ use crate::core::build_steps::tool::RustcPrivateCompilers;
 use crate::core::build_steps::{
     check, clean, clippy, compile, dist, doc, gcc, install, llvm, run, setup, test, tool, vendor,
 };
-use crate::core::builder::cli_paths::CLIStepPath;
 use crate::core::builder::step_stack::StepRecord;
 pub use crate::core::builder::step_stack::StepStack;
 use crate::core::config::flags::Subcommand;
@@ -361,7 +360,7 @@ struct CommandLineStepDescription {
     kind: Kind,
 }
 
-#[derive(Clone, PartialOrd, Ord, PartialEq, Eq)]
+#[derive(Clone, PartialOrd, Ord, PartialEq, Eq, Hash)]
 pub struct TaskPath {
     pub path: PathBuf,
 }
@@ -373,7 +372,7 @@ impl Debug for TaskPath {
 }
 
 /// Collection of paths used to match a task rule.
-#[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq, Hash)]
 pub enum PathSet {
     /// A collection of individual paths or aliases.
     ///
@@ -415,34 +414,6 @@ impl PathSet {
         p.path.ends_with(needle) || p.path.starts_with(needle)
     }
 
-    /// Returns true if self is matched by any of the command-line selectors,
-    /// and mutates those selectors to flag them as will-be-executed.
-    fn match_and_flag_selectors(&self, selectors: &mut [CLIStepPath]) -> bool {
-        let mut check_and_flag = |p| {
-            let mut result = false;
-            for selector in selectors.iter_mut() {
-                let matched = Self::check(p, &selector.path);
-                if matched {
-                    selector.will_be_executed = true;
-                    result = true;
-                }
-            }
-            result
-        };
-
-        match self {
-            PathSet::Set(set) => {
-                // Flag all matching selectors, not just the first match.
-                let mut matched = false;
-                for p in set {
-                    matched |= check_and_flag(p);
-                }
-                matched
-            }
-            PathSet::Suite(suite) => check_and_flag(suite),
-        }
-    }
-
     /// A convenience wrapper for Steps which know they have no aliases and all their sets contain only a single path.
     ///
     /// This can be used with [`ShouldRun::crate_or_deps`], [`ShouldRun::path`], or [`ShouldRun::alias`].
@@ -633,38 +604,11 @@ impl<'a> ShouldRun<'a> {
         self
     }
 
-    /// Handles individual files (not directories) within a test suite.
-    fn is_suite_path(&self, requested_path: &Path) -> Option<&PathSet> {
-        self.paths.iter().find(|pathset| match pathset {
-            PathSet::Suite(suite) => requested_path.starts_with(&suite.path),
-            PathSet::Set(_) => false,
-        })
-    }
-
     pub fn suite_path(mut self, suite: &str) -> Self {
         self.paths.insert(PathSet::Suite(TaskPath { path: suite.into() }));
         self
     }
 
-    /// Given a set of requested paths, return the subset which match the Step for this `ShouldRun`,
-    /// removing the matches from `paths`.
-    ///
-    /// NOTE: this returns multiple PathSets to allow for the possibility of multiple units of work
-    /// within the same step. For example, `test::Crate` allows testing multiple crates in the same
-    /// cargo invocation, which are put into separate sets because they aren't aliases.
-    ///
-    /// The reason we return PathSet instead of PathBuf is to allow for aliases that mean the same thing
-    /// (for now, just `all_krates` and `paths`, but we may want to add an `aliases` function in the future?)
-    fn pathsets_for_paths_flagging_matches(&self, paths: &mut [CLIStepPath]) -> Vec<PathSet> {
-        let mut sets = vec![];
-        for pathset in &self.paths {
-            if pathset.match_and_flag_selectors(paths) {
-                sets.push(pathset.clone());
-            }
-        }
-        sets
-    }
-
     /// When the corresponding step is run "by default" (without explicit command-line paths),
     /// act as though the user had explicitly specified these paths.
     fn default_pathsets(&self) -> Vec<PathSet> {

src/bootstrap/src/core/builder/tests.rs

@@ -2119,11 +2119,11 @@ mod snapshot {
         [test] compiletest-run-make 2 <target1>
         [build] rustc 1 <host> -> rustc 2 <target1>
         [build] rustdoc 1 <host>
+        [build] rustc 2 <target1> -> std 2 <target1>
+        [build] rustdoc 2 <target1>
         [build] rustc 0 <host> -> RustdocGUITest 1 <host>
         [test] rustdoc-gui 2 <target1>
         [test] compiletest-incremental 2 <target1>
-        [build] rustc 2 <target1> -> std 2 <target1>
-        [build] rustdoc 2 <target1>
         ");
     }
 

src/bootstrap/src/lib.rs

@@ -15,7 +15,11 @@
 //!
 //! More documentation can be found in each respective module below, and you can
 //! also check out the `src/bootstrap/README.md` file for more information.
+
+// tidy-alphabetical-start
 #![allow(clippy::assertions_on_constants, reason = "false positive for `assert!(cfg!(..))`")]
+#![allow(clippy::map_clone, reason = "false positive for `|x: &&Foo| Foo::clone(x)`")]
+// tidy-alphabetical-end
 
 use std::cell::Cell;
 use std::collections::{BTreeSet, HashMap, HashSet};