From 53447d81d05b75fb9923db08b6029381f691a9c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Sun, 27 Jun 2021 11:10:36 +0200 Subject: [PATCH 1/7] fix dead link for method in trait of blanket impl from third party crate --- src/librustdoc/clean/types.rs | 2 +- src/librustdoc/html/format.rs | 58 +++++++++++++++++++------------ src/librustdoc/html/render/mod.rs | 18 +++++----- 3 files changed, 47 insertions(+), 31 deletions(-) diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index e716e09b8b3f0..859746b6a2df7 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -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); diff --git a/src/librustdoc/html/format.rs b/src/librustdoc/html/format.rs index c08fe47696bf1..bed4bae2492c6 100644 --- a/src/librustdoc/html/format.rs +++ b/src/librustdoc/html/format.rs @@ -472,7 +472,18 @@ impl clean::GenericArgs { } } -crate fn href(did: DefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec)> { +// Possible errors when computing href link source for a `DefId` +crate enum HrefError { + // `DefId` is in an unknown location. This seems to happen when building without dependencies + // but a trait from a dependency is still visible + UnknownLocation, + // Unavailable because private + Unavailable, + // Not in external cache, href link should be in same page + NotInExternalCache, +} + +crate fn href(did: DefId, cx: &Context<'_>) -> Result<(String, ItemType, Vec), HrefError> { let cache = &cx.cache(); let relative_to = &cx.current; fn to_module_fqp(shortty: ItemType, fqp: &[String]) -> &[String] { @@ -480,7 +491,7 @@ crate fn href(did: DefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec) -> Option<(String, ItemType, Vec { - 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::UnknownLocation), + }, + ) + } else { + return Err(HrefError::NotInExternalCache); + } } }; let last = &fqp.last().unwrap()[..]; @@ -518,7 +532,7 @@ crate fn href(did: DefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec( 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("::"), @@ -675,7 +689,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#"{}"#, @@ -907,7 +921,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, " 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. @@ -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) { + Ok(p) => Some(format!("{}#{}.{}", p.0, ty, name)), + Err(HrefError::UnknownLocation) => None, + Err(_) => Some(format!("#{}.{}", ty, name)), + } } }; let vis = meth.visibility.print_with_space(meth.def_id, cx).to_string(); @@ -904,7 +906,7 @@ fn render_assoc_item( w.reserve(header_len + "{".len() + "".len()); write!( w, - "{indent}{vis}{constness}{asyncness}{unsafety}{defaultness}{abi}fn {name}\ + "{indent}{vis}{constness}{asyncness}{unsafety}{defaultness}{abi}fn {name}\ {generics}{decl}{notable_traits}{where_clause}", indent = indent_str, vis = vis, @@ -913,7 +915,7 @@ fn render_assoc_item( unsafety = unsafety, defaultness = defaultness, abi = abi, - href = href, + 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), From fc97fbeef3cbd21d94a321faead3ee64b1a14a61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Mon, 5 Jul 2021 01:30:48 +0200 Subject: [PATCH 2/7] add mcve as test --- src/test/rustdoc/auxiliary/issue-86620-1.rs | 11 +++++++++++ src/test/rustdoc/issue-86620.rs | 8 ++++++++ 2 files changed, 19 insertions(+) create mode 100644 src/test/rustdoc/auxiliary/issue-86620-1.rs create mode 100644 src/test/rustdoc/issue-86620.rs diff --git a/src/test/rustdoc/auxiliary/issue-86620-1.rs b/src/test/rustdoc/auxiliary/issue-86620-1.rs new file mode 100644 index 0000000000000..f6debf6fb4e63 --- /dev/null +++ b/src/test/rustdoc/auxiliary/issue-86620-1.rs @@ -0,0 +1,11 @@ +#![crate_name = "issue_86620_1"] + +pub trait VZip { + fn vzip() -> usize; +} + +impl VZip for T { + fn vzip() -> usize { + 0 + } +} diff --git a/src/test/rustdoc/issue-86620.rs b/src/test/rustdoc/issue-86620.rs new file mode 100644 index 0000000000000..2703cac0dc39d --- /dev/null +++ b/src/test/rustdoc/issue-86620.rs @@ -0,0 +1,8 @@ +// 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 +pub struct S; From afb5b2150a011c779f49fc6df1cf32811271230e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Mon, 5 Jul 2021 01:33:51 +0200 Subject: [PATCH 3/7] review - better error names/doc --- src/librustdoc/html/format.rs | 15 ++++++++------- src/librustdoc/html/render/mod.rs | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/librustdoc/html/format.rs b/src/librustdoc/html/format.rs index bed4bae2492c6..08499cef33ef1 100644 --- a/src/librustdoc/html/format.rs +++ b/src/librustdoc/html/format.rs @@ -474,11 +474,12 @@ impl clean::GenericArgs { // Possible errors when computing href link source for a `DefId` crate enum HrefError { - // `DefId` is in an unknown location. This seems to happen when building without dependencies - // but a trait from a dependency is still visible - UnknownLocation, - // Unavailable because private - Unavailable, + /// 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, } @@ -491,7 +492,7 @@ crate fn href(did: DefId, cx: &Context<'_>) -> Result<(String, ItemType, Vec) -> Result<(String, ItemType, Vec href_relative_parts(module_fqp, relative_to), - ExternalLocation::Unknown => return Err(HrefError::UnknownLocation), + ExternalLocation::Unknown => return Err(HrefError::DocumentationNotBuilt), }, ) } else { diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index c814ff3075d05..e6a10c47497b1 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -869,7 +869,7 @@ fn render_assoc_item( match href(did.expect_def_id(), cx) { Ok(p) => Some(format!("{}#{}.{}", p.0, ty, name)), - Err(HrefError::UnknownLocation) => None, + Err(HrefError::DocumentationNotBuilt) => None, Err(_) => Some(format!("#{}.{}", ty, name)), } } From 441a350e2c15f88b550921f911edc9e039164f15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Mon, 5 Jul 2021 02:09:20 +0200 Subject: [PATCH 4/7] add check on anchor link --- src/test/rustdoc/issue-86620.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/rustdoc/issue-86620.rs b/src/test/rustdoc/issue-86620.rs index 2703cac0dc39d..b14e266f7f98d 100644 --- a/src/test/rustdoc/issue-86620.rs +++ b/src/test/rustdoc/issue-86620.rs @@ -5,4 +5,5 @@ 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; From a75e6292c52673df2aa090f16873e162028a39d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Tue, 6 Jul 2021 20:19:54 +0200 Subject: [PATCH 5/7] only remove link for TyMethod --- src/librustdoc/html/render/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index e6a10c47497b1..a701607540720 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -867,10 +867,10 @@ fn render_assoc_item( ItemType::TyMethod }; - match href(did.expect_def_id(), cx) { - Ok(p) => Some(format!("{}#{}.{}", p.0, ty, name)), - Err(HrefError::DocumentationNotBuilt) => None, - Err(_) => Some(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)), } } }; From c6ae96de966941ad28c615d52e8a3e42f58fd4e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Sun, 11 Jul 2021 02:18:21 +0200 Subject: [PATCH 6/7] comment about link without a href --- src/librustdoc/html/render/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index a701607540720..09760e4063b11 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -915,6 +915,7 @@ fn render_assoc_item( unsafety = unsafety, defaultness = defaultness, abi = abi, + // 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), From 450c28a45e387fb54bbc84cf88c3556f93bcf1b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Thu, 15 Jul 2021 00:30:11 +0200 Subject: [PATCH 7/7] add assertions on existing tests with missing doc source --- src/test/rustdoc/extern-default-method.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/rustdoc/extern-default-method.rs b/src/test/rustdoc/extern-default-method.rs index 810c591c53e1e..efa2025b4b9c2 100644 --- a/src/test/rustdoc/extern-default-method.rs +++ b/src/test/rustdoc/extern-default-method.rs @@ -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;