rustdoc: calculate visibility on-demand #91408

jyn514 · 2021-12-01T00:02:22Z

inline_stmt_id is very hacky and will probably lose all the memory improvements I was hoping for. Maybe we can change inlined items to have the DefId of the use statement directly instead of storing both?

Helps with #90852 (doesn't fix it because I haven't yet switched to ty::Visibility directly).

r? @camelid

src/librustdoc/clean/mod.rs

jyn514 · 2021-12-01T00:04:21Z

src/librustdoc/clean/types.rs

+    /// If this item was inlined, the DefId of the `use` statement.
+    crate inline_stmt_id: Option<DefId>,


src/librustdoc/clean/types.rs

jyn514 · 2021-12-01T00:33:49Z

@bors try @rust-timer queue

rust-timer · 2021-12-01T00:33:50Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-12-01T00:33:56Z

⌛ Trying commit 19fdd05f4e02e7a40d8f639e38cb7ead3a8e2ab2 with merge fbec2c38e225491dba4fab51850edaaacea88976...

bors · 2021-12-01T01:53:20Z

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

rust-timer · 2021-12-01T01:53:22Z

Queued fbec2c38e225491dba4fab51850edaaacea88976 with parent 207c80f, future comparison URL.

rust-timer · 2021-12-01T04:31:43Z

Finished benchmarking commit (fbec2c38e225491dba4fab51850edaaacea88976): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

Moderate improvement in instruction counts (up to -0.9% on full builds of helloworld)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led to changes in compiler perf.

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

camelid · 2021-12-01T21:48:59Z

Up to 1% improvements on doc instructions counts :)

max-rss is a bit mixed, though I think overall positive.

camelid · 2021-12-01T21:50:09Z

inline_stmt_id is very hacky and will probably lose all the memory improvements I was hoping for. Maybe we can change inlined items to have the DefId of the use statement directly instead of storing both?

Hmm, I'd rather avoid inline_stmt_id, but using the use's DefId for the inlined items seems even hackier.

src/librustdoc/clean/mod.rs

camelid · 2021-12-01T21:56:41Z

src/librustdoc/clean/types.rs

+            }
+            _ => {}
+        }
+        // If this item was inlined, show the visibility of the `use` statement, not the definition.


This doesn't seem quite correct. Take this code for example:

// crate a pub struct S { pub x: i32, y: i32, } // crate b pub(crate) use a::S;

If --document-private-items is passed, then I think the documentation for b::S should look like this (although I may not be remembering the inlining rules quite right):

pub(crate) struct S { pub(crate) x: i32, y: i32, }

But with your change, I think it will look like this:

pub(crate) struct S { pub(crate) x: i32, pub(crate) y: i32, }

Can you test that code (or a variant of it to get the inlining to happen) before and after this change to check?

Or perhaps this won't happen because inlining doesn't occur for struct fields? But I feel like this sort of bug could probably occur with other items.

This is exactly the same behavior as before, whatever that happened to be. I just moved the logic from try_inline to visibility().

also, I don't think this can happen for struct fields because you can't import them directly, only the struct itself. Enum variants can be imported, but they're special-cased earlier anyway.

I couldn't get y to appear with pub(crate) use (maybe due to #91094 ?) but with pub use the docs look like this on nightly:

so uhh yeah this is a pre-existing bug, good catch 😅

This is exactly the same behavior as before, whatever that happened to be. I just moved the logic from try_inline to visibility().

Ah, I see.

camelid · 2021-12-01T21:57:37Z

src/librustdoc/clean/types.rs

+        let def_id = match self.inline_stmt_id {
+            Some(inlined) => inlined,
+            None => def_id,
+        };


Shouldn't this check be done before the big match *self.kind { above? It seems like this does extra work for inlined items, and the code's a bit harder to understand.

No, because then all the uses of def_id in the match would be wrong (e.g. associated_item and overriding the ExternCrateItem def_id).

also I'm not sure what you mean by extra work - this isn't doing an early return, it's just reassigning def_id.

What I'm suggesting is changing this to be an early return. What's the point of calculating def_id earlier if this code overrides it anyway?

- lots of staring really hard at the existing code to make sure the behavior matches - store `inline_stmt_id` and `crate_stmt_id` to be able to get their visibility (rustdoc stores the IDs of the item definition in `clean::Item`, not the re-export)

camelid · 2021-12-01T23:19:36Z

src/librustdoc/clean/types.rs

+        /// The id of the `extern crate` statement, not the crate itself.
+        crate_stmt_id: LocalDefId,


