Rollup merge of #147506 - Zalathar:isolate, r=jieyouxu

compiletest: Isolate public APIs and minimize public surface area

As part of my ongoing efforts to improve directive parsing, I would like to be able to make internal changes without worrying about whether they're going to break the rustdoc-gui-test tool. That tool currently uses compiletest as a dependency to help with directive parsing.

This PR therefore isolates all of compiletest's public APIs into two dedicated modules, one used by rustdoc-gui-test, and one used by the compiletest binary in `compiletest/src/bin/main.rs`.

All other modules (and crate-root items) are then made non-`pub` to achieve the API isolation. Doing so reveals some unused items, which have been removed.

(To reduce the amount of immediate textual churn, this PR does not comprehensively replace `pub` with `pub(crate)` throughout the whole crate; that could be done in a follow-up PR.)

---

Ideally, rustdoc-gui-test would not depend on compiletest at all, but properly fixing that is out of scope for this PR.
- #143827

r? jieyouxu

Author: Zalathar

Cargo.lock

@@ -4805,7 +4805,6 @@ name = "rustdoc-gui-test"
 version = "0.1.0"
 dependencies = [
  "build_helper",
- "camino",
  "compiletest",
  "getopts",
  "walkdir",

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

@@ -1974,9 +1974,6 @@ HELP: You can add it into `bootstrap.toml` in `rust.codegen-backends = [{name:?}
         } else if mode == "rustdoc-js" {
             panic!("need nodejs to run rustdoc-js suite");
         }
-        if let Some(ref npm) = builder.config.npm {
-            cmd.arg("--npm").arg(npm);
-        }
         if builder.config.rust_optimize_tests {
             cmd.arg("--optimize-tests");
         }

src/tools/compiletest/src/bin/main.rs

@@ -1,23 +1,3 @@
-use std::env;
-use std::io::IsTerminal;
-use std::sync::Arc;
-
-use compiletest::{early_config_check, parse_config, run_tests};
-
 fn main() {
-    tracing_subscriber::fmt::init();
-
-    // colored checks stdout by default, but for some reason only stderr is a terminal.
-    // compiletest *does* print many things to stdout, but it doesn't really matter.
-    if std::io::stderr().is_terminal()
-        && matches!(std::env::var("NO_COLOR").as_deref(), Err(_) | Ok("0"))
-    {
-        colored::control::set_override(true);
-    }
-
-    let config = Arc::new(parse_config(env::args().collect()));
-
-    early_config_check(&config);
-
-    run_tests(config);
+    compiletest::cli::main();
 }

src/tools/compiletest/src/cli.rs

@@ -0,0 +1,26 @@
+//! Isolates the APIs used by `bin/main.rs`, to help minimize the surface area
+//! of public exports from the compiletest library crate.
+
+use std::env;
+use std::io::IsTerminal;
+use std::sync::Arc;
+
+use crate::{early_config_check, parse_config, run_tests};
+
+pub fn main() {
+    tracing_subscriber::fmt::init();
+
+    // colored checks stdout by default, but for some reason only stderr is a terminal.
+    // compiletest *does* print many things to stdout, but it doesn't really matter.
+    if std::io::stderr().is_terminal()
+        && matches!(std::env::var("NO_COLOR").as_deref(), Err(_) | Ok("0"))
+    {
+        colored::control::set_override(true);
+    }
+
+    let config = Arc::new(parse_config(env::args().collect()));
+
+    early_config_check(&config);
+
+    run_tests(config);
+}

src/tools/compiletest/src/common.rs

@@ -631,8 +631,6 @@ pub struct Config {
 
     /// Path to a NodeJS executable. Used for JS doctests, emscripten and WASM tests.
     pub nodejs: Option<String>,
-    /// Path to a npm executable. Used for rustdoc GUI tests.
-    pub npm: Option<String>,
 
     /// Whether to rerun tests even if the inputs are unchanged.
     pub force_rerun: bool,
@@ -686,110 +684,6 @@ pub struct Config {
 }
 
 impl Config {
-    /// Incomplete config intended for `src/tools/rustdoc-gui-test` **only** as
-    /// `src/tools/rustdoc-gui-test` wants to reuse `compiletest`'s directive -> test property
-    /// handling for `//@ {compile,run}-flags`, do not use for any other purpose.
-    ///
-    /// FIXME(#143827): this setup feels very hacky. It so happens that `tests/rustdoc-gui/`
-    /// **only** uses `//@ {compile,run}-flags` for now and not any directives that actually rely on
-    /// info that is assumed available in a fully populated [`Config`].
-    pub fn incomplete_for_rustdoc_gui_test() -> Config {
-        // FIXME(#143827): spelling this out intentionally, because this is questionable.
-        //
-        // For instance, `//@ ignore-stage1` will not work at all.
-        Config {
-            mode: TestMode::Rustdoc,
-            // E.g. this has no sensible default tbh.
-            suite: TestSuite::Ui,
-
-            // Dummy values.
-            edition: Default::default(),
-            bless: Default::default(),
-            fail_fast: Default::default(),
-            compile_lib_path: Utf8PathBuf::default(),
-            run_lib_path: Utf8PathBuf::default(),
-            rustc_path: Utf8PathBuf::default(),
-            cargo_path: Default::default(),
-            stage0_rustc_path: Default::default(),
-            query_rustc_path: Default::default(),
-            rustdoc_path: Default::default(),
-            coverage_dump_path: Default::default(),
-            python: Default::default(),
-            jsondocck_path: Default::default(),
-            jsondoclint_path: Default::default(),
-            llvm_filecheck: Default::default(),
-            llvm_bin_dir: Default::default(),
-            run_clang_based_tests_with: Default::default(),
-            src_root: Utf8PathBuf::default(),
-            src_test_suite_root: Utf8PathBuf::default(),
-            build_root: Utf8PathBuf::default(),
-            build_test_suite_root: Utf8PathBuf::default(),
-            sysroot_base: Utf8PathBuf::default(),
-            stage: Default::default(),
-            stage_id: String::default(),
-            debugger: Default::default(),
-            run_ignored: Default::default(),
-            with_rustc_debug_assertions: Default::default(),
-            with_std_debug_assertions: Default::default(),
-            filters: Default::default(),
-            skip: Default::default(),
-            filter_exact: Default::default(),
-            force_pass_mode: Default::default(),
-            run: Default::default(),
-            runner: Default::default(),
-            host_rustcflags: Default::default(),
-            target_rustcflags: Default::default(),
-            rust_randomized_layout: Default::default(),
-            optimize_tests: Default::default(),
-            target: Default::default(),
-            host: Default::default(),
-            cdb: Default::default(),
-            cdb_version: Default::default(),
-            gdb: Default::default(),
-            gdb_version: Default::default(),
-            lldb_version: Default::default(),
-            llvm_version: Default::default(),
-            system_llvm: Default::default(),
-            android_cross_path: Default::default(),
-            adb_path: Default::default(),
-            adb_test_dir: Default::default(),
-            adb_device_status: Default::default(),
-            lldb_python_dir: Default::default(),
-            verbose: Default::default(),
-            color: Default::default(),
-            remote_test_client: Default::default(),
-            compare_mode: Default::default(),
-            rustfix_coverage: Default::default(),
-            has_html_tidy: Default::default(),
-            has_enzyme: Default::default(),
-            channel: Default::default(),
-            git_hash: Default::default(),
-            cc: Default::default(),
-            cxx: Default::default(),
-            cflags: Default::default(),
-            cxxflags: Default::default(),
-            ar: Default::default(),
-            target_linker: Default::default(),
-            host_linker: Default::default(),
-            llvm_components: Default::default(),
-            nodejs: Default::default(),
-            npm: Default::default(),
-            force_rerun: Default::default(),
-            only_modified: Default::default(),
-            target_cfgs: Default::default(),
-            builtin_cfg_names: Default::default(),
-            supported_crate_types: Default::default(),
-            nocapture: Default::default(),
-            nightly_branch: Default::default(),
-            git_merge_commit_email: Default::default(),
-            profiler_runtime: Default::default(),
-            diff_command: Default::default(),
-            minicore_path: Default::default(),
-            default_codegen_backend: CodegenBackend::Llvm,
-            override_codegen_backend: None,
-        }
-    }
-
     /// FIXME: this run scheme is... confusing.
     pub fn run_enabled(&self) -> bool {
         self.run.unwrap_or_else(|| {
@@ -825,7 +719,8 @@ impl Config {
         self.target_cfg().abi == abi
     }
 
-    pub fn matches_family(&self, family: &str) -> bool {
+    #[cfg_attr(not(test), expect(dead_code, reason = "only used by tests for `ignore-{family}`"))]
+    pub(crate) fn matches_family(&self, family: &str) -> bool {
         self.target_cfg().families.iter().any(|f| f == family)
     }
 

src/tools/compiletest/src/directives.rs

@@ -198,8 +198,6 @@ pub struct TestProps {
     pub filecheck_flags: Vec<String>,
     /// Don't automatically insert any `--check-cfg` args
     pub no_auto_check_cfg: bool,
-    /// Run tests which require enzyme being build
-    pub has_enzyme: bool,
     /// Build and use `minicore` as `core` stub for `no_core` tests in cross-compilation scenarios
     /// that don't otherwise want/need `-Z build-std`.
     pub add_core_stubs: bool,
@@ -314,7 +312,6 @@ impl TestProps {
             llvm_cov_flags: vec![],
             filecheck_flags: vec![],
             no_auto_check_cfg: false,
-            has_enzyme: false,
             add_core_stubs: false,
             core_stubs_compile_flags: vec![],
             dont_require_annotations: Default::default(),

src/tools/compiletest/src/lib.rs

@@ -3,20 +3,22 @@
 #[cfg(test)]
 mod tests;
 
-pub mod common;
+pub mod cli;
+mod common;
 mod debuggers;
-pub mod diagnostics;
-pub mod directives;
-pub mod edition;
-pub mod errors;
+mod diagnostics;
+mod directives;
+mod edition;
+mod errors;
 mod executor;
 mod json;
 mod output_capture;
 mod panic_hook;
 mod raise_fd_limit;
 mod read2;
-pub mod runtest;
-pub mod util;
+mod runtest;
+pub mod rustdoc_gui_test;
+mod util;
 
 use core::panic;
 use std::collections::HashSet;
@@ -48,7 +50,7 @@ use crate::executor::{CollectedTest, ColorConfig};
 /// The config mostly reflects command-line arguments, but there might also be
 /// some code here that inspects environment variables or even runs executables
 /// (e.g. when discovering debugger versions).
-pub fn parse_config(args: Vec<String>) -> Config {
+fn parse_config(args: Vec<String>) -> Config {
     let mut opts = Options::new();
     opts.reqopt("", "compile-lib-path", "path to host shared libraries", "PATH")
         .reqopt("", "run-lib-path", "path to target shared libraries", "PATH")
@@ -462,7 +464,6 @@ pub fn parse_config(args: Vec<String>) -> Config {
         host_linker: matches.opt_str("host-linker"),
         llvm_components: matches.opt_str("llvm-components").unwrap(),
         nodejs: matches.opt_str("nodejs"),
-        npm: matches.opt_str("npm"),
 
         force_rerun: matches.opt_present("force-rerun"),
 
@@ -486,22 +487,15 @@ pub fn parse_config(args: Vec<String>) -> Config {
     }
 }
 
-pub fn opt_str(maybestr: &Option<String>) -> &str {
-    match *maybestr {
-        None => "(none)",
-        Some(ref s) => s,
-    }
-}
-
-pub fn opt_str2(maybestr: Option<String>) -> String {
+fn opt_str2(maybestr: Option<String>) -> String {
     match maybestr {
         None => "(none)".to_owned(),
         Some(s) => s,
     }
 }
 
 /// Called by `main` after the config has been parsed.
-pub fn run_tests(config: Arc<Config>) {
+fn run_tests(config: Arc<Config>) {
     debug!(?config, "run_tests");
 
     panic_hook::install_panic_hook();
@@ -639,7 +633,7 @@ impl TestCollector {
 /// FIXME(Zalathar): Now that we no longer rely on libtest, try to overhaul
 /// test discovery to take into account the filters/tests specified on the
 /// command-line, instead of having to enumerate everything.
-pub(crate) fn collect_and_make_tests(config: Arc<Config>) -> Vec<CollectedTest> {
+fn collect_and_make_tests(config: Arc<Config>) -> Vec<CollectedTest> {
     debug!("making tests from {}", config.src_test_suite_root);
     let common_inputs_stamp = common_inputs_stamp(&config);
     let modified_tests =
@@ -851,7 +845,7 @@ fn collect_tests_from_dir(
 }
 
 /// Returns true if `file_name` looks like a proper test file name.
-pub fn is_test(file_name: &str) -> bool {
+fn is_test(file_name: &str) -> bool {
     if !file_name.ends_with(".rs") {
         return false;
     }
@@ -1131,7 +1125,7 @@ fn check_for_overlapping_test_paths(found_path_stems: &HashSet<Utf8PathBuf>) {
     }
 }
 
-pub fn early_config_check(config: &Config) {
+fn early_config_check(config: &Config) {
     if !config.has_html_tidy && config.mode == TestMode::Rustdoc {
         warning!("`tidy` (html-tidy.org) is not installed; diffs will not be generated");
     }