-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Conversation
I just realized it would probably be more readable if the order of the definitions in |
There was a problem hiding this 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.
3154152
to
3c51038
Compare
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. |
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) |
📌 Commit 3c51038 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (2e055d9): comparison url. Summary: This change led to very large relevant improvements 🎉 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
CC @nnethercote: these new results seem related to the perf regressions that were being attributed to #88447. |
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 :) |
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 aspub(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.