doctests: fix merging on stable #137899

notriddle · 2025-03-02T21:35:24Z

The generated multi-test harness relies on nightly-only APIs, so the only way to run it on stable is to enable them.

To prevent the executing test case from getting at any of the stuff that the harness uses, they're built as two separate crates. The test bundle isn't built with RUSTC_BOOTSTRAP, while the runner harness is.

rustbot · 2025-03-02T21:35:28Z

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

GuillaumeGomez · 2025-03-02T21:37:20Z

Good catch! r=me once CI pass. Also is there a way to test it?

fmease · 2025-03-02T22:32:47Z

src/librustdoc/doctest.rs

+        // The merged test harness uses the `test` crate, so we need to actually allow it.
+        // This will not expose nightly features on stable, because crate attrs disable
+        // merging, and `#![feature]` is required to be a crate attr.


I'm afraid that isn't true in practice because partition_source is pretty naive and buggy at the moment as it only detects crate attributes that literally start with the string #![! Literally adding a space between one of these tokens succeeds in circumventing partition_source's checks.

I presume code like the following would start compiling on stable once this PR hits stable (edition 2024):

//! ``` //! #! [feature(rustc_attrs)] //! #! [rustc_coherence_is_core] //! impl i32 { fn identity(self) -> Self { self } } //! fn main() { assert_eq!(1 + 1, 2i32.identity()); } //! ```

We should address that bug really really soon, ideally blocking this PR??

