rustdoc: never link to unnamable items #143849

lolbinarycat · 2025-07-12T17:21:30Z

rustbot · 2025-07-13T17:18:53Z

rustbot has assigned @GuillaumeGomez.
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-07-14T10:43:40Z

tests/rustdoc/reexport/auxiliary/wrap-unnamable-type.rs

+        type Ty = Bar;
+    }
+    pub struct Bar;
+};


Do we need to have this as a dependency for this test? Can't it be done within the current crate?

Also, what happens if the const is named X?

The bug only happens during cross-crate re-exports. Otherwise a separate failsafe kicks in.

I was under the impression that the _ was a delibraerately injected placeholder, but maybe it is just the constant name, if so we'll need a more principled approach.

Ok, yeah, you're right, my assumptions were wrong. Just looking at the path isn't enough.

Need to check what kind of item (block more precisely) the parent is. If it's not a module, then it's likely wrong.

any idea what datastructure would retain that info in the cross-crate case? I'm somewhat struggling to understand how we even reconstruct path info and such from an rlib.

Path doesn't matter here, you need to check what is its parent.

GuillaumeGomez · 2025-07-16T10:02:27Z

src/librustdoc/html/format.rs

+/// Checks if the given defid refers to an item that is unnamable, such as one defined in a const block.
+fn is_unnamable(tcx: TyCtxt<'_>, did: DefId) -> bool {
+    let mut cur_did = did;
+    while let Some(parent) = tcx.opt_parent(cur_did) {


I think this code is wrong: we can link to an impl associated item even if the impl is in a const/function block.

right, i think impls should break with a return false and only modules should continue up...

well technically we can only link to an impl item if the thing being impl'd on is nameable, but i'm not sure if that's something observable.

Just discovered that we can have modules in any blocks. Dark magic. XD

Also, if the item is not public, it'll be stripped so should be fine. (for impl block as parents)

GuillaumeGomez · 2025-07-31T22:21:07Z

Thanks for the fix!

@bors r+ rollup

bors · 2025-07-31T22:21:10Z

📌 Commit 05a62c8 has been approved by GuillaumeGomez

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - #143849 (rustdoc: never link to unnamable items) - #144683 (Simplify library dependencies on `compiler-builtins`) - #144691 (Extend `is_case_difference` to handle digit-letter confusables) - #144700 (rustdoc-json: Move `#[macro_export]` from `Other` to it's own variant) - #144751 (Add correct dynamic_lib_extension for aix) - #144757 (Ping Muscraft when emitter change) - #144759 (triagebot: Label `compiler-builtins` T-libs) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #143849 - lolbinarycat:rustdoc-priv-normalize-143222, r=GuillaumeGomez rustdoc: never link to unnamable items fixes #143222

panstromek · 2025-08-04T13:42:51Z

Ping from triage:

Looks like this PR introduced a regression in #144768.

Based on detailed results, the regression is in the def_kind query which this PR uses. The number of invocations is increased by 10-20% on some benchmarks.

Is this regression expected or justified?

GuillaumeGomez · 2025-08-04T14:03:58Z

I expected a regression, but not this big. I'll revert the PR for now and we can iterate over the performance improvement.

panstromek · 2025-08-04T14:28:30Z

Ok. It doesn't seem that big to require a revert to me, so just to make sure I didn't add confusion here: 10%-20% change
is on the number of def_kind invocations, but the overall regression is around 0.5% on a few doc benchmarks.

Here's the perf run: https://perf.rust-lang.org/compare.html?start=6c02dd4eae83befde07dc4782395e2005055e9fa&end=e3ee7f7aea5b45af3b42b5e4713da43876a65ac9&stat=instructions:u

GuillaumeGomez · 2025-08-04T14:29:50Z

Ah I see. Didn't check the report yet so spoke too quickly. Still, we will need to bring down the numbers. :)

lolbinarycat · 2025-08-04T16:49:18Z

Obvious ideas optimizations:

only run this when -Znormalize-docs is enabled
only run this on types that have been normalized or are being normalized

the former is much easier to implement, but the perf improvement would go away if we ever make normalizing docs the default. the latter should actually be a substantial improvement, but is slightly trickier to implement. I'll see what i can do.

GuillaumeGomez · 2025-08-04T17:09:23Z

That's a good start already. I also thought about caching this query with compiler queries.

lolbinarycat · 2025-08-04T17:17:14Z

I suppose caching would improve things if an item is being linked to from multiple locations, but i do think it would be more ideal to try to simply disable normalization when it would cause a situation like this.

GuillaumeGomez · 2025-08-04T17:18:39Z

I meant that in addition to your ideas. I'd like your suggestions to be implemented first, and then we'll see if we need to go further. I'll let you implement your ideas and then we can check perf results on them.

lolbinarycat · 2025-08-04T17:33:15Z

Making it a query would also cache it across incremental builds, right? that might be worth it.

This comment has been minimized.

Sign in to view

lolbinarycat force-pushed the rustdoc-priv-normalize-143222 branch from fb01811 to 950634d Compare July 12, 2025 20:05

lolbinarycat changed the title ~~add regression test for RUST-143222~~ rustdoc: never link to unnamable items Jul 12, 2025

lolbinarycat marked this pull request as ready for review July 13, 2025 17:18

rustbot assigned GuillaumeGomez Jul 13, 2025

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

GuillaumeGomez reviewed Jul 14, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

lolbinarycat force-pushed the rustdoc-priv-normalize-143222 branch from ef5c252 to 0952022 Compare July 15, 2025 21:14

GuillaumeGomez reviewed Jul 16, 2025

View reviewed changes

lolbinarycat force-pushed the rustdoc-priv-normalize-143222 branch from 0952022 to aab9c69 Compare July 19, 2025 19:27

lolbinarycat added 5 commits July 22, 2025 17:11

add regression test for RUST-143222

293f95f

rustdoc: never try to link to unnamable types

4762694

fix regression test

94700f8

rustdoc: actually never link to unnamable types

f57283d

impl items are never unnamable

05a62c8

lolbinarycat force-pushed the rustdoc-priv-normalize-143222 branch from aab9c69 to 05a62c8 Compare July 22, 2025 22:11

GuillaumeGomez approved these changes Jul 31, 2025

View reviewed changes

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 Jul 31, 2025

jhpratt mentioned this pull request Aug 1, 2025

Rollup of 7 pull requests #144768

Merged

bors merged commit e6b80f3 into rust-lang:master Aug 1, 2025
11 checks passed

rustbot added this to the 1.90.0 milestone Aug 1, 2025

rust-timer added a commit that referenced this pull request Aug 1, 2025

Unrolled build for #143849

2c4a23a

Rollup merge of #143849 - lolbinarycat:rustdoc-priv-normalize-143222, r=GuillaumeGomez rustdoc: never link to unnamable items fixes #143222

rustdoc: never link to unnamable items #143849

rustdoc: never link to unnamable items #143849

Uh oh!

Conversation

lolbinarycat commented Jul 12, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

rustbot commented Jul 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Jul 31, 2025

Uh oh!

bors commented Jul 31, 2025

Uh oh!

Uh oh!

panstromek commented Aug 4, 2025

Uh oh!

GuillaumeGomez commented Aug 4, 2025

Uh oh!

panstromek commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Aug 4, 2025

Uh oh!

lolbinarycat commented Aug 4, 2025

Uh oh!

GuillaumeGomez commented Aug 4, 2025

Uh oh!

lolbinarycat commented Aug 4, 2025

Uh oh!

GuillaumeGomez commented Aug 4, 2025

Uh oh!

lolbinarycat commented Aug 4, 2025

Uh oh!

Uh oh!

lolbinarycat commented Jul 12, 2025 •

edited by rustbot

Loading

GuillaumeGomez Jul 16, 2025 •

edited

Loading

panstromek commented Aug 4, 2025 •

edited

Loading