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

Author: jyn514

src/librustdoc/clean/types.rs

@@ -439,15 +439,17 @@ 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<DefId>,
     /// The url fragment to append to the link
     pub(crate) fragment: Option<String>,
 }
 
 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,

src/librustdoc/html/markdown.rs

@@ -338,83 +338,102 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for CodeBlocks<'_, 'a, I> {
 }
 
 /// Make headings links with anchor IDs and build up TOC.
-struct LinkReplacer<'a, 'b, I: Iterator<Item = Event<'a>>> {
+struct LinkReplacer<'a, I: Iterator<Item = Event<'a>>> {
     inner: I,
     links: &'a [RenderedLink],
-    shortcut_link: Option<&'b RenderedLink>,
+    shortcut_link: Option<&'a RenderedLink>,
 }
 
-impl<'a, I: Iterator<Item = Event<'a>>> LinkReplacer<'a, '_, I> {
+impl<'a, I: Iterator<Item = Event<'a>>> LinkReplacer<'a, I> {
     fn new(iter: I, links: &'a [RenderedLink]) -> Self {
         LinkReplacer { inner: iter, links, shortcut_link: None }
     }
 }
 
-impl<'a: 'b, 'b, I: Iterator<Item = Event<'a>>> Iterator for LinkReplacer<'a, 'b, I> {
+impl<'a, I: Iterator<Item = Event<'a>>> Iterator for LinkReplacer<'a, I> {
     type Item = Event<'a>;
 
     fn next(&mut self) -> Option<Self::Item> {
+        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.
             _ => {}
         }
 

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};

src/test/rustdoc/disambiguator_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 {
+    () => ()
+}