From b46412f6d7fde0c24780b0808aa7aefbc18d9e1e Mon Sep 17 00:00:00 2001 From: binarycat Date: Sun, 16 Mar 2025 15:42:04 -0500 Subject: [PATCH] rustdoc: be more strict about "Methods from Deref" hack: is_doc_subtype_of always returns true for TyAlias it's worth noting that this function is only used in the handling of "Methods from Deref", and we were previously assuming all generic parameters were meaningless, so this is still an improvment from the status quo. this change means that we will have strictly less false positives without adding any new false negitives. Co-authored-by: Guillaume Gomez --- src/librustdoc/clean/types.rs | 14 ++++++++++ src/librustdoc/html/render/mod.rs | 16 +++++++++-- src/librustdoc/html/render/sidebar.rs | 5 +++- .../deref/deref-methods-24686-target.rs | 27 +++++++++++++++++++ 4 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 tests/rustdoc/deref/deref-methods-24686-target.rs diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 3f9023659db..257687ae0a3 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -1533,6 +1533,10 @@ impl Type { matches!(self, Type::BorrowedRef { .. }) } + fn is_type_alias(&self) -> bool { + matches!(self, Type::Path { path: Path { res: Res::Def(DefKind::TyAlias, _), .. } }) + } + /// Check if two types are "the same" for documentation purposes. /// /// This is different from `Eq`, because it knows that things like @@ -1561,6 +1565,16 @@ impl Type { } else { (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`. + if self_cleared.is_type_alias() || other_cleared.is_type_alias() { + return true; + } + match (self_cleared, other_cleared) { // Recursive cases. (Type::Tuple(a), Type::Tuple(b)) => { diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index b2ad2fa773a..900de90c736 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -89,7 +89,7 @@ pub(crate) fn ensure_trailing_slash(v: &str) -> impl fmt::Display { /// Specifies whether rendering directly implemented trait items or ones from a certain Deref /// impl. -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub(crate) enum AssocItemRender<'a> { All, DerefFor { trait_: &'a clean::Path, type_: &'a clean::Type, deref_mut_: bool }, @@ -1296,7 +1296,8 @@ fn render_assoc_items_inner( info!("Documenting associated items of {:?}", containing_item.name); let cache = &cx.shared.cache; let Some(v) = cache.impls.get(&it) else { return }; - let (non_trait, traits): (Vec<_>, _) = v.iter().partition(|i| i.inner_impl().trait_.is_none()); + let (mut non_trait, traits): (Vec<_>, _) = + v.iter().partition(|i| i.inner_impl().trait_.is_none()); if !non_trait.is_empty() { let mut close_tags = >::with_capacity(1); let mut tmp_buf = String::new(); @@ -1314,6 +1315,16 @@ fn render_assoc_items_inner( AssocItemRender::DerefFor { trait_, type_, deref_mut_ } => { let id = cx.derive_id(small_url_encode(format!("deref-methods-{:#}", type_.print(cx)))); + // the `impls.get` above only looks at the outermost type, + // and the Deref impl may only be implemented for certain + // values of generic parameters. + // for example, if an item impls `Deref<[u8]>`, + // we should not show methods from `[MaybeUninit]`. + // this `retain` filters out any instances where + // the types do not line up perfectly. + non_trait.retain(|impl_| { + type_.is_doc_subtype_of(&impl_.inner_impl().for_, &cx.shared.cache) + }); let derived_id = cx.derive_id(&id); close_tags.push(""); write_str( @@ -1392,6 +1403,7 @@ fn render_assoc_items_inner( } } +/// `derefs` is the set of all deref targets that have already been handled. fn render_deref_methods( mut w: impl Write, cx: &Context<'_>, diff --git a/src/librustdoc/html/render/sidebar.rs b/src/librustdoc/html/render/sidebar.rs index 3130815af0b..9c78dcdc571 100644 --- a/src/librustdoc/html/render/sidebar.rs +++ b/src/librustdoc/html/render/sidebar.rs @@ -533,7 +533,10 @@ fn sidebar_deref_methods<'a>( debug!("found inner_impl: {impls:?}"); let mut ret = impls .iter() - .filter(|i| i.inner_impl().trait_.is_none()) + .filter(|i| { + i.inner_impl().trait_.is_none() + && real_target.is_doc_subtype_of(&i.inner_impl().for_, &c) + }) .flat_map(|i| get_methods(i.inner_impl(), true, used_links, deref_mut, cx.tcx())) .collect::>(); if !ret.is_empty() { diff --git a/tests/rustdoc/deref/deref-methods-24686-target.rs b/tests/rustdoc/deref/deref-methods-24686-target.rs new file mode 100644 index 00000000000..e019488ca80 --- /dev/null +++ b/tests/rustdoc/deref/deref-methods-24686-target.rs @@ -0,0 +1,27 @@ +#![crate_name = "foo"] + +// test for https://github.com/rust-lang/rust/issues/24686 +use std::ops::Deref; + +pub struct Foo(T); +impl Foo { + pub fn get_i32(&self) -> i32 { self.0 } +} +impl Foo { + pub fn get_u32(&self) -> u32 { self.0 } +} + +// Note that the same href is used both on the method itself, +// and on the sidebar items. +//@ has foo/struct.Bar.html +//@ has - '//a[@href="#method.get_i32"]' 'get_i32' +//@ !has - '//a[@href="#method.get_u32"]' 'get_u32' +//@ count - '//ul[@class="block deref-methods"]//a' 1 +//@ count - '//a[@href="#method.get_i32"]' 2 +pub struct Bar(Foo); +impl Deref for Bar { + type Target = Foo; + fn deref(&self) -> &Foo { + &self.0 + } +}