From 89ae59a360b18e34b4cdbec8e53c9a4e05b7db14 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 5 Jul 2020 22:10:18 -0400 Subject: [PATCH 1/6] Remove needless .to_owned() for link --- src/librustdoc/passes/collect_intra_doc_links.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index f497f341e112d..b6707a55959c7 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -695,9 +695,9 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { // This is an anchor to an element of the current page, nothing to do in here! continue; } - (parts[0].to_owned(), Some(parts[1].to_owned())) + (parts[0], Some(parts[1].to_owned())) } else { - (parts[0].to_owned(), None) + (parts[0], None) }; let resolved_self; let mut path_str; From d5495e215559e503e7ad6d3321bb3786184fc22b Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 5 Jul 2020 22:45:44 -0400 Subject: [PATCH 2/6] Refactor `ItemLink` into its own struct --- src/librustdoc/clean/types.rs | 18 +++++++++++++++--- .../passes/collect_intra_doc_links.rs | 4 ++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index a458cdab30303..949790cfe26e1 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -425,10 +425,22 @@ pub struct Attributes { pub cfg: Option>, pub span: Option, /// map from Rust paths to resolved defs and potential URL fragments - pub links: Vec<(String, Option, Option)>, + pub links: Vec, pub inner_docs: bool, } +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] +/// A link that has not yet been rendered. +/// +/// This link will be turned into a rendered link by [`Attributes::links`] +pub struct ItemLink { + /// The original link written in the markdown + pub(crate) link: String, + pub(crate) did: Option, + /// The url fragment to append to the link + pub(crate) fragment: Option, +} + impl Attributes { /// Extracts the content from an attribute `#[doc(cfg(content))]`. pub fn extract_cfg(mi: &ast::MetaItem) -> Option<&ast::MetaItem> { @@ -611,8 +623,8 @@ impl Attributes { self.links .iter() - .filter_map(|&(ref s, did, ref fragment)| { - match did { + .filter_map(|ItemLink { link: s, did, fragment }| { + match *did { Some(did) => { if let Some((mut href, ..)) = href(did) { if let Some(ref fragment) = *fragment { diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index b6707a55959c7..a0cffc92ce195 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -904,7 +904,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { if let Res::PrimTy(_) = res { match disambiguator { Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => { - item.attrs.links.push((ori_link, None, fragment)) + item.attrs.links.push(ItemLink { link: ori_link, did: None, fragment }); } Some(other) => { report_mismatch(other, Disambiguator::Primitive); @@ -955,7 +955,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { } } let id = register_res(cx, res); - item.attrs.links.push((ori_link, Some(id), fragment)); + item.attrs.links.push(ItemLink { link: ori_link, did: Some(id), fragment }); } } From 31a7b6e8323b6e9880f4691e624cf6aa6289f6a7 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 5 Jul 2020 23:05:18 -0400 Subject: [PATCH 3/6] Refactor RenderedLink into its own type --- src/librustdoc/clean/types.rs | 21 ++++++++++++++------- src/librustdoc/html/markdown.rs | 21 +++++++++++---------- src/librustdoc/html/render/mod.rs | 4 ++-- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 949790cfe26e1..d79553a8be8de 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -118,7 +118,7 @@ impl Item { self.attrs.collapsed_doc_value() } - pub fn links(&self) -> Vec<(String, String)> { + pub fn links(&self) -> Vec { self.attrs.links(&self.def_id.krate) } @@ -441,6 +441,13 @@ pub struct ItemLink { pub(crate) fragment: Option, } +pub struct RenderedLink { + /// The text the link was original written as + pub(crate) original_text: String, + /// The URL to put in the `href` + pub(crate) href: String, +} + impl Attributes { /// Extracts the content from an attribute `#[doc(cfg(content))]`. pub fn extract_cfg(mi: &ast::MetaItem) -> Option<&ast::MetaItem> { @@ -617,7 +624,7 @@ impl Attributes { /// Gets links as a vector /// /// Cache must be populated before call - pub fn links(&self, krate: &CrateNum) -> Vec<(String, String)> { + pub fn links(&self, krate: &CrateNum) -> Vec { use crate::html::format::href; use crate::html::render::CURRENT_DEPTH; @@ -631,7 +638,7 @@ impl Attributes { href.push_str("#"); href.push_str(fragment); } - Some((s.clone(), href)) + Some(RenderedLink { original_text: s.clone(), href }) } else { None } @@ -651,16 +658,16 @@ impl Attributes { }; // This is a primitive so the url is done "by hand". let tail = fragment.find('#').unwrap_or_else(|| fragment.len()); - Some(( - s.clone(), - format!( + Some(RenderedLink { + original_text: s.clone(), + href: format!( "{}{}std/primitive.{}.html{}", url, if !url.ends_with('/') { "/" } else { "" }, &fragment[..tail], &fragment[tail..] ), - )) + }) } else { panic!("This isn't a primitive?!"); } diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index d54b8ea747899..e6484cd1c2c05 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -34,6 +34,7 @@ use std::fmt::Write; use std::ops::Range; use std::str; +use crate::clean::RenderedLink; use crate::doctest; use crate::html::highlight; use crate::html::toc::TocBuilder; @@ -52,7 +53,7 @@ fn opts() -> Options { pub struct Markdown<'a>( pub &'a str, /// A list of link replacements. - pub &'a [(String, String)], + pub &'a [RenderedLink], /// The current list of used header IDs. pub &'a mut IdMap, /// Whether to allow the use of explicit error codes in doctest lang strings. @@ -78,7 +79,7 @@ pub struct MarkdownHtml<'a>( pub &'a Option, ); /// A tuple struct like `Markdown` that renders only the first paragraph. -pub struct MarkdownSummaryLine<'a>(pub &'a str, pub &'a [(String, String)]); +pub struct MarkdownSummaryLine<'a>(pub &'a str, pub &'a [RenderedLink]); #[derive(Copy, Clone, PartialEq, Debug)] pub enum ErrorCodes { @@ -339,11 +340,11 @@ impl<'a, I: Iterator>> Iterator for CodeBlocks<'_, 'a, I> { /// Make headings links with anchor IDs and build up TOC. struct LinkReplacer<'a, 'b, I: Iterator>> { inner: I, - links: &'b [(String, String)], + links: &'b [RenderedLink], } impl<'a, 'b, I: Iterator>> LinkReplacer<'a, 'b, I> { - fn new(iter: I, links: &'b [(String, String)]) -> Self { + fn new(iter: I, links: &'b [RenderedLink]) -> Self { LinkReplacer { inner: iter, links } } } @@ -354,8 +355,8 @@ impl<'a, 'b, I: Iterator>> Iterator for LinkReplacer<'a, 'b, I> fn next(&mut self) -> Option { let event = self.inner.next(); if let Some(Event::Start(Tag::Link(kind, dest, text))) = event { - if let Some(&(_, ref replace)) = self.links.iter().find(|link| link.0 == *dest) { - Some(Event::Start(Tag::Link(kind, replace.to_owned().into(), text))) + if let Some(link) = self.links.iter().find(|link| link.original_text == *dest) { + Some(Event::Start(Tag::Link(kind, link.href.clone().into(), text))) } else { Some(Event::Start(Tag::Link(kind, dest, text))) } @@ -855,8 +856,8 @@ impl Markdown<'_> { return String::new(); } let replacer = |_: &str, s: &str| { - if let Some(&(_, ref replace)) = links.iter().find(|link| &*link.0 == s) { - Some((replace.clone(), s.to_owned())) + if let Some(link) = links.iter().find(|link| &*link.original_text == s) { + Some((link.original_text.clone(), link.href.clone())) } else { None } @@ -933,8 +934,8 @@ impl MarkdownSummaryLine<'_> { } let replacer = |_: &str, s: &str| { - if let Some(&(_, ref replace)) = links.iter().find(|link| &*link.0 == s) { - Some((replace.clone(), s.to_owned())) + if let Some(rendered_link) = links.iter().find(|link| &*link.original_text == s) { + Some((rendered_link.original_text.clone(), rendered_link.href.clone())) } else { None } diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index e4aba8963c762..9dc85881482f5 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -63,7 +63,7 @@ use rustc_span::symbol::{sym, Symbol}; use serde::ser::SerializeSeq; use serde::{Serialize, Serializer}; -use crate::clean::{self, AttributesExt, Deprecation, GetDefId, SelfTy, TypeKind}; +use crate::clean::{self, AttributesExt, Deprecation, GetDefId, RenderedLink, SelfTy, TypeKind}; use crate::config::RenderInfo; use crate::config::RenderOptions; use crate::docfs::{DocFS, PathError}; @@ -1780,7 +1780,7 @@ fn render_markdown( w: &mut Buffer, cx: &Context, md_text: &str, - links: Vec<(String, String)>, + links: Vec, prefix: &str, is_hidden: bool, ) { From 9815010d8f6a46621ee96cb824f9939b9db0ae47 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 5 Jul 2020 23:38:31 -0400 Subject: [PATCH 4/6] Remove disambiguators from link text Related to https://github.com/rust-lang/rust/issues/65354 - Pass through the replacement text to `markdown.rs` - Add some tests - Add a state machine that actually replaces the text when parsing Markdown --- src/librustdoc/clean/types.rs | 16 +++- src/librustdoc/html/markdown.rs | 89 +++++++++++++++---- src/librustdoc/html/render/mod.rs | 3 +- .../passes/collect_intra_doc_links.rs | 22 ++++- src/test/rustdoc/disambiguator_removed.rs | 33 +++++++ 5 files changed, 141 insertions(+), 22 deletions(-) create mode 100644 src/test/rustdoc/disambiguator_removed.rs diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index d79553a8be8de..05c90a7c4034f 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -436,6 +436,11 @@ pub struct Attributes { pub struct ItemLink { /// The original link written in the markdown pub(crate) link: String, + /// The link text displayed in the HTML. + /// + /// This may not be the same as `link` if there was a disambiguator + /// in an intra-doc link (e.g. [`fn@f`]) + pub(crate) link_text: String, pub(crate) did: Option, /// The url fragment to append to the link pub(crate) fragment: Option, @@ -444,6 +449,8 @@ pub struct ItemLink { pub struct RenderedLink { /// The text the link was original written as pub(crate) original_text: String, + /// The text to display in the HTML + pub(crate) new_text: String, /// The URL to put in the `href` pub(crate) href: String, } @@ -630,7 +637,7 @@ impl Attributes { self.links .iter() - .filter_map(|ItemLink { link: s, did, fragment }| { + .filter_map(|ItemLink { link: s, link_text, did, fragment }| { match *did { Some(did) => { if let Some((mut href, ..)) = href(did) { @@ -638,7 +645,11 @@ impl Attributes { href.push_str("#"); href.push_str(fragment); } - Some(RenderedLink { original_text: s.clone(), href }) + Some(RenderedLink { + original_text: s.clone(), + new_text: link_text.clone(), + href, + }) } else { None } @@ -660,6 +671,7 @@ impl Attributes { let tail = fragment.find('#').unwrap_or_else(|| fragment.len()); Some(RenderedLink { original_text: s.clone(), + new_text: link_text.clone(), href: format!( "{}{}std/primitive.{}.html{}", url, diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index e6484cd1c2c05..6d634bac7622d 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -340,29 +340,86 @@ impl<'a, I: Iterator>> Iterator for CodeBlocks<'_, 'a, I> { /// Make headings links with anchor IDs and build up TOC. struct LinkReplacer<'a, 'b, I: Iterator>> { inner: I, - links: &'b [RenderedLink], + links: &'a [RenderedLink], + shortcut_link: Option<&'b RenderedLink>, } -impl<'a, 'b, I: Iterator>> LinkReplacer<'a, 'b, I> { - fn new(iter: I, links: &'b [RenderedLink]) -> Self { - LinkReplacer { inner: iter, links } +impl<'a, I: Iterator>> LinkReplacer<'a, '_, I> { + fn new(iter: I, links: &'a [RenderedLink]) -> Self { + LinkReplacer { inner: iter, links, shortcut_link: None } } } -impl<'a, 'b, I: Iterator>> Iterator for LinkReplacer<'a, 'b, I> { +impl<'a: 'b, 'b, I: Iterator>> Iterator for LinkReplacer<'a, 'b, I> { type Item = Event<'a>; fn next(&mut self) -> Option { - let event = self.inner.next(); - if let Some(Event::Start(Tag::Link(kind, dest, text))) = event { - if let Some(link) = self.links.iter().find(|link| link.original_text == *dest) { - Some(Event::Start(Tag::Link(kind, link.href.clone().into(), text))) - } else { - Some(Event::Start(Tag::Link(kind, dest, text))) + let mut event = self.inner.next(); + + // Remove disambiguators from shortcut links (`[fn@f]`) + match &mut event { + Some(Event::Start(Tag::Link( + pulldown_cmark::LinkType::ShortcutUnknown, + dest, + title, + ))) => { + debug!("saw start of shortcut link to {} with title {}", dest, title); + let link = if let Some(link) = + self.links.iter().find(|&link| *link.original_text == **dest) + { + // Not sure why this is necessary - maybe the broken_link_callback doesn't always work? + *dest = CowStr::Borrowed(link.href.as_ref()); + Some(link) + } else { + self.links.iter().find(|&link| *link.href == **dest) + }; + if let Some(link) = link { + trace!("it matched"); + assert!(self.shortcut_link.is_none(), "shortcut links cannot be nested"); + self.shortcut_link = Some(link); + } } - } else { - event + Some(Event::End(Tag::Link(pulldown_cmark::LinkType::ShortcutUnknown, dest, _))) => { + debug!("saw end of shortcut link to {}", dest); + if let Some(_link) = self.links.iter().find(|&link| *link.href == **dest) { + assert!(self.shortcut_link.is_some(), "saw closing link without opening tag"); + self.shortcut_link = None; + } + } + // Handle backticks in inline code blocks + Some(Event::Code(text)) => { + trace!("saw code {}", text); + if let Some(link) = self.shortcut_link { + trace!("original text was {}", link.original_text); + if **text == link.original_text[1..link.original_text.len() - 1] { + debug!("replacing {} with {}", text, link.new_text); + *text = link.new_text.clone().into(); + } + } + } + // Replace plain text in links + Some(Event::Text(text)) => { + trace!("saw text {}", text); + if let Some(link) = self.shortcut_link { + trace!("original text was {}", link.original_text); + if **text == *link.original_text { + debug!("replacing {} with {}", text, link.new_text); + *text = link.new_text.clone().into(); + } + } + } + Some(Event::Start(Tag::Link(_, dest, _))) => { + if let Some(link) = self.links.iter().find(|&link| *link.original_text == **dest) { + // Not sure why this is necessary - maybe the broken_link_callback doesn't always work? + *dest = CowStr::Borrowed(link.href.as_ref()); + } + } + // Anything else couldn't have been a valid Rust path + _ => {} } + + // Yield the modified event + event } } @@ -857,7 +914,7 @@ impl Markdown<'_> { } let replacer = |_: &str, s: &str| { if let Some(link) = links.iter().find(|link| &*link.original_text == s) { - Some((link.original_text.clone(), link.href.clone())) + Some((link.href.clone(), link.new_text.clone())) } else { None } @@ -934,8 +991,8 @@ impl MarkdownSummaryLine<'_> { } let replacer = |_: &str, s: &str| { - if let Some(rendered_link) = links.iter().find(|link| &*link.original_text == s) { - Some((rendered_link.original_text.clone(), rendered_link.href.clone())) + if let Some(link) = links.iter().find(|link| &*link.original_text == s) { + Some((link.href.clone(), link.new_text.clone())) } else { None } diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 9dc85881482f5..318b10e2af2b3 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -64,8 +64,7 @@ use serde::ser::SerializeSeq; use serde::{Serialize, Serializer}; use crate::clean::{self, AttributesExt, Deprecation, GetDefId, RenderedLink, SelfTy, TypeKind}; -use crate::config::RenderInfo; -use crate::config::RenderOptions; +use crate::config::{RenderInfo, RenderOptions}; use crate::docfs::{DocFS, PathError}; use crate::doctree; use crate::error::Error; diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index a0cffc92ce195..ff3c19da3cf75 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -685,6 +685,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { continue; } + //let had_backticks = ori_link.contains("`"); let link = ori_link.replace("`", ""); let parts = link.split('#').collect::>(); let (link, extra_fragment) = if parts.len() > 2 { @@ -700,6 +701,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { (parts[0], None) }; let resolved_self; + let link_text; let mut path_str; let disambiguator; let (mut res, mut fragment) = { @@ -716,6 +718,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { continue; } + // We stripped ` characters for `path_str`. + // The original link might have had multiple pairs of backticks, + // but we don't handle this case. + //link_text = if had_backticks { format!("`{}`", path_str) } else { path_str.to_owned() }; + link_text = path_str.to_owned(); + // In order to correctly resolve intra-doc-links we need to // pick a base AST node to work from. If the documentation for // this module came from an inner comment (//!) then we anchor @@ -904,7 +912,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { if let Res::PrimTy(_) = res { match disambiguator { Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => { - item.attrs.links.push(ItemLink { link: ori_link, did: None, fragment }); + item.attrs.links.push(ItemLink { + link: ori_link, + link_text: path_str.to_owned(), + did: None, + fragment, + }); } Some(other) => { report_mismatch(other, Disambiguator::Primitive); @@ -955,7 +968,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { } } let id = register_res(cx, res); - item.attrs.links.push(ItemLink { link: ori_link, did: Some(id), fragment }); + item.attrs.links.push(ItemLink { + link: ori_link, + link_text, + did: Some(id), + fragment, + }); } } diff --git a/src/test/rustdoc/disambiguator_removed.rs b/src/test/rustdoc/disambiguator_removed.rs new file mode 100644 index 0000000000000..74411870e9f8a --- /dev/null +++ b/src/test/rustdoc/disambiguator_removed.rs @@ -0,0 +1,33 @@ +#![deny(intra_doc_link_resolution_failure)] +// first try backticks +/// Trait: [`trait@Name`], fn: [`fn@Name`], [`Name`][`macro@Name`] +// @has disambiguator_removed/struct.AtDisambiguator.html +// @has - '//a[@href="../disambiguator_removed/trait.Name.html"][code]' "Name" +// @has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name" +// @has - '//a[@href="../disambiguator_removed/macro.Name.html"][code]' "Name" +pub struct AtDisambiguator; + +/// fn: [`Name()`], macro: [`Name!`] +// @has disambiguator_removed/struct.SymbolDisambiguator.html +// @has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name()" +// @has - '//a[@href="../disambiguator_removed/macro.Name.html"][code]' "Name!" +pub struct SymbolDisambiguator; + +// Now make sure that backticks aren't added if they weren't already there +/// [fn@Name] +// @has disambiguator_removed/trait.Name.html +// @has - '//a[@href="../disambiguator_removed/fn.Name.html"]' "Name" +// @!has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name" + +// FIXME: this will turn !() into ! alone +/// [Name!()] +// @has - '//a[@href="../disambiguator_removed/macro.Name.html"]' "Name!" +pub trait Name {} + +#[allow(non_snake_case)] +pub fn Name() {} + +#[macro_export] +macro_rules! Name { + () => () +} From 9d7e797514ed1ea60a761d272c1ba8426bc31739 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 29 Aug 2020 15:13:28 -0400 Subject: [PATCH 5/6] display_for -> suggestion_for --- src/librustdoc/passes/collect_intra_doc_links.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index ff3c19da3cf75..feadf82a2b57e 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -685,7 +685,6 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { continue; } - //let had_backticks = ori_link.contains("`"); let link = ori_link.replace("`", ""); let parts = link.split('#').collect::>(); let (link, extra_fragment) = if parts.len() > 2 { @@ -1053,7 +1052,7 @@ impl Disambiguator { } /// Return (description of the change, suggestion) - fn display_for(self, path_str: &str) -> (&'static str, String) { + fn suggestion_for(self, path_str: &str) -> (&'static str, String) { const PREFIX: &str = "prefix with the item kind"; const FUNCTION: &str = "add parentheses"; const MACRO: &str = "add an exclamation mark"; @@ -1308,7 +1307,7 @@ fn suggest_disambiguator( sp: Option, link_range: &Option>, ) { - let (action, mut suggestion) = disambiguator.display_for(path_str); + let (action, mut suggestion) = disambiguator.suggestion_for(path_str); let help = format!("to link to the {}, {}", disambiguator.descr(), action); if let Some(sp) = sp { From 18c14fde0d293a18fbd3c14487b52e1ce7daa205 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 29 Aug 2020 15:19:43 -0400 Subject: [PATCH 6/6] Misc cleanup - Preserve suffixes when displaying - Rename test file to match `intra-link*` - Remove unnecessary .clone()s - Improve comments and naming - Fix more bugs and add tests - Escape intra-doc link example in public documentation --- src/librustdoc/clean/types.rs | 6 +- src/librustdoc/html/markdown.rs | 65 ++++++++++++------- .../passes/collect_intra_doc_links.rs | 22 +++++-- src/test/rustdoc/disambiguator_removed.rs | 33 ---------- .../intra-link-disambiguators-removed.rs | 51 +++++++++++++++ 5 files changed, 114 insertions(+), 63 deletions(-) delete mode 100644 src/test/rustdoc/disambiguator_removed.rs create mode 100644 src/test/rustdoc/intra-link-disambiguators-removed.rs diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 05c90a7c4034f..223fda84871e9 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -439,7 +439,7 @@ pub struct ItemLink { /// The link text displayed in the HTML. /// /// This may not be the same as `link` if there was a disambiguator - /// in an intra-doc link (e.g. [`fn@f`]) + /// in an intra-doc link (e.g. \[`fn@f`\]) pub(crate) link_text: String, pub(crate) did: Option, /// The url fragment to append to the link @@ -447,7 +447,9 @@ pub struct ItemLink { } pub struct RenderedLink { - /// The text the link was original written as + /// The text the link was original written as. + /// + /// This could potentially include disambiguators and backticks. pub(crate) original_text: String, /// The text to display in the HTML pub(crate) new_text: String, diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index 6d634bac7622d..58d75c0747295 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -338,83 +338,102 @@ impl<'a, I: Iterator>> Iterator for CodeBlocks<'_, 'a, I> { } /// Make headings links with anchor IDs and build up TOC. -struct LinkReplacer<'a, 'b, I: Iterator>> { +struct LinkReplacer<'a, I: Iterator>> { inner: I, links: &'a [RenderedLink], - shortcut_link: Option<&'b RenderedLink>, + shortcut_link: Option<&'a RenderedLink>, } -impl<'a, I: Iterator>> LinkReplacer<'a, '_, I> { +impl<'a, I: Iterator>> LinkReplacer<'a, I> { fn new(iter: I, links: &'a [RenderedLink]) -> Self { LinkReplacer { inner: iter, links, shortcut_link: None } } } -impl<'a: 'b, 'b, I: Iterator>> Iterator for LinkReplacer<'a, 'b, I> { +impl<'a, I: Iterator>> Iterator for LinkReplacer<'a, I> { type Item = Event<'a>; fn next(&mut self) -> Option { + use pulldown_cmark::LinkType; + let mut event = self.inner.next(); - // Remove disambiguators from shortcut links (`[fn@f]`) + // Replace intra-doc links and remove disambiguators from shortcut links (`[fn@f]`). match &mut event { + // This is a shortcut link that was resolved by the broken_link_callback: `[fn@f]` + // Remove any disambiguator. Some(Event::Start(Tag::Link( - pulldown_cmark::LinkType::ShortcutUnknown, + // [fn@f] or [fn@f][] + LinkType::ShortcutUnknown | LinkType::CollapsedUnknown, dest, title, ))) => { debug!("saw start of shortcut link to {} with title {}", dest, title); - let link = if let Some(link) = - self.links.iter().find(|&link| *link.original_text == **dest) - { - // Not sure why this is necessary - maybe the broken_link_callback doesn't always work? - *dest = CowStr::Borrowed(link.href.as_ref()); - Some(link) - } else { - self.links.iter().find(|&link| *link.href == **dest) - }; + // If this is a shortcut link, it was resolved by the broken_link_callback. + // So the URL will already be updated properly. + let link = self.links.iter().find(|&link| *link.href == **dest); + // Since this is an external iterator, we can't replace the inner text just yet. + // Store that we saw a link so we know to replace it later. if let Some(link) = link { trace!("it matched"); assert!(self.shortcut_link.is_none(), "shortcut links cannot be nested"); self.shortcut_link = Some(link); } } - Some(Event::End(Tag::Link(pulldown_cmark::LinkType::ShortcutUnknown, dest, _))) => { + // Now that we're done with the shortcut link, don't replace any more text. + Some(Event::End(Tag::Link( + LinkType::ShortcutUnknown | LinkType::CollapsedUnknown, + dest, + _, + ))) => { debug!("saw end of shortcut link to {}", dest); - if let Some(_link) = self.links.iter().find(|&link| *link.href == **dest) { + if self.links.iter().find(|&link| *link.href == **dest).is_some() { assert!(self.shortcut_link.is_some(), "saw closing link without opening tag"); self.shortcut_link = None; } } - // Handle backticks in inline code blocks + // Handle backticks in inline code blocks, but only if we're in the middle of a shortcut link. + // [`fn@f`] Some(Event::Code(text)) => { trace!("saw code {}", text); if let Some(link) = self.shortcut_link { trace!("original text was {}", link.original_text); + // NOTE: this only replaces if the code block is the *entire* text. + // If only part of the link has code highlighting, the disambiguator will not be removed. + // e.g. [fn@`f`] + // This is a limitation from `collect_intra_doc_links`: it passes a full link, + // and does not distinguish at all between code blocks. + // So we could never be sure we weren't replacing too much: + // [fn@my_`f`unc] is treated the same as [my_func()] in that pass. + // + // NOTE: &[1..len() - 1] is to strip the backticks if **text == link.original_text[1..link.original_text.len() - 1] { debug!("replacing {} with {}", text, link.new_text); - *text = link.new_text.clone().into(); + *text = CowStr::Borrowed(&link.new_text); } } } - // Replace plain text in links + // Replace plain text in links, but only in the middle of a shortcut link. + // [fn@f] Some(Event::Text(text)) => { trace!("saw text {}", text); if let Some(link) = self.shortcut_link { trace!("original text was {}", link.original_text); + // NOTE: same limitations as `Event::Code` if **text == *link.original_text { debug!("replacing {} with {}", text, link.new_text); - *text = link.new_text.clone().into(); + *text = CowStr::Borrowed(&link.new_text); } } } + // If this is a link, but not a shortcut link, + // replace the URL, since the broken_link_callback was not called. Some(Event::Start(Tag::Link(_, dest, _))) => { if let Some(link) = self.links.iter().find(|&link| *link.original_text == **dest) { - // Not sure why this is necessary - maybe the broken_link_callback doesn't always work? *dest = CowStr::Borrowed(link.href.as_ref()); } } - // Anything else couldn't have been a valid Rust path + // Anything else couldn't have been a valid Rust path, so no need to replace the text. _ => {} } diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index feadf82a2b57e..9a705293376a0 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -717,11 +717,11 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { continue; } - // We stripped ` characters for `path_str`. - // The original link might have had multiple pairs of backticks, - // but we don't handle this case. - //link_text = if had_backticks { format!("`{}`", path_str) } else { path_str.to_owned() }; - link_text = path_str.to_owned(); + // We stripped `()` and `!` when parsing the disambiguator. + // Add them back to be displayed, but not prefix disambiguators. + link_text = disambiguator + .map(|d| d.display_for(path_str)) + .unwrap_or_else(|| path_str.to_owned()); // In order to correctly resolve intra-doc-links we need to // pick a base AST node to work from. If the documentation for @@ -1000,6 +1000,18 @@ enum Disambiguator { } impl Disambiguator { + /// The text that should be displayed when the path is rendered as HTML. + /// + /// NOTE: `path` is not the original link given by the user, but a name suitable for passing to `resolve`. + fn display_for(&self, path: &str) -> String { + match self { + // FIXME: this will have different output if the user had `m!()` originally. + Self::Kind(DefKind::Macro(MacroKind::Bang)) => format!("{}!", path), + Self::Kind(DefKind::Fn) => format!("{}()", path), + _ => path.to_owned(), + } + } + /// (disambiguator, path_str) fn from_str(link: &str) -> Result<(Self, &str), ()> { use Disambiguator::{Kind, Namespace as NS, Primitive}; diff --git a/src/test/rustdoc/disambiguator_removed.rs b/src/test/rustdoc/disambiguator_removed.rs deleted file mode 100644 index 74411870e9f8a..0000000000000 --- a/src/test/rustdoc/disambiguator_removed.rs +++ /dev/null @@ -1,33 +0,0 @@ -#![deny(intra_doc_link_resolution_failure)] -// first try backticks -/// Trait: [`trait@Name`], fn: [`fn@Name`], [`Name`][`macro@Name`] -// @has disambiguator_removed/struct.AtDisambiguator.html -// @has - '//a[@href="../disambiguator_removed/trait.Name.html"][code]' "Name" -// @has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name" -// @has - '//a[@href="../disambiguator_removed/macro.Name.html"][code]' "Name" -pub struct AtDisambiguator; - -/// fn: [`Name()`], macro: [`Name!`] -// @has disambiguator_removed/struct.SymbolDisambiguator.html -// @has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name()" -// @has - '//a[@href="../disambiguator_removed/macro.Name.html"][code]' "Name!" -pub struct SymbolDisambiguator; - -// Now make sure that backticks aren't added if they weren't already there -/// [fn@Name] -// @has disambiguator_removed/trait.Name.html -// @has - '//a[@href="../disambiguator_removed/fn.Name.html"]' "Name" -// @!has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name" - -// FIXME: this will turn !() into ! alone -/// [Name!()] -// @has - '//a[@href="../disambiguator_removed/macro.Name.html"]' "Name!" -pub trait Name {} - -#[allow(non_snake_case)] -pub fn Name() {} - -#[macro_export] -macro_rules! Name { - () => () -} diff --git a/src/test/rustdoc/intra-link-disambiguators-removed.rs b/src/test/rustdoc/intra-link-disambiguators-removed.rs new file mode 100644 index 0000000000000..26d05b484b919 --- /dev/null +++ b/src/test/rustdoc/intra-link-disambiguators-removed.rs @@ -0,0 +1,51 @@ +// ignore-tidy-linelength +#![deny(intra_doc_link_resolution_failure)] +// first try backticks +/// Trait: [`trait@Name`], fn: [`fn@Name`], [`Name`][`macro@Name`] +// @has intra_link_disambiguators_removed/struct.AtDisambiguator.html +// @has - '//a[@href="../intra_link_disambiguators_removed/trait.Name.html"][code]' "Name" +// @has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"][code]' "Name" +// @has - '//a[@href="../intra_link_disambiguators_removed/macro.Name.html"][code]' "Name" +pub struct AtDisambiguator; + +/// fn: [`Name()`], macro: [`Name!`] +// @has intra_link_disambiguators_removed/struct.SymbolDisambiguator.html +// @has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"][code]' "Name()" +// @has - '//a[@href="../intra_link_disambiguators_removed/macro.Name.html"][code]' "Name!" +pub struct SymbolDisambiguator; + +// Now make sure that backticks aren't added if they weren't already there +/// [fn@Name] +// @has intra_link_disambiguators_removed/trait.Name.html +// @has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"]' "Name" +// @!has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"][code]' "Name" + +// FIXME: this will turn !() into ! alone +/// [Name!()] +// @has - '//a[@href="../intra_link_disambiguators_removed/macro.Name.html"]' "Name!" +pub trait Name {} + +#[allow(non_snake_case)] + +// Try collapsed reference links +/// [macro@Name][] +// @has intra_link_disambiguators_removed/fn.Name.html +// @has - '//a[@href="../intra_link_disambiguators_removed/macro.Name.html"]' "Name" + +// Try links that have the same text as a generated URL +/// Weird URL aligned [../intra_link_disambiguators_removed/macro.Name.html][trait@Name] +// @has - '//a[@href="../intra_link_disambiguators_removed/trait.Name.html"]' "../intra_link_disambiguators_removed/macro.Name.html" +pub fn Name() {} + +#[macro_export] +// Rustdoc doesn't currently handle links that have weird interspersing of inline code blocks. +/// [fn@Na`m`e] +// @has intra_link_disambiguators_removed/macro.Name.html +// @has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"]' "fn@Name" + +// It also doesn't handle any case where the code block isn't the whole link text: +/// [trait@`Name`] +// @has - '//a[@href="../intra_link_disambiguators_removed/trait.Name.html"]' "trait@Name" +macro_rules! Name { + () => () +}