Rollup merge of #78923 - jyn514:intra-doc-comments, r=Manishearth

Cleanup and comment intra-doc link pass

r? ```@Manishearth```
cc ```@seeplusplus```

Author: jonas-schievink

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -1,3 +1,7 @@
+//! This module implements [RFC 1946]: Intra-rustdoc-links
+//!
+//! [RFC 1946]: https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md
+
 use rustc_ast as ast;
 use rustc_data_structures::stable_set::FxHashSet;
 use rustc_errors::{Applicability, DiagnosticBuilder};
@@ -27,7 +31,7 @@ use std::cell::Cell;
 use std::mem;
 use std::ops::Range;
 
-use crate::clean::*;
+use crate::clean::{self, Crate, GetDefId, Import, Item, ItemLink, PrimitiveType};
 use crate::core::DocContext;
 use crate::fold::DocFolder;
 use crate::html::markdown::markdown_links;
@@ -42,10 +46,10 @@ pub const COLLECT_INTRA_DOC_LINKS: Pass = Pass {
 };
 
 pub fn collect_intra_doc_links(krate: Crate, cx: &DocContext<'_>) -> Crate {
-    let mut coll = LinkCollector::new(cx);
-    coll.fold_crate(krate)
+    LinkCollector::new(cx).fold_crate(krate)
 }
 
+/// Top-level errors emitted by this pass.
 enum ErrorKind<'a> {
     Resolve(Box<ResolutionFailure<'a>>),
     AnchorFailure(AnchorFailure),
@@ -58,18 +62,37 @@ impl<'a> From<ResolutionFailure<'a>> for ErrorKind<'a> {
 }
 
 #[derive(Debug)]
+/// A link failed to resolve.
 enum ResolutionFailure<'a> {
     /// This resolved, but with the wrong namespace.
-    /// `Namespace` is the expected namespace (as opposed to the actual).
-    WrongNamespace(Res, Namespace),
+    ///
+    /// `Namespace` is the namespace specified with a disambiguator
+    /// (as opposed to the actual namespace of the `Res`).
+    WrongNamespace(Res, /* disambiguated */ Namespace),
     /// The link failed to resolve. `resolution_failure` should look to see if there's
     /// a more helpful error that can be given.
-    NotResolved { module_id: DefId, partial_res: Option<Res>, unresolved: Cow<'a, str> },
-    /// should not ever happen
+    NotResolved {
+        /// The scope the link was resolved in.
+        module_id: DefId,
+        /// If part of the link resolved, this has the `Res`.
+        ///
+        /// In `[std::io::Error::x]`, `std::io::Error` would be a partial resolution.
+        partial_res: Option<Res>,
+        /// The remaining unresolved path segments.
+        ///
+        /// In `[std::io::Error::x]`, `x` would be unresolved.
+        unresolved: Cow<'a, str>,
+    },
+    /// This happens when rustdoc can't determine the parent scope for an item.
+    ///
+    /// It is always a bug in rustdoc.
     NoParentItem,
     /// This link has malformed generic parameters; e.g., the angle brackets are unbalanced.
     MalformedGenerics(MalformedGenerics),
-    /// used to communicate that this should be ignored, but shouldn't be reported to the user
+    /// Used to communicate that this should be ignored, but shouldn't be reported to the user
+    ///
+    /// This happens when there is no disambiguator and one of the namespaces
+    /// failed to resolve.
     Dummy,
 }
 
@@ -115,7 +138,9 @@ enum MalformedGenerics {
 }
 
 impl ResolutionFailure<'a> {
