Repository navigation
[rustdoc] Render Deref methods on aliased items - #163571
GuillaumeGomez wants to merge 6 commits into
Conversation
rustbot
added
S-waiting-on-review
|
|
| let mut assoc_types = Vec::new(); | ||
| let mut assoc_fns = Vec::new(); | ||
| let mut methods = Vec::new(); | ||
| if let Some(v) = cache.impls.get(&did) { |
There was a problem hiding this comment.
Most of this diff comes from the removal of one level of indent which became (a few lines above):
let v = match cache.impls.get(&item_def_id) {
Some(impls) => impls.as_slice(),
None if aliased_type.is_some() => &[],
None => return,
};Since we don't render anything, we should have returned early. Now it's done, but ugly diff.
| @@ -54,7 +54,7 @@ impl std::ops::Deref for GenericStruct<u32> { | |||
| pub struct Wrap<T>(GenericStruct<T>); | |||
|
|
|||
| //@ has 'foo/type.Alias.html' | |||
| //@ !has - '//h2' 'Methods from Deref<Target = u32>' | |||
| //@ has - '//h2' 'Methods from Deref<Target = u32>' | |||
There was a problem hiding this comment.
Now we have a Deref on type aliases, so we update here. :3
| let for_ty = self.cx.tcx().type_of(impl_did).skip_binder(); | ||
| let reject_cx = DeepRejectCtxt::relate_infer_infer(self.cx.tcx()); | ||
| if !reject_cx.types_may_unify(aliased_ty, for_ty) { | ||
| if !super::impl_may_apply_to_type_alias(self.cx.tcx(), self_did, impl_did) { |
There was a problem hiding this comment.
Like I mentioned in the first comment, took this code and re-used it to filter which Deref was matching the aliased ty.
| let mut deref_impl = None; | ||
| let mut has_deref_mut = false; | ||
|
|
||
| for impl_ in impls { |
There was a problem hiding this comment.
To avoid doing two loops (one to get Deref and the other to check if we also implement DerefMut), inlined it into one.
| ) -> bool { | ||
| let alias_ty = tcx.type_of(alias_def_id).skip_binder(); | ||
| let for_ty = tcx.type_of(impl_def_id).skip_binder(); | ||
| DeepRejectCtxt::relate_infer_infer(tcx).types_may_unify(alias_ty, for_ty) |
There was a problem hiding this comment.
I thinkj we can use types_may_unify in the type alias case of is_doc_subtype_of to improve #24686 significantly.
WIP diff of this idea
diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs
index a4ec40a0035..09f253ef643 100644
--- a/src/librustdoc/clean/types.rs
+++ b/src/librustdoc/clean/types.rs
@@ -1464,7 +1464,7 @@ fn is_type_alias(&self) -> bool {
///
/// An owned type is also the same as its borrowed variants (this is commutative),
/// but `&T` is not the same as `&mut T`.
- pub(crate) fn is_doc_subtype_of(&self, other: &Self, cache: &Cache) -> bool {
+ pub(crate) fn is_doc_subtype_of(&self, other: &Self, cx: &Context) -> bool {
// Strip the references so that it can compare the actual types, unless both are references.
// If both are references, leave them alone and compare the mutabilities later.
let (self_cleared, other_cleared) = if !self.is_borrowed_ref() || !other.is_borrowed_ref() {
@@ -1473,29 +1473,35 @@ pub(crate) fn is_doc_subtype_of(&self, other: &Self, cache: &Cache) -> bool {
(self, other)
};
- // FIXME: `Cache` does not have the data required to unwrap type aliases,
- // so we just assume they are equal.
- // This is only remotely acceptable because we were previously
- // assuming all types were equal when used
- // as a generic parameter of a type in `Deref::Target`.
+ // FIXME(checked_type_alias): Once the feature is complete or stable, rewrite this
+ // to use type unification.
+ // Be aware of `tests/rustdoc-html/type-alias/deeply-nested-112515.rs` which might
+ // regress.
if self_cleared.is_type_alias() || other_cleared.is_type_alias() {
- return true;
+ if let Some(self_did) = self_cleared.def_id(&cx.cache()) &&
+ let Some(other_did) = other_cleared.def_id(&cx.cache())
+ {
+ let tcx = cx.tcx();
+
+ return crate::clean::ty::DeepRejectCtxt::relate_infer_infer(cx.tcx()).types_may_unify(tcx.type_of(self_did).skip_binder(), tcx.type_of(other_did).skip_binder());
+ }
+
}
match (self_cleared, other_cleared) {
// Recursive cases.
(Type::Tuple(a), Type::Tuple(b)) => {
- a.iter().eq_by(b, |a, b| a.is_doc_subtype_of(b, cache))
+ a.iter().eq_by(b, |a, b| a.is_doc_subtype_of(b, cx))
}
- (Type::Slice(a), Type::Slice(b)) => a.is_doc_subtype_of(b, cache),
- (Type::Array(a, al), Type::Array(b, bl)) => al == bl && a.is_doc_subtype_of(b, cache),
+ (Type::Slice(a), Type::Slice(b)) => a.is_doc_subtype_of(b, cx),
+ (Type::Array(a, al), Type::Array(b, bl)) => al == bl && a.is_doc_subtype_of(b, cx),
(Type::RawPointer(mutability, type_), Type::RawPointer(b_mutability, b_type_)) => {
- mutability == b_mutability && type_.is_doc_subtype_of(b_type_, cache)
+ mutability == b_mutability && type_.is_doc_subtype_of(b_type_, cx)
}
(
Type::BorrowedRef { mutability, type_, .. },
Type::BorrowedRef { mutability: b_mutability, type_: b_type_, .. },
- ) => mutability == b_mutability && type_.is_doc_subtype_of(b_type_, cache),
+ ) => mutability == b_mutability && type_.is_doc_subtype_of(b_type_, cx),
// Placeholders are equal to all other types.
(Type::Infer, _) | (_, Type::Infer) => true,
// Generics match everything on the right, but not on the left.
@@ -1509,13 +1515,13 @@ pub(crate) fn is_doc_subtype_of(&self, other: &Self, cache: &Cache) -> bool {
a.def_id() == b.def_id()
&& a.generics()
.zip(b.generics())
- .map(|(ag, bg)| ag.zip(bg).all(|(at, bt)| at.is_doc_subtype_of(bt, cache)))
+ .map(|(ag, bg)| ag.zip(bg).all(|(at, bt)| at.is_doc_subtype_of(bt, cx)))
.unwrap_or(true)
}
// Other cases, such as primitives, just use recursion.
(a, b) => a
- .def_id(cache)
- .and_then(|a| Some((a, b.def_id(cache)?)))
+ .def_id(cx.cache())
+ .and_then(|a| Some((a, b.def_id(cx.cache())?)))
.map(|(a, b)| a == b)
.unwrap_or(false),
}
diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs
index a9ecf6d66d0..184591d350e 100644
--- a/src/librustdoc/html/render/mod.rs
+++ b/src/librustdoc/html/render/mod.rs
@@ -1506,7 +1506,7 @@ fn render_assoc_items_inner(
// this `retain` filters out any instances where
// the types do not line up perfectly.
inherent_impls.retain(|impl_| {
- type_.is_doc_subtype_of(&impl_.inner_impl().for_, &cx.shared.cache)
+ type_.is_doc_subtype_of(&impl_.inner_impl().for_, cx)
});
let derived_id = cx.derive_id(&id);
if let Some(def_id) = type_.def_id(cx.cache()) {
@@ -1694,7 +1694,7 @@ fn notable_traits_button(ty: &clean::Type, cx: &Context<'_>) -> Option<impl fmt:
impl_.polarity == ty::ImplPolarity::Positive
// Two different types might have the same did,
// without actually being the same.
- && ty.is_doc_subtype_of(&impl_.for_, cx.cache())
+ && ty.is_doc_subtype_of(&impl_.for_, cx)
})
.filter_map(|impl_| impl_.trait_.as_ref())
.filter_map(|trait_| cx.cache().traits.get(&trait_.def_id()))
@@ -1724,7 +1724,7 @@ fn notable_traits_decl(ty: &clean::Type, cx: &Context<'_>) -> (String, String) {
.filter(|impl_| impl_.polarity == ty::ImplPolarity::Positive)
.filter(|impl_| {
// Two different types might have the same did, without actually being the same.
- ty.is_doc_subtype_of(&impl_.for_, cx.cache())
+ ty.is_doc_subtype_of(&impl_.for_, cx)
})
.filter_map(|impl_| {
if let Some(trait_) = &impl_.trait_
diff --git a/src/librustdoc/html/render/sidebar.rs b/src/librustdoc/html/render/sidebar.rs
index 6fa033f927c..cdd429da918 100644
--- a/src/librustdoc/html/render/sidebar.rs
+++ b/src/librustdoc/html/render/sidebar.rs
@@ -561,7 +561,7 @@ fn sidebar_deref_methods<'a>(
.iter()
.filter(|i| {
i.inner_impl().trait_.is_none()
- && real_target.is_doc_subtype_of(&i.inner_impl().for_, c)
+ && real_target.is_doc_subtype_of(&i.inner_impl().for_, cx)
})
.flat_map(|i| {
get_methods(| if !impl_.is_negative_trait_impl() && applies(impl_) { | ||
| deref_impl = Some(impl_); | ||
| } | ||
| } else if impl_.trait_did() == tcx.lang_items().deref_mut_trait() && applies(impl_) { |
There was a problem hiding this comment.
Maybe Impl should have utility functions trait_is_deref and trait_is_deref_mut so we don't need to keep using this snippet everywhere?
| let deref = if let Some(impl_) = v.iter().find(|i| { | ||
| i.trait_did() == cx.tcx().lang_items().deref_trait() && !i.is_negative_trait_impl() | ||
| }) { | ||
| let deref_mut = v.iter().any(|i| i.trait_did() == cx.tcx().lang_items().deref_mut_trait()); | ||
| Some((impl_, deref_mut)) | ||
| } else { | ||
| aliased_type | ||
| .and_then(|aliased_did| super::aliased_type_deref_impl(cx, item_def_id, aliased_did)) | ||
| }; |
There was a problem hiding this comment.
This exactly repeats a snippet used earlier in render/mod.rs
I think instead of aliased_type_deref_impl we should just have a get_deref_impl that works in both the general and aliased case (might need a doc comment to explain what all the parameters mean, or maybe just a renaming of the parameters).
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
one more thing: can you check locally if these change fixes the issues with |
Confirmed it's fixed. Added the code from the first comment of the issue as regression test. |
|
Applied all suggestions. @rustbot ready |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Had to remove the |
| if trait_.as_ref().is_some_and(|t| t.is_deref_trait(tcx)) | ||
| && tcx.impl_polarity(def_id) != ty::ImplPolarity::Negative |
There was a problem hiding this comment.
the impl polarity test maybe should go in is_deref_trait, as we'll usually care about that, but then it might need a new name? subjective.
There was a problem hiding this comment.
Hum... Fair point, gonna rename it is_non_negative_deref_trait.
There was a problem hiding this comment.
On second thought, in some cases the the trait polarity is queried ahead of time, so I'll them them separate for now.
| if let Some(self_def_id) = self_cleared.def_id(&cache) | ||
| && let Some(other_def_id) = other_cleared.def_id(&cache) | ||
| { | ||
| return impl_may_apply_to_type_alias(cx.tcx(), self_def_id, other_def_id); |
There was a problem hiding this comment.
this (and my patch) are still subtly wrong, as the special rustdoc-specific rules is_doc_subtype_of implements won't apply when type aliases are involved. still an improvement over the status quo, but worth a note.
There was a problem hiding this comment.
if i understand the code correctly, Option<u8> and Option<&u8> will compare as doc subtypes, but Option<AliasOfRefU8> and Option<u8> won't.
There was a problem hiding this comment.
Might be worth a test to confirm that. Gonna add one.
There was a problem hiding this comment.
Nope, it works. Adding test. =D
There was a problem hiding this comment.
Ah no nevermind, the filtering is not working. Too laxist. Oh well, that will be a follow-up. :')
|
Added a test which illustrates the current limitations around type aliases. |
This comment has been minimized.
This comment has been minimized.
c106bca to
7259f23
Compare
This comment has been minimized.
This comment has been minimized.
|
CI passed. |
This comment has been minimized.
This comment has been minimized.
…s of `Type::is_doc_subtype_of`
7259f23 to
dcb9f0b
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This branch has not been deployed
4 participants
View all comments
Fixes #134868.
Fixes #24686.
The biggest issue was to make it work only for the matching aliased type generics. Luckily for me, it was already done in
write_shared.rs(where we generate the JS file containing the impls that we then put into the generated web pages, which makes me wonder why we don't put that directly into the page by default, but debate for another day...).r? @lolbinarycat