From 18c14fde0d293a18fbd3c14487b52e1ce7daa205 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 29 Aug 2020 15:19:43 -0400 Subject: [PATCH] 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 05c90a7c403..223fda84871 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 6d634bac762..58d75c07472 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 feadf82a2b5..9a705293376 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 74411870e9f..00000000000 --- 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 00000000000..26d05b484b9 --- /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 { + () => () +}