Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doctests: fix merging on stable #137899

Merged
merged 2 commits into from
Mar 10, 2025
Merged

doctests: fix merging on stable #137899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d6eeea
doctests: fix merging on stable
notriddle Mar 2, 2025
9cf531d
doctests: build test bundle and harness separately
notriddle Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 125 additions & 45 deletions src/librustdoc/doctest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ pub(crate) fn generate_args_file(file_path: &Path, options: &RustdocOptions) ->
.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()];
let mut content = vec![];

for cfg in &options.cfgs {
content.push(format!("--cfg={cfg}"));
Expand Down Expand Up @@ -513,12 +513,18 @@ pub(crate) struct RunnableDocTest {
line: usize,
edition: Edition,
no_run: bool,
is_multiple_tests: bool,
merged_test_code: Option<String>,
}

impl RunnableDocTest {
fn path_for_merged_doctest(&self) -> PathBuf {
self.test_opts.outdir.path().join(format!("doctest_{}.rs", self.edition))
fn path_for_merged_doctest_bundle(&self) -> PathBuf {
self.test_opts.outdir.path().join(format!("doctest_bundle_{}.rs", self.edition))
}
fn path_for_merged_doctest_runner(&self) -> PathBuf {
self.test_opts.outdir.path().join(format!("doctest_runner_{}.rs", self.edition))
}
Comment on lines +523 to +525
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(definitely not blocking) With some work it might be possible to generate a single doctest_runner.rs for all editions unless I'm missing something glaringly obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically, yes, but that would require a bunch of refactoring to generate the runner, and doesn't seem worth it for a niche feature like writing doctests with a different edition than the crate.

fn is_multiple_tests(&self) -> bool {
self.merged_test_code.is_some()
}
}

Expand All @@ -537,91 +543,108 @@ fn run_test(
let rust_out = add_exe_suffix("rust_out".to_owned(), &rustdoc_options.target);
let output_file = doctest.test_opts.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);
// Common arguments used for compiling the doctest runner.
// On merged doctests, the compiler is invoked twice: once for the test code itself,
// and once for the runner wrapper (which needs to use `#![feature]` on stable).
let mut compiler_args = vec![];

compiler.arg(format!("@{}", doctest.global_opts.args_file.display()));
compiler_args.push(format!("@{}", doctest.global_opts.args_file.display()));

if let Some(sysroot) = &rustdoc_options.maybe_sysroot {
compiler.arg(format!("--sysroot={}", sysroot.display()));
compiler_args.push(format!("--sysroot={}", sysroot.display()));
}

compiler.arg("--edition").arg(doctest.edition.to_string());
if !doctest.is_multiple_tests {
// Setting these environment variables is unneeded if this is a merged doctest.
compiler.env("UNSTABLE_RUSTDOC_TEST_PATH", &doctest.test_opts.path);
compiler.env(
"UNSTABLE_RUSTDOC_TEST_LINE",
format!("{}", doctest.line as isize - doctest.full_test_line_offset as isize),
);
}
compiler.arg("-o").arg(&output_file);
compiler_args.extend_from_slice(&["--edition".to_owned(), doctest.edition.to_string()]);
if langstr.test_harness {
compiler.arg("--test");
compiler_args.push("--test".to_owned());
}
if rustdoc_options.json_unused_externs.is_enabled() && !langstr.compile_fail {
compiler.arg("--error-format=json");
compiler.arg("--json").arg("unused-externs");
compiler.arg("-W").arg("unused_crate_dependencies");
compiler.arg("-Z").arg("unstable-options");
compiler_args.push("--error-format=json".to_owned());
compiler_args.extend_from_slice(&["--json".to_owned(), "unused-externs".to_owned()]);
compiler_args.extend_from_slice(&["-W".to_owned(), "unused_crate_dependencies".to_owned()]);
compiler_args.extend_from_slice(&["-Z".to_owned(), "unstable-options".to_owned()]);
}

if doctest.no_run && !langstr.compile_fail && rustdoc_options.persist_doctests.is_none() {
// FIXME: why does this code check if it *shouldn't* persist doctests
// -- shouldn't it be the negation?
compiler.arg("--emit=metadata");
compiler_args.push("--emit=metadata".to_owned());
}
compiler.arg("--target").arg(match &rustdoc_options.target {
TargetTuple::TargetTuple(s) => s,
TargetTuple::TargetJson { path_for_rustdoc, .. } => {
path_for_rustdoc.to_str().expect("target path must be valid unicode")
}
});
compiler_args.extend_from_slice(&[
"--target".to_owned(),
match &rustdoc_options.target {
TargetTuple::TargetTuple(s) => s.clone(),
TargetTuple::TargetJson { path_for_rustdoc, .. } => {
path_for_rustdoc.to_str().expect("target path must be valid unicode").to_owned()
}
},
]);
if let ErrorOutputType::HumanReadable { kind, color_config } = rustdoc_options.error_format {
let short = kind.short();
let unicode = kind == HumanReadableErrorType::Unicode;

if short {
compiler.arg("--error-format").arg("short");
compiler_args.extend_from_slice(&["--error-format".to_owned(), "short".to_owned()]);
}
if unicode {
compiler.arg("--error-format").arg("human-unicode");
compiler_args
.extend_from_slice(&["--error-format".to_owned(), "human-unicode".to_owned()]);
}

match color_config {
ColorConfig::Never => {
compiler.arg("--color").arg("never");
compiler_args.extend_from_slice(&["--color".to_owned(), "never".to_owned()]);
}
ColorConfig::Always => {
compiler.arg("--color").arg("always");
compiler_args.extend_from_slice(&["--color".to_owned(), "always".to_owned()]);
}
ColorConfig::Auto => {
compiler.arg("--color").arg(if supports_color { "always" } else { "never" });
compiler_args.extend_from_slice(&[
"--color".to_owned(),
if supports_color { "always" } else { "never" }.to_owned(),
]);
}
}
}

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.args(&compiler_args);

// If this is a merged doctest, we need to write it into a file instead of using stdin
// because if the size of the merged doctests is too big, it'll simply break stdin.
if doctest.is_multiple_tests {
if doctest.is_multiple_tests() {
// It makes the compilation failure much faster if it is for a combined doctest.
compiler.arg("--error-format=short");
let input_file = doctest.path_for_merged_doctest();
let input_file = doctest.path_for_merged_doctest_bundle();
if std::fs::write(&input_file, &doctest.full_test_code).is_err() {
// If we cannot write this file for any reason, we leave. All combined tests will be
// tested as standalone tests.
return Err(TestFailure::CompileError);
}
compiler.arg(input_file);
if !rustdoc_options.nocapture {
// If `nocapture` is disabled, then we don't display rustc's output when compiling
// the merged doctests.
compiler.stderr(Stdio::null());
}
// bundled tests are an rlib, loaded by a separate runner executable
compiler
.arg("--crate-type=lib")
.arg("--out-dir")
.arg(doctest.test_opts.outdir.path())
.arg(input_file);
} else {
compiler.arg("--crate-type=bin").arg("-o").arg(&output_file);
// Setting these environment variables is unneeded if this is a merged doctest.
compiler.env("UNSTABLE_RUSTDOC_TEST_PATH", &doctest.test_opts.path);
compiler.env(
"UNSTABLE_RUSTDOC_TEST_LINE",
format!("{}", doctest.line as isize - doctest.full_test_line_offset as isize),
);
compiler.arg("-");
compiler.stdin(Stdio::piped());
compiler.stderr(Stdio::piped());
Expand All @@ -630,8 +653,65 @@ fn run_test(
debug!("compiler invocation for doctest: {compiler:?}");

let mut child = compiler.spawn().expect("Failed to spawn rustc process");
let output = if doctest.is_multiple_tests {
let output = if let Some(merged_test_code) = &doctest.merged_test_code {
// compile-fail tests never get merged, so this should always pass
let status = child.wait().expect("Failed to wait");

// the actual test runner is a separate component, built with nightly-only features;
// build it now
let runner_input_file = doctest.path_for_merged_doctest_runner();

let mut runner_compiler =
wrapped_rustc_command(&rustdoc_options.test_builder_wrappers, rustc_binary);
// the test runner does not contain any user-written code, so this doesn't allow
// the user to exploit nightly-only features on stable
runner_compiler.env("RUSTC_BOOTSTRAP", "1");
runner_compiler.args(compiler_args);
runner_compiler.args(&["--crate-type=bin", "-o"]).arg(&output_file);
let mut extern_path = std::ffi::OsString::from(format!(
"--extern=doctest_bundle_{edition}=",
edition = doctest.edition
));
for extern_str in &rustdoc_options.extern_strs {
if let Some((_cratename, path)) = extern_str.split_once('=') {
// Direct dependencies of the tests themselves are
// indirect dependencies of the test runner.
// They need to be in the library search path.
let dir = Path::new(path)
.parent()
.filter(|x| x.components().count() > 0)
.unwrap_or(Path::new("."));
runner_compiler.arg("-L").arg(dir);
}
}
Comment on lines +675 to +686
Copy link
Member

@fmease fmease Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering, is this actually necessary? Did you have a concrete case where this was needed?

Locally I've experimented with the consequences of removing it, couldn't find any problems and all tests pass without it, too. It's possible that I missed something.

Everything seems to work out just fine without this extra logic because any --externs and -Ls (most likely -Ldependency=s from Cargo) passed to rustdoc for the "overarching host crate" (the crate containing the doctest) get stuffed into the args file rustdoc-cfgs which we use for compiling both the bundle and the runner.

And if the (non-Cargo) user / Cargo itself were to pass too few -L flags to the host crate, then the latter wouldn't even compile and we wouldn't even reach this function run_test. Example:

directory/foreign.rs (rustc directory/foreign.rs --crate-type=lib --out-dir directory/):

pub const VALUE: bool = true;

file.rs:

//! ```
//! assert!(foreign::VALUE);
//! ```
  • rustdoc file.rs --edition 2024 --test --extern foreign=directory/libforeign.rlib passes just fine without this loop over extern_strs because the argsfile (shared by the bundle & the runner compiler) contains --extern foreign=directory/libforeign.rlib. This loop would've just added -Ldirectory which is unnecessary.
  • (rustdoc file.rs --edition 2024 --test --extern foreign -Ldependency also passes because the argsfile contains -Ldirectory and --extern=foreign but ofc that isn't relevant to this loop as it skips such externs)
  • if you only want to add foreign to the doctest, not the host crate: rustdoc file.rs --edition 2024 --test -Zunstable-options --doctest-compilation-args '--extern foreign=directory/libforeign.rlib': This also passes just fine because of the argsfile and it obviously doesn't trigger this loop at all

Copy link
Member

@fmease fmease Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly for more complex crate graphs (a.rs <- b.rs <- c.rs(with doctest)):

  1. rustc directory/a.rs --crate-type=lib --out-dir directory
  2. rustc directory/b.rs --crate-type=lib --edition 2018 --extern a -Ldirectory (or similar)
  3. finally either of
    • rustdoc c.rs --edition 2024 --test --extern b=directory/libb.rlib -Ldependency=directory also passes without this loop which would've added -Ldirectory
    • (similarly, rustdoc c.rs --edition 2024 --test -Zunstable-options --doctest-compilation-args '--extern foreign=separate/libforeign.rlib -Ldependency=separate')

let output_bundle_file = doctest
.test_opts
.outdir
.path()
.join(format!("libdoctest_bundle_{edition}.rlib", edition = doctest.edition));
extern_path.push(&output_bundle_file);
runner_compiler.arg(extern_path);
runner_compiler.arg(&runner_input_file);
if std::fs::write(&runner_input_file, &merged_test_code).is_err() {
// If we cannot write this file for any reason, we leave. All combined tests will be
// tested as standalone tests.
return Err(TestFailure::CompileError);
}
if !rustdoc_options.nocapture {
// If `nocapture` is disabled, then we don't display rustc's output when compiling
// the merged doctests.
runner_compiler.stderr(Stdio::null());
}
runner_compiler.arg("--error-format=short");
debug!("compiler invocation for doctest runner: {runner_compiler:?}");

let status = if !status.success() {
status
} else {
let mut child_runner = runner_compiler.spawn().expect("Failed to spawn rustc process");
child_runner.wait().expect("Failed to wait")
};

process::Output { status, stdout: Vec::new(), stderr: Vec::new() }
} else {
let stdin = child.stdin.as_mut().expect("Failed to open stdin");
Expand Down Expand Up @@ -708,15 +788,15 @@ fn run_test(
cmd.arg(&output_file);
} else {
cmd = Command::new(&output_file);
if doctest.is_multiple_tests {
if doctest.is_multiple_tests() {
cmd.env("RUSTDOC_DOCTEST_BIN_PATH", &output_file);
}
}
if let Some(run_directory) = &rustdoc_options.test_run_directory {
cmd.current_dir(run_directory);
}

let result = if doctest.is_multiple_tests || rustdoc_options.nocapture {
let result = if doctest.is_multiple_tests() || rustdoc_options.nocapture {
cmd.status().map(|status| process::Output {
status,
stdout: Vec::new(),
Expand Down Expand Up @@ -1003,7 +1083,7 @@ fn doctest_run_fn(
line: scraped_test.line,
edition: scraped_test.edition(&rustdoc_options),
no_run: scraped_test.no_run(&rustdoc_options),
is_multiple_tests: false,
merged_test_code: None,
};
let res =
run_test(runnable_test, &rustdoc_options, doctest.supports_color, report_unused_externs);
Expand Down
37 changes: 25 additions & 12 deletions src/librustdoc/doctest/runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub(crate) struct DocTestRunner {
crate_attrs: FxIndexSet<String>,
ids: String,
output: String,
output_merged_tests: String,
supports_color: bool,
nb_tests: usize,
}
Expand All @@ -24,6 +25,7 @@ impl DocTestRunner {
crate_attrs: FxIndexSet::default(),
ids: String::new(),
output: String::new(),
output_merged_tests: String::new(),
supports_color: true,
nb_tests: 0,
}
Expand Down Expand Up @@ -55,7 +57,8 @@ impl DocTestRunner {
scraped_test,
ignore,
self.nb_tests,
&mut self.output
&mut self.output,
&mut self.output_merged_tests,
),
));
self.supports_color &= doctest.supports_color;
Expand All @@ -78,25 +81,28 @@ impl DocTestRunner {
"
.to_string();

let mut code_prefix = String::new();

for crate_attr in &self.crate_attrs {
code.push_str(crate_attr);
code.push('\n');
code_prefix.push_str(crate_attr);
code_prefix.push('\n');
}

if opts.attrs.is_empty() {
// If there aren't any attributes supplied by #![doc(test(attr(...)))], then allow some
// lints that are commonly triggered in doctests. The crate-level test attributes are
// commonly used to make tests fail in case they trigger warnings, so having this there in
// that case may cause some tests to pass when they shouldn't have.
code.push_str("#![allow(unused)]\n");
code_prefix.push_str("#![allow(unused)]\n");
}

// Next, any attributes that came from the crate root via #![doc(test(attr(...)))].
for attr in &opts.attrs {
code.push_str(&format!("#![{attr}]\n"));
code_prefix.push_str(&format!("#![{attr}]\n"));
}

code.push_str("extern crate test;\n");
writeln!(code, "extern crate doctest_bundle_{edition} as doctest_bundle;").unwrap();

let test_args = test_args.iter().fold(String::new(), |mut x, arg| {
write!(x, "{arg:?}.to_string(),").unwrap();
Expand Down Expand Up @@ -161,20 +167,20 @@ the same process\");
std::process::Termination::report(test::test_main(test_args, Vec::from(TESTS), None))
}}",
nb_tests = self.nb_tests,
output = self.output,
output = self.output_merged_tests,
ids = self.ids,
)
.expect("failed to generate test code");
let runnable_test = RunnableDocTest {
full_test_code: code,
full_test_code: format!("{code_prefix}{code}", code = self.output),
full_test_line_offset: 0,
test_opts: test_options,
global_opts: opts.clone(),
langstr: LangString::default(),
line: 0,
edition,
no_run: false,
is_multiple_tests: true,
merged_test_code: Some(code),
};
let ret =
run_test(runnable_test, rustdoc_options, self.supports_color, |_: UnusedExterns| {});
Expand All @@ -189,14 +195,15 @@ fn generate_mergeable_doctest(
ignore: bool,
id: usize,
output: &mut String,
output_merged_tests: &mut String,
) -> String {
let test_id = format!("__doctest_{id}");

if ignore {
// We generate nothing else.
writeln!(output, "mod {test_id} {{\n").unwrap();
writeln!(output, "pub mod {test_id} {{}}\n").unwrap();
} else {
writeln!(output, "mod {test_id} {{\n{}{}", doctest.crates, doctest.maybe_crate_attrs)
writeln!(output, "pub mod {test_id} {{\n{}{}", doctest.crates, doctest.maybe_crate_attrs)
.unwrap();
if doctest.has_main_fn {
output.push_str(&doctest.everything_else);
Expand All @@ -216,11 +223,17 @@ fn main() {returns_result} {{
)
.unwrap();
}
writeln!(
output,
"\npub fn __main_fn() -> impl std::process::Termination {{ main() }} \n}}\n"
)
.unwrap();
}
let not_running = ignore || scraped_test.langstr.no_run;
writeln!(
output,
output_merged_tests,
"
mod {test_id} {{
pub const TEST: test::TestDescAndFn = test::TestDescAndFn::new_doctest(
{test_name:?}, {ignore}, {file:?}, {line}, {no_run}, {should_panic},
test::StaticTestFn(
Expand All @@ -242,7 +255,7 @@ test::StaticTestFn(
if let Some(bin_path) = crate::__doctest_mod::doctest_path() {{
test::assert_test_result(crate::__doctest_mod::doctest_runner(bin_path, {id}))
}} else {{
test::assert_test_result(self::main())
test::assert_test_result(doctest_bundle::{test_id}::__main_fn())
}}
",
)
Expand Down
Loading
Loading