Auto merge of #96501 - jyn514:individual-paths, r=Mark-Simulacrum

Pass all paths to `Step::run` at once when using `ShouldRun::krate`

Helps with https://github.com/rust-lang/rust/pull/95503. The goal is to run `cargo test -p rustc_data_structures -p rustc_lint_defs` instead of `cargo test -p rustc_data_structures; cargo test -p rustc_lint_defs`, which should both recompile less and avoid replaying cached warnings.

This was surprisingly complicated. The main changes are:
1. Invert the order of iteration in `StepDescription::run`.

    Previously, it did something like:
    ```python
    for path in paths:
    for (step, should_run) in should_runs:
        if let Some(set) = should_run.pathset_for_path(path):
        step.run(builder, set)
    ```

    That worked ok for individual paths, but didn't allow passing more than one path at a time to `Step::run`
    (since `pathset_for_paths` only had one path available to it).
    Change it to instead look at the intersection of `paths` and `should_run.paths`:

    ```python
    for (step, should_run) in should_runs:
    if let Some(set) = should_run.pathset_for_paths(paths):
        step.run(builder, set)
    ```

2. Change `pathset_for_path` to take multiple pathsets.

    The goal is to avoid `x test library/alloc` testing *all* library crates, instead of just alloc.
    The changes here are similarly subtle, to use the intersection between the paths rather than all
    paths in `should_run.paths`. I added a test for the behavior to try and make it more clear.

    Note that we use pathsets instead of just paths to allow for sets with multiple aliases (*cough* `all_krates` *cough*).
    See the documentation added in the next commit for more detail.

3. Change `StepDescription::run` to explicitly handle 0 paths.

    Before this was implicitly handled by the `for` loop, which just didn't excute when there were no paths.
    Now it needs a check, to avoid trying to run all steps (this is a problem for steps that use `default_condition`).

4. Change `RunDescription` to have a list of pathsets, rather than a single path.

5. Remove paths as they're matched

    This allows checking at the end that no invalid paths are left over.
    Note that if two steps matched the same path, this will no longer run both;
    but that's a bug anyway.

6. Handle suite paths separately from regular sets.

    Running multiple suite paths at once instead of in separate `make_run` invocations is both tricky and not particularly useful.
    The respective test Steps already handle this by introspecting the original paths.

    Avoid having to deal with it by moving suite handling into a seperate loop than `PathSet::Set` checks.

`@rustbot` label +A-rustbuild

src/bootstrap/builder.rs

@@ -91,7 +91,7 @@ pub trait Step: 'static + Clone + Debug + PartialEq + Eq + Hash {
 pub struct RunConfig<'a> {
     pub builder: &'a Builder<'a>,
     pub target: TargetSelection,
-    pub path: PathBuf,
+    pub paths: Vec<PathSet>,
 }
 
 impl RunConfig<'_> {
