fix: stabilize -Zrustdoc-depinfo

[weihanglo]

View all comments

---> FCP <---

What does this PR try to resolve?

With this stabilization,
cargo doc unconditionally emits rustdoc depinfo
via rustdoc --emit=dep-info.
Before this, cargo doc used to track file changes via filesystem traversal just like build scripts, and it cannot detect these cases:

• Cargo target source files are outside the package root, e.g., lib.path = "../lib.rs"
• Using include_str! to include files outisde pakcage root, e.g., #[doc = include_str!("../outside/pkgroot")]
• path attribute pointing to outside package root, e.g., #[path = "../outside/pkgroot"]
• Using env! in doc attribute, e.g., #[doc = env!("…")]

The depinfo files are emitted to Cargo's fingerprint which is a private location so we don't have to commit any stability around it if we want to move it somewhere else in future versions.

Fixes #15370

How to test and review this PR?

While this is a bugfix, I assume we still need an FCP for it.

[rustbot]

r? @epage

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ehuss, @epage, @weihanglo
• @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
• Random selection from ehuss, epage

[weihanglo]

Oops. This is not going to work without flagging every cargo doc test case nightly-only 🥲

[weihanglo]

@rfcbot fcp merge T-cargo

[rust-rfcbot]

@weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

• @0xPoe
• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

Concerns:

