-
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
rustdoc: Remove Crate.primitives
#90447
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
64e0ea7
to
ee2ada4
Compare
This comment has been minimized.
This comment has been minimized.
This involves no longer filtering out impls on primitive types that aren't defined with `#[doc(primitive)]` somewhere in the dependency graph. But, I don't think that filtering is necessary or really has any effect.
ee2ada4
to
d663b8a
Compare
Wait, what does this mean? doc(primitive) can only go on modules, I don't know what it has to do with impls ... |
if let Some(target_prim) = target.primitive_type() { | ||
cleaner.prims.insert(target_prim); | ||
} else if let Some(target_did) = target.def_id_no_primitives() { | ||
if let Some(target_did) = target.def_id_no_primitives() { | ||
// `impl Deref<Target = S> for S` |
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 in particular makes me worried we'll no longer show Deref methods if an item derefs to a primitive type. Can you add a test for that case?
The job Click to see the possible cause of the failure (guessed by this bot)
|
Sorry, perhaps I wasn't clear. Let me explain my understanding of the old and new bheavior with an example. Take this impl: impl SomeTrait for u8 { ... } Before this PR, that impl would only be kept if After this PR, that impl would be unconditionally kept (although the current code might not work correctly; I'll add some tests). |
Come to think of it, I wonder if |
It turns out that removing it fixes some latent bugs in rustdoc 😆 Specifically, the Notable traits tooltip appears in more places, e.g. the definition (not the std re-export) of
There might be regressions mixed into the results too, but this is promising! |
So far, the only changes I've found make the |
@bors rollup=never (This may affect perf.) |
Running rustdoc on |
I'll try it on |
In theory, we don't care about most of the foreign impls. The |
But in practice, the |
Well, good luck in removing it then. 😉 Im really interested in the perf change if any. |
The hard part of removing it is making sure I don't break anything, but if anything, this will be including more information in the docs, so it shouldn't be possible to have a serious bug (modulo panics). |
☔ The latest upstream changes (presumably #90385) made this pull request unmergeable. Please resolve the merge conflicts. |
…ives, r=notriddle Remove Crate::primitives field It is a new approach to rust-lang#90447. Instead of removing primitives from everywhere (ie from `BadImplStripper`), I just removed them from the `Crate` type, allowing to reduce its size. cc `@camelid` r? `@notriddle`
This involves no longer filtering out impls on primitive types that
aren't defined with
#[doc(primitive)]
somewhere in the dependencygraph. But, I don't think that filtering is necessary or really has any
effect.