Fix broken handling of primitive items

- Fix broken handling of primitive associated items
- Remove fragment hack

  Fixes 83083

- more logging
- Update CrateNum hacks

  The CrateNum has no relation to where in the dependency tree the crate
  is, only when it's loaded. Explicitly special-case core instead of
  assuming it will be the first DefId.

- Update and add tests
- Cache calculation of primitive locations

  This could possibly be avoided by passing a Cache into
  collect_intra_doc_links; but that's a much larger change, and doesn't
  seem valuable other than for this.

Author: jyn514

src/librustdoc/clean/types.rs

@@ -461,60 +461,20 @@ impl Item {
             .map_or(&[][..], |v| v.as_slice())
             .iter()
             .filter_map(|ItemLink { link: s, link_text, did, ref fragment }| {
-                match did {
-                    Some(did) => {
-                        if let Ok((mut href, ..)) = href(*did, cx) {
-                            if let Some(ref fragment) = *fragment {
-                                href.push('#');
-                                href.push_str(fragment);
-                            }
-                            Some(RenderedLink {
-                                original_text: s.clone(),
-                                new_text: link_text.clone(),
-                                href,
-                            })
-                        } else {
-                            None
-                        }
-                    }
-                    // FIXME(83083): using fragments as a side-channel for
-                    // primitive names is very unfortunate
-                    None => {
-                        let relative_to = &cx.current;
-                        if let Some(ref fragment) = *fragment {
-                            let url = match cx.cache().extern_locations.get(&self.def_id.krate()) {
-                                Some(&ExternalLocation::Local) => {
-                                    if relative_to[0] == "std" {
-                                        let depth = relative_to.len() - 1;
-                                        "../".repeat(depth)
-                                    } else {
-                                        let depth = relative_to.len();
-                                        format!("{}std/", "../".repeat(depth))
-                                    }
-                                }
-                                Some(ExternalLocation::Remote(ref s)) => {
-                                    format!("{}/std/", s.trim_end_matches('/'))
-                                }
-                                Some(ExternalLocation::Unknown) | None => {
-                                    format!("{}/std/", crate::DOC_RUST_LANG_ORG_CHANNEL)
-                                }
-                            };
-                            // This is a primitive so the url is done "by hand".
-                            let tail = fragment.find('#').unwrap_or_else(|| fragment.len());
-                            Some(RenderedLink {
-                                original_text: s.clone(),
-                                new_text: link_text.clone(),
-                                href: format!(
-                                    "{}primitive.{}.html{}",
-                                    url,
-                                    &fragment[..tail],
-                                    &fragment[tail..]
-                                ),
-                            })
-                        } else {
-                            panic!("This isn't a primitive?!");
-                        }
+                debug!(?did);
+                if let Ok((mut href, ..)) = href(*did, cx) {
+                    debug!(?href);
+                    if let Some(ref fragment) = *fragment {
+                        href.push('#');
+                        href.push_str(fragment);
                     }
+                    Some(RenderedLink {
+                        original_text: s.clone(),
+                        new_text: link_text.clone(),
+                        href,
+                    })
+                } else {
+                    None
                 }
             })
             .collect()
@@ -531,18 +491,10 @@ impl Item {
             .get(&self.def_id)
             .map_or(&[][..], |v| v.as_slice())
             .iter()
-            .filter_map(|ItemLink { link: s, link_text, did, fragment }| {
-                // FIXME(83083): using fragments as a side-channel for
-                // primitive names is very unfortunate
-                if did.is_some() || fragment.is_some() {
-                    Some(RenderedLink {
-                        original_text: s.clone(),
-                        new_text: link_text.clone(),
-                        href: String::new(),
-                    })
-                } else {
-                    None
-                }
+            .map(|ItemLink { link: s, link_text, .. }| RenderedLink {
+                original_text: s.clone(),
+                new_text: link_text.clone(),
+                href: String::new(),
             })
             .collect()
     }
@@ -963,7 +915,7 @@ crate struct Attributes {
     crate other_attrs: Vec<ast::Attribute>,
 }
 
-#[derive(Clone, Debug, Default, PartialEq, Eq, Hash)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
 /// A link that has not yet been rendered.
 ///
 /// This link will be turned into a rendered link by [`Item::links`].
@@ -975,7 +927,7 @@ crate struct ItemLink {
     /// 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>,
+    pub(crate) did: DefId,
     /// The url fragment to append to the link
     pub(crate) fragment: Option<String>,
 }
@@ -1802,6 +1754,39 @@ impl PrimitiveType {
             Never => sym::never,
         }
     }
+
+    /// Returns the DefId of the module with `doc(primitive)` for this primitive type.
+    /// Panics if there is no such module.
+    ///
+    /// This gives precedence to primitives defined in the current crate, and deprioritizes primitives defined in `core`,
+    /// but otherwise, if multiple crates define the same primitive, there is no guarantee of which will be picked.
+    /// In particular, if a crate depends on both `std` and another crate that also defines `doc(primitive)`, then
+    /// it's entirely random whether `std` or the other crate is picked. (no_std crates are usually fine unless multiple dependencies define a primitive.)
+    crate fn primitive_locations(tcx: TyCtxt<'_>) -> &FxHashMap<PrimitiveType, DefId> {
+        static PRIMITIVE_LOCATIONS: OnceCell<FxHashMap<PrimitiveType, DefId>> = OnceCell::new();
+        PRIMITIVE_LOCATIONS.get_or_init(|| {
+            let mut primitive_locations = FxHashMap::default();
+            // NOTE: technically this misses crates that are only passed with `--extern` and not loaded when checking the crate.
+            // This is a degenerate case that I don't plan to support.
+            for &crate_num in tcx.crates(()) {
+                let e = ExternalCrate { crate_num };
+                let crate_name = e.name(tcx);
+                debug!(?crate_num, ?crate_name);
+                for &(def_id, prim) in &e.primitives(tcx) {
+                    // HACK: try to link to std instead where possible
+                    if crate_name == sym::core && primitive_locations.get(&prim).is_some() {
+                        continue;
+                    }
+                    primitive_locations.insert(prim, def_id);
+                }
+            }
+            let local_primitives = ExternalCrate { crate_num: LOCAL_CRATE }.primitives(tcx);
+            for (def_id, prim) in local_primitives {
+                primitive_locations.insert(prim, def_id);
+            }
+            primitive_locations
+        })
+    }
 }
 
 impl From<ast::IntTy> for PrimitiveType {

src/librustdoc/formats/cache.rs

@@ -6,7 +6,7 @@ use rustc_middle::middle::privacy::AccessLevels;
 use rustc_middle::ty::TyCtxt;
 use rustc_span::symbol::sym;
 
-use crate::clean::{self, GetDefId, ItemId};
+use crate::clean::{self, GetDefId, ItemId, PrimitiveType};
 use crate::config::RenderOptions;
 use crate::fold::DocFolder;
 use crate::formats::item_type::ItemType;
@@ -159,17 +159,16 @@ impl Cache {
             self.external_paths.insert(e.def_id(), (vec![name.to_string()], ItemType::Module));
         }
 
-        // Cache where all known primitives have their documentation located.
-        //
-        // Favor linking to as local extern as possible, so iterate all crates in
-        // reverse topological order.
-        for &e in krate.externs.iter().rev() {
-            for &(def_id, prim) in &e.primitives(tcx) {
-                self.primitive_locations.insert(prim, def_id);
-            }
-        }
-        for &(def_id, prim) in &krate.primitives {
-            self.primitive_locations.insert(prim, def_id);
+        // FIXME: avoid this clone (requires implementing Default manually)
+        self.primitive_locations = PrimitiveType::primitive_locations(tcx).clone();
+        for (prim, &def_id) in &self.primitive_locations {
+            let crate_name = tcx.crate_name(def_id.krate);
+            // Recall that we only allow primitive modules to be at the root-level of the crate.
+            // If that restriction is ever lifted, this will have to include the relative paths instead.
+            self.external_paths.insert(
+                def_id,
+                (vec![crate_name.to_string(), prim.as_sym().to_string()], ItemType::Primitive),
+            );
         }
 
         krate = CacheBuilder { tcx, cache: self }.fold_crate(krate);

src/librustdoc/html/format.rs

@@ -509,14 +509,19 @@ crate fn href_with_root_path(
         if shortty == ItemType::Module { fqp } else { &fqp[..fqp.len() - 1] }
     }
 
-    if !did.is_local() && !cache.access_levels.is_public(did) && !cache.document_private {
+    if !did.is_local()
+        && !cache.access_levels.is_public(did)
+        && !cache.document_private
+        && !cache.primitive_locations.values().any(|&id| id == did)
+    {
         return Err(HrefError::Private);
     }
 
     let mut is_remote = false;
     let (fqp, shortty, mut url_parts) = match cache.paths.get(&did) {
         Some(&(ref fqp, shortty)) => (fqp, shortty, {
             let module_fqp = to_module_fqp(shortty, fqp);
+            debug!(?fqp, ?shortty, ?module_fqp);
             href_relative_parts(module_fqp, relative_to)
         }),
         None => {
@@ -548,6 +553,7 @@ crate fn href_with_root_path(
             url_parts.insert(0, root);
         }
     }
+    debug!(?url_parts);
     let last = &fqp.last().unwrap()[..];
     let filename;
     match shortty {

src/librustdoc/json/conversions.rs

@@ -30,9 +30,7 @@ impl JsonRenderer<'_> {
             .get(&item.def_id)
             .into_iter()
             .flatten()
-            .filter_map(|clean::ItemLink { link, did, .. }| {
-                did.map(|did| (link.clone(), from_item_id(did.into())))
-            })
+            .map(|clean::ItemLink { link, did, .. }| (link.clone(), from_item_id((*did).into())))
             .collect();
         let docs = item.attrs.collapsed_doc_value();
         let attrs = item

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -14,7 +14,7 @@ use rustc_hir::def::{
 };
 use rustc_hir::def_id::{CrateNum, DefId};
 use rustc_middle::ty::TyCtxt;
-use rustc_middle::{bug, ty};
+use rustc_middle::{bug, span_bug, ty};
 use rustc_resolve::ParentScope;
 use rustc_session::lint::Lint;
 use rustc_span::hygiene::{MacroKind, SyntaxContext};
@@ -98,14 +98,10 @@ impl Res {
         }
     }
 
-    fn def_id(self) -> DefId {
-        self.opt_def_id().expect("called def_id() on a primitive")
-    }
-
-    fn opt_def_id(self) -> Option<DefId> {
+    fn def_id(self, tcx: TyCtxt<'_>) -> DefId {
         match self {
-            Res::Def(_, id) => Some(id),
-            Res::Primitive(_) => None,
+            Res::Def(_, id) => id,
+            Res::Primitive(prim) => *PrimitiveType::primitive_locations(tcx).get(&prim).unwrap(),
         }
     }
 
@@ -237,10 +233,7 @@ enum AnchorFailure {
     /// 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.
+    /// the item if you want to provide your own anchor.
     RustdocAnchorConflict(Res),
 }
 
@@ -388,7 +381,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                         ty::AssocKind::Const => "associatedconstant",
                         ty::AssocKind::Type => "associatedtype",
                     };
-                    let fragment = format!("{}#{}.{}", prim_ty.as_sym(), out, item_name);
+                    let fragment = format!("{}.{}", out, item_name);
                     (Res::Primitive(prim_ty), fragment, Some((kind.as_def_kind(), item.def_id)))
                 })
         })
@@ -475,14 +468,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                     return handle_variant(self.cx, res, extra_fragment);
                 }
                 // Not a trait item; just return what we found.
-                Res::Primitive(ty) => {
-                    if extra_fragment.is_some() {
-                        return Err(ErrorKind::AnchorFailure(
-                            AnchorFailure::RustdocAnchorConflict(res),
-                        ));
-                    }
-                    return Ok((res, Some(ty.as_sym().to_string())));
-                }
                 _ => return Ok((res, extra_fragment.clone())),
             }
         }
