From ee6b228b6a8b874ca8b6e88cd58d4618cf20fb20 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Thu, 9 Mar 2023 13:41:13 -0700 Subject: [PATCH] rustdoc: handle generics better when matching notable traits This commit makes the `clean::Type::is_same` non-commutative, so that a generic `impl` matches a concrete return, but a generic return does not match a concrete `impl`. It makes slice and vector Write for `u8` not match on every generic return value. --- src/librustdoc/clean/types.rs | 46 +++++++++++++++++-- src/librustdoc/clean/types/tests.rs | 11 +++++ src/librustdoc/html/render/mod.rs | 4 +- .../notable-trait/doc-notable_trait-slice.rs | 6 +++ .../notable-trait/notable-trait-generics.rs | 35 ++++++++++++++ 5 files changed, 96 insertions(+), 6 deletions(-) create mode 100644 tests/rustdoc/notable-trait/notable-trait-generics.rs diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 6d8380c5fcc..5217de7aa8a 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -1471,11 +1471,37 @@ pub(crate) fn without_borrowed_ref(&self) -> &Type { result } - /// Check if two types are "potentially the same". + pub(crate) fn is_borrowed_ref(&self) -> bool { + matches!(self, Type::BorrowedRef { .. }) + } + + /// Check if two types are "the same" for documentation purposes. + /// /// This is different from `Eq`, because it knows that things like /// `Placeholder` are possible matches for everything. + /// + /// This relation is not commutative when generics are involved: + /// + /// ```ignore(private) + /// # // see types/tests.rs:is_same_generic for the real test + /// use rustdoc::format::cache::Cache; + /// use rustdoc::clean::types::{Type, PrimitiveType}; + /// let cache = Cache::new(false); + /// let generic = Type::Generic(rustc_span::symbol::sym::Any); + /// let unit = Type::Primitive(PrimitiveType::Unit); + /// assert!(!generic.is_same(&unit, &cache)); + /// assert!(unit.is_same(&generic, &cache)); + /// ``` + /// + /// 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_same(&self, other: &Self, cache: &Cache) -> bool { - match (self, other) { + let (self_cleared, other_cleared) = if !self.is_borrowed_ref() || !other.is_borrowed_ref() { + (self.without_borrowed_ref(), other.without_borrowed_ref()) + } else { + (self, other) + }; + match (self_cleared, other_cleared) { // Recursive cases. (Type::Tuple(a), Type::Tuple(b)) => { a.len() == b.len() && a.iter().zip(b).all(|(a, b)| a.is_same(b, cache)) @@ -1489,9 +1515,21 @@ pub(crate) fn is_same(&self, other: &Self, cache: &Cache) -> bool { Type::BorrowedRef { mutability, type_, .. }, Type::BorrowedRef { mutability: b_mutability, type_: b_type_, .. }, ) => mutability == b_mutability && type_.is_same(b_type_, cache), - // Placeholders and generics are equal to all other types. + // Placeholders are equal to all other types. (Type::Infer, _) | (_, Type::Infer) => true, - (Type::Generic(_), _) | (_, Type::Generic(_)) => true, + // Generics match everything on the right, but not on the left. + (_, Type::Generic(_)) => true, + (Type::Generic(_), _) => false, + // Paths account for both the path itself and its generics. + (Type::Path { path: a }, Type::Path { path: b }) => { + a.def_id() == b.def_id() + && a.generics() + .zip(b.generics()) + .map(|(ag, bg)| { + ag.iter().zip(bg.iter()).all(|(at, bt)| at.is_same(bt, cache)) + }) + .unwrap_or(true) + } // Other cases, such as primitives, just use recursion. (a, b) => a .def_id(cache) diff --git a/src/librustdoc/clean/types/tests.rs b/src/librustdoc/clean/types/tests.rs index 20627c2cfc1..7df87a9804a 100644 --- a/src/librustdoc/clean/types/tests.rs +++ b/src/librustdoc/clean/types/tests.rs @@ -69,3 +69,14 @@ fn should_not_trim() { run_test("\t line1 \n\t line2", "line1 \nline2"); run_test(" \tline1 \n \tline2", "line1 \nline2"); } + +#[test] +fn is_same_generic() { + use crate::clean::types::{PrimitiveType, Type}; + use crate::formats::cache::Cache; + let cache = Cache::new(false); + let generic = Type::Generic(rustc_span::symbol::sym::Any); + let unit = Type::Primitive(PrimitiveType::Unit); + assert!(!generic.is_same(&unit, &cache)); + assert!(unit.is_same(&generic, &cache)); +} diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index e6a040d02e5..f8c26dc4706 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -1291,7 +1291,7 @@ pub(crate) fn notable_traits_button(ty: &clean::Type, cx: &mut Context<'_>) -> O if let Some(impls) = cx.cache().impls.get(&did) { for i in impls { let impl_ = i.inner_impl(); - if !impl_.for_.without_borrowed_ref().is_same(ty.without_borrowed_ref(), cx.cache()) { + if !ty.is_same(&impl_.for_, cx.cache()) { // Two different types might have the same did, // without actually being the same. continue; @@ -1327,7 +1327,7 @@ fn notable_traits_decl(ty: &clean::Type, cx: &Context<'_>) -> (String, String) { for i in impls { let impl_ = i.inner_impl(); - if !impl_.for_.without_borrowed_ref().is_same(ty.without_borrowed_ref(), cx.cache()) { + if !ty.is_same(&impl_.for_, cx.cache()) { // Two different types might have the same did, // without actually being the same. continue; diff --git a/tests/rustdoc/notable-trait/doc-notable_trait-slice.rs b/tests/rustdoc/notable-trait/doc-notable_trait-slice.rs index 2411da8cd45..ef206710b4b 100644 --- a/tests/rustdoc/notable-trait/doc-notable_trait-slice.rs +++ b/tests/rustdoc/notable-trait/doc-notable_trait-slice.rs @@ -18,3 +18,9 @@ pub fn bare_fn_matches() -> &'static [SomeStruct] { pub fn bare_fn_no_matches() -> &'static [OtherStruct] { &[] } + +// @has doc_notable_trait_slice/fn.bare_fn_mut_no_matches.html +// @count - '//script[@id="notable-traits-data"]' 0 +pub fn bare_fn_mut_no_matches() -> &'static mut [SomeStruct] { + &mut [] +} diff --git a/tests/rustdoc/notable-trait/notable-trait-generics.rs b/tests/rustdoc/notable-trait/notable-trait-generics.rs new file mode 100644 index 00000000000..7bfe9d43ea9 --- /dev/null +++ b/tests/rustdoc/notable-trait/notable-trait-generics.rs @@ -0,0 +1,35 @@ +#![feature(doc_notable_trait)] + +// Notable traits SHOULD be shown when the `impl` has a generic type and the +// return type has a concrete type. +pub mod generic_return { + pub struct Wrapper(T); + + #[doc(notable_trait)] + pub trait NotableTrait {} + + impl NotableTrait for Wrapper {} + + // @has notable_trait_generics/generic_return/fn.returning.html + // @!has - '//a[@class="tooltip"]/@data-notable-ty' 'Wrapper' + pub fn returning() -> Wrapper { + loop {} + } +} + +// Notable traits SHOULD NOT be shown when the `impl` has a concrete type and +// the return type has a generic type. +pub mod generic_impl { + pub struct Wrapper(T); + + #[doc(notable_trait)] + pub trait NotableTrait {} + + impl NotableTrait for Wrapper {} + + // @has notable_trait_generics/generic_impl/fn.returning.html + // @has - '//a[@class="tooltip"]/@data-notable-ty' 'Wrapper' + pub fn returning() -> Wrapper { + loop {} + } +}