Skip to content
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: show trait implementors in sidebar #91277

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/librustdoc/formats/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,8 @@ impl Impl {
crate fn trait_did(&self) -> Option<DefId> {
self.inner_impl().trait_.as_ref().map(|t| t.def_id())
}

crate fn is_local(&self, cache: &cache::Cache) -> bool {
self.inner_impl().for_.def_id(cache).map_or(true, |d| cache.paths.contains_key(&d))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function name is not accurate (see #86798). What do you intend for the behavior to be?

}
}
6 changes: 5 additions & 1 deletion src/librustdoc/html/render/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ crate struct Context<'tcx> {
pub(super) deref_id_map: RefCell<FxHashMap<DefId, String>>,
/// The map used to ensure all generated 'id=' attributes are unique.
pub(super) id_map: RefCell<IdMap>,
/// Converts the rustc id of an item to the html id so we can link to it
pub(super) item_to_html_id_map: RefCell<FxHashMap<clean::ItemId, String>>,
/// Shared mutable state.
///
/// Issue for improving the situation: [#82381][]
Expand All @@ -73,7 +75,7 @@ crate struct Context<'tcx> {

// `Context` is cloned a lot, so we don't want the size to grow unexpectedly.
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Context<'_>, 144);
rustc_data_structures::static_assert_size!(Context<'_>, 184);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This increase seems to be fine, as when cloned, we create a new map, so perf is neutral (#91257)


/// Shared mutable state used in [`Context`] and elsewhere.
crate struct SharedContext<'tcx> {
Expand Down Expand Up @@ -517,6 +519,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
render_redirect_pages: false,
id_map: RefCell::new(id_map),
deref_id_map: RefCell::new(FxHashMap::default()),
item_to_html_id_map: RefCell::new(FxHashMap::default()),
shared: Rc::new(scx),
include_sources,
};
Expand All @@ -541,6 +544,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
dst: self.dst.clone(),
render_redirect_pages: self.render_redirect_pages,
deref_id_map: RefCell::new(FxHashMap::default()),
item_to_html_id_map: RefCell::new(FxHashMap::default()),
id_map: RefCell::new(IdMap::new()),
shared: Rc::clone(&self.shared),
include_sources: self.include_sources,
Expand Down
148 changes: 84 additions & 64 deletions src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1613,6 +1613,7 @@ fn render_impl(
);
write!(w, "<summary>")
}