-    // This resolved fully (not just partially) but is erroneous for some other reason
+    /// This resolved fully (not just partially) but is erroneous for some other reason
+    ///
+    /// Returns the full resolution of the link, if present.
     fn full_res(&self) -> Option<Res> {
         match self {
             Self::WrongNamespace(res, _) => Some(*res),
@@ -125,13 +150,30 @@ impl ResolutionFailure<'a> {
 }
 
 enum AnchorFailure {
+    /// User error: `[std#x#y]` is not valid
     MultipleAnchors,
+    /// The anchor provided by the user conflicts with Rustdoc's generated anchor.
+    ///
+    /// This is an unfortunate state of affairs. Not every item that can be
+    /// linked to has its own page; sometimes it is a subheading within a page,
+    /// like for associated items. In those cases, rustdoc uses an anchor to
+    /// link to the subheading. Since you can't have two anchors for the same
+    /// link, Rustdoc disallows having a user-specified anchor.
+    ///
+    /// Most of the time this is fine, because you can just link to the page of
+    /// the item if you want to provide your own anchor. For primitives, though,
+    /// rustdoc uses the anchor as a side channel to know which page to link to;
+    /// it doesn't show up in the generated link. Ideally, rustdoc would remove
+    /// this limitation, allowing you to link to subheaders on primitives.
     RustdocAnchorConflict(Res),
 }
 
 struct LinkCollector<'a, 'tcx> {
     cx: &'a DocContext<'tcx>,
-    // NOTE: this may not necessarily be a module in the current crate
+    /// A stack of modules used to decide what scope to resolve in.
+    ///
+    /// The last module will be used if the parent scope of the current item is
+    /// unknown.
     mod_ids: Vec<DefId>,
     /// This is used to store the kind of associated items,
     /// because `clean` and the disambiguator code expect them to be different.
@@ -144,6 +186,12 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         LinkCollector { cx, mod_ids: Vec::new(), kind_side_channel: Cell::new(None) }
     }
 
+    /// Given a full link, parse it as an [enum struct variant].
+    ///
+    /// In particular, this will return an error whenever there aren't three
+    /// full path segments left in the link.
+    ///
+    /// [enum struct variant]: hir::VariantData::Struct
     fn variant_field(
         &self,
         path_str: &'path str,
@@ -235,6 +283,10 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         }
     }
 
+    /// Given a primitive type, try to resolve an associated item.
+    ///
+    /// HACK(jynelson): `item_str` is passed in instead of derived from `item_name` so the
+    /// lifetimes on `&'path` will work.
     fn resolve_primitive_associated_item(
         &self,
         prim_ty: hir::PrimTy,
@@ -286,14 +338,17 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
     }
 
     /// Resolves a string as a macro.