The crate attribute checks should not be textual but syntactic (to account for things like #/**/!/**/[…], too.

I guess I can be convinced that this needn't block this PR because this is obviously a crazy corner case / "exploit" nobody would think about immediately.

If you trick rustdoc into letting you use a feature attribute in a merged doctest, it still won't work, because merged doctests are inside modules, and feature flags aren't allowed in modules.

warning: crate-level attribute should be in the root module --> src/lib.rs:7:5 | 5 | #! [feature(rustc_attrs)] | ^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_attributes)]` on by default

Playground

Yes, I'm aware of that.

The reason it still works for rustc_attrs and test specifically is because the feature is also present in the crate root (and the non-crate-root attribute is just a warning):

#![allow(unused_extern_crates)] #![allow(internal_features)] #![feature(test)] #![feature(rustc_attrs)]

See the generated crate (notice #![feature(rustc_attrs)] in the crate root + the #! [feature(rustc_attrs)] in mod __doctest_0):

#![allow(unused_extern_crates)] #![allow(internal_features)] #![feature(test)] #![feature(rustc_attrs)] #![allow(unused)] extern crate test; mod __doctest_0 { #! [feature(rustc_attrs)] #! [rustc_coherence_is_core] impl i32 { fn identity(self) -> Self { self } } fn main() { assert_eq!(1 + 1, 2i32.identity()); } pub const TEST: test::TestDescAndFn = test::TestDescAndFn::new_doctest( "again.rs - (line 1)", false, "again.rs", 1, false, false, test::StaticTestFn( || { if let Some(bin_path) = crate::__doctest_mod::doctest_path() { test::assert_test_result(crate::__doctest_mod::doctest_runner(bin_path, 0)) } else { test::assert_test_result(self::main()) } }, )); } mod __doctest_mod { use std::sync::OnceLock; use std::path::PathBuf; pub static BINARY_PATH: OnceLock<PathBuf> = OnceLock::new(); pub const RUN_OPTION: &str = "RUSTDOC_DOCTEST_RUN_NB_TEST"; #[allow(unused)] pub fn doctest_path() -> Option<&'static PathBuf> { self::BINARY_PATH.get() } #[allow(unused)] pub fn doctest_runner(bin: &std::path::Path, test_nb: usize) -> Result<(), String> { let out = std::process::Command::new(bin) .env(self::RUN_OPTION, test_nb.to_string()) .args(std::env::args().skip(1).collect::<Vec<_>>()) .output() .expect("failed to run command"); if !out.status.success() { Err(String::from_utf8_lossy(&out.stderr).to_string()) } else { Ok(()) } } } #[rustc_main] fn main() -> std::process::ExitCode { const TESTS: [test::TestDescAndFn; 1] = [__doctest_0::TEST]; let test_marker = std::ffi::OsStr::new(__doctest_mod::RUN_OPTION); let test_args = &["rustdoctest".to_string(),]; const ENV_BIN: &'static str = "RUSTDOC_DOCTEST_BIN_PATH"; if let Ok(binary) = std::env::var(ENV_BIN) { let _ = crate::__doctest_mod::BINARY_PATH.set(binary.into()); unsafe { std::env::remove_var(ENV_BIN); } return std::process::Termination::report(test::test_main(test_args, Vec::from(TESTS), None)); } else if let Ok(nb_test) = std::env::var(__doctest_mod::RUN_OPTION) { if let Ok(nb_test) = nb_test.parse::<usize>() { if let Some(test) = TESTS.get(nb_test) { if let test::StaticTestFn(f) = test.testfn { return std::process::Termination::report(f()); } } } panic!("Unexpected value for `{}`", __doctest_mod::RUN_OPTION); } eprintln!("WARNING: No rustdoc doctest environment variable provided so doctests will be run in the same process"); std::process::Termination::report(test::test_main(test_args, Vec::from(TESTS), None)) }

@fmease Yes, that's a problem.

I've pushed a second commit that should solve this problem by building the test bundle and the test harness as a stable library crate that exports all the main functions, plus a harness with unstable features that runs them). This should make it impossible for a user to accidentally use a nightly-only feature.

I've also added a test case to ensure this.

The reason it still works for rustc_attrs and test specifically

Hmmm, I've now done local testing of the original version of this PR and it's possible that I am partially mistaken, it's all so confusing. The example from #137899 (comment) might not compile after all on stable even with RUSTC_BOOTSTRAP=1? I don't really know. I've tried RUSTC_BOOTSTRAP=-1 rustdoc +FIRST_COMMIT_ONLY file.rs --edition 2024 --test (-1 makes rustc/rustdoc think it's stable) and that still leads to

error[E0554]: `#![feature]` may not be used on the nightly release channel --> file.rs:2:1 | 2 | #! [feature(rustc_attrs)] | ^^^^^^^^^^^^^^^^^^^^^^^^^

However, I don't know if RUSTC_BOOTSTRAP=-1 is a proper replacement for "bootstrap profile == dist" (it that how actual stable distributions of rustc/rustdoc come to life?). I'm flabbergasted as to why that doesn't compile.

Still, the following "exploit" does compile (and I'm more than confused what's the difference to the previous code) under RUSTC_BOOTSTRAP=-1 rustdoc +FIRST_COMMIT_ONLY file.rs --edition 2024 --test:

//! ``` //! #! [feature(rustc_attrs)] //! #[rustc_coinductive] trait Trait {} //! ```

Important
In any case, it seems like my original comment still holds in that the first commit is exploitable.

Another currently less severe example: Assuming --doctest-compilation-args is stable (which it isn't yet), this would be a different category of exploit: RUSTC_BOOTSTRAP=-1 rustdoc +FIRST_COMMIT_ONLY file.rs --edition 2024 --test --doctest-compilation-args -Zfmt-debug=none (where the RUSTC_BOOTSTRAP=-1 is just a placeholder for "stable distribution"):

//! ``` //! #[derive(Debug)] struct Pair; //! assert!(format!("{Pair:?}").is_empty()); //! ```

@fmease

It looks like #![rustc_coherence_is_core] is silently ignored when not at the crate root. Which means, if I run run with --persist-doctests, the resulting code gets the "cannot define inherent impl outside of core" error.

I don't know the specifics about this rustdoc issue, but:

However, I don't know if RUSTC_BOOTSTRAP=-1 is a proper replacement for "bootstrap profile == dist" (it that how actual stable distributions of rustc/rustdoc come to life?).

It is not for the same purpose and is not a substitute for bootstrap configuration. The dist profile of bootstrap sets additional things. RUSTC_BOOTSTRAP=-1 is an internal-testing-only flag (I added it only recently in #132993 mostly to test rustc nightly-gated diagnostics differences) to trick rustc into thinking it is a stable-channel compiler and is not a substitute for dist config.

Bootstrap itself does not use RUSTC_BOOTSTRAP=-1 for producing dist versions of rustc/rustdoc, and instead proper channel info is used.

For the intentions of this PR, I think it's best forgotten that RUSTC_BOOTSTRAP=-1 exists.

notriddle · 2025-03-03T02:47:18Z

@rustbot review

rustbot · 2025-03-03T02:47:45Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

src/librustdoc/doctest.rs

fmease

I've gone through all files and tested things locally. This is definitely the approach we should take. Very good idea, thanks!

The code constructing the compiler invocations is messier than I'd hoped (out of necessity!). There's definitely room for cleanups and refactoring which can be done in follow-up PRs. I must say — only having gone through the changes once — I'm not 100% confident yet I didn't miss potential issues with the way flags are passed since it's all very delicate (cf. the string-based extraction of transitive deps from --extern (don't we need to ~~"import"~~ adjust transitives -Ls too?)).

I might want to go over it again but generally +1 from me. (The last 4 commits should be squashed into one.)

We likely want to nominate this for stable backport too and force a point release. Backports should generally be small and low risk (since it obviously prevents real users from testing the changes "outside of production") which this PR isn't I'd say. So we might want to think about alternative strategies (like only shipping the first commit for a release???).

fmease · 2025-03-04T14:24:57Z

src/librustdoc/doctest.rs

+    fn path_for_merged_doctest_runner(&self) -> PathBuf {
+        self.test_opts.outdir.path().join(format!("doctest_runner_{}.rs", self.edition))
+    }


(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

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.

src/librustdoc/doctest.rs

notriddle · 2025-03-04T17:24:38Z

don't we need to "import" transitives -Ls too?)

compiler_args should already have the -Ls for transitive deps, otherwise building doctests wouldn't have worked in the first place.

We need to generate -Ls for direct deps because the direct deps of the test bundle become transitive deps of the runner.

bors · 2025-03-07T17:51:06Z

☔ The latest upstream changes (presumably #138155) made this pull request unmergeable. Please resolve the merge conflicts.

fmease · 2025-03-10T00:24:00Z

src/librustdoc/doctest.rs

+        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);
+            }
+        }


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

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