@@ -150,11 +150,16 @@ impl Debug for TaskPath {
 /// Collection of paths used to match a task rule.
 #[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)]
 pub enum PathSet {
-    /// A collection of individual paths.
+    /// A collection of individual paths or aliases.
     ///
     /// These are generally matched as a path suffix. For example, a
-    /// command-line value of `libstd` will match if `src/libstd` is in the
+    /// command-line value of `std` will match if `library/std` is in the
     /// set.
+    ///
+    /// NOTE: the paths within a set should always be aliases of one another.
+    /// For example, `src/librustdoc` and `src/tools/rustdoc` should be in the same set,
+    /// but `library/core` and `library/std` generally should not, unless there's no way (for that Step)
+    /// to build them separately.
     Set(BTreeSet<TaskPath>),
     /// A "suite" of paths.
     ///
@@ -177,26 +182,65 @@ impl PathSet {
     }
 
     fn has(&self, needle: &Path, module: Option<Kind>) -> bool {
-        let check = |p: &TaskPath| {
-            if let (Some(p_kind), Some(kind)) = (&p.kind, module) {
-                p.path.ends_with(needle) && *p_kind == kind
-            } else {
-                p.path.ends_with(needle)
-            }
-        };
-
         match self {
-            PathSet::Set(set) => set.iter().any(check),
-            PathSet::Suite(suite) => check(suite),
+            PathSet::Set(set) => set.iter().any(|p| Self::check(p, needle, module)),
+            PathSet::Suite(suite) => Self::check(suite, needle, module),
         }
     }
 
-    fn path(&self, builder: &Builder<'_>) -> PathBuf {
+    // internal use only
+    fn check(p: &TaskPath, needle: &Path, module: Option<Kind>) -> bool {
+        if let (Some(p_kind), Some(kind)) = (&p.kind, module) {
+            p.path.ends_with(needle) && *p_kind == kind
+        } else {
+            p.path.ends_with(needle)
+        }
+    }
+
+    /// Return all `TaskPath`s in `Self` that contain any of the `needles`, removing the
+    /// matched needles.
+    ///
+    /// This is used for `StepDescription::krate`, which passes all matching crates at once to
+    /// `Step::make_run`, rather than calling it many times with a single crate.
+    /// See `tests.rs` for examples.
+    fn intersection_removing_matches(
+        &self,
+        needles: &mut Vec<&Path>,
+        module: Option<Kind>,
+    ) -> PathSet {
+        let mut check = |p| {
+            for (i, n) in needles.iter().enumerate() {
+                let matched = Self::check(p, n, module);
+                if matched {
+                    needles.remove(i);
+                    return true;
+                }
+            }
+            false
+        };
+        match self {
+            PathSet::Set(set) => PathSet::Set(set.iter().filter(|&p| check(p)).cloned().collect()),
+            PathSet::Suite(suite) => {
+                if check(suite) {
+                    self.clone()
+                } else {
+                    PathSet::empty()
+                }
+            }
+        }
+    }
+
+    /// 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::krate`], [`ShouldRun::path`], or [`ShouldRun::alias`].
+    #[track_caller]
+    pub fn assert_single_path(&self) -> &TaskPath {
         match self {
             PathSet::Set(set) => {
-                set.iter().next().map(|p| &p.path).unwrap_or(&builder.build.src).clone()
+                assert_eq!(set.len(), 1, "called assert_single_path on multiple paths");
+                set.iter().next().unwrap()
             }
-            PathSet::Suite(path) => path.path.clone(),
+            PathSet::Suite(_) => unreachable!("called assert_single_path on a Suite path"),
         }
     }
 }
@@ -213,8 +257,8 @@ impl StepDescription {
         }
     }
 
-    fn maybe_run(&self, builder: &Builder<'_>, pathset: &PathSet) {
-        if self.is_excluded(builder, pathset) {
+    fn maybe_run(&self, builder: &Builder<'_>, pathsets: Vec<PathSet>) {
+        if pathsets.iter().any(|set| self.is_excluded(builder, set)) {
             return;
         }
 
@@ -222,7 +266,7 @@ impl StepDescription {
         let targets = if self.only_hosts { &builder.hosts } else { &builder.targets };
 
         for target in targets {
-            let run = RunConfig { builder, path: pathset.path(builder), target: *target };
+            let run = RunConfig { builder, paths: pathsets.clone(), target: *target };
             (self.make_run)(run);
         }
     }
@@ -261,46 +305,55 @@ impl StepDescription {
             for (desc, should_run) in v.iter().zip(&should_runs) {
                 if desc.default && should_run.is_really_default() {
                     for pathset in &should_run.paths {
-                        desc.maybe_run(builder, pathset);
+                        desc.maybe_run(builder, vec![pathset.clone()]);
                     }
                 }
             }
         }
 
-        for path in paths {
-            // strip CurDir prefix if present
-            let path = match path.strip_prefix(".") {
-                Ok(p) => p,
-                Err(_) => path,
-            };
+        // strip CurDir prefix if present
+        let mut paths: Vec<_> =
+            paths.into_iter().map(|p| p.strip_prefix(".").unwrap_or(p)).collect();
 
-            let mut attempted_run = false;
+        // 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 (desc, should_run) in v.iter().zip(&should_runs) {
-                if let Some(suite) = should_run.is_suite_path(path) {
-                    attempted_run = true;
-                    desc.maybe_run(builder, suite);
-                } else if let Some(pathset) = should_run.pathset_for_path(path, desc.kind) {
-                    attempted_run = true;
-                    desc.maybe_run(builder, pathset);
+                if let Some(suite) = should_run.is_suite_path(&path) {
+                    desc.maybe_run(builder, vec![suite.clone()]);
+                    return false;
                 }
             }
+            true
+        });
 
-            if !attempted_run {
-                eprintln!(
-                    "error: no `{}` rules matched '{}'",
-                    builder.kind.as_str(),
-                    path.display()
-                );
-                eprintln!(
-                    "help: run `x.py {} --help --verbose` to show a list of available paths",
-                    builder.kind.as_str()
-                );
-                eprintln!(
-                    "note: if you are adding a new Step to bootstrap itself, make sure you register it with `describe!`"
-                );
-                std::process::exit(1);
+        if paths.is_empty() {
+            return;
+        }
+
+        // Handle all PathSets.
+        for (desc, should_run) in v.iter().zip(&should_runs) {
+            let pathsets = should_run.pathset_for_paths_removing_matches(&mut paths, desc.kind);
+            if !pathsets.is_empty() {
+                desc.maybe_run(builder, pathsets);
             }
         }
+
+        if !paths.is_empty() {
+            eprintln!("error: no `{}` rules matched {:?}", builder.kind.as_str(), paths,);
+            eprintln!(
+                "help: run `x.py {} --help --verbose` to show a list of available paths",
+                builder.kind.as_str()
+            );
+            eprintln!(
+                "note: if you are adding a new Step to bootstrap itself, make sure you register it with `describe!`"
+            );
+            #[cfg(not(test))]
+            std::process::exit(1);
+            #[cfg(test)]
+            // so we can use #[should_panic]
+            panic!()
+        }
     }
 }
 
@@ -370,7 +423,7 @@ impl<'a> ShouldRun<'a> {
     /// Indicates it should run if the command-line selects the given crate or
     /// any of its (local) dependencies.
     ///
-    /// `make_run` will be called separately for each matching command-line path.
+    /// `make_run` will be called a single time with all matching command-line paths.
     pub fn krate(mut self, name: &str) -> Self {
         for krate in self.builder.in_tree_crates(name, None) {
             let path = krate.local_path(self.builder);
@@ -417,9 +470,10 @@ impl<'a> ShouldRun<'a> {
         self
     }
 
-    pub fn is_suite_path(&self, path: &Path) -> Option<&PathSet> {
+    /// 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(p) => path.starts_with(&p.path),
+            PathSet::Suite(suite) => requested_path.starts_with(&suite.path),
             PathSet::Set(_) => false,
         })
     }
@@ -435,8 +489,28 @@ impl<'a> ShouldRun<'a> {
         self
     }
 
-    fn pathset_for_path(&self, path: &Path, kind: Kind) -> Option<&PathSet> {
-        self.paths.iter().find(|pathset| pathset.has(path, Some(kind)))
+    /// 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 pathset_for_paths_removing_matches(
+        &self,
+        paths: &mut Vec<&Path>,
+        kind: Kind,
+    ) -> Vec<PathSet> {
+        let mut sets = vec![];
+        for pathset in &self.paths {
+            let subset = pathset.intersection_removing_matches(paths, Some(kind));
+            if subset != PathSet::empty() {
+                sets.push(subset);
+            }
+        }
+        sets
     }
 }
 

src/bootstrap/builder/tests.rs

@@ -3,7 +3,11 @@ use crate::config::{Config, TargetSelection};
 use std::thread;
 
 fn configure(cmd: &str, host: &[&str], target: &[&str]) -> Config {
-    let mut config = Config::parse(&[cmd.to_owned()]);
+    configure_with_args(&[cmd.to_owned()], host, target)
+}
+
+fn configure_with_args(cmd: &[String], host: &[&str], target: &[&str]) -> Config {
+    let mut config = Config::parse(cmd);
     // don't save toolstates
     config.save_toolstates = None;
     config.dry_run = true;
@@ -46,6 +50,41 @@ fn run_build(paths: &[PathBuf], config: Config) -> Cache {
     builder.cache
 }
 
+fn check_cli<const N: usize>(paths: [&str; N]) {
+    run_build(
+        &paths.map(PathBuf::from),
+        configure_with_args(&paths.map(String::from), &["A"], &["A"]),
+    );
+}
+
+#[test]
+fn test_valid() {
+    // make sure multi suite paths are accepted
+    check_cli(["test", "src/test/ui/attr-start.rs", "src/test/ui/attr-shebang.rs"]);
+}
+
+#[test]
+#[should_panic]
+fn test_invalid() {
+    // make sure that invalid paths are caught, even when combined with valid paths
+    check_cli(["test", "library/std", "x"]);
+}
+
+#[test]
+fn test_intersection() {
+    let set = PathSet::Set(
+        ["library/core", "library/alloc", "library/std"].into_iter().map(TaskPath::parse).collect(),
+    );
+    let mut command_paths =
+        vec![Path::new("library/core"), Path::new("library/alloc"), Path::new("library/stdarch")];
+    let subset = set.intersection_removing_matches(&mut command_paths, None);
+    assert_eq!(
+        subset,
+        PathSet::Set(["library/core", "library/alloc"].into_iter().map(TaskPath::parse).collect())
+    );
+    assert_eq!(command_paths, vec![Path::new("library/stdarch")]);
+}
+
 #[test]
 fn test_exclude() {
     let mut config = configure("test", &["A"], &["A"]);
@@ -539,7 +578,7 @@ mod dist {
                 target: host,
                 mode: Mode::Std,
                 test_kind: test::TestKind::Test,
-                krate: INTERNER.intern_str("std"),
+                crates: vec![INTERNER.intern_str("std")],
             },]
         );
     }

src/bootstrap/cache.rs

@@ -270,7 +270,7 @@ impl Cache {
 
 #[cfg(test)]
 impl Cache {
-    pub fn all<S: Ord + Copy + Step>(&mut self) -> Vec<(S, S::Output)> {
+    pub fn all<S: Ord + Clone + Step>(&mut self) -> Vec<(S, S::Output)> {
         let cache = self.0.get_mut();
         let type_id = TypeId::of::<S>();
         let mut v = cache
@@ -278,7 +278,7 @@ impl Cache {
             .map(|b| b.downcast::<HashMap<S, S::Output>>().expect("correct type"))
             .map(|m| m.into_iter().collect::<Vec<_>>())
             .unwrap_or_default();
-        v.sort_by_key(|&(a, _)| a);
+        v.sort_by_key(|(s, _)| s.clone());
         v
     }
 

src/bootstrap/lib.rs

@@ -170,6 +170,7 @@ mod job {
     pub unsafe fn setup(_build: &mut crate::Build) {}
 }
 
+pub use crate::builder::PathSet;
 use crate::cache::{Interned, INTERNER};
 pub use crate::config::Config;
 pub use crate::flags::Subcommand;

src/bootstrap/test.rs

@@ -1856,12 +1856,12 @@ impl Step for RustcGuide {
     }
 }
 
-#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct CrateLibrustc {
     compiler: Compiler,
     target: TargetSelection,
     test_kind: TestKind,
-    krate: Interned<String>,
+    crates: Vec<Interned<String>>,
 }
 
 impl Step for CrateLibrustc {
@@ -1877,10 +1877,14 @@ impl Step for CrateLibrustc {
         let builder = run.builder;
         let host = run.build_triple();
         let compiler = builder.compiler_for(builder.top_stage, host, host);
-        let krate = builder.crate_paths[&run.path];
+        let crates = run
+            .paths
+            .iter()
+            .map(|p| builder.crate_paths[&p.assert_single_path().path].clone())
+            .collect();
         let test_kind = builder.kind.into();
 
-        builder.ensure(CrateLibrustc { compiler, target: run.target, test_kind, krate });
+        builder.ensure(CrateLibrustc { compiler, target: run.target, test_kind, crates });
     }
 
     fn run(self, builder: &Builder<'_>) {
@@ -1889,18 +1893,18 @@ impl Step for CrateLibrustc {
             target: self.target,
             mode: Mode::Rustc,
             test_kind: self.test_kind,
-            krate: self.krate,
+            crates: self.crates,
         });
     }
 }
 
-#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct Crate {
     pub compiler: Compiler,
     pub target: TargetSelection,
     pub mode: Mode,
     pub test_kind: TestKind,
-    pub krate: Interned<String>,
+    pub crates: Vec<Interned<String>>,
 }
 
 impl Step for Crate {
@@ -1916,9 +1920,13 @@ impl Step for Crate {
         let host = run.build_triple();
         let compiler = builder.compiler_for(builder.top_stage, host, host);
         let test_kind = builder.kind.into();
-        let krate = builder.crate_paths[&run.path];
+        let crates = run
+            .paths
+            .iter()
+            .map(|p| builder.crate_paths[&p.assert_single_path().path].clone())
+            .collect();
 
-        builder.ensure(Crate { compiler, target: run.target, mode: Mode::Std, test_kind, krate });
+        builder.ensure(Crate { compiler, target: run.target, mode: Mode::Std, test_kind, crates });
     }
 
     /// Runs all unit tests plus documentation tests for a given crate defined
@@ -1934,7 +1942,6 @@ impl Step for Crate {
         let target = self.target;
         let mode = self.mode;
         let test_kind = self.test_kind;
-        let krate = self.krate;
 
         builder.ensure(compile::Std { compiler, target });
         builder.ensure(RemoteCopyLibs { compiler, target });
@@ -1975,7 +1982,9 @@ impl Step for Crate {
             DocTests::Yes => {}
         }
 
-        cargo.arg("-p").arg(krate);
+        for krate in &self.crates {
+            cargo.arg("-p").arg(krate);
+        }
 
         // The tests are going to run with the *target* libraries, so we need to
         // ensure that those libraries show up in the LD_LIBRARY_PATH equivalent.
@@ -2011,8 +2020,8 @@ impl Step for Crate {
         }
 
         builder.info(&format!(
-            "{} {} stage{} ({} -> {})",
-            test_kind, krate, compiler.stage, &compiler.host, target
+            "{} {:?} stage{} ({} -> {})",
+            test_kind, self.crates, compiler.stage, &compiler.host, target
         ));
         let _time = util::timeit(&builder);
         try_run(builder, &mut cargo.into());