Rollup merge of #160548 - Zalathar:coverage-mode, r=jieyouxu

bootstrap: Register `coverage-map` and `coverage-run` aliases via a separate step

Using a separate step lets us remove the `default_to_suites_only` hack that was added in #159131.

In the past we did have separate steps for the aliases, which were removed by #135097. A key difference now is that the new step is normal Rust code without any clunky macros.

Importantly, this PR still preserves the desired skipping behaviour, e.g. `./x test --skip=tests` continues to skip the coverage tests as intended.

r? jieyouxu (or bootstrap)

Author: JonathanBrouwer

src/bootstrap/src/core/build_steps/test.rs

@@ -44,7 +44,7 @@ use crate::utils::helpers::{
     up_to_date,
 };
 use crate::utils::render_tests::{add_flags_and_try_run_tests, try_run_tests};
-use crate::{CLang, CodegenBackendKind, GitRepo, Mode, PathSet, TestTarget, envify, exit};
+use crate::{CLang, CodegenBackendKind, GitRepo, Mode, TestTarget, envify, exit};
 
 mod compiletest;
 pub mod failed_tests;
@@ -1974,72 +1974,41 @@ impl Coverage {
     const SUITE: &'static str = "coverage";
     const ALL_MODES: &[CompiletestMode] =
         &[CompiletestMode::CoverageMap, CompiletestMode::CoverageRun];
+
+    fn new(run: &RunConfig<'_>, mode: CompiletestMode) -> Self {
+        let compiler = run.builder.compiler(run.builder.top_stage, run.build_triple());
+        let target = run.target;
+        Coverage { compiler, target, mode }
+    }
 }
 
 impl CommandLineStep for Coverage {
     type Output = ();
     /// Compiletest will automatically skip the "coverage-run" tests if necessary.
     const IS_HOST: bool = false;
 
-    fn should_run(mut run: ShouldRun<'_>) -> ShouldRun<'_> {
-        // Support various invocation styles, including:
+    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+        // Handle these invocation styles:
+        // - `./x test` (including coverage tests)
         // - `./x test coverage`
+        // - `./x test tests/coverage`
         // - `./x test tests/coverage/trivial.rs`
-        // - `./x test coverage-map`
-        // - `./x test coverage-run -- tests/coverage/trivial.rs`
-        run = run.suite_path(Self::PATH);
-        for mode in Self::ALL_MODES {
-            run = run.alias(mode.as_str());
-        }
-
-        // Allow `./x test --skip=tests` to properly skip the coverage tests,
-        // by not treating the `coverage-map` and `coverage-run` aliases as
-        // implied command-line arguments.
-        run = run.default_to_suites_only();
-
-        run
+        // - `./x test tests/coverage/trivial.rs --skip=coverage-run`
+        run.suite_path(Coverage::PATH)
     }
 
     fn is_default_step(_builder: &Builder<'_>) -> bool {
         true
     }
 
     fn make_run(run: RunConfig<'_>) {
-        let compiler = run.builder.compiler(run.builder.top_stage, run.build_triple());
-        let target = run.target;
-
-        // List of (coverage) test modes that the coverage test suite will be
-        // run in. It's OK for this to contain duplicates, because the call to
-        // `Builder::ensure` below will take care of deduplication.
-        let mut modes = vec![];
-
-        // From the pathsets that were selected on the command-line (or by default),
-        // determine which modes to run in.
-        for path in &run.paths {
-            match path {
-                PathSet::Set(_) => {
-                    for &mode in Self::ALL_MODES {
-                        if path.assert_single_path().path == Path::new(mode.as_str()) {
-                            modes.push(mode);
-                            break;
-                        }
-                    }
-                }
-                PathSet::Suite(_) => {
-                    modes.extend_from_slice(Self::ALL_MODES);
-                    break;
-                }
-            }
-        }
-
-        // Skip any modes that were explicitly skipped/excluded on the command-line.
+        // Run the tests in all coverage-test modes, but skip any modes that
+        // were explicitly skipped on the command-line (e.g. `--skip=coverage-run`).
         // FIXME(Zalathar): Integrate this into central skip handling somehow?
-        modes.retain(|mode| {
-            !run.builder.config.skip.iter().any(|skip| skip == Path::new(mode.as_str()))
-        });
-
-        for mode in modes {
-            run.builder.ensure(Coverage { compiler, target, mode });
+        for &mode in Coverage::ALL_MODES {
+            if !run.builder.config.skip.iter().any(|skip| skip == Path::new(mode.as_str())) {
+                run.builder.ensure(Coverage::new(&run, mode));
+            }
         }
     }
 
@@ -2058,6 +2027,49 @@ impl CommandLineStep for Coverage {
     }
 }
 
+/// Registers the `coverage-map` and `coverage-run` aliases, which are then
+/// forwarded to the [`Coverage`] step.
+///
+/// If the aliases were registered by [`Coverage`] directly, they would also
+/// be treated as implied command-line arguments when run by default.
+/// That would cause things like `./x test --skip=tests` to still run coverage
+/// tests, which is undesirable.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub enum CoverageModeAlias {}
+
+impl CommandLineStep for CoverageModeAlias {
+    type Output = ();
+
+    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+        // Register the aliases "coverage-map" and "coverage-run", to handle
+        // these invocation styles:
+        // - `./x test coverage-map`
+        // - `./x test coverage-run -- tests/coverage/trivial.rs`
+        Coverage::ALL_MODES.iter().fold(run, |run, mode| run.alias(mode.as_str()))
+    }
+
+    fn is_default_step(_builder: &Builder<'_>) -> bool {
+        false
+    }
+
+    fn make_run(run: RunConfig<'_>) {
+        for path in &run.paths {
+            let single_path = &path.assert_single_path().path;
+            for &mode in Coverage::ALL_MODES {
+                if single_path == Path::new(mode.as_str()) {
+                    // Instead of creating an intermediate `CoverageModeAlias`
+                    // step instance, delegate straight to `Coverage`.
+                    run.builder.ensure(Coverage::new(&run, mode));
+                }
+            }
+        }
+    }
+
+    fn run(self, _builder: &Builder<'_>) {
+        unreachable!("never instantiated; `make_run` creates a Coverage step instead");
+    }
+}
+
 test!(CoverageRunRustdoc {
     path: "tests/coverage-run-rustdoc",
     mode: CompiletestMode::CoverageRun,

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

@@ -2,6 +2,6 @@
 source: src/bootstrap/src/core/builder/cli_paths/tests.rs
 expression: test coverage-map
 ---
-[Test] test::Coverage
+[Test] test::CoverageModeAlias
     targets: [aarch64-unknown-linux-gnu]
     - Set({coverage-map})

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

@@ -2,6 +2,6 @@
 source: src/bootstrap/src/core/builder/cli_paths/tests.rs
 expression: test coverage-run
 ---
-[Test] test::Coverage
+[Test] test::CoverageModeAlias
     targets: [aarch64-unknown-linux-gnu]
     - Set({coverage-run})

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

@@ -524,13 +524,11 @@ pub struct ShouldRun<'a> {
 
     // use a BTreeSet to maintain sort order
     paths: BTreeSet<PathSet>,
-
-    default_to_suites_only: bool,
 }
 
 impl<'a> ShouldRun<'a> {
     fn new(builder: &'a Builder<'_>, kind: Kind) -> ShouldRun<'a> {
-        ShouldRun { builder, kind, paths: BTreeSet::new(), default_to_suites_only: false }
+        ShouldRun { builder, kind, paths: BTreeSet::new() }
     }
 
     /// The corresponding step should run if the bootstrap command-line selects
@@ -643,26 +641,10 @@ impl<'a> ShouldRun<'a> {
         sets
     }
 
-    /// When generating pathsets for a step that is being run "by default"
-    /// (i.e. when running bootstrap without an explicit command-line path),
-    /// discard any paths that were not registered as test suites.
-    ///
-    /// This is basically a hack to make path-based skipping work properly for
-    /// coverage tests, since otherwise the `coverage-map` and `coverage-run`
-    /// aliases would prevent `./x test --skip=tests` from skipping them.
-    pub(crate) fn default_to_suites_only(mut self) -> Self {
-        self.default_to_suites_only = true;
-        self
-    }
-
     /// 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> {
-        let mut default_pathsets = self.paths.iter().cloned().collect::<Vec<_>>();
-        if self.default_to_suites_only {
-            default_pathsets.retain(|p| matches!(p, PathSet::Suite(_)));
-        }
-        default_pathsets
+        self.paths.iter().cloned().collect::<Vec<_>>()
     }
 }
 
@@ -898,6 +880,7 @@ impl<'a> Builder<'a> {
                 test::Ui,
                 test::Crashes,
                 test::Coverage,
+                test::CoverageModeAlias,
                 test::MirOpt,
                 test::CodegenLlvm,
                 test::CodegenUnits,