rustc directory/a.rs --crate-type=lib --out-dir directory

rustc directory/b.rs --crate-type=lib --edition 2018 --extern a -Ldirectory (or similar)

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')

fmease · 2025-03-10T00:39:52Z

We need to generate -Ls for direct deps because the direct deps of the test bundle become transitive deps of the runner.

I don't think this is necessary as written up in #137899 (comment). However, I won't block this PR on my concern (which could also be unjustified). I'm also a bit wary of generating extra -L… instead of -Ldependency=… (the latter is "safer"/stricter as it only permits transitive deps which is what we want I guess) but I don't actually know if the current version with -L… can lead to any new compilation failures down the line (due to ambiguities?).

Fixes rust-lang#137898 The generated multi-test harness relies on nightly-only APIs, so the only way to run it on stable is to enable them. Since tests that use crate attrs don't be merged, there's no way to use nightly-only features on it anyway.

This prevents the included test case from getting at nightly-only features when run on stable. The harness builds with RUSTC_BOOTSTRAP, but the bundle doesn't.

fmease · 2025-03-10T00:56:44Z

I've rebased and fixed a small typo (#[feature] to #![feature] in a comment).

fmease · 2025-03-10T00:59:03Z

@bors r=fmease,GuillaumeGomez rollup=always p=6
p=6: Fixes a P-critical issue.

bors · 2025-03-10T00:59:05Z

📌 Commit 9cf531d has been approved by fmease,GuillaumeGomez

It is now in the queue for this repository.

fmease · 2025-03-10T01:02:47Z

Beta and stable backport accepted as per Zulip polls:

In both cases, out of 8 (7 active) members, 4 members voted for, 0 against, 1 abstained.

bors · 2025-03-10T01:25:22Z

⌛ Testing commit 9cf531d with merge c8a5072...

bors · 2025-03-10T04:31:07Z

☀️ Test successful - checks-actions
Approved by: fmease,GuillaumeGomez
Pushing c8a5072 to master...

rust-log-analyzer · 2025-03-10T04:31:34Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)


gh pr comment ${HEAD_PR} -F output.log
shell: /usr/bin/bash -e {0}
##[endgroup]
fatal: ambiguous argument 'HEAD^1': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
##[error]Process completed with exit code 128.
Post job cleanup.

rust-timer · 2025-03-10T08:30:59Z

Finished benchmarking commit (c8a5072): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 779.777s -> 778.705s (-0.14%)
Artifact size: 365.16 MiB -> 365.15 MiB (-0.00%)

notriddle added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 2, 2025

notriddle requested a review from GuillaumeGomez March 2, 2025 21:35

rustbot assigned fmease Mar 2, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 2, 2025

GuillaumeGomez approved these changes Mar 2, 2025

View reviewed changes

fmease requested changes Mar 2, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2025

GuillaumeGomez reviewed Mar 3, 2025

View reviewed changes

src/librustdoc/doctest.rs Show resolved Hide resolved

notriddle requested review from GuillaumeGomez and fmease March 3, 2025 23:29

fmease approved these changes Mar 4, 2025

View reviewed changes

notriddle force-pushed the merged-doctests-stable branch from 2afe69c to c67b75a Compare March 4, 2025 16:46

fmease reviewed Mar 4, 2025

View reviewed changes

src/librustdoc/doctest.rs Show resolved Hide resolved

notriddle force-pushed the merged-doctests-stable branch 2 times, most recently from cf175d7 to 6e3e206 Compare March 4, 2025 17:09