Why can't this use Item.inline_stmt_id? Or I guess is this not quite the same as inline_stmt_id? Still, it feels like they should be merged somehow since they're similar.

bors · 2021-12-02T09:45:36Z

☔ The latest upstream changes (presumably #91291) made this pull request unmergeable. Please resolve the merge conflicts.

camelid · 2021-12-08T03:52:17Z

@bors rollup=never (since the clean/types.rs changes may impact perf)

jyn514 · 2022-02-11T04:25:46Z

not planning to follow up on this

Wardenfar · 2022-06-20T00:34:34Z

@camelid is there any interest in resolving the merge conflict on this PR ?

… r=notriddle Make rustdoc Item::visibility computed on-demand This is a take-over of rust-lang#91408. Helps with rust-lang#90852 (needs to use `ty::Visibility` directly too). cc `@camelid` r? `@notriddle`

Make rustdoc Item::visibility computed on-demand This is a take-over of rust-lang/rust#91408. Helps with rust-lang/rust#90852 (needs to use `ty::Visibility` directly too). cc `@camelid` r? `@notriddle`

jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. I-compiletime Issue: Problems and improvements with respect to compile times. labels Dec 1, 2021

rust-highfive assigned camelid Dec 1, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2021

jyn514 force-pushed the visibility branch from f172f44 to 9b440b7 Compare December 1, 2021 00:02

jyn514 commented Dec 1, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

jyn514 force-pushed the visibility branch 3 times, most recently from fc9f894 to 19fdd05 Compare December 1, 2021 00:31

jyn514 changed the title ~~WIP: rustdoc: calculate visibility on-demand~~ rustdoc: calculate visibility on-demand Dec 1, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 1, 2021

jyn514 marked this pull request as ready for review December 1, 2021 00:35

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 1, 2021

camelid reviewed Dec 1, 2021

View reviewed changes

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-visibility Area: Visibility / privacy and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2021

jyn514 force-pushed the visibility branch from 19fdd05 to 4ad14ac Compare December 1, 2021 22:37

jyn514 mentioned this pull request Dec 1, 2021

rustdoc: Inlining a foreign public struct with private fields shows the fields as private in the current crate #91440

Open

camelid reviewed Dec 1, 2021

View reviewed changes

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2022

jyn514 closed this Feb 11, 2022

GuillaumeGomez mentioned this pull request Oct 28, 2022

Make rustdoc Item::visibility computed on-demand #103690

Merged

jyn514 deleted the visibility branch November 3, 2022 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: calculate visibility on-demand #91408

rustdoc: calculate visibility on-demand #91408

jyn514 commented Dec 1, 2021 •

edited

Loading

jyn514 Dec 1, 2021

This comment has been minimized.

jyn514 commented Dec 1, 2021

rust-timer commented Dec 1, 2021

bors commented Dec 1, 2021

bors commented Dec 1, 2021

rust-timer commented Dec 1, 2021

rust-timer commented Dec 1, 2021

camelid commented Dec 1, 2021

camelid commented Dec 1, 2021

camelid Dec 1, 2021 •

edited

Loading

camelid Dec 1, 2021

jyn514 Dec 1, 2021

jyn514 Dec 1, 2021

jyn514 Dec 1, 2021

camelid Dec 1, 2021

camelid Dec 1, 2021

jyn514 Dec 1, 2021

jyn514 Dec 1, 2021

camelid Dec 1, 2021

camelid Dec 1, 2021 •

edited

Loading

bors commented Dec 2, 2021

camelid commented Dec 8, 2021

jyn514 commented Feb 11, 2022

Wardenfar commented Jun 20, 2022

		/// If this item was inlined, the DefId of the `use` statement.
		crate inline_stmt_id: Option<DefId>,

		/// The id of the `extern crate` statement, not the crate itself.
		crate_stmt_id: LocalDefId,

rustdoc: calculate visibility on-demand #91408

rustdoc: calculate visibility on-demand #91408

Conversation

jyn514 commented Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

jyn514 commented Dec 1, 2021

rust-timer commented Dec 1, 2021

bors commented Dec 1, 2021

bors commented Dec 1, 2021

rust-timer commented Dec 1, 2021

rust-timer commented Dec 1, 2021

camelid commented Dec 1, 2021

camelid commented Dec 1, 2021

camelid Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

bors commented Dec 2, 2021

camelid commented Dec 8, 2021

jyn514 commented Feb 11, 2022

Wardenfar commented Jun 20, 2022

jyn514 commented Dec 1, 2021 •

edited

Loading

camelid Dec 1, 2021 •

edited

Loading

camelid Dec 1, 2021 •

edited

Loading