render_impl_summary(
w,
cx,
Expand Down Expand Up @@ -1691,7 +1692,7 @@ pub(crate) fn render_impl_summary(
// in documentation pages for trait with automatic implementations like "Send" and "Sync".
aliases: &[String],
) {
let id = cx.derive_id(match i.inner_impl().trait_ {
let html_id = cx.derive_id(match i.inner_impl().trait_ {
Some(ref t) => {
if is_on_foreign_type {
get_id_for_impl_on_foreign_type(&i.inner_impl().for_, t, cx)
Expand All @@ -1701,14 +1702,17 @@ pub(crate) fn render_impl_summary(
}
None => "impl".to_string(),
});

cx.item_to_html_id_map.borrow_mut().insert(i.impl_item.def_id, html_id.clone());

let aliases = if aliases.is_empty() {
String::new()
} else {
format!(" data-aliases=\"{}\"", aliases.join(","))
};
write!(w, "<div id=\"{}\" class=\"impl has-srclink\"{}>", id, aliases);
write!(w, "<div id=\"{}\" class=\"impl has-srclink\"{}>", html_id, aliases);
render_rightside(w, cx, &i.impl_item, containing_item);
write!(w, "<a href=\"#{}\" class=\"anchor\"></a>", id);
write!(w, "<a href=\"#{}\" class=\"anchor\"></a>", html_id);
write!(w, "<h3 class=\"code-header in-band\">");

if let Some(use_absolute) = use_absolute {
Expand Down Expand Up @@ -2214,20 +2218,12 @@ fn get_id_for_impl_on_foreign_type(
small_url_encode(format!("impl-{:#}-for-{:#}", trait_.print(cx), for_.print(cx)))
}

fn extract_for_impl_name(item: &clean::Item, cx: &Context<'_>) -> Option<(String, String)> {
match *item.kind {
clean::ItemKind::ImplItem(ref i) => {
i.trait_.as_ref().map(|trait_| {
// Alternative format produces no URLs,
// so this parameter does nothing.
(
format!("{:#}", i.for_.print(cx)),
get_id_for_impl_on_foreign_type(&i.for_, trait_, cx),
)
})
}
_ => None,
}
fn extract_for_impl_name(item: &Impl, cx: &Context<'_>) -> (String, String) {
let i = item.inner_impl();
let trait_ = i.trait_.as_ref().unwrap();
// Alternative format produces no URLs,
// so this parameter does nothing.
(format!("{:#}", i.for_.print(cx)), get_id_for_impl_on_foreign_type(&i.for_, trait_, cx))
}

fn sidebar_trait(cx: &Context<'_>, buf: &mut Buffer, it: &clean::Item, t: &clean::Trait) {
Expand All @@ -2236,101 +2232,125 @@ fn sidebar_trait(cx: &Context<'_>, buf: &mut Buffer, it: &clean::Item, t: &clean
fn print_sidebar_section(
out: &mut Buffer,
items: &[clean::Item],
before: &str,
title: &str,
id: &str,
filter: impl Fn(&clean::Item) -> bool,
write: impl Fn(&mut Buffer, &str),
after: &str,
) {
let mut items = items
.iter()
.filter_map(|m| match m.name {
Some(ref name) if filter(m) => Some(name.as_str()),
_ => None,
})
.collect::<Vec<_>>();
print_sidebar_section_with(
out,
title,
id,
items.iter().filter(|i| filter(i)).filter_map(|i| i.name.map(|n| n.as_str())),
|buf, s| write(buf, &s),
)
}

fn print_sidebar_section_with<T: Ord>(
out: &mut Buffer,
title: &str,
id: &str,
iter: impl Iterator<Item = T>,
write: impl Fn(&mut Buffer, T),
) {
let mut items = iter.collect::<Vec<_>>();

if !items.is_empty() {
items.sort_unstable();
out.push_str(before);
write!(
out,
"<h3 class=\"sidebar-title\"><a href=\"#{}\">\
{}</a></h3><div class=\"sidebar-links\">",
id, title
);
for item in items.into_iter() {
write(out, &item);
write(out, item);
}
out.push_str(after);
out.push_str("</div>");
}
}

print_sidebar_section(
buf,
&t.items,
"<h3 class=\"sidebar-title\"><a href=\"#associated-types\">\
Associated Types</a></h3><div class=\"sidebar-links\">",
"Associated Types",
"associated-types",
|m| m.is_associated_type(),
|out, sym| write!(out, "<a href=\"#associatedtype.{0}\">{0}</a>", sym),
"</div>",
);

print_sidebar_section(
buf,
&t.items,
"<h3 class=\"sidebar-title\"><a href=\"#associated-const\">\
Associated Constants</a></h3><div class=\"sidebar-links\">",
"Associated Constants",
"associated-const",
|m| m.is_associated_const(),
|out, sym| write!(out, "<a href=\"#associatedconstant.{0}\">{0}</a>", sym),
"</div>",
);

print_sidebar_section(
buf,
&t.items,
"<h3 class=\"sidebar-title\"><a href=\"#required-methods\">\
Required Methods</a></h3><div class=\"sidebar-links\">",
"Required Methods",
"required-methods",
|m| m.is_ty_method(),
|out, sym| write!(out, "<a href=\"#tymethod.{0}\">{0}</a>", sym),
"</div>",
);

print_sidebar_section(
buf,
&t.items,
"<h3 class=\"sidebar-title\"><a href=\"#provided-methods\">\
Provided Methods</a></h3><div class=\"sidebar-links\">",
"Provided Methods",
"provided-methods",
|m| m.is_method(),
|out, sym| write!(out, "<a href=\"#method.{0}\">{0}</a>", sym),
"</div>",
);

let cache = cx.cache();
if let Some(implementors) = cache.implementors.get(&it.def_id.expect_def_id()) {
let mut res = implementors
.iter()
.filter(|i| {
i.inner_impl().for_.def_id(cache).map_or(false, |d| !cache.paths.contains_key(&d))
})
.filter_map(|i| extract_for_impl_name(&i.impl_item, cx))
.collect::<Vec<_>>();

if !res.is_empty() {
res.sort();
buf.push_str(
"<h3 class=\"sidebar-title\"><a href=\"#foreign-impls\">\
Implementations on Foreign Types</a></h3>\
<div class=\"sidebar-links\">",
);
for (name, id) in res.into_iter() {
write!(buf, "<a href=\"#{}\">{}</a>", id, Escape(&name));
}
buf.push_str("</div>");
}
}
let did = it.def_id.expect_def_id();

let (local_impl, foreign_impls) = cache
.implementors
.get(&did)
.iter()
.flat_map(|x| *x)
.partition::<Vec<_>, _>(|i| i.is_local(cache));

print_sidebar_section_with(
buf,
"Implementations on Foreign Types",
"foreign-impls",
foreign_impls.iter().map(|i| extract_for_impl_name(i, cx)),
|buf, (name, id)| write!(buf, "<a href=\"#{}\">{}</a>", id, Escape(&name)),
);

sidebar_assoc_items(cx, buf, it);

buf.push_str("<h3 class=\"sidebar-title\"><a href=\"#implementors\">Implementors</a></h3>");
print_sidebar_section_with(
buf,
"Implementors",
"implementors",
local_impl.iter().filter(|i| i.inner_impl().polarity == ty::ImplPolarity::Positive).map(
|i| {
(
format!("{:#}", i.inner_impl().for_.print(cx)),
cx.item_to_html_id_map
.borrow()
.get(&i.impl_item.def_id)
.unwrap_or_else(|| panic!("Not in index {:#?}", i))
.clone(),
)
},
),
|buf, (name, id)| write!(buf, "<a href=\"#{}\">{}</a>", id, Escape(&name)),
);

if t.is_auto {
buf.push_str(
"<h3 class=\"sidebar-title\"><a \
href=\"#synthetic-implementors\">Auto Implementors</a></h3>",
);
// FIXME: List Auto Implementors
}

buf.push_str("</div>")
Expand Down
5 changes: 2 additions & 3 deletions src/librustdoc/html/render/print_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -741,9 +741,7 @@ fn item_trait(w: &mut Buffer, cx: &Context<'_>, it: &clean::Item, t: &clean::Tra
}
}

let (local, foreign) = implementors.iter().partition::<Vec<_>, _>(|i| {
i.inner_impl().for_.def_id(cache).map_or(true, |d| cache.paths.contains_key(&d))
});
let (local, foreign) = implementors.iter().partition::<Vec<_>, _>(|i| i.is_local(cache));

let (mut synthetic, mut concrete): (Vec<&&Impl>, Vec<&&Impl>) =
local.iter().partition(|i| i.inner_impl().kind.is_auto());
Expand Down Expand Up @@ -784,6 +782,7 @@ fn item_trait(w: &mut Buffer, cx: &Context<'_>, it: &clean::Item, t: &clean::Tra
"Implementors",
"<div class=\"item-list\" id=\"implementors-list\">",
);

for implementor in concrete {
render_implementor(cx, implementor, it, w, &implementor_dups, &[]);
}
Expand Down
49 changes: 49 additions & 0 deletions src/test/rustdoc/sidebar-links-impls.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// issue #56018: "Implementations on Foreign Types" sidebar items should link to specific impls

#![crate_name = "foo"]

// @has foo/trait.Foo.html
// @has - '//*[@class="sidebar-title"]/a[@href="#foreign-impls"]' 'Implementations on Foreign Types'
// @has - '//h2[@id="foreign-impls"]' 'Implementations on Foreign Types'
// @has - '//*[@class="sidebar-links"]/a[@href="#impl-Foo-for-u32"]' 'u32'
// @has - '//div[@id="impl-Foo-for-u32"]//h3[@class="code-header in-band"]' 'impl Foo for u32'
// @has - '//*[@class="sidebar-links"]/a[@href="#impl-Foo-for-%26%27a%20str"]' "&'a str"
// @has - '//div[@id="impl-Foo-for-%26%27a%20str"]//h3[@class="code-header in-band"]' "impl<'a> Foo for &'a str"
pub trait Foo {}

impl Foo for u32 {}

impl<'a> Foo for &'a str {}

// Issue #91118" "Implementors column on trait page is always empty"

pub struct B;
pub struct A;
struct C;

// @has foo/trait.Bar.html
// @has - '//*[@class="sidebar-title"]/a[@href="#implementors"]' 'Implementors'
// @has - '//h2[@id="implementors"]' 'Implementors'
// @!has - '//*[@class="sidebar-title"]/a[@href="#foreign-impls"]'
// @!has - '//h2[@id="foreign-impls"]'

// @has - '//*[@class="sidebar-links"]/a[@href="#impl-Bar"]' '&A'
// @has - '//*[@class="sidebar-links"]/a[@href="#impl-Bar-1"]' 'A'
// @has - '//*[@class="sidebar-links"]/a[@href="#impl-Bar-2"]' 'B'

// @has - '//div[@id="impl-Bar"]//h3[@class="code-header in-band"]' 'impl Bar for &A'
// @has - '//div[@id="impl-Bar-1"]//h3[@class="code-header in-band"]' 'impl Bar for A'
// @has - '//div[@id="impl-Bar-2"]//h3[@class="code-header in-band"]' 'impl Bar for B'
pub trait Bar {}
impl Bar for B {}
impl Bar for A {}
impl Bar for &A {}
impl Bar for C {}

// @has foo/trait.NotImpled.html
// @!has - '//*[@class="sidebar-title"]/a[@href="#implementors"]'
// FIXME: Is this the semantics we want
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specificly the page is documented like

image

Because we only write the "Implementations on Foreign Types" header when they are present (

if !foreign.is_empty() {
) wheras we don't for local impl (
write_small_section_header(
w,
"implementors",
"Implementors",
"<div class=\"item-list\" id=\"implementors-list\">",
);
for implementor in concrete {
render_implementor(cx, implementor, it, w, &implementor_dups, &[]);
}
)

If theirs a reason for this, its fine to keep, otherwize I'll change it to be consistant

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because the implementors section also list implementations on foreign types (as you can see here for example).

// @has - '//h2[@id="implementors"]' 'Implementors'
// @!has - '//*[@class="sidebar-title"]/a[@href="#foreign-impls"]'
// @!has - '//h2[@id="foreign-impls"]'
pub trait NotImpled {}
16 changes: 0 additions & 16 deletions src/test/rustdoc/sidebar-links-to-foreign-impl.rs

This file was deleted.