• comma-in-path (fix: stabilize -Zrustdoc-depinfo #17020 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rustbot]

This PR was rebased onto a different master 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.

[0xPoe]

I found one regression case from this feature:

Problem

cargo doc fails when the target directory path contains a comma.

Cargo places rustdoc's dep-info file under its private fingerprint directory. It then embeds that path into rustdoc's comma-separated --emit argument:

--emit=html-static-files,html-non-static-files,dep-info=/path/to/target,comma/debug/.fingerprint/... /doc-lib-foo.d                                                                                       

Rustdoc splits the entire value on commas, causing the portion after the comma in the path to be interpreted as another emission type:

 error: unrecognized emission type: comma/debug/.fingerprint/...                                    

A comma is valid in a target directory name, and the same cargo doc command succeeds when rustdoc dep-info is not enabled.

Expected behavior (if you are using the current PR, there is no need to add the -Zrustdoc-depinfo):

 cargo doc -Zrustdoc-depinfo --target-dir target,comma                                              

should succeed and generate:

 target,comma/doc/comma_target/index.html                                                           

Actual behavior: it will fail with an unrecognized emission type error.

Steps

 $ cargo new --lib --name comma_target cargo-rustdoc-depinfo-comma-repro                            
 $ cd cargo-rustdoc-depinfo-comma-repro                                                             
 $ cargo +nightly doc -Zrustdoc-depinfo --target-dir 'target,comma' -vv                             

For comparison, this succeeds:

 $ cargo +nightly doc --target-dir 'target,comma'                                                   

I think this is a very corner case, so I’m not sure if it’s really worth fixing. But it does look like a regression, although I guess not many people would put a comma in the path. So I’m not sure, @weihanglo, what do you think? And do we need to bother fixing this?

[weihanglo]

@rfcbot concern comma-in-path

[weihanglo]

Filed rust-lang/rust#159743

[weihanglo]

One workaround: fall back to the old file walk solution when comma is detected.

[0xPoe]

Cargo’s rustdoc dep-info injection conflicts with output modes passed after --

I tried this code with Cargo built from #17020:

#!/usr/bin/env bash

set -u

cargo_under_test=${CARGO_UNDER_TEST:-cargo}
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT

"$cargo_under_test" new --lib --name t "$tmp/t" >/dev/null
cd "$tmp/t"

cat >src/lib.rs <<'RS'
/// ```
/// assert_eq!(2 + 2, 4);
/// ```
pub fn answer() {}
RS

check() {
    echo "### $1"
    shift

    set +e
    "$@" 2>&1 | sed 's/^/    /'
    status=${PIPESTATUS[0]}
    set -e

    echo "    exit status: $status"
    echo
}

check "raw JSON output" \
    "$cargo_under_test" rustdoc -- \
    -Zunstable-options --output-format json

if [[ -f target/doc/t.json ]]; then
    echo "target/doc/t.json exists"
else
    echo "target/doc/t.json is missing"
fi
echo

"$cargo_under_test" clean >/dev/null

check "raw rustdoc test mode" \
    "$cargo_under_test" rustdoc -- --test

I expected to see this happen:

### raw JSON output
    ...
    Finished ...
    exit status: 0

target/doc/t.json exists

### raw rustdoc test mode
    running 1 test
    test src/lib.rs - answer ... ok

    test result: ok. 1 passed
    ...
    exit status: 0

Both commands worked before rustdoc dep-info was made unconditional for Cargo doc units.

Instead, this happened:

### raw JSON output
    error: the `--emit=html-static-files` flag is not supported with `--output-format=json`

    error: could not document `t`
    exit status: 101

target/doc/t.json is missing

### raw rustdoc test mode
    error: the `--test` flag and the `--emit` flag are not supported together

    error: could not document `t`
    exit status: 101

Cargo determines the documentation output mode before processing the arguments after --. Therefore, for both commands, Cargo still has:

wants_doc_json_output() == false

and injects its internal rustdoc dep-info argument:

--emit=html-static-files,html-non-static-files,dep-info=<cargo-fingerprint-path>

The user-provided arguments are appended later, producing invocations equivalent to:

rustdoc \
  --emit=html-static-files,html-non-static-files,dep-info=<path> \
  --output-format json

and:

rustdoc \
  --emit=html-static-files,html-non-static-files,dep-info=<path> \
  --test

Rustdoc rejects both combinations.

Relevant Cargo code:

• Cargo adds its internal --emit:
  

  cargo/src/compiler/mod.rs

  Lines 894 to 912 in 5f1508c

  	// `--emit` would reject or drop the JSON doc output,
  	// so skip it and fall back to package fingerprint for JSON doc units.
  	//
  	// If we have `--emit=json-files` available,
  	// we could pass that along with `--emit=dep-info`.
  	// see rust-lang/rust#155679
  	if !build_runner.bcx.build_config.intent.wants_doc_json_output() {
  	// html-static-files is required for keeping the shared styling resources
  	// html-non-static-files is required for keeping the original rustdoc emission
  	let mut arg = if build_runner.bcx.gctx.cli_unstable().rustdoc_mergeable_info {
  	// toolchain resources are written at the end, at the same time as merging
  	OsString::from("--emit=html-non-static-files,dep-info=")
  	} else {
  	// if not using mergeable CCI, everything is written every time
  	OsString::from("--emit=html-static-files,html-non-static-files,dep-info=")
  	};
  	arg.push(rustdoc_dep_info_loc(build_runner, unit));
  	rustdoc.arg(arg);
  	}

• Arguments after cargo rustdoc -- are appended later:
  

  cargo/src/compiler/mod.rs

  Lines 971 to 976 in 5f1508c

  	rustdoc::add_output_format(build_runner, &mut rustdoc)?;
  	if let Some(args) = build_runner.bcx.extra_args_for(unit) {
  	rustdoc.args(args);
  	}
  	rustdoc.args(&unit.rustdocflags);

Why I think this is a valid regression

Before the stabilization change, both commands reached rustdoc without Cargo’s internal --emit and succeeded. After the change, they fail solely because Cargo applies dep-info to a rustdoc invocation whose final output mode is controlled by trailing arguments.

Even if custom rustdoc modes are considered outside the precise dep-info feature’s scope, Cargo should avoid enabling the feature for those invocations rather than constructing a combination rustdoc explicitly rejects.

Version

Verified with 5f1508c3317158aaca1e24c31cbc5522072d24e6

@weihanglo I am not sure if we consider this a real regression, as I discussed with you offline. The rustdoc did refuse the mixed use of --test with --emit. But I still worry that, in the cargo rustdoc -- --test case, there is no point in adding emit args by default? Please let me know if I am mistaken.

[weihanglo]

Thanks for the investigation in #17020 (comment), Dongpo!

From https://doc.rust-lang.org/nightly/cargo/reference/config.html#buildrustdocflags:

Caution: Due to the low-level nature of passing flags directly to the compiler, this may cause a conflict with future versions of Cargo which may issue the same or similar flags on its own which may interfere with the flags you specify. This is an area where Cargo may not always be backwards compatible.

That said, that is the doc of build.rustdocflags not the cargo rustdoc -- <flags>. However, I think they are under the same policy. If we haven't yet called it out in cargo rustc / cargo rustdoc, we should.

OTOH, I am not sure how useful cargo rustdoc -- --test is. Doc units won't pass --extern or build the lib. Unless the lib is really self-contained, it would eventually fail. As for cargo rustdoc -- --output-format=json, there are crates using it for sure, though rustdoc --output-format is unstable, and cargo has its own variant. One can argue that the crate should migrate to cargo native way if it is using nightly already.

[0xPoe]

That said, that is the doc of build.rustdocflags not the cargo rustdoc -- <flags>. However, I think they are under the same policy. If we haven't yet called it out in cargo rustc / cargo rustdoc, we should.

Sounds reasonable to me. The -- --test case is trivial and somehow broken in most cases(I guess users would rarely try to run it by manually constructing --extern args). So I think there is no point in blocking stabilization because of this case. For rustdoc --output-format, I also second your opinion. We should encourage users to do it with Cargo in its own variant. It also requires -Z, so I think it is OK to ask users to migrate.

And for the comma path issue, I think I am okay with detecting it and falling back to the old behavior, but I am not sure whether it is worth doing so, as I guess it would be very rare for people to use a comma in the path. So I am also fine to ignore that corner case and move forward without fixing it. We can always come up with a fix to solve the issue in the future when we actually have a real use case for it?

[weihanglo]

And for the comma path issue, I think I am okay with detecting it and falling back to the old behavior, but I am not sure whether it is worth doing so, as I guess it would be very rare for people to use a comma in the path. So I am also fine to ignore that corner case and move forward without fixing it. We can always come up with a fix to solve the issue in the future when we actually have a real use case for it?

It is a hard blocker to me. It is not that uncommon people have something like /projects/July 28, 2026/some-cool-stuff, or /projects/MyCompanyName, LLC/.

A good news is that probably with rust-lang/rust#159473 and the new build dir layout, we can completely isolate rustdoc artifacts per invocation, so we no longer need to specify a full path for --emit=dep-info.