Skip to content

rustdoc: links from items in a trait impl are inconsistent #83068

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Apr 14, 2021
Merged

rustdoc: links from items in a trait impl are inconsistent #83068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e1b6bb2
fix links from trait impl methods to trait declaration
mockersf Mar 11, 2021
4596436
add anchors links on hover to items from trait impl
mockersf Mar 11, 2021
838c2ef
fix source link when in a trait implementation
mockersf Mar 13, 2021
df100f7
format css
mockersf Mar 13, 2021
b9f9a2a
tidy format rust
mockersf Mar 13, 2021
26ffef0
add test
mockersf Mar 13, 2021
7f39530
forgot test assertions for default method
mockersf Mar 13, 2021
e36ca09
remove line length ignore
mockersf Apr 13, 2021
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
82 changes: 65 additions & 17 deletions src/librustdoc/html/render/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,10 +912,9 @@ fn render_assoc_item(
let cache = cx.cache();
let tcx = cx.tcx();
let name = meth.name.as_ref().unwrap();
let anchor = format!("#{}.{}", meth.type_(), name);
let href = match link {
AssocItemLink::Anchor(Some(ref id)) => format!("#{}", id),
AssocItemLink::Anchor(None) => anchor,
AssocItemLink::Anchor(None) => format!("#{}.{}", meth.type_(), name),
AssocItemLink::GotoSource(did, provided_methods) => {
// We're creating a link from an impl-item to the corresponding
// trait-item and need to map the anchored type accordingly.
Expand All @@ -925,7 +924,9 @@ fn render_assoc_item(
ItemType::TyMethod
};

href(did, cache).map(|p| format!("{}#{}.{}", p.0, ty, name)).unwrap_or(anchor)
href(did, cache)
.map(|p| format!("{}#{}.{}", p.0, ty, name))
.unwrap_or_else(|| format!("#{}.{}", ty, name))
}
};
let vis = meth.visibility.print_with_space(tcx, meth.def_id, cache).to_string();
Expand Down Expand Up @@ -1452,14 +1453,32 @@ fn render_impl(
} else {
(true, " hidden")
};
let in_trait_class = if trait_.is_some() { " trait-impl" } else { "" };
match *item.kind {
clean::MethodItem(..) | clean::TyMethodItem(_) => {
// Only render when the method is not static or we allow static methods
if render_method_item {
let id = cx.derive_id(format!("{}.{}", item_type, name));
write!(w, "<h4 id=\"{}\" class=\"{}{}\">", id, item_type, extra_class);
let source_id = trait_
.and_then(|trait_| {
trait_.items.iter().find(|item| {
item.name.map(|n| n.as_str().eq(&name.as_str())).unwrap_or(false)
})
})
.map(|item| format!("{}.{}", item.type_(), name));
write!(
w,
"<h4 id=\"{}\" class=\"{}{}{}\">",
id, item_type, extra_class, in_trait_class,
);
w.write_str("<code>");
render_assoc_item(w, item, link.anchor(&id), ItemType::Impl, cx);
render_assoc_item(
w,
item,
link.anchor(source_id.as_ref().unwrap_or(&id)),
ItemType::Impl,
cx,
);
w.write_str("</code>");
render_stability_since_raw(
w,
Expand All @@ -1468,29 +1487,50 @@ fn render_impl(
outer_version,
outer_const_version,
);
write!(w, "<a href=\"#{}\" class=\"anchor\"></a>", id);
write_srclink(cx, item, w);
w.write_str("</h4>");
}
}
clean::TypedefItem(ref tydef, _) => {
let id = cx.derive_id(format!("{}.{}", ItemType::AssocType, name));
write!(w, "<h4 id=\"{}\" class=\"{}{}\"><code>", id, item_type, extra_class);
let source_id = format!("{}.{}", ItemType::AssocType, name);
let id = cx.derive_id(source_id.clone());
write!(
w,
"<h4 id=\"{}\" class=\"{}{}{}\"><code>",
id, item_type, extra_class, in_trait_class
);
assoc_type(
w,
item,
&Vec::new(),
Some(&tydef.type_),
link.anchor(&id),
link.anchor(if trait_.is_some() { &source_id } else { &id }),
"",
cx.cache(),
tcx,
);
w.write_str("</code></h4>");
w.write_str("</code>");
write!(w, "<a href=\"#{}\" class=\"anchor\"></a>", id);
w.write_str("</h4>");
}
clean::AssocConstItem(ref ty, ref default) => {
let id = cx.derive_id(format!("{}.{}", item_type, name));
write!(w, "<h4 id=\"{}\" class=\"{}{}\"><code>", id, item_type, extra_class);
assoc_const(w, item, ty, default.as_ref(), link.anchor(&id), "", cx);
let source_id = format!("{}.{}", item_type, name);
let id = cx.derive_id(source_id.clone());
write!(
w,
"<h4 id=\"{}\" class=\"{}{}{}\"><code>",
id, item_type, extra_class, in_trait_class
);
assoc_const(
w,
item,
ty,
default.as_ref(),
link.anchor(if trait_.is_some() { &source_id } else { &id }),
"",
cx,
);
w.write_str("</code>");
render_stability_since_raw(
w,
Expand All @@ -1499,23 +1539,31 @@ fn render_impl(
outer_version,
outer_const_version,
);
write!(w, "<a href=\"#{}\" class=\"anchor\"></a>", id);
write_srclink(cx, item, w);
w.write_str("</h4>");
}
clean::AssocTypeItem(ref bounds, ref default) => {
let id = cx.derive_id(format!("{}.{}", item_type, name));
write!(w, "<h4 id=\"{}\" class=\"{}{}\"><code>", id, item_type, extra_class);
let source_id = format!("{}.{}", item_type, name);
let id = cx.derive_id(source_id.clone());
write!(
w,
"<h4 id=\"{}\" class=\"{}{}{}\"><code>",
id, item_type, extra_class, in_trait_class
);
assoc_type(
w,
item,
bounds,
default.as_ref(),
link.anchor(&id),
link.anchor(if trait_.is_some() { &source_id } else { &id }),
"",
cx.cache(),
tcx,
);
w.write_str("</code></h4>");
w.write_str("</code>");
write!(w, "<a href=\"#{}\" class=\"anchor\"></a>", id);
w.write_str("</h4>");
}
clean::StrippedItem(..) => return,
_ => panic!("can't make docs for trait item with name {:?}", item.name),
Expand Down Expand Up @@ -1605,7 +1653,7 @@ fn render_impl(
true,
outer_version,
outer_const_version,
None,
Some(t),
show_def_docs,
);
}
Expand Down
6 changes: 4 additions & 2 deletions src/librustdoc/html/static/rustdoc.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ h3.impl, h3.method, h4.method, h3.type, h4.type, h4.associatedconstant {
margin-bottom: 10px;
position: relative;
}
h3.impl, h3.method, h3.type {
h3.impl, h3.method, h4.method.trait-impl, h3.type,
h4.type.trait-impl, h4.associatedconstant.trait-impl {
padding-left: 15px;
}

Expand Down Expand Up @@ -655,7 +656,8 @@ a {
display: initial;
}

.in-band:hover > .anchor, .impl:hover > .anchor {
.in-band:hover > .anchor, .impl:hover > .anchor, .method.trait-impl:hover > .anchor,
.type.trait-impl:hover > .anchor, .associatedconstant.trait-impl:hover > .anchor {
display: inline-block;
position: absolute;
}
Expand Down
65 changes: 65 additions & 0 deletions src/test/rustdoc/trait-impl-items-links-and-anchors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
pub trait MyTrait {
type Assoc;
const VALUE: u32;
fn trait_function(&self);
fn defaulted(&self) {}
fn defaulted_override(&self) {}
}


impl MyTrait for String {
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedtype.Assoc-1"]//a[@class="type"]/@href' #associatedtype.Assoc
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedtype.Assoc-1"]//a[@class="anchor"]/@href' #associatedtype.Assoc-1
type Assoc = ();
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedconstant.VALUE-1"]//a[@class="constant"]/@href' #associatedconstant.VALUE
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedconstant.VALUE-1"]//a[@class="anchor"]/@href' #associatedconstant.VALUE-1
const VALUE: u32 = 5;
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.trait_function"]//a[@class="fnname"]/@href' #tymethod.trait_function
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.trait_function"]//a[@class="anchor"]/@href' #method.trait_function
fn trait_function(&self) {}
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.defaulted_override-1"]//a[@class="fnname"]/@href' #method.defaulted_override
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.defaulted_override-1"]//a[@class="anchor"]/@href' #method.defaulted_override-1
fn defaulted_override(&self) {}
}

impl MyTrait for Vec<u8> {
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedtype.Assoc-2"]//a[@class="type"]/@href' #associatedtype.Assoc
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedtype.Assoc-2"]//a[@class="anchor"]/@href' #associatedtype.Assoc-2
type Assoc = ();
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedconstant.VALUE-2"]//a[@class="constant"]/@href' #associatedconstant.VALUE
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedconstant.VALUE-2"]//a[@class="anchor"]/@href' #associatedconstant.VALUE-2
const VALUE: u32 = 5;
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.trait_function"]//a[@class="fnname"]/@href' #tymethod.trait_function
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.trait_function-1"]//a[@class="anchor"]/@href' #method.trait_function-1
fn trait_function(&self) {}
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.defaulted_override-2"]//a[@class="fnname"]/@href' #method.defaulted_override
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.defaulted_override-2"]//a[@class="anchor"]/@href' #method.defaulted_override-2
fn defaulted_override(&self) {}
}

impl MyTrait for MyStruct {
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedtype.Assoc-3"]//a[@class="type"]/@href' #associatedtype.Assoc
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedtype.Assoc-3"]//a[@class="anchor"]/@href' #associatedtype.Assoc-3
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="associatedtype.Assoc"]//a[@class="type"]/@href' ../trait_impl_items_links_and_anchors/trait.MyTrait.html#associatedtype.Assoc
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="associatedtype.Assoc"]//a[@class="anchor"]/@href' #associatedtype.Assoc
type Assoc = bool;
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedconstant.VALUE-3"]//a[@class="constant"]/@href' #associatedconstant.VALUE
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="associatedconstant.VALUE-3"]//a[@class="anchor"]/@href' #associatedconstant.VALUE-3
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="associatedconstant.VALUE"]//a[@class="constant"]/@href' ../trait_impl_items_links_and_anchors/trait.MyTrait.html#associatedconstant.VALUE
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="associatedconstant.VALUE"]//a[@class="anchor"]/@href' #associatedconstant.VALUE
const VALUE: u32 = 20;
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.trait_function-2"]//a[@class="fnname"]/@href' #tymethod.trait_function
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.trait_function-2"]//a[@class="anchor"]/@href' #method.trait_function-2
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="method.trait_function"]//a[@class="fnname"]/@href' ../trait_impl_items_links_and_anchors/trait.MyTrait.html#tymethod.trait_function
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="method.trait_function"]//a[@class="anchor"]/@href' #method.trait_function
fn trait_function(&self) {}
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.defaulted_override-3"]//a[@class="fnname"]/@href' #method.defaulted_override
Copy link
Member