fmease added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Mar 4, 2025

fmease reviewed Mar 10, 2025

View reviewed changes

notriddle added 2 commits March 10, 2025 01:43

doctests: fix merging on stable

5d6eeea

Fixes rust-lang#137898 The generated multi-test harness relies on nightly-only APIs, so the only way to run it on stable is to enable them. Since tests that use crate attrs don't be merged, there's no way to use nightly-only features on it anyway.

doctests: build test bundle and harness separately

9cf531d

This prevents the included test case from getting at nightly-only features when run on stable. The harness builds with RUSTC_BOOTSTRAP, but the bundle doesn't.

fmease force-pushed the merged-doctests-stable branch from 6e3e206 to 9cf531d Compare March 10, 2025 00:53

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2025

fmease added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Mar 10, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 10, 2025

bors merged commit c8a5072 into rust-lang:master Mar 10, 2025
7 checks passed

rustbot added this to the 1.87.0 milestone Mar 10, 2025

notriddle deleted the merged-doctests-stable branch March 10, 2025 07:10

notriddle mentioned this pull request Mar 12, 2025

Backport: Merged doctests unused in stable edition 2024 #138418

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doctests: fix merging on stable #137899

doctests: fix merging on stable #137899

notriddle commented Mar 2, 2025 •

edited

Loading

rustbot commented Mar 2, 2025

GuillaumeGomez commented Mar 2, 2025

fmease Mar 2, 2025 •

edited

Loading

fmease Mar 2, 2025 •

edited

Loading

notriddle Mar 2, 2025 •

edited

Loading

fmease Mar 2, 2025

fmease Mar 2, 2025 •

edited

Loading

fmease Mar 2, 2025 •

edited

Loading

notriddle Mar 3, 2025

fmease Mar 3, 2025 •

edited

Loading

notriddle Mar 3, 2025

jieyouxu Mar 3, 2025 •

edited

Loading

notriddle commented Mar 3, 2025

rustbot commented Mar 3, 2025

fmease left a comment •

edited

Loading

fmease Mar 4, 2025

notriddle Mar 4, 2025

notriddle commented Mar 4, 2025

bors commented Mar 7, 2025

fmease Mar 10, 2025 •

edited

Loading

fmease Mar 10, 2025 •

edited

Loading

fmease commented Mar 10, 2025 •

edited

Loading

fmease commented Mar 10, 2025

fmease commented Mar 10, 2025

bors commented Mar 10, 2025

fmease commented Mar 10, 2025 •

edited

Loading

bors commented Mar 10, 2025

bors commented Mar 10, 2025

rust-log-analyzer commented Mar 10, 2025

rust-timer commented Mar 10, 2025

doctests: fix merging on stable #137899

doctests: fix merging on stable #137899

Conversation

notriddle commented Mar 2, 2025 • edited Loading

rustbot commented Mar 2, 2025

GuillaumeGomez commented Mar 2, 2025

fmease Mar 2, 2025 • edited Loading

Choose a reason for hiding this comment

fmease Mar 2, 2025 • edited Loading

Choose a reason for hiding this comment

notriddle Mar 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Mar 2, 2025 • edited Loading

Choose a reason for hiding this comment

fmease Mar 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

notriddle commented Mar 3, 2025

rustbot commented Mar 3, 2025

fmease left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notriddle commented Mar 4, 2025

bors commented Mar 7, 2025

fmease Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

fmease Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

fmease commented Mar 10, 2025 • edited Loading

fmease commented Mar 10, 2025

fmease commented Mar 10, 2025

bors commented Mar 10, 2025

fmease commented Mar 10, 2025 • edited Loading

bors commented Mar 10, 2025

bors commented Mar 10, 2025

rust-log-analyzer commented Mar 10, 2025

rust-timer commented Mar 10, 2025

Overall result: ❌ regressions - no action needed

notriddle commented Mar 2, 2025 •

edited

Loading

fmease Mar 2, 2025 •

edited

Loading

fmease Mar 2, 2025 •

edited

Loading

notriddle Mar 2, 2025 •

edited

Loading

fmease Mar 2, 2025 •

edited

Loading

fmease Mar 2, 2025 •

edited

Loading

fmease Mar 3, 2025 •

edited

Loading

jieyouxu Mar 3, 2025 •

edited

Loading

fmease left a comment •

edited

Loading

fmease Mar 10, 2025 •

edited

Loading

fmease Mar 10, 2025 •

edited

Loading

fmease commented Mar 10, 2025 •

edited

Loading

fmease commented Mar 10, 2025 •

edited

Loading