rustdoc: Skip doc link resolution for non-exported items

[petrochenkov]

No description provided.

[rustbot]

r? @jsha

(rustbot has picked a reviewer for you, use r? to override)

[petrochenkov]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 80beaa506705af5943b272dd0bc262e4668f47db with merge aac432a34739ec6dd1fd66a287cf34e17a8c49f8...

[bors]

☀️ Try build successful - checks-actions
Build commit: aac432a34739ec6dd1fd66a287cf34e17a8c49f8 (aac432a34739ec6dd1fd66a287cf34e17a8c49f8)

[rust-timer]

Finished benchmarking commit (aac432a34739ec6dd1fd66a287cf34e17a8c49f8): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	2.2%	[1.5%, 3.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.6%	[-1.6%, -1.6%]	1
All ❌✅ (primary)	3.2%	[3.2%, 3.2%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

[petrochenkov]

Are benchmarks run with --document-private-items by chance?

Anyway, this is a cheap optimization that can skip extra work in some cases, and it's done in other situations (ResolveDocLinks::ExportedMetadata), so it still makes sense to land it.

[apiraino]

@rustbot r? compiler

who can have a second look at this from the perspective of T-compiler?

edit: for context, this PR was discussed in a previous T-compiler meeting (on zulip) trying to stimulate a review but apparently the domain of knowledge involved in the review does not allow a simple random review assignment. We need another T-compiler person able to review these changes

[eholk]

@rustbot r? @jyn514

[jyn514]

The code looks good and the change makes sense to me (this is just a perf optimization, right, it shouldn't affect behavior?).

r=me with the suggested test case added.

[jyn514]

Are you sure is_exported is the correct thing here, and not is_reachable? Can you add a test case for

mod private {
  /// [core::str::FromStr]
  pub struct Bar;
}

pub fn foo() -> private::Bar {
    private::Bar
}

and make sure it behaves the same both before and after this change?

[petrochenkov]

In both cases Bar is not documented and Bar in pub fn foo() -> Bar in the generated doc is not a link.

Rustdoc uses is_reachable in only one place when it generates json output, and I'm not sure why.

[jyn514]

Actually hold on - all private items should already be stripped from the crate at this point (see

rust/src/librustdoc/passes/strip_private.rs

Lines 15 to 17 in 0e8f0b0

	/// Strip private items from the point of view of a crate or externally from a
	/// crate, specified by the `xcrate` flag.
	pub(crate) fn strip_private(mut krate: clean::Crate, cx: &mut DocContext<'_>) -> clean::Crate {

). I think that's why this isn't showing a perf improvement, it's not actually affecting rustdoc's behavior. What issue were you running into before you added the !is_exported() check to the intra-doc links pass?

[petrochenkov]

@jyn514

What issue were you running into before you added the !is_exported() check to the intra-doc links pass?

And ICE when documenting liballoc.

Documenting alloc v0.0.0 (C:\msys64\home\we\rust\library\alloc)
thread 'rustc' panicked at 'no resolutions for a doc link', compiler\rustc_metadata\src\rmeta\encoder.rs:2251:18

rustdoc still tries to request resolution for a link on some non-exported item.

It seems like it doesn't happen often due to the item stripping pass, so there's no effect on performance, but does happen.

[petrochenkov]

Inner doc comment on the alloc::vec::in_place_collect module, which is indeed not exported (it's fully private).

[src\librustdoc\passes\collect_intra_doc_links.rs:377] path_str = "Vec"
[src\librustdoc\passes\collect_intra_doc_links.rs:377] ns = MacroNS
[src\librustdoc\passes\collect_intra_doc_links.rs:377] item_id = DefId(0:6408 ~ alloc[76f0]::vec::in_place_collect)
[src\librustdoc\passes\collect_intra_doc_links.rs:377] module_id = DefId(0:6408 ~ alloc[76f0]::vec::in_place_collect)
thread 'rustc' panicked at 'no resolutions for a doc link', compiler\rustc_metadata\src\rmeta\encoder.rs:2255:18

[petrochenkov]

@rustbot ready

[jyn514]

rustdoc still tries to request resolution for a link on some non-exported item.

It seems like it doesn't happen often due to the item stripping pass, so there's no effect on performance, but does happen.

This seems like a pre-existing bug I would like to investigate, but it doesn't have to block this PR.

@bors r+

[bors]

📌 Commit bec4eab has been approved by jyn514

It is now in the queue for this repository.

[bors]

⌛ Testing commit bec4eab with merge fd46bb408dba5d3e0a34a4e490ef1f838264f616...

[bors]

💔 Test failed - checks-actions

[ehuss]

@bors retry

github incident

[rust-log-analyzer]

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

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

[bors]

⌛ Testing commit bec4eab with merge 8be3c2b...

[bors]

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 8be3c2b to master...

[rust-timer]

Finished benchmarking commit (8be3c2b): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.5%	[1.1%, 1.7%]	3
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-1.3%	[-3.1%, -0.5%]	6
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

Cycles

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