Skip to content

Conversation

@jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 2, 2025

This is useful for changing the default for whether doctests are merged or not. Currently, that default is solely controlled by edition = 2024, which adds a high switching cost to get doctest merging. This flag allows opting in even on earlier editions.

Unlike the edition = 2024 default, --merge-doctests=yes gives a hard error if merging fails instead of falling back to running standalone tests. The user has explicitly said they want merging, so we shouldn't silently do something else.

--merge-doctests=auto is equivalent to the current 2024 edition behavior, but available on earlier editions.

Helps with #141240. @epage said in that issue he would like a per-doctest opt-in, and that seems useful to me in addition to this flag, but I think it's a separate use case from changing the default.

@rustbot 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 Dec 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 2, 2025

r? @notriddle

rustbot has assigned @notriddle.
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

@jyn514

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 changed the title rustdoc: Add unstable --merge-doctests=yes/no flag rustdoc: Add unstable --merge-doctests=yes/no/auto flag Dec 2, 2025
@jyn514 jyn514 force-pushed the force-doctest-merge branch 2 times, most recently from dde2ec7 to 0ff6d88 Compare December 2, 2025 22:23
Comment on lines +1 to +5
doctest_bundle_2018.rs:$LINE:$COL: error: `unmangled_name` redeclared with a different signature: this signature doesn't match the previous declaration
error: aborting due to 1 previous error
error: failed to merge doctests
|
= note: requested explicitly on the command line with `--merge-doctests=yes`
Copy link
Member Author

Choose a reason for hiding this comment

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

this error kinda sucks, but this is an unstable feature anyway, and with -C save-temps people will actually be able to inspect the merged code, which should make this easier to debug.

oh, maybe this should recommend -C save-temps as a note?

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Dec 2, 2025
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Dec 3, 2025

hm this broke the reference’s doctest somehow

      failures:
      
      ---- items/constant-items.md - Constant_items (line 71) stdout ----
      Test compiled successfully, but it's marked `compile_fail`.
      ---- items/constant-items.md - Constant_items (line 97) stdout ----
      Test compiled successfully, but it's marked `compile_fail`.
      
      failures:
          items/constant-items.md - Constant_items (line 71)
          items/constant-items.md - Constant_items (line 97)

@jyn514
Copy link
Member Author

jyn514 commented Dec 3, 2025

i'm completely unable to reproduce this :/ i see ci has this output which i don't see locally:

[2025-12-03T00:03:32Z WARN  mdbook::preprocess::cmd] The command wasn't found, is the "spec" preprocessor installed?
[2025-12-03T00:03:32Z WARN  mdbook::preprocess::cmd] 	Command: cargo run --release --manifest-path mdbook-spec/Cargo.toml

@jyn514
Copy link
Member Author

jyn514 commented Dec 3, 2025

i ran rustdoc +stage2 --test src/items/constant-items.md manually and it doesn't even show the same line numbers as CI :/ what on earth

@jyn514 jyn514 force-pushed the force-doctest-merge branch from b1e2ca9 to e2aa3fc Compare December 3, 2025 17:46
@rustbot
Copy link
Collaborator

rustbot commented Dec 3, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

&& !(has_macro_def && everything_else.contains("$crate"));
can_merge
} else {
can_merge_doctests != MergeDoctests::Never
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I reading this correctly that this overrides standalone_crate?

From the PR description (emphasis mine)

This is useful for changing the default for whether doctests are merged or not.

For me, default implies that this is what we do in the absence of other information. standalone_crate is an obvious one for being respected. compile_fail is another. test_harness I'm unsure about.

Now, for any of the detection based on the code (global allocator, crate attrs, macros, $crate, etc, I see that being part of the autodetection.

Copy link
Contributor

Choose a reason for hiding this comment

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

In case it is relevant, https://github.com/epage/rust/commits/merged_crate is where I left off on my work on the attribute

Copy link
Member Author

Choose a reason for hiding this comment

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

excellent catch, omg. fixed this and added a test.

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Dec 3, 2025

something really weird is going on with compiletest; locally it applies the normalize-stdout directive, but in CI it ignores it and replaces $DIR. the two don't match, hence the failure, but I don't understand why $DIR would possibly work in CI but not locally.

for now I'm just going to change the normalization to match compiletest's normalization.

@jyn514
Copy link
Member Author

jyn514 commented Dec 3, 2025

oh, i wonder if this is about absolute vs relative paths :/

@jyn514
Copy link
Member Author

jyn514 commented Dec 3, 2025

i think the proper fix here is that rustdoc should use to_embeddable_absolute_path when -Z ui-testing is set. but i've already spent a bunch of time on this so i'm just going to open an issue and hopefully someone gets to it later.

This is useful for changing the *default* for whether doctests are
merged or not. Currently, that default is solely controlled by
`edition = 2024`, which adds a high switching cost to get doctest
merging. This flag allows opt-ing in even on earlier additions.

Unlike the `edition = 2024` default, `--merge-doctests=yes` gives a hard
error if merging fails instead of falling back to running standalone
tests. The user has explicitly said they want merging, so we shouldn't
silently do something else.

`--merge-doctests=auto` is equivalent to the current 2024 edition
behavior, but available on earlier editions.
This allows viewing failed merged doctests.
@jyn514 jyn514 force-pushed the force-doctest-merge branch from 646667a to 415953a Compare December 3, 2025 22:43
&& crate_attrs.is_empty()
// We try to look at the contents of the test to detect whether it should be merged.
// This is not a complete list of possible failures, but it catches many cases.
let will_probably_fail = has_global_allocator
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know why I'm getting a chuckle at that variable name :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-run-make Area: port run-make Makefiles to rmake.rs 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants