Rollup merge of #76078 - jyn514:no-disambiguator, r=manishearth

Remove disambiguators from intra doc link text

Closes https://github.com/rust-lang/rust/issues/65354.
r? @Manishearth

The commits are mostly atomic, but there might be some mix between them here and there. I recommend reading 'refactor ItemLink' and 'refactor RenderedLink' on their own though, lots of churn without any logic changes.

5 files changed, 229 insertions, 40 deletions

diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs
index a458cdab303..223fda84871 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<RenderedLink> {
         self.attrs.links(&self.def_id.krate)
     }
 
@@ -425,10 +425,38 @@ pub struct Attributes {
     pub cfg: Option<Arc<Cfg>>,
     pub span: Option<rustc_span::Span>,
     /// map from Rust paths to resolved defs and potential URL fragments
-    pub links: Vec<(String, Option<DefId>, Option<String>)>,
+    pub links: Vec<ItemLink>,
     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,
+    /// 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<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.
+    ///
+    /// This could potentially include disambiguators and backticks.
+    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,
+}
+
 impl Attributes {
     /// Extracts the content from an attribute `#[doc(cfg(content))]`.
     pub fn extract_cfg(mi: &ast::MetaItem) -> Option<&ast::MetaItem> {
@@ -605,21 +633,25 @@ 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<RenderedLink> {
         use crate::html::format::href;
         use crate::html::render::CURRENT_DEPTH;
 
         self.links
             .iter()
-            .filter_map(|&(ref s, did, ref fragment)| {
-                match did {
+            .filter_map(|ItemLink { link: s, link_text, did, fragment }| {
+                match *did {
                     Some(did) => {
                         if let Some((mut href, ..)) = href(did) {
                             if let Some(ref fragment) = *fragment {
                                 href.push_str("#");
                                 href.push_str(fragment);
                             }
-                            Some((s.clone(), href))
+                            Some(RenderedLink {
+                                original_text: s.clone(),
+                                new_text: link_text.clone(),
+                                href,
+                            })
                         } else {
                             None
                         }
@@ -639,16 +671,17 @@ 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(),
+                                new_text: link_text.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 098ece9a1d5..a8c60e4a76d 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<Playground>,
 );
 /// 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 {
@@ -337,31 +338,107 @@ 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: &'b [(String, String)],
+    links: &'a [RenderedLink],
+    shortcut_link: Option<&'a RenderedLink>,
 }
 
-impl<'a, 'b, I: Iterator<Item = Event<'a>>> LinkReplacer<'a, 'b, I> {
-    fn new(iter: I, links: &'b [(String, String)]) -> Self {
-        LinkReplacer { inner: iter, links }
+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, 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> {
-        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)))
-            } else {
-                Some(Event::Start(Tag::Link(kind, dest, text)))
+        use pulldown_cmark::LinkType;
+
+        let mut event = self.inner.next();
+
+        // 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(
+                // [fn@f] or [fn@f][]
+                LinkType::ShortcutUnknown | LinkType::CollapsedUnknown,
+                dest,
+                title,
+            ))) => {
+                debug!("saw start of shortcut link to {} with title {}", dest, title);
+                // 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);
+                }
             }
-        } else {
-            event
+            // 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 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, 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 = CowStr::Borrowed(&link.new_text);
+                    }
+                }
+            }
+            // 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 = 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) {
+                    *dest = CowStr::Borrowed(link.href.as_ref());
+                }
+            }
+            // Anything else couldn't have been a valid Rust path, so no need to replace the text.
+            _ => {}
         }
+
+        // Yield the modified event
+        event
     }
 }
 
@@ -855,8 +932,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.href.clone(), link.new_text.clone()))
             } else {
                 None
             }
@@ -933,8 +1010,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(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 57b3b9502a3..f095f67b54c 100644
--- a/src/librustdoc/html/render/mod.rs
+++ b/src/librustdoc/html/render/mod.rs
@@ -63,9 +63,8 @@ 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::config::RenderInfo;
-use crate::config::RenderOptions;
+use crate::clean::{self, AttributesExt, Deprecation, GetDefId, RenderedLink, SelfTy, TypeKind};
+use crate::config::{RenderInfo, RenderOptions};
 use crate::docfs::{DocFS, PathError};
 use crate::doctree;
 use crate::error::Error;
@@ -1774,7 +1773,7 @@ fn render_markdown(
     w: &mut Buffer,
     cx: &Context,
     md_text: &str,
-    links: Vec<(String, String)>,
+    links: Vec<RenderedLink>,
     prefix: &str,
     is_hidden: bool,
 ) {
diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs
index a4f5530815c..13bfca4c5cd 100644
--- a/src/librustdoc/passes/collect_intra_doc_links.rs
+++ b/src/librustdoc/passes/collect_intra_doc_links.rs
@@ -697,11 +697,12 @@ 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 link_text;
             let mut path_str;
             let disambiguator;
             let (mut res, mut fragment) = {
@@ -718,6 +719,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
                     continue;
                 }
 
+                // 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
                 // this module came from an inner comment (//!) then we anchor
@@ -906,7 +913,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((ori_link, 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);
@@ -957,7 +969,12 @@ 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,
+                    link_text,
+                    did: Some(id),
+                    fragment,
+                });
             }
         }
 
@@ -985,6 +1002,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};
@@ -1037,7 +1066,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";
@@ -1292,7 +1321,7 @@ fn suggest_disambiguator(
     sp: Option<rustc_span::Span>,
     link_range: &Option<Range<usize>>,
 ) {
-    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 {
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 {
+    () => ()
+}