@@ -517,6 +502,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                 let (res, fragment, side_channel) =
                     self.resolve_associated_item(ty_res, item_name, ns, module_id)?;
                 let result = if extra_fragment.is_some() {
+                    // NOTE: can never be a primitive since `side_channel.is_none()` only when `res`
+                    // is a trait (and the side channel DefId is always an associated item).
                     let diag_res = side_channel.map_or(res, |(k, r)| Res::Def(k, r));
                     Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(diag_res)))
                 } else {
@@ -1152,7 +1139,7 @@ impl LinkCollector<'_, '_> {
             module_id = DefId { krate, index: CRATE_DEF_INDEX };
         }
 
-        let (mut res, mut fragment) = self.resolve_with_disambiguator_cached(
+        let (mut res, fragment) = self.resolve_with_disambiguator_cached(
             ResolutionInfo {
                 module_id,
                 dis: disambiguator,
@@ -1174,16 +1161,7 @@ impl LinkCollector<'_, '_> {
             if let Some(prim) = resolve_primitive(path_str, TypeNS) {
                 // `prim@char`
                 if matches!(disambiguator, Some(Disambiguator::Primitive)) {
-                    if fragment.is_some() {
-                        anchor_failure(
-                            self.cx,
-                            diag_info,
-                            AnchorFailure::RustdocAnchorConflict(prim),
-                        );
-                        return None;
-                    }
                     res = prim;
-                    fragment = Some(prim.name(self.cx.tcx).to_string());
                 } else {
                     // `[char]` when a `char` module is in scope
                     let candidates = vec![res, prim];
@@ -1303,12 +1281,17 @@ impl LinkCollector<'_, '_> {
                     }
                 }
 
-                Some(ItemLink { link: ori_link.link, link_text, did: None, fragment })
+                Some(ItemLink {
+                    link: ori_link.link,
+                    link_text,
+                    did: res.def_id(self.cx.tcx),
+                    fragment,
+                })
             }
             Res::Def(kind, id) => {
                 verify(kind, id)?;
                 let id = clean::register_res(self.cx, rustc_hir::def::Res::Def(kind, id));
-                Some(ItemLink { link: ori_link.link, link_text, did: Some(id), fragment })
+                Some(ItemLink { link: ori_link.link, link_text, did: id, fragment })
             }
         }
     }
@@ -2069,8 +2052,11 @@ fn anchor_failure(cx: &DocContext<'_>, diag_info: DiagnosticInfo<'_>, failure: A
             diag.span_label(sp, "invalid anchor");
         }
         if let AnchorFailure::RustdocAnchorConflict(Res::Primitive(_)) = failure {
-            diag.note("this restriction may be lifted in a future release");
-            diag.note("see https://github.com/rust-lang/rust/issues/83083 for more information");
+            if let Some(sp) = sp {
+                span_bug!(sp, "anchors should be allowed now");
+            } else {
+                bug!("anchors should be allowed now");
+            }
         }
     });
 }
@@ -2198,10 +2184,11 @@ fn handle_variant(
     use rustc_middle::ty::DefIdTree;
 
     if extra_fragment.is_some() {
+        // NOTE: `res` can never be a primitive since this function is only called when `tcx.def_kind(res) == DefKind::Variant`.
         return Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(res)));
     }
     cx.tcx
-        .parent(res.def_id())
+        .parent(res.def_id(cx.tcx))
         .map(|parent| {
             let parent_def = Res::Def(DefKind::Enum, parent);
             let variant = cx.tcx.expect_variant_res(res.as_hir_res().unwrap());

src/test/rustdoc-ui/intra-doc/anchors.rs

@@ -37,13 +37,3 @@ pub fn bar() {}
 /// Damn enum's variants: [Enum::A#whatever].
 //~^ ERROR `Enum::A#whatever` contains an anchor
 pub fn enum_link() {}
-
-/// Primitives?
-///
-/// [u32#hello]
-//~^ ERROR `u32#hello` contains an anchor
-pub fn x() {}
-
-/// [prim@usize#x]
-//~^ ERROR `prim@usize#x` contains an anchor
-pub mod usize {}