Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid documenting top-level private imports #91094

Merged
merged 2 commits into from
Nov 23, 2021

Conversation

inquisitivecrystal
Copy link
Contributor

PR #88447 aimed to make rustdoc's --document-private-items mode only document imports that are visible outside the importing module. Unfortunately, I inadvertently set things up so that imports at the crate top-level are always documented, regardless of their visibility. This behavior was unintended and is not desirable.

This PR treats top-level imports as never being visible outside their parent module. In practice, the only way a top-level import can be visible externally is if it's fully public, and there's a seperate check for that.

It's worth calling attention to the fact that this change means that pub(crate) imports will be visible in lower level modules, but not at the top-level. This is because, at the top level of the crate, pub(crate) means the same thing as pub(self).

It turned out that there were existing tests checking for the only behavior, which I didn't notice at the time of my previous PR. I have updated them to check for the new behavior and substantially extended them to handle differences between the top-level module and lower level modules. I may have gone overboard, so please tell me if there's anything I should cut.

r? @jyn514

Fixes #90865.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 20, 2021
@inquisitivecrystal inquisitivecrystal changed the title Rustdoc top mod Avoid documenting top-level private imports Nov 20, 2021
@inquisitivecrystal inquisitivecrystal added A-visibility Area: Visibility / privacy. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 20, 2021
@inquisitivecrystal
Copy link
Contributor Author

I just realized it would probably be more readable if the order of the definitions in src/test/rustdoc/auxiliary/reexports.rs was the same as the order of the imports in src/test/rustdoc/reexports.rs and src/test/rustdoc/reexports-priv.rs. I'll fix that and force-push later.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ... Quite a lot of tests 😂 the change to rustdoc itself looks fine, I'm going to wait to review the tests until you reorder them like you mentioned.

@jyn514 jyn514 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2021
@inquisitivecrystal
Copy link
Contributor Author

inquisitivecrystal commented Nov 21, 2021

This is ... Quite a lot of tests 😂 the change to rustdoc itself looks fine, I'm going to wait to review the tests until you reorder them like you mentioned.

Done!

The way the existing tests were set up does imports with a bunch of different sorts of things. I suppose the idea is to make sure that different imported types don't break things? I've mirrored that in my new tests. It's possible that it would be helpful to test for different things, like different types of visibility, but that's not how the existing tests are set up.

@jyn514 jyn514 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 Nov 21, 2021
@jyn514
Copy link
Member

jyn514 commented Nov 23, 2021

I think these tests are pretty overkill 😆 but overkill never hurt anything. Thanks for the quick fix!

@bors r+ p=1 (needs to land before the beta promotion on Friday)

@bors
Copy link
Contributor

bors commented Nov 23, 2021

📌 Commit 3c51038 has been approved by jyn514

@bors 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 Nov 23, 2021
@bors
Copy link
Contributor

bors commented Nov 23, 2021

⌛ Testing commit 3c51038 with merge 2e055d9...

@bors
Copy link
Contributor

bors commented Nov 23, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 2e055d9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 23, 2021
@bors bors merged commit 2e055d9 into rust-lang:master Nov 23, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 23, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2e055d9): comparison url.

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

  • Very large improvement in instruction counts (up to -45.1% on full builds of webrender-wrench)

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

@rustbot label: -perf-regression

@inquisitivecrystal
Copy link
Contributor Author

CC @nnethercote: these new results seem related to the perf regressions that were being attributed to #88447.

@nnethercote
Copy link
Contributor

Excellent! Instruction counts went from ~5.9 billion to ~10.9 billion in #90769. This commit changes them from ~11.1 billion to ~6.1 billion. So it seems like there was a bit of movement elsewhere, but this PR gets back all of the big regression :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-visibility Area: Visibility / privacy. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rustdoc is inlining use ... imports when --document-private-items is passed
7 participants