[rustdoc] If re-export is private, get the next item until a public one is found or expose the private item directly #113374

GuillaumeGomez · 2023-07-05T16:47:08Z

If we have:

use Private as Something;

pub fn foo() -> Something {}

Then Something will be replaced by Private.

r? @notriddle

fmease · 2023-07-05T21:33:03Z

I haven't looked too much at the implementation yet but do you think it'd be worthwhile to kick off a perf run since it looks like in the worst case this is running a nested loop with a nesting level of up to 4 (!) whenever it cleans a hir qpath.

GuillaumeGomez · 2023-07-05T21:39:57Z

Good point!

@bors try @rust-timer queue

bors · 2023-07-05T21:40:06Z

⌛ Trying commit 573a7f4ee75cbd7640143e6687d9d06cec1d7062 with merge cfeb9c6f60982932992f2550ba9b986fcc4e9ef7...

bors · 2023-07-05T22:57:41Z

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

rust-timer · 2023-07-06T02:08:58Z

Finished benchmarking commit (cfeb9c6f60982932992f2550ba9b986fcc4e9ef7): comparison URL.

Overall result: ❌ regressions - 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 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.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[0.9%, 0.9%]	1

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.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	2.2%	[1.7%, 2.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.6%	[-2.1%, -1.1%]	2
All ❌✅ (primary)	0.4%	[0.4%, 0.4%]	1

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: 658.838s -> 658.176s (-0.10%)

GuillaumeGomez · 2023-07-06T17:23:01Z

Seems like the regression isn't too big. At least way less than what I originally feared.

fmease · 2023-07-06T19:23:50Z

Yay, seems like it! Or our benchmark & stress test suite is not big enough :P

I'm gonna go over the changes tonight or tomorrow hopefully if I find the time.

I wonder if there is any rustc API in rustc_resolve or anything from the intra-doc link module that we could leverage but it doesn't seem so, dunno

GuillaumeGomez · 2023-07-06T19:30:25Z

The problem here is trying to retrieve the DefId and then the HIR element again. Back and forth fun.

Yay, seems like it! Or our benchmark & stress test suite is not big enough :P

It's a possibility indeed! :)

I'm gonna go over the changes tonight or tomorrow hopefully if I find the time.

Thanks!

notriddle

Is there no facility to accomplish this already in the compiler? I know it suggests paths to import, which means it should try to avoid giving inaccessible ones.

If not, here's a suggestion for fixing (and more thoroughly testing) this function.

src/librustdoc/clean/mod.rs

tests/rustdoc/issue-81141-private-reexport-in-public-api.rs

src/librustdoc/clean/mod.rs

GuillaumeGomez · 2023-07-07T11:43:12Z

Is there no facility to accomplish this already in the compiler? I know it suggests paths to import, which means it should try to avoid giving inaccessible ones.

I didn't find them at least. Doesn't mean they don't exist, but I missed them if they do.

GuillaumeGomez · 2023-07-07T11:56:13Z

I applied your suggestions @notriddle, thanks a lot!

notriddle

Looks good. Awaiting fmease’s feedback

src/librustdoc/clean/mod.rs

fmease

Let's say I'm not super familiar with this area of the code (fetching things from HIR and how re-exports are represented) but it looks reasonable.

fmease · 2023-07-07T14:33:11Z

src/librustdoc/clean/mod.rs

+    for child in
+        cx.tcx.module_children_local(parent_def_id).iter().filter(move |c| c.ident == ident)
+    {
+        if let Res::Def(DefKind::Ctor(..), _) | Res::SelfCtor(..) = child.res {


Of course, we could at some point support Ctor but that's low priority. This would only be reachable with const generics (and const items if/once we render their body). E.g. Container<{ Alias }> with use Type::Ctor as Alias.

For now it's something we exclude everywhere in rustdoc when looking for reexports. We can indeed add support for it later on.

fmease · 2023-07-07T14:36:43Z

src/librustdoc/clean/mod.rs

+                                if let Res::Def(DefKind::Ctor(..), _) | Res::SelfCtor(..) = res {
+                                    continue;
+                                }
+                                if !cx.tcx.is_doc_hidden(use_def_id) &&


This doesn't respect --document-private-items and --document-hidden-items. Not sure what the desired behavior would be though. Do we want to keep showing the private or hidden alias when such a flag is passed? Not blocking.

I was planning to actually fix it in a follow-up (all set-up in my calendar and everything :D).

src/librustdoc/clean/mod.rs

GuillaumeGomez · 2023-07-07T15:03:45Z

Updated the function name.

src/librustdoc/clean/mod.rs

GuillaumeGomez · 2023-07-07T15:59:40Z

Added the handling for super and for :: (2015 edition).

fmease

Technically speaking, edition:2015 isn't necessary because it's the default in tests but I like that it's there since it makes it explicit to the reader.

It makes me a bit uneasy that we straight up interpret kw::PathRoot as kw::Crate in any old edition instead of checking parent.ident.span.is_rust_2015() first (if that's actually how you properly check this), although I'm not sure if it can cause any problems right now. For example in Rust 2018+, ::dep always refers to a crate but it's unclear to me if in such case fn first_non_private is actually reached (e.g. I've quickly tried pub use ::dep as alias; but that didn't lead to first_non_private being called).

Edit: I think we are good to go. Thanks for working on it and for considering my feedback! :)
Edit: (Typo in commit message: Correctly handly super and ::)

GuillaumeGomez · 2023-07-27T08:55:53Z

To use @notriddle's, I needed to the remove the cache since I need a mutable access to DocContext to clean the path. Let's hope it won't impact the perf check too much...

@bors try @rust-timer queue

bors · 2023-07-27T08:56:02Z

⌛ Trying commit 2461d0c with merge 5224c258f207a3f5b4c32980f776a9317118ef1e...

bors · 2023-07-27T10:13:16Z

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

rust-timer · 2023-07-27T14:18:14Z

Finished benchmarking commit (5224c258f207a3f5b4c32980f776a9317118ef1e): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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)	1.5%	[0.3%, 2.7%]	2
Regressions ❌ (secondary)	0.3%	[0.3%, 0.5%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[0.3%, 2.7%]	2

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)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Binary size

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

Bootstrap: 652.287s -> 652.99s (0.11%)

GuillaumeGomez · 2023-07-27T14:25:06Z

So to sum things up:

method	mean	range
transmute + cache	1.1%	[0.6%, 2.3%]
arena + cache	2.6%	[2.6%, 2.6%]
early clean	1.5%	[0.3%, 2.7%]

I think the current result is good enough. What do you think @notriddle ?

notriddle · 2023-07-27T14:37:19Z

They all seem close enough to me.

GuillaumeGomez · 2023-07-27T14:38:43Z

Yep, hence why I suggest to keep the current implementation (the one you suggested) without transmute.

notriddle · 2023-07-27T14:41:20Z

@bors r=notriddle,fmease

bors · 2023-07-27T14:41:22Z

📌 Commit 2461d0c has been approved by notriddle,fmease

It is now in the queue for this repository.

GuillaumeGomez · 2023-07-27T14:41:57Z

@bors rollup=never

bors · 2023-07-27T15:56:53Z

⌛ Testing commit 2461d0c with merge 9339f44...

bors · 2023-07-27T17:45:05Z

☀️ Test successful - checks-actions
Approved by: notriddle,fmease
Pushing 9339f44 to master...

rust-timer · 2023-07-28T02:56:22Z

Finished benchmarking commit (9339f44): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)	1.0%	[0.3%, 2.7%]	4
Regressions ❌ (secondary)	0.4%	[0.3%, 0.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[0.3%, 2.7%]	4

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)	2.4%	[2.2%, 2.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Binary size

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

Bootstrap: 654.151s -> 654.227s (0.01%)

rylev · 2023-08-05T12:54:33Z

@GuillaumeGomez This looks very much like a real regression (the spike in the graph below is the change caused by this PR):

Looks like we might be spending much more time inrender_mod_item. Any ideas?

GuillaumeGomez · 2023-08-05T15:21:38Z

It's exactly as showed by the perf results. We considered the perf regression to be acceptable in regard to the fix.

rylev · 2023-08-07T12:24:59Z

Marking as triaged then

@rustbot label: +perf-regression-triaged

lqd · 2023-08-07T12:26:46Z

(it already was marked as perf-regression-triaged :) (but without a super clear justification, nbd)

rustbot assigned notriddle Jul 5, 2023

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 Jul 5, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 5, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 6, 2023

notriddle requested changes Jul 6, 2023

View reviewed changes

src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved

tests/rustdoc/issue-81141-private-reexport-in-public-api.rs Outdated Show resolved Hide resolved

src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved

notriddle approved these changes Jul 7, 2023

View reviewed changes

fmease reviewed Jul 7, 2023

View reviewed changes

src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the private-to-public-path branch from a813891 to 662530d Compare July 7, 2023 14:32

fmease approved these changes Jul 7, 2023

View reviewed changes

GuillaumeGomez force-pushed the private-to-public-path branch from 662530d to 1d1951b Compare July 7, 2023 15:03

fmease requested changes Jul 7, 2023

View reviewed changes

src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the private-to-public-path branch from 0f35e59 to 6861f33 Compare July 7, 2023 15:59

fmease approved these changes Jul 7, 2023

View reviewed changes

GuillaumeGomez force-pushed the private-to-public-path branch from 6861f33 to 4a1e06b Compare July 7, 2023 17:39

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 27, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 27, 2023

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 27, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 27, 2023

bors merged commit 9339f44 into rust-lang:master Jul 27, 2023

rustbot added this to the 1.73.0 milestone Jul 27, 2023

GuillaumeGomez deleted the private-to-public-path branch July 27, 2023 18:19

GuillaumeGomez added the perf-regression-triaged The performance regression has been triaged. label Jul 28, 2023

teohhanhui mentioned this pull request Oct 4, 2023

Rename crate::exid::ExId to crate::obj_id::ObjId automerge/automerge#755

Closed

[rustdoc] If re-export is private, get the next item until a public one is found or expose the private item directly #113374

[rustdoc] If re-export is private, get the next item until a public one is found or expose the private item directly #113374

Conversation

GuillaumeGomez commented Jul 5, 2023

fmease commented Jul 5, 2023

GuillaumeGomez commented Jul 5, 2023

This comment has been minimized.

bors commented Jul 5, 2023

bors commented Jul 5, 2023

This comment has been minimized.

rust-timer commented Jul 6, 2023

Overall result: ❌ regressions - no action needed

GuillaumeGomez commented Jul 6, 2023

fmease commented Jul 6, 2023 • edited Loading

GuillaumeGomez commented Jul 6, 2023

notriddle left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 7, 2023

GuillaumeGomez commented Jul 7, 2023

notriddle left a comment

Choose a reason for hiding this comment

fmease left a comment • edited Loading

Choose a reason for hiding this comment

fmease Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 7, 2023

GuillaumeGomez commented Jul 7, 2023

fmease left a comment • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 27, 2023

This comment has been minimized.

bors commented Jul 27, 2023

bors commented Jul 27, 2023

This comment has been minimized.

rust-timer commented Jul 27, 2023

Overall result: ❌ regressions - ACTION NEEDED

GuillaumeGomez commented Jul 27, 2023

notriddle commented Jul 27, 2023

GuillaumeGomez commented Jul 27, 2023

notriddle commented Jul 27, 2023

bors commented Jul 27, 2023

GuillaumeGomez commented Jul 27, 2023

bors commented Jul 27, 2023

bors commented Jul 27, 2023

rust-timer commented Jul 28, 2023

Overall result: ❌ regressions - ACTION NEEDED

rylev commented Aug 5, 2023

GuillaumeGomez commented Aug 5, 2023

rylev commented Aug 7, 2023

lqd commented Aug 7, 2023 • edited Loading

fmease commented Jul 6, 2023 •

edited

Loading

fmease left a comment •

edited

Loading

fmease Jul 7, 2023 •

edited

Loading

fmease Jul 7, 2023 •

edited

Loading

fmease left a comment •

edited

Loading

lqd commented Aug 7, 2023 •

edited

Loading