Rollup merge of #122840 - GuillaumeGomez:rustdoc-test-too-many-args, r=notriddle,Urgau,jieyouxu

`rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many Fixes #122722. Thanks to this I discovered that rust was using ``@`` to add arguments from a file, quite convenient. If there are too many `cfg` arguments given to `rustdoc --test`, it'll now put them into a temporary file and passing it as argument to the rustc command. I added a test with 100_000 `cfg` arguments to ensure it'll not break again. r? `@notrid`
2024-03-24 01:05:53 +01:00 · 2024-03-24 01:05:53 +01:00 · 73038c0874
commit 73038c0874
parent 2dcc96883e bc4f1697fa
6 changed files with 211 additions and 79 deletions
--- a/src/librustdoc/doctest.rs
+++ b/src/librustdoc/doctest.rs
@ -20,9 +20,9 @@ use rustc_span::source_map::SourceMap;
 use rustc_span::symbol::sym;
 use rustc_span::{BytePos, FileName, Pos, Span, DUMMY_SP};
 use rustc_target::spec::{Target, TargetTriple};
-use tempfile::Builder as TempFileBuilder;

 use std::env;
+use std::fs::File;
 use std::io::{self, Write};
 use std::panic;
 use std::path::{Path, PathBuf};
@ -31,6 +31,8 @@ use std::str;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};

+use tempfile::{Builder as TempFileBuilder, TempDir};
+
 use crate::clean::{types::AttributesExt, Attributes};
 use crate::config::Options as RustdocOptions;
 use crate::html::markdown::{self, ErrorCodes, Ignore, LangString};
@ -48,7 +50,55 @@ pub(crate) struct GlobalTestOptions {
    pub(crate) attrs: Vec<String>,
 }

-pub(crate) fn run(options: RustdocOptions) -> Result<(), ErrorGuaranteed> {
+pub(crate) fn generate_args_file(file_path: &Path, options: &RustdocOptions) -> Result<(), String> {
+    let mut file = File::create(file_path)
+        .map_err(|error| format!("failed to create args file: {error:?}"))?;
+
+    // We now put the common arguments into the file we created.
+    let mut content = vec!["--crate-type=bin".to_string()];
+
+    for cfg in &options.cfgs {
+        content.push(format!("--cfg={cfg}"));
+    }
+    if !options.check_cfgs.is_empty() {
+        content.push("-Zunstable-options".to_string());
+        for check_cfg in &options.check_cfgs {
+            content.push(format!("--check-cfg={check_cfg}"));
+        }
+    }
+
+    if let Some(sysroot) = &options.maybe_sysroot {
+        content.push(format!("--sysroot={}", sysroot.display()));
+    }
+    for lib_str in &options.lib_strs {
+        content.push(format!("-L{lib_str}"));
+    }
+    for extern_str in &options.extern_strs {
+        content.push(format!("--extern={extern_str}"));
+    }
+    content.push("-Ccodegen-units=1".to_string());
+    for codegen_options_str in &options.codegen_options_strs {
+        content.push(format!("-C{codegen_options_str}"));
+    }
+    for unstable_option_str in &options.unstable_opts_strs {
+        content.push(format!("-Z{unstable_option_str}"));
+    }
+
+    let content = content.join("\n");
+
+    file.write(content.as_bytes())
+        .map_err(|error| format!("failed to write arguments to temporary file: {error:?}"))?;
+    Ok(())
+}
+
+fn get_doctest_dir() -> io::Result<TempDir> {
+    TempFileBuilder::new().prefix("rustdoctest").tempdir()
+}
+
+pub(crate) fn run(
+    dcx: &rustc_errors::DiagCtxt,
+    options: RustdocOptions,
+) -> Result<(), ErrorGuaranteed> {
    let input = config::Input::File(options.input.clone());

    let invalid_codeblock_attributes_name = crate::lint::INVALID_CODEBLOCK_ATTRIBUTES.name;
@ -118,6 +168,15 @@ pub(crate) fn run(options: RustdocOptions) -> Result<(), ErrorGuaranteed> {
    let externs = options.externs.clone();
    let json_unused_externs = options.json_unused_externs;

+    let temp_dir = match get_doctest_dir()
+        .map_err(|error| format!("failed to create temporary directory: {error:?}"))
+    {
+        Ok(temp_dir) => temp_dir,
+        Err(error) => return crate::wrap_return(dcx, Err(error)),
+    };
+    let file_path = temp_dir.path().join("rustdoc-cfgs");
+    crate::wrap_return(dcx, generate_args_file(&file_path, &options))?;
+
    let (tests, unused_extern_reports, compiling_test_count) =
        interface::run_compiler(config, |compiler| {
            compiler.enter(|queries| {
@ -134,6 +193,7 @@ pub(crate) fn run(options: RustdocOptions) -> Result<(), ErrorGuaranteed> {
                        Some(compiler.sess.psess.clone_source_map()),
                        None,
                        enable_per_target_ignores,
+                        file_path,
                    );

                    let mut hir_collector = HirCollector {
@ -322,44 +382,35 @@ fn run_test(
    test: &str,
    crate_name: &str,
    line: usize,
-    rustdoc_options: RustdocOptions,
+    rustdoc_options: IndividualTestOptions,
    mut lang_string: LangString,
    no_run: bool,
-    runtool: Option<String>,
-    runtool_args: Vec<String>,
-    target: TargetTriple,
    opts: &GlobalTestOptions,
    edition: Edition,
-    outdir: DirState,
    path: PathBuf,
-    test_id: &str,
    report_unused_externs: impl Fn(UnusedExterns),
 ) -> Result<(), TestFailure> {
-    let (test, line_offset, supports_color) =
-        make_test(test, Some(crate_name), lang_string.test_harness, opts, edition, Some(test_id));
+    let (test, line_offset, supports_color) = make_test(
+        test,
+        Some(crate_name),
+        lang_string.test_harness,
+        opts,
+        edition,
+        Some(&rustdoc_options.test_id),
+    );

    // Make sure we emit well-formed executable names for our target.
-    let rust_out = add_exe_suffix("rust_out".to_owned(), &target);
-    let output_file = outdir.path().join(rust_out);
+    let rust_out = add_exe_suffix("rust_out".to_owned(), &rustdoc_options.target);
+    let output_file = rustdoc_options.outdir.path().join(rust_out);

    let rustc_binary = rustdoc_options
        .test_builder
        .as_deref()
        .unwrap_or_else(|| rustc_interface::util::rustc_path().expect("found rustc"));
    let mut compiler = wrapped_rustc_command(&rustdoc_options.test_builder_wrappers, rustc_binary);
-    compiler.arg("--crate-type").arg("bin");
-    for cfg in &rustdoc_options.cfgs {
-        compiler.arg("--cfg").arg(&cfg);
-    }
-    if !rustdoc_options.check_cfgs.is_empty() {
-        compiler.arg("-Z").arg("unstable-options");
-        for check_cfg in &rustdoc_options.check_cfgs {
-            compiler.arg("--check-cfg").arg(&check_cfg);
-        }
-    }
-    if let Some(sysroot) = rustdoc_options.maybe_sysroot {
-        compiler.arg("--sysroot").arg(sysroot);
-    }
+
+    compiler.arg(&format!("@{}", rustdoc_options.arg_file.display()));
+
    compiler.arg("--edition").arg(&edition.to_string());
    compiler.env("UNSTABLE_RUSTDOC_TEST_PATH", path);
    compiler.env("UNSTABLE_RUSTDOC_TEST_LINE", format!("{}", line as isize - line_offset as isize));
@ -367,29 +418,17 @@ fn run_test(
    if lang_string.test_harness {
        compiler.arg("--test");
    }
-    if rustdoc_options.json_unused_externs.is_enabled() && !lang_string.compile_fail {
+    if rustdoc_options.is_json_unused_externs_enabled && !lang_string.compile_fail {
        compiler.arg("--error-format=json");
        compiler.arg("--json").arg("unused-externs");
-        compiler.arg("-Z").arg("unstable-options");
        compiler.arg("-W").arg("unused_crate_dependencies");
+        compiler.arg("-Z").arg("unstable-options");
    }
-    for lib_str in &rustdoc_options.lib_strs {
-        compiler.arg("-L").arg(&lib_str);
-    }
-    for extern_str in &rustdoc_options.extern_strs {
-        compiler.arg("--extern").arg(&extern_str);
-    }
-    compiler.arg("-Ccodegen-units=1");
-    for codegen_options_str in &rustdoc_options.codegen_options_strs {
-        compiler.arg("-C").arg(&codegen_options_str);
-    }
-    for unstable_option_str in &rustdoc_options.unstable_opts_strs {
-        compiler.arg("-Z").arg(&unstable_option_str);
-    }
-    if no_run && !lang_string.compile_fail && rustdoc_options.persist_doctests.is_none() {
+
+    if no_run && !lang_string.compile_fail && rustdoc_options.should_persist_doctests {
        compiler.arg("--emit=metadata");
    }
-    compiler.arg("--target").arg(match target {
+    compiler.arg("--target").arg(match rustdoc_options.target {
        TargetTriple::TargetTriple(s) => s,
        TargetTriple::TargetJson { path_for_rustdoc, .. } => {
            path_for_rustdoc.to_str().expect("target path must be valid unicode").to_string()
@ -485,10 +524,10 @@ fn run_test(
    let mut cmd;

    let output_file = make_maybe_absolute_path(output_file);
-    if let Some(tool) = runtool {
+    if let Some(tool) = rustdoc_options.runtool {
        let tool = make_maybe_absolute_path(tool.into());
        cmd = Command::new(tool);
-        cmd.args(runtool_args);
+        cmd.args(rustdoc_options.runtool_args);
        cmd.arg(output_file);
    } else {
        cmd = Command::new(output_file);
@ -897,6 +936,56 @@ fn partition_source(s: &str, edition: Edition) -> (String, String, String) {
    (before, after, crates)
 }

+pub(crate) struct IndividualTestOptions {
+    test_builder: Option<PathBuf>,
+    test_builder_wrappers: Vec<PathBuf>,
+    is_json_unused_externs_enabled: bool,
+    should_persist_doctests: bool,
+    error_format: ErrorOutputType,
+    test_run_directory: Option<PathBuf>,
+    nocapture: bool,
+    arg_file: PathBuf,
+    outdir: DirState,
+    runtool: Option<String>,
+    runtool_args: Vec<String>,
+    target: TargetTriple,
+    test_id: String,
+}
+
+impl IndividualTestOptions {
+    fn new(options: &RustdocOptions, arg_file: &Path, test_id: String) -> Self {
+        let outdir = if let Some(ref path) = options.persist_doctests {
+            let mut path = path.clone();
+            path.push(&test_id);
+
+            if let Err(err) = std::fs::create_dir_all(&path) {
+                eprintln!("Couldn't create directory for doctest executables: {err}");
+                panic::resume_unwind(Box::new(()));
+            }
+
+            DirState::Perm(path)
+        } else {
+            DirState::Temp(get_doctest_dir().expect("rustdoc needs a tempdir"))
+        };
+
+        Self {
+            test_builder: options.test_builder.clone(),
+            test_builder_wrappers: options.test_builder_wrappers.clone(),
+            is_json_unused_externs_enabled: options.json_unused_externs.is_enabled(),
+            should_persist_doctests: options.persist_doctests.is_none(),
+            error_format: options.error_format,
+            test_run_directory: options.test_run_directory.clone(),
+            nocapture: options.nocapture,
+            arg_file: arg_file.into(),
+            outdir,
+            runtool: options.runtool.clone(),
+            runtool_args: options.runtool_args.clone(),
+            target: options.target.clone(),
+            test_id,
+        }
+    }
+}
+
 pub(crate) trait Tester {
    fn add_test(&mut self, test: String, config: LangString, line: usize);
    fn get_line(&self) -> usize {
@ -941,6 +1030,7 @@ pub(crate) struct Collector {
    visited_tests: FxHashMap<(String, usize), usize>,
    unused_extern_reports: Arc<Mutex<Vec<UnusedExterns>>>,
    compiling_test_count: AtomicUsize,
+    arg_file: PathBuf,
 }

 impl Collector {
@ -952,6 +1042,7 @@ impl Collector {
        source_map: Option<Lrc<SourceMap>>,
        filename: Option<PathBuf>,
        enable_per_target_ignores: bool,
+        arg_file: PathBuf,
    ) -> Collector {
        Collector {
            tests: Vec::new(),
@ -967,6 +1058,7 @@ impl Collector {
            visited_tests: FxHashMap::default(),
            unused_extern_reports: Default::default(),
            compiling_test_count: AtomicUsize::new(0),
+            arg_file,
        }
    }

@ -1009,13 +1101,9 @@ impl Tester for Collector {
        let crate_name = self.crate_name.clone();
        let opts = self.opts.clone();
        let edition = config.edition.unwrap_or(self.rustdoc_options.edition);
-        let rustdoc_options = self.rustdoc_options.clone();
-        let runtool = self.rustdoc_options.runtool.clone();
-        let runtool_args = self.rustdoc_options.runtool_args.clone();
-        let target = self.rustdoc_options.target.clone();
-        let target_str = target.to_string();
+        let target_str = self.rustdoc_options.target.to_string();
        let unused_externs = self.unused_extern_reports.clone();
-        let no_run = config.no_run || rustdoc_options.no_run;
+        let no_run = config.no_run || self.rustdoc_options.no_run;
        if !config.compile_fail {
            self.compiling_test_count.fetch_add(1, Ordering::SeqCst);
        }
@ -1049,23 +1137,9 @@ impl Tester for Collector {
                self.visited_tests.entry((file.clone(), line)).and_modify(|v| *v += 1).or_insert(0)
            },
        );
-        let outdir = if let Some(mut path) = rustdoc_options.persist_doctests.clone() {
-            path.push(&test_id);

-            if let Err(err) = std::fs::create_dir_all(&path) {
-                eprintln!("Couldn't create directory for doctest executables: {err}");
-                panic::resume_unwind(Box::new(()));
-            }
-
-            DirState::Perm(path)
-        } else {
-            DirState::Temp(
-                TempFileBuilder::new()
-                    .prefix("rustdoctest")
-                    .tempdir()
-                    .expect("rustdoc needs a tempdir"),
-            )
-        };
+        let rustdoc_test_options =
+            IndividualTestOptions::new(&self.rustdoc_options, &self.arg_file, test_id);

        debug!("creating test {name}: {test}");
        self.tests.push(test::TestDescAndFn {
@ -1096,17 +1170,12 @@ impl Tester for Collector {
                    &test,
                    &crate_name,
                    line,
-                    rustdoc_options,
+                    rustdoc_test_options,
                    config,
                    no_run,
-                    runtool,
-                    runtool_args,
-                    target,
                    &opts,
                    edition,
-                    outdir,
                    path,
-                    &test_id,
                    report_unused_externs,
                );

--- a/src/librustdoc/lib.rs
+++ b/src/librustdoc/lib.rs
@ -664,7 +664,7 @@ fn usage(argv0: &str) {
 /// A result type used by several functions under `main()`.
 type MainResult = Result<(), ErrorGuaranteed>;

-fn wrap_return(dcx: &rustc_errors::DiagCtxt, res: Result<(), String>) -> MainResult {
+pub(crate) fn wrap_return(dcx: &rustc_errors::DiagCtxt, res: Result<(), String>) -> MainResult {
    match res {
        Ok(()) => dcx.has_errors().map_or(Ok(()), Err),
        Err(err) => Err(dcx.err(err)),
@ -731,7 +731,7 @@ fn main_args(

    match (options.should_test, options.markdown_input()) {
        (true, true) => return wrap_return(&diag, markdown::test(options)),
-        (true, false) => return doctest::run(options),
+        (true, false) => return doctest::run(&diag, options),
        (false, true) => {
            let input = options.input.clone();
            let edition = options.edition;
--- a/src/librustdoc/markdown.rs
+++ b/src/librustdoc/markdown.rs
@ -3,11 +3,13 @@ use std::fs::{create_dir_all, read_to_string, File};
 use std::io::prelude::*;
 use std::path::Path;

+use tempfile::tempdir;
+
 use rustc_span::edition::Edition;
 use rustc_span::DUMMY_SP;

 use crate::config::{Options, RenderOptions};
-use crate::doctest::{Collector, GlobalTestOptions};
+use crate::doctest::{generate_args_file, Collector, GlobalTestOptions};
 use crate::html::escape::Escape;
 use crate::html::markdown;
 use crate::html::markdown::{
@ -146,6 +148,12 @@ pub(crate) fn test(options: Options) -> Result<(), String> {
        .map_err(|err| format!("{input}: {err}", input = options.input.display()))?;
    let mut opts = GlobalTestOptions::default();
    opts.no_crate_inject = true;
+
+    let temp_dir =
+        tempdir().map_err(|error| format!("failed to create temporary directory: {error:?}"))?;
+    let file_path = temp_dir.path().join("rustdoc-cfgs");
+    generate_args_file(&file_path, &options)?;
+
    let mut collector = Collector::new(
        options.input.display().to_string(),
        options.clone(),
@ -154,6 +162,7 @@ pub(crate) fn test(options: Options) -> Result<(), String> {
        None,
        Some(options.input),
        options.enable_per_target_ignores,
+        file_path,
    );
    collector.set_position(DUMMY_SP);
    let codes = ErrorCodes::from(options.unstable_features.is_nightly_build());
--- a/src/tools/run-make-support/src/lib.rs
+++ b/src/tools/run-make-support/src/lib.rs
@ -9,8 +9,8 @@ pub fn out_dir() -> PathBuf {
    env::var_os("TMPDIR").unwrap().into()
 }

-fn setup_common_build_cmd() -> Command {
-    let rustc = env::var("RUSTC").unwrap();
+fn setup_common_build_cmd(command: &str) -> Command {
+    let rustc = env::var(command).unwrap();
    let mut cmd = Command::new(rustc);
    cmd.arg("--out-dir").arg(out_dir()).arg("-L").arg(out_dir());
    cmd
@ -33,6 +33,10 @@ pub fn aux_build() -> AuxBuildInvocationBuilder {
    AuxBuildInvocationBuilder::new()
 }

+pub fn rustdoc() -> Rustdoc {
+    Rustdoc::new()
+}
+
 #[derive(Debug)]
 pub struct RustcInvocationBuilder {
    cmd: Command,
@ -40,7 +44,7 @@ pub struct RustcInvocationBuilder {

 impl RustcInvocationBuilder {
    fn new() -> Self {
-        let cmd = setup_common_build_cmd();
+        let cmd = setup_common_build_cmd("RUSTC");
        Self { cmd }
    }

@ -74,7 +78,7 @@ pub struct AuxBuildInvocationBuilder {

 impl AuxBuildInvocationBuilder {
    fn new() -> Self {
-        let mut cmd = setup_common_build_cmd();
+        let mut cmd = setup_common_build_cmd("RUSTC");
        cmd.arg("--crate-type=lib");
        Self { cmd }
    }
@ -97,6 +101,35 @@ impl AuxBuildInvocationBuilder {
    }
 }

+#[derive(Debug)]
+pub struct Rustdoc {
+    cmd: Command,
+}
+
+impl Rustdoc {
+    fn new() -> Self {
+        let cmd = setup_common_build_cmd("RUSTDOC");
+        Self { cmd }
+    }
+
+    pub fn arg(&mut self, arg: &str) -> &mut Self {
+        self.cmd.arg(arg);
+        self
+    }
+
+    #[track_caller]
+    pub fn run(&mut self) -> Output {
+        let caller_location = std::panic::Location::caller();
+        let caller_line_number = caller_location.line();
+
+        let output = self.cmd.output().unwrap();
+        if !output.status.success() {
+            handle_failed_output(&format!("{:#?}", self.cmd), output, caller_line_number);
+        }
+        output
+    }
+}
+
 fn run_common(bin_name: &str) -> (Command, Output) {
    let target = env::var("TARGET").unwrap();

--- a/tests/run-make/rustdoc-test-args/foo.rs
+++ b/tests/run-make/rustdoc-test-args/foo.rs
@ -0,0 +1,3 @@
+//! ```
+//! let x = 12;
+//! ```
--- a/tests/run-make/rustdoc-test-args/rmake.rs
+++ b/tests/run-make/rustdoc-test-args/rmake.rs
@ -0,0 +1,18 @@
+extern crate run_make_support;
+
+use run_make_support::{out_dir, rustdoc};
+use std::{fs, iter};
+use std::path::Path;
+
+fn generate_a_lot_of_cfgs(path: &Path) {
+    let content = iter::repeat("--cfg=a\n").take(100_000).collect::<String>();
+    fs::write(path, content.as_bytes()).expect("failed to create args file");
+}
+
+fn main() {
+    let arg_file = out_dir().join("args");
+    generate_a_lot_of_cfgs(&arg_file);
+
+    let arg_file = format!("@{}", arg_file.display());
+    rustdoc().arg("--test").arg(&arg_file).arg("foo.rs").run();
+}