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

fix dead link for method in trait of blanket impl from third party crate #86662

Merged
merged 7 commits into from
Jul 16, 2021
Merged
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
2 changes: 1 addition & 1 deletion src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ impl Item {
.filter_map(|ItemLink { link: s, link_text, did, ref fragment }| {
match did {
Some(did) => {
if let Some((mut href, ..)) = href(did.clone(), cx) {
if let Ok((mut href, ..)) = href(did.clone(), cx) {
if let Some(ref fragment) = *fragment {
href.push('#');
href.push_str(fragment);
Expand Down
59 changes: 37 additions & 22 deletions src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -472,15 +472,27 @@ impl clean::GenericArgs {
}
}

crate fn href(did: DefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec<String>)> {
// Possible errors when computing href link source for a `DefId`
crate enum HrefError {
Copy link
Member

@jyn514 jyn514 Jun 27, 2021

Choose a reason for hiding this comment

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

This would help quite a lot with #82014 I think :) good idea.

(So I remember: the perma-link for this is 8f19606)

/// This item is known to rustdoc, but from a crate that does not have documentation generated.
///
/// This can only happen for non-local items.
DocumentationNotBuilt,
/// This can only happen for non-local items when `--document-private-items` is not passed.
Private,
// Not in external cache, href link should be in same page
NotInExternalCache,
}

crate fn href(did: DefId, cx: &Context<'_>) -> Result<(String, ItemType, Vec<String>), HrefError> {
let cache = &cx.cache();
let relative_to = &cx.current;
fn to_module_fqp(shortty: ItemType, fqp: &[String]) -> &[String] {
if shortty == ItemType::Module { &fqp[..] } else { &fqp[..fqp.len() - 1] }
}

if !did.is_local() && !cache.access_levels.is_public(did) && !cache.document_private {
Copy link
Member

Choose a reason for hiding this comment

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

So, !is_local looks correct to me, but for a non-trivial reason - the strip_private pass should get rid of private local items before they ever get here. Can you change this to an assert instead?

Suggested change
if !did.is_local() && !cache.access_levels.is_public(did) && !cache.document_private {
if did.is_local() {
// Private items should have already been stripped.
assert!(cache.access_levels.is_public(did) || cache.document_private);
}
if !cache.access_levels.is_public(did) && !cache.document_private {

(I know this is unrelated to your change - I can make a separate PR for this if you like.)

Copy link
Contributor Author

@mockersf mockersf Jul 4, 2021

Choose a reason for hiding this comment

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

adding this assert made the following tests fail:

failures:
    [rustdoc] rustdoc/all.rs
    [rustdoc] rustdoc/async-fn.rs
    [rustdoc] rustdoc/blanket-reexport-item.rs
    [rustdoc] rustdoc/impl-trait-alias.rs
    [rustdoc] rustdoc/inline_cross/renamed-via-module.rs
    [rustdoc] rustdoc/inline_local/glob-private-no-defaults.rs
    [rustdoc] rustdoc/inline_local/glob-private.rs
    [rustdoc] rustdoc/inline_local/issue-28537.rs
    [rustdoc] rustdoc/inline_local/issue-32343.rs
    [rustdoc] rustdoc/inline_local/trait-vis.rs
    [rustdoc] rustdoc/intra-doc/cross-crate/hidden.rs
    [rustdoc] rustdoc/issue-34473.rs
    [rustdoc] rustdoc/issue-35488.rs
    [rustdoc] rustdoc/issue-46767.rs
    [rustdoc] rustdoc/namespaces.rs
    [rustdoc] rustdoc/redirect-map.rs
    [rustdoc] rustdoc/redirect-rename.rs
    [rustdoc] rustdoc/redirect.rs
    [rustdoc] rustdoc/search-index.rs
    [rustdoc] rustdoc/struct-arg-pattern.rs
    [rustdoc] rustdoc/synthetic_auto/complex.rs
    [rustdoc] rustdoc/synthetic_auto/project.rs

Copy link
Member

Choose a reason for hiding this comment

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

ahhhh of course it did :( I'll open an issue

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I was just confused: the proper assert is

        assert!(cache.access_levels.is_public(did) || cache.document_private || cache.paths.get(&did).is_none(), "{:?}", did);

Copy link
Member

Choose a reason for hiding this comment

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

well, there was a bug after all: #87048

return None;
return Err(HrefError::Private);
}

let (fqp, shortty, mut url_parts) = match cache.paths.get(&did) {
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -489,22 +501,25 @@ crate fn href(did: DefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec<Str
href_relative_parts(module_fqp, relative_to)
}),
None => {
let &(ref fqp, shortty) = cache.external_paths.get(&did)?;
let module_fqp = to_module_fqp(shortty, fqp);
(
fqp,
shortty,
match cache.extern_locations[&did.krate] {
ExternalLocation::Remote(ref s) => {
let s = s.trim_end_matches('/');
let mut s = vec![&s[..]];
s.extend(module_fqp[..].iter().map(String::as_str));
s
}
ExternalLocation::Local => href_relative_parts(module_fqp, relative_to),
ExternalLocation::Unknown => return None,
},
)
if let Some(&(ref fqp, shortty)) = cache.external_paths.get(&did) {
let module_fqp = to_module_fqp(shortty, fqp);
(
fqp,
shortty,
match cache.extern_locations[&did.krate] {
ExternalLocation::Remote(ref s) => {
let s = s.trim_end_matches('/');
let mut s = vec![&s[..]];
s.extend(module_fqp[..].iter().map(String::as_str));
s
}
ExternalLocation::Local => href_relative_parts(module_fqp, relative_to),
ExternalLocation::Unknown => return Err(HrefError::DocumentationNotBuilt),
},
)
} else {
return Err(HrefError::NotInExternalCache);
}
}
};
let last = &fqp.last().unwrap()[..];
Expand All @@ -518,7 +533,7 @@ crate fn href(did: DefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec<Str
url_parts.push(&filename);
}
}
Some((url_parts.join("/"), shortty, fqp.to_vec()))
Ok((url_parts.join("/"), shortty, fqp.to_vec()))
}

/// Both paths should only be modules.
Expand Down Expand Up @@ -567,7 +582,7 @@ fn resolved_path<'a, 'cx: 'a>(
write!(w, "{}{:#}", &last.name, last.args.print(cx))?;
} else {
let path = if use_absolute {
if let Some((_, _, fqp)) = href(did, cx) {
if let Ok((_, _, fqp)) = href(did, cx) {
format!(
"{}::{}",
fqp[..fqp.len() - 1].join("::"),
Expand Down Expand Up @@ -675,7 +690,7 @@ crate fn anchor<'a, 'cx: 'a>(
) -> impl fmt::Display + 'a {
let parts = href(did.into(), cx);
display_fn(move |f| {
if let Some((url, short_ty, fqp)) = parts {
if let Ok((url, short_ty, fqp)) = parts {
write!(
f,
r#"<a class="{}" href="{}" title="{} {}">{}</a>"#,
Expand Down Expand Up @@ -907,7 +922,7 @@ fn fmt_type<'cx>(
// look at).
box clean::ResolvedPath { did, .. } => {
match href(did.into(), cx) {
Some((ref url, _, ref path)) if !f.alternate() => {
Ok((ref url, _, ref path)) if !f.alternate() => {
write!(
f,
"<a class=\"type\" href=\"{url}#{shortty}.{name}\" \
Expand Down
19 changes: 11 additions & 8 deletions src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ use crate::formats::{AssocItemRender, Impl, RenderMode};
use crate::html::escape::Escape;
use crate::html::format::{
href, print_abi_with_space, print_constness_with_space, print_default_space,
print_generic_bounds, print_where_clause, Buffer, PrintWithSpace,
print_generic_bounds, print_where_clause, Buffer, HrefError, PrintWithSpace,
};
use crate::html::markdown::{Markdown, MarkdownHtml, MarkdownSummaryLine};

Expand Down Expand Up @@ -856,8 +856,8 @@ fn render_assoc_item(
) {
let name = meth.name.as_ref().unwrap();
let href = match link {
AssocItemLink::Anchor(Some(ref id)) => format!("#{}", id),
AssocItemLink::Anchor(None) => format!("#{}.{}", meth.type_(), name),
AssocItemLink::Anchor(Some(ref id)) => Some(format!("#{}", id)),
AssocItemLink::Anchor(None) => Some(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 @@ -867,9 +867,11 @@ fn render_assoc_item(
ItemType::TyMethod
};

href(did.expect_def_id(), cx)
.map(|p| format!("{}#{}.{}", p.0, ty, name))
.unwrap_or_else(|| format!("#{}.{}", ty, name))
match (href(did.expect_def_id(), cx), ty) {
(Ok(p), ty) => Some(format!("{}#{}.{}", p.0, ty, name)),
(Err(HrefError::DocumentationNotBuilt), ItemType::TyMethod) => None,
(Err(_), ty) => Some(format!("#{}.{}", ty, name)),
}
}
};
let vis = meth.visibility.print_with_space(meth.def_id, cx).to_string();
Expand Down Expand Up @@ -904,7 +906,7 @@ fn render_assoc_item(
w.reserve(header_len + "<a href=\"\" class=\"fnname\">{".len() + "</a>".len());
write!(
w,
"{indent}{vis}{constness}{asyncness}{unsafety}{defaultness}{abi}fn <a href=\"{href}\" class=\"fnname\">{name}</a>\
"{indent}{vis}{constness}{asyncness}{unsafety}{defaultness}{abi}fn <a {href} class=\"fnname\">{name}</a>\
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
{generics}{decl}{notable_traits}{where_clause}",
indent = indent_str,
vis = vis,
Expand All @@ -913,7 +915,8 @@ fn render_assoc_item(
unsafety = unsafety,
defaultness = defaultness,
abi = abi,
href = href,
// links without a href are valid - https://www.w3schools.com/tags/att_a_href.asp
href = href.map(|href| format!("href=\"{}\"", href)).unwrap_or_else(|| "".to_string()),
name = name,
generics = g.print(cx),
decl = d.full_print(header_len, indent, header.asyncness, cx),
Expand Down
11 changes: 11 additions & 0 deletions src/test/rustdoc/auxiliary/issue-86620-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#![crate_name = "issue_86620_1"]

pub trait VZip {
fn vzip() -> usize;
}

impl<T> VZip for T {
fn vzip() -> usize {
0
}
}
2 changes: 2 additions & 0 deletions src/test/rustdoc/extern-default-method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,6 @@
extern crate rustdoc_extern_default_method as ext;

// @count extern_default_method/struct.Struct.html '//*[@id="method.provided"]' 1
// @has extern_default_method/struct.Struct.html '//div[@id="method.provided"]//a[@class="fnname"]/@href' #method.provided
// @has extern_default_method/struct.Struct.html '//div[@id="method.provided"]//a[@class="anchor"]/@href' #method.provided
pub use ext::Struct;
9 changes: 9 additions & 0 deletions src/test/rustdoc/issue-86620.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// aux-build:issue-86620-1.rs

extern crate issue_86620_1;

use issue_86620_1::*;

// @!has issue_86620/struct.S.html '//div[@id="method.vzip"]//a[@class="fnname"]/@href' #tymethod.vzip
// @has issue_86620/struct.S.html '//div[@id="method.vzip"]//a[@class="anchor"]/@href' #method.vzip
pub struct S;