Choose a reason for hiding this comment

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

I'm lost here: what's the point of testing that this exists on the trait when we're currently on a trait implementation on a type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is on the struct impl for the trait on the trait doc page. the method.defaulted_override-3 is targeting the impl, not the trait itself

Copy link
Member

Choose a reason for hiding this comment

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

Apparently not because the href you test is "#method.defaulted_override".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right, in the h4 with id method.defaulted_override-3, there is now the function name (class fnname that links to it's trait definition, so #method.defaulted_override, and anchor (class anchor) that links to itself (the line under this one)

this pr makes it so that every item in a trait impl is that way

Copy link
Member

Choose a reason for hiding this comment

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

That part makes sense, but why are you testing that on the trait page whereas we "are" (so to speak) on the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as the type implements the trait, it is present on both page

// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.defaulted_override-3"]//a[@class="anchor"]/@href' #method.defaulted_override-3
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="method.defaulted_override"]//a[@class="fnname"]/@href' ../trait_impl_items_links_and_anchors/trait.MyTrait.html#method.defaulted_override
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="method.defaulted_override"]//a[@class="anchor"]/@href' #method.defaulted_override
fn defaulted_override(&self) {}
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="method.defaulted"]//a[@class="fnname"]/@href' ../trait_impl_items_links_and_anchors/trait.MyTrait.html#method.defaulted
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="method.defaulted"]//a[@class="anchor"]/@href' #method.defaulted
}

pub struct MyStruct;