-    fn macro_resolve(
+    ///
+    /// FIXME(jynelson): Can this be unified with `resolve()`?
+    fn resolve_macro(
         &self,
         path_str: &'a str,
         module_id: DefId,
     ) -> Result<Res, ResolutionFailure<'a>> {
         let cx = self.cx;
         let path = ast::Path::from_ident(Ident::from_str(path_str));
         cx.enter_resolver(|resolver| {
+            // FIXME(jynelson): does this really need 3 separate lookups?
             if let Ok((Some(ext), res)) = resolver.resolve_macro_path(
                 &path,
                 None,
@@ -326,6 +381,11 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         })
     }
 
+    /// Convenience wrapper around `resolve_str_path_error`.
+    ///
+    /// This also handles resolving `true` and `false` as booleans.
+    /// NOTE: `resolve_str_path_error` knows only about paths, not about types.
+    /// Associated items will never be resolved by this function.
     fn resolve_path(&self, path_str: &str, ns: Namespace, module_id: DefId) -> Option<Res> {
         let result = self.cx.enter_resolver(|resolver| {
             resolver.resolve_str_path_error(DUMMY_SP, &path_str, ns, module_id)
@@ -339,12 +399,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         }
     }
 
-    /// Resolves a string as a path within a particular namespace. Also returns an optional
-    /// URL fragment in the case of variants and methods.
+    /// Resolves a string as a path within a particular namespace. Returns an
+    /// optional URL fragment in the case of variants and methods.
     fn resolve<'path>(
         &self,
         path_str: &'path str,
         ns: Namespace,
+        // FIXME(#76467): This is for `Self`, and it's wrong.
         current_item: &Option<String>,
         module_id: DefId,
         extra_fragment: &Option<String>,
@@ -353,15 +414,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
 
         if let Some(res) = self.resolve_path(path_str, ns, module_id) {
             match res {
+                // FIXME(#76467): make this fallthrough to lookup the associated
+                // item a separate function.
                 Res::Def(DefKind::AssocFn | DefKind::AssocConst, _) => {
                     assert_eq!(ns, ValueNS);
-                    // Fall through: In case this is a trait item, skip the
-                    // early return and try looking for the trait.
                 }
                 Res::Def(DefKind::AssocTy, _) => {
                     assert_eq!(ns, TypeNS);
-                    // Fall through: In case this is a trait item, skip the
-                    // early return and try looking for the trait.
                 }
                 Res::Def(DefKind::Variant, _) => {
                     return handle_variant(cx, res, extra_fragment);
@@ -410,7 +469,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
             })?;
 
         // FIXME: are these both necessary?
-        let ty_res = if let Some(ty_res) = is_primitive(&path_root, TypeNS)
+        let ty_res = if let Some(ty_res) = resolve_primitive(&path_root, TypeNS)
             .map(|(_, res)| res)
             .or_else(|| self.resolve_path(&path_root, TypeNS, module_id))
         {
@@ -452,8 +511,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                     // There should only ever be one associated item that matches from any inherent impl
                     .next()
                     // Check if item_name belongs to `impl SomeTrait for SomeItem`
-                    // This gives precedence to `impl SomeItem`:
-                    // Although having both would be ambiguous, use impl version for compat. sake.
+                    // FIXME(#74563): This gives precedence to `impl SomeItem`:
+                    // Although having both would be ambiguous, use impl version for compatibility's sake.
                     // To handle that properly resolve() would have to support
                     // something like [`ambi_fn`](<SomeStruct as SomeTrait>::ambi_fn)
                     .or_else(|| {
@@ -480,6 +539,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                     })
                 } else if ns == Namespace::ValueNS {
                     debug!("looking for variants or fields named {} for {:?}", item_name, did);
+                    // FIXME(jynelson): why is this different from
+                    // `variant_field`?
                     match cx.tcx.type_of(did).kind() {
                         ty::Adt(def, _) => {
                             let field = if def.is_enum() {
@@ -577,7 +638,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
     ) -> Option<Res> {
         // resolve() can't be used for macro namespace
         let result = match ns {
-            Namespace::MacroNS => self.macro_resolve(path_str, module_id).map_err(ErrorKind::from),
+            Namespace::MacroNS => self.resolve_macro(path_str, module_id).map_err(ErrorKind::from),
             Namespace::TypeNS | Namespace::ValueNS => self
                 .resolve(path_str, ns, current_item, module_id, extra_fragment)
                 .map(|(res, _)| res),
@@ -593,6 +654,11 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
     }
 }
 
+/// Look to see if a resolved item has an associated item named `item_name`.
+///
+/// Given `[std::io::Error::source]`, where `source` is unresolved, this would
+/// find `std::error::Error::source` and return
+/// `<io::Error as error::Error>::source`.
 fn resolve_associated_trait_item(
     did: DefId,
     module: DefId,
@@ -601,12 +667,12 @@ fn resolve_associated_trait_item(
     cx: &DocContext<'_>,
 ) -> Option<(ty::AssocKind, DefId)> {
     let ty = cx.tcx.type_of(did);
-    // First consider automatic impls: `impl From<T> for T`
+    // First consider blanket impls: `impl From<T> for T`
     let implicit_impls = crate::clean::get_auto_trait_and_blanket_impls(cx, ty, did);
     let mut candidates: Vec<_> = implicit_impls
         .flat_map(|impl_outer| {
             match impl_outer.inner {
-                ImplItem(impl_) => {
+                clean::ImplItem(impl_) => {
                     debug!("considering auto or blanket impl for trait {:?}", impl_.trait_);
                     // Give precedence to methods that were overridden
                     if !impl_.provided_trait_methods.contains(&*item_name.as_str()) {
@@ -669,7 +735,7 @@ fn resolve_associated_trait_item(
                 .map(|assoc| (assoc.kind, assoc.def_id))
         }));
     }
-    // FIXME: warn about ambiguity
+    // FIXME(#74563): warn about ambiguity
     debug!("the candidates were {:?}", candidates);
     candidates.pop()
 }
@@ -719,20 +785,15 @@ fn traits_implemented_by(cx: &DocContext<'_>, type_: DefId, module: DefId) -> Fx
     iter.collect()
 }
 
-/// Check for resolve collisions between a trait and its derive
+/// Check for resolve collisions between a trait and its derive.
 ///
-/// These are common and we should just resolve to the trait in that case
+/// These are common and we should just resolve to the trait in that case.
 fn is_derive_trait_collision<T>(ns: &PerNS<Result<(Res, T), ResolutionFailure<'_>>>) -> bool {
-    if let PerNS {
+    matches!(*ns, PerNS {
         type_ns: Ok((Res::Def(DefKind::Trait, _), _)),
         macro_ns: Ok((Res::Def(DefKind::Macro(MacroKind::Derive), _), _)),
         ..
-    } = *ns
-    {
-        true
-    } else {
-        false
-    }
+    })
 }
 
 impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
@@ -772,29 +833,30 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
         }
 
         let current_item = match item.inner {
-            ModuleItem(..) => {
+            clean::ModuleItem(..) => {
                 if item.attrs.inner_docs {
                     if item.def_id.is_top_level_module() { item.name.clone() } else { None }
                 } else {
                     match parent_node.or(self.mod_ids.last().copied()) {
                         Some(parent) if !parent.is_top_level_module() => {
-                            // FIXME: can we pull the parent module's name from elsewhere?
                             Some(self.cx.tcx.item_name(parent).to_string())
                         }
                         _ => None,
                     }
                 }
             }
-            ImplItem(Impl { ref for_, .. }) => {
+            clean::ImplItem(clean::Impl { ref for_, .. }) => {
                 for_.def_id().map(|did| self.cx.tcx.item_name(did).to_string())
             }
             // we don't display docs on `extern crate` items anyway, so don't process them.
-            ExternCrateItem(..) => {
+            clean::ExternCrateItem(..) => {
                 debug!("ignoring extern crate item {:?}", item.def_id);
                 return self.fold_item_recur(item);
             }
-            ImportItem(Import { kind: ImportKind::Simple(ref name, ..), .. }) => Some(name.clone()),
-            MacroItem(..) => None,
+            clean::ImportItem(Import { kind: clean::ImportKind::Simple(ref name, ..), .. }) => {
+                Some(name.clone())
+            }
+            clean::MacroItem(..) => None,
             _ => item.name.clone(),
         };
 
@@ -803,6 +865,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
         }
 
         // find item's parent to resolve `Self` in item's docs below
+        // FIXME(#76467, #75809): this is a mess and doesn't handle cross-crate
+        // re-exports
         let parent_name = self.cx.as_local_hir_id(item.def_id).and_then(|item_hir| {
             let parent_hir = self.cx.tcx.hir().get_parent_item(item_hir);
             let item_parent = self.cx.tcx.hir().find(parent_hir);
@@ -870,7 +934,6 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
             };
             // NOTE: if there are links that start in one crate and end in another, this will not resolve them.
             // This is a degenerate case and it's not supported by rustdoc.
-            // FIXME: this will break links that start in `#[doc = ...]` and end as a sugared doc. Should this be supported?
             for (ori_link, link_range) in markdown_links(&combined_docs) {
                 let link = self.resolve_link(
                     &item,
@@ -888,15 +951,13 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
             }
         }
 
-        if item.is_mod() && !item.attrs.inner_docs {
-            self.mod_ids.push(item.def_id);
-        }
-
         if item.is_mod() {
-            let ret = self.fold_item_recur(item);
+            if !item.attrs.inner_docs {
+                self.mod_ids.push(item.def_id);
+            }
 
+            let ret = self.fold_item_recur(item);
             self.mod_ids.pop();
-
             ret
         } else {
             self.fold_item_recur(item)
@@ -905,6 +966,9 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
 }
 
 impl LinkCollector<'_, '_> {
+    /// This is the entry point for resolving an intra-doc link.
+    ///
+    /// FIXME(jynelson): this is way too many arguments
     fn resolve_link(
         &self,
         item: &Item,
@@ -943,129 +1007,120 @@ impl LinkCollector<'_, '_> {
         } else {
             (parts[0], None)
         };
-        let resolved_self;
-        let link_text;
-        let mut path_str;
-        let disambiguator;
-        let stripped_path_string;
-        let (mut res, mut fragment) = {
-            path_str = if let Ok((d, path)) = Disambiguator::from_str(&link) {
-                disambiguator = Some(d);
-                path
-            } else {
-                disambiguator = None;
-                &link
-            }
-            .trim();
 
-            if path_str.contains(|ch: char| !(ch.is_alphanumeric() || ":_<>, ".contains(ch))) {
-                return None;
-            }
+        // Parse and strip the disambiguator from the link, if present.
+        let (mut path_str, disambiguator) = if let Ok((d, path)) = Disambiguator::from_str(&link) {
+            (path.trim(), Some(d))
+        } else {
+            (link.trim(), None)
+        };
 
-            // 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
-            // our name resolution *inside* the module.  If, on the other
-            // hand it was an outer comment (///) then we anchor the name
-            // resolution in the parent module on the basis that the names
-            // used are more likely to be intended to be parent names.  For
-            // this, we set base_node to None for inner comments since
-            // we've already pushed this node onto the resolution stack but
-            // for outer comments we explicitly try and resolve against the
-            // parent_node first.
-            let base_node = if item.is_mod() && item.attrs.inner_docs {
-                self.mod_ids.last().copied()
-            } else {
-                parent_node
-            };
+        if path_str.contains(|ch: char| !(ch.is_alphanumeric() || ":_<>, ".contains(ch))) {
+            return None;
+        }
 
-            let mut module_id = if let Some(id) = base_node {
-                id
-            } else {
-                debug!("attempting to resolve item without parent module: {}", path_str);
-                let err_kind = ResolutionFailure::NoParentItem.into();
-                resolution_failure(
-                    self,
-                    &item,
-                    path_str,
-                    disambiguator,
-                    dox,
-                    link_range,
-                    smallvec![err_kind],
-                );
-                return None;
-            };
+        // We stripped `()` and `!` when parsing the disambiguator.
+        // Add them back to be displayed, but not prefix disambiguators.
+        let 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
+        // our name resolution *inside* the module.  If, on the other
+        // hand it was an outer comment (///) then we anchor the name
+        // resolution in the parent module on the basis that the names
+        // used are more likely to be intended to be parent names.  For
+        // this, we set base_node to None for inner comments since
+        // we've already pushed this node onto the resolution stack but
+        // for outer comments we explicitly try and resolve against the
+        // parent_node first.
+        let base_node = if item.is_mod() && item.attrs.inner_docs {
+            self.mod_ids.last().copied()
+        } else {
+            parent_node
+        };
 
-            // replace `Self` with suitable item's parent name
-            if path_str.starts_with("Self::") {
-                if let Some(ref name) = parent_name {
-                    resolved_self = format!("{}::{}", name, &path_str[6..]);
-                    path_str = &resolved_self;
-                }
-            } else if path_str.starts_with("crate::") {
-                use rustc_span::def_id::CRATE_DEF_INDEX;
-
-                // HACK(jynelson): rustc_resolve thinks that `crate` is the crate currently being documented.
-                // But rustdoc wants it to mean the crate this item was originally present in.
-                // To work around this, remove it and resolve relative to the crate root instead.
-                // HACK(jynelson)(2): If we just strip `crate::` then suddenly primitives become ambiguous
-                // (consider `crate::char`). Instead, change it to `self::`. This works because 'self' is now the crate root.
-                resolved_self = format!("self::{}", &path_str["crate::".len()..]);
-                path_str = &resolved_self;
-                module_id = DefId { krate, index: CRATE_DEF_INDEX };
-            }
+        let mut module_id = if let Some(id) = base_node {
+            id
+        } else {
+            // This is a bug.
+            debug!("attempting to resolve item without parent module: {}", path_str);
+            let err_kind = ResolutionFailure::NoParentItem.into();
+            resolution_failure(
+                self,
+                &item,
+                path_str,
+                disambiguator,
+                dox,
+                link_range,
+                smallvec![err_kind],
+            );
+            return None;
+        };
 
-            // Strip generics from the path.
-            if path_str.contains(['<', '>'].as_slice()) {
-                stripped_path_string = match strip_generics_from_path(path_str) {
-                    Ok(path) => path,
-                    Err(err_kind) => {
-                        debug!("link has malformed generics: {}", path_str);
-                        resolution_failure(
-                            self,
-                            &item,
-                            path_str,
-                            disambiguator,
-                            dox,
-                            link_range,
-                            smallvec![err_kind],
-                        );
-                        return None;
-                    }
-                };
-                path_str = &stripped_path_string;
+        let resolved_self;
+        // replace `Self` with suitable item's parent name
+        if path_str.starts_with("Self::") {
+            if let Some(ref name) = parent_name {
+                resolved_self = format!("{}::{}", name, &path_str[6..]);
+                path_str = &resolved_self;
             }
+        } else if path_str.starts_with("crate::") {
+            use rustc_span::def_id::CRATE_DEF_INDEX;
+
+            // HACK(jynelson): rustc_resolve thinks that `crate` is the crate currently being documented.
+            // But rustdoc wants it to mean the crate this item was originally present in.
+            // To work around this, remove it and resolve relative to the crate root instead.
+            // HACK(jynelson)(2): If we just strip `crate::` then suddenly primitives become ambiguous
+            // (consider `crate::char`). Instead, change it to `self::`. This works because 'self' is now the crate root.
+            // FIXME(#78696): This doesn't always work.
+            resolved_self = format!("self::{}", &path_str["crate::".len()..]);
+            path_str = &resolved_self;
+            module_id = DefId { krate, index: CRATE_DEF_INDEX };
+        }
 
-            // Sanity check to make sure we don't have any angle brackets after stripping generics.
-            assert!(!path_str.contains(['<', '>'].as_slice()));
+        // Strip generics from the path.
+        let stripped_path_string;
+        if path_str.contains(['<', '>'].as_slice()) {
+            stripped_path_string = match strip_generics_from_path(path_str) {
+                Ok(path) => path,
+                Err(err_kind) => {
+                    debug!("link has malformed generics: {}", path_str);
+                    resolution_failure(
+                        self,
+                        &item,
+                        path_str,
+                        disambiguator,
+                        dox,
+                        link_range,
+                        smallvec![err_kind],
+                    );
+                    return None;
+                }
+            };
+            path_str = &stripped_path_string;
+        }
+        // Sanity check to make sure we don't have any angle brackets after stripping generics.
+        assert!(!path_str.contains(['<', '>'].as_slice()));
 
-            // The link is not an intra-doc link if it still contains commas or spaces after
-            // stripping generics.
-            if path_str.contains([',', ' '].as_slice()) {
-                return None;
-            }
+        // The link is not an intra-doc link if it still contains commas or spaces after
+        // stripping generics.
+        if path_str.contains([',', ' '].as_slice()) {
+            return None;
+        }
 
-            match self.resolve_with_disambiguator(
-                disambiguator,
-                item,
-                dox,
-                path_str,
-                current_item,
-                module_id,
-                extra_fragment,
-                &ori_link,
-                link_range.clone(),
-            ) {
-                Some(x) => x,
-                None => return None,
-            }
-        };
+        let (mut res, mut fragment) = self.resolve_with_disambiguator(
+            disambiguator,
+            item,
+            dox,
+            path_str,
+            current_item,
+            module_id,
+            extra_fragment,
+            &ori_link,
+            link_range.clone(),
+        )?;
 
         // Check for a primitive which might conflict with a module
         // Report the ambiguity and require that the user specify which one they meant.
@@ -1075,7 +1130,7 @@ impl LinkCollector<'_, '_> {
             None | Some(Disambiguator::Namespace(Namespace::TypeNS) | Disambiguator::Primitive)
         ) && !matches!(res, Res::PrimTy(_))
         {
-            if let Some((path, prim)) = is_primitive(path_str, TypeNS) {
+            if let Some((path, prim)) = resolve_primitive(path_str, TypeNS) {
                 // `prim@char`
                 if matches!(disambiguator, Some(Disambiguator::Primitive)) {
                     if fragment.is_some() {
@@ -1168,11 +1223,13 @@ impl LinkCollector<'_, '_> {
                     privacy_error(cx, &item, &path_str, dox, link_range);
                 }
             }
-            let id = register_res(cx, res);
+            let id = clean::register_res(cx, res);
             Some(ItemLink { link: ori_link, link_text, did: Some(id), fragment })
         }
     }
 
+    /// After parsing the disambiguator, resolve the main part of the link.
+    // FIXME(jynelson): wow this is just so much
     fn resolve_with_disambiguator(
         &self,
         disambiguator: Option<Disambiguator>,
@@ -1232,7 +1289,7 @@ impl LinkCollector<'_, '_> {
                 // Try everything!
                 let mut candidates = PerNS {
                     macro_ns: self
-                        .macro_resolve(path_str, base_node)
+                        .resolve_macro(path_str, base_node)
                         .map(|res| (res, extra_fragment.clone())),
                     type_ns: match self.resolve(
                         path_str,
@@ -1320,10 +1377,10 @@ impl LinkCollector<'_, '_> {
                 }
             }
             Some(MacroNS) => {
-                match self.macro_resolve(path_str, base_node) {
+                match self.resolve_macro(path_str, base_node) {
                     Ok(res) => Some((res, extra_fragment)),
                     Err(mut kind) => {
-                        // `macro_resolve` only looks in the macro namespace. Try to give a better error if possible.
+                        // `resolve_macro` only looks in the macro namespace. Try to give a better error if possible.
                         for &ns in &[TypeNS, ValueNS] {
                             if let Some(res) = self.check_full_res(
                                 ns,
@@ -1354,9 +1411,15 @@ impl LinkCollector<'_, '_> {
 }
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
+/// Disambiguators for a link.
 enum Disambiguator {
+    /// `prim@`
+    ///
+    /// This is buggy, see <https://github.com/rust-lang/rust/pull/77875#discussion_r503583103>
     Primitive,
+    /// `struct@` or `f()`
     Kind(DefKind),
+    /// `type@`
     Namespace(Namespace),
 }
 
@@ -1373,7 +1436,7 @@ impl Disambiguator {
         }
     }
 
-    /// (disambiguator, path_str)
+    /// Given a link, parse and return `(disambiguator, path_str)`
     fn from_str(link: &str) -> Result<(Self, &str), ()> {
         use Disambiguator::{Kind, Namespace as NS, Primitive};
 
@@ -1424,6 +1487,7 @@ impl Disambiguator {
         }
     }
 
+    /// Used for error reporting.
     fn suggestion(self) -> Suggestion {
         let kind = match self {
             Disambiguator::Primitive => return Suggestion::Prefix("prim"),
@@ -1490,9 +1554,13 @@ impl Disambiguator {
     }
 }
 
+/// A suggestion to show in a diagnostic.
 enum Suggestion {
+    /// `struct@`
     Prefix(&'static str),
+    /// `f()`
     Function,
+    /// `m!`
     Macro,
 }
 
@@ -1582,6 +1650,11 @@ fn report_diagnostic(
     });
 }
 
+/// Reports a link that failed to resolve.
+///
+/// This also tries to resolve any intermediate path segments that weren't
+/// handled earlier. For example, if passed `Item::Crate(std)` and `path_str`
+/// `std::io::Error::x`, this will resolve `std::io::Error`.
 fn resolution_failure(
     collector: &LinkCollector<'_, '_>,
     item: &Item,
@@ -1816,6 +1889,7 @@ fn resolution_failure(
     );
 }
 
+/// Report an anchor failure.
 fn anchor_failure(
     cx: &DocContext<'_>,
     item: &Item,
@@ -1840,6 +1914,7 @@ fn anchor_failure(
     });
 }
 
+/// Report an ambiguity error, where there were multiple possible resolutions.
 fn ambiguity_error(
     cx: &DocContext<'_>,
     item: &Item,
@@ -1886,6 +1961,8 @@ fn ambiguity_error(
     });
 }
 
+/// In case of an ambiguity or mismatched disambiguator, suggest the correct
+/// disambiguator.
 fn suggest_disambiguator(
     disambiguator: Disambiguator,
     diag: &mut DiagnosticBuilder<'_>,
@@ -1911,6 +1988,7 @@ fn suggest_disambiguator(
     }
 }
 
+/// Report a link from a public item to a private one.
 fn privacy_error(
     cx: &DocContext<'_>,
     item: &Item,
@@ -1978,7 +2056,8 @@ const PRIMITIVES: &[(Symbol, Res)] = &[
     (sym::char, Res::PrimTy(hir::PrimTy::Char)),
 ];
 
-fn is_primitive(path_str: &str, ns: Namespace) -> Option<(Symbol, Res)> {
+/// Resolve a primitive type or value.
+fn resolve_primitive(path_str: &str, ns: Namespace) -> Option<(Symbol, Res)> {
     is_bool_value(path_str, ns).or_else(|| {
         if ns == TypeNS {
             // FIXME: this should be replaced by a lookup in PrimitiveTypeTable
@@ -1990,6 +2069,7 @@ fn is_primitive(path_str: &str, ns: Namespace) -> Option<(Symbol, Res)> {
     })
 }
 
+/// Resolve a primitive value.
 fn is_bool_value(path_str: &str, ns: Namespace) -> Option<(Symbol, Res)> {
     if ns == TypeNS && (path_str == "true" || path_str == "false") {
         Some((sym::bool, Res::PrimTy(hir::PrimTy::Bool)))