Rollup merge of #148080 - GuillaumeGomez:fix-jump-def-links, r=lolbinarycat

Zalathar · web-flow · commit 87ba9a525166 · 2025-11-12T12:26:37.000+11:00
[rustdoc] Fix invalid jump to def macro link generation Follow-up of #147820. I realized that when there was no intra-doc link linking to the same item, then the generated link for macros in jump to def would be invalid. To make the code less redundant, I merged the "registering" of items and the href generation use the same code for macros. r? `````@notriddle`````
diff --git a/src/librustdoc/clean/inline.rs b/src/librustdoc/clean/inline.rs
@@ -227,6 +227,30 @@ pub(crate) fn item_relative_path(tcx: TyCtxt<'_>, def_id: DefId) -> Vec<Symbol>
     tcx.def_path(def_id).data.into_iter().filter_map(|elem| elem.data.get_opt_name()).collect()
 }
 
+/// Get the public Rust path to an item. This is used to generate the URL to the item's page.
+///
+/// In particular: we handle macro differently: if it's not a macro 2.0 oe a built-in macro, then
+/// it is generated at the top-level of the crate and its path will be `[crate_name, macro_name]`.
+pub(crate) fn get_item_path(tcx: TyCtxt<'_>, def_id: DefId, kind: ItemType) -> Vec<Symbol> {
+    let crate_name = tcx.crate_name(def_id.krate);
+    let relative = item_relative_path(tcx, def_id);
+
+    if let ItemType::Macro = kind {
+        // Check to see if it is a macro 2.0 or built-in macro
+        // More information in <https://rust-lang.github.io/rfcs/1584-macros.html>.
+        if matches!(
+            CStore::from_tcx(tcx).load_macro_untracked(def_id, tcx),
+            LoadedMacro::MacroDef { def, .. } if !def.macro_rules
+        ) {
+            once(crate_name).chain(relative).collect()
+        } else {
+            vec![crate_name, *relative.last().expect("relative was empty")]
+        }
+    } else {
+        once(crate_name).chain(relative).collect()
+    }
+}
+
 /// Record an external fully qualified name in the external_paths cache.
 ///
 /// These names are used later on by HTML rendering to generate things like
@@ -240,27 +264,12 @@ pub(crate) fn record_extern_fqn(cx: &mut DocContext<'_>, did: DefId, kind: ItemT
         return;
     }
 
-    let crate_name = cx.tcx.crate_name(did.krate);
-
-    let relative = item_relative_path(cx.tcx, did);
-    let fqn = if let ItemType::Macro = kind {
-        // Check to see if it is a macro 2.0 or built-in macro
-        if matches!(
-            CStore::from_tcx(cx.tcx).load_macro_untracked(did, cx.tcx),
-            LoadedMacro::MacroDef { def, .. } if !def.macro_rules
-        ) {
-            once(crate_name).chain(relative).collect()
-        } else {
-            vec![crate_name, *relative.last().expect("relative was empty")]
-        }
-    } else {
-        once(crate_name).chain(relative).collect()
-    };
+    let item_path = get_item_path(cx.tcx, did, kind);
 
     if did.is_local() {
-        cx.cache.exact_paths.insert(did, fqn);
+        cx.cache.exact_paths.insert(did, item_path);
     } else {
-        cx.cache.external_paths.insert(did, (fqn, kind));
+        cx.cache.external_paths.insert(did, (item_path, kind));
     }
 }
 
diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs
@@ -40,6 +40,7 @@ use crate::clean::utils::{is_literal_expr, print_evaluated_const};
 use crate::core::DocContext;
 use crate::formats::cache::Cache;
 use crate::formats::item_type::ItemType;
+use crate::html::format::HrefInfo;
 use crate::html::render::Context;
 use crate::passes::collect_intra_doc_links::UrlFragment;
 
@@ -519,16 +520,16 @@ impl Item {
             .iter()
             .filter_map(|ItemLink { link: s, link_text, page_id: id, fragment }| {
                 debug!(?id);
-                if let Ok((mut href, ..)) = href(*id, cx) {
-                    debug!(?href);
+                if let Ok(HrefInfo { mut url, .. }) = href(*id, cx) {
+                    debug!(?url);
                     if let Some(ref fragment) = *fragment {
-                        fragment.render(&mut href, cx.tcx())
+                        fragment.render(&mut url, cx.tcx())
                     }
                     Some(RenderedLink {
                         original_text: s.clone(),
                         new_text: link_text.clone(),
                         tooltip: link_tooltip(*id, fragment, cx).to_string(),
-                        href,
+                        href: url,
                     })
                 } else {
                     None
diff --git a/src/librustdoc/display.rs b/src/librustdoc/display.rs
@@ -10,19 +10,18 @@ pub(crate) trait Joined: IntoIterator {
     ///
     /// The performance of `joined` is slightly better than `format`, since it doesn't need to use a `Cell` to keep track of whether [`fmt`](Display::fmt)
     /// was already called (`joined`'s API doesn't allow it be called more than once).
-    fn joined(self, sep: impl Display, f: &mut Formatter<'_>) -> fmt::Result;
+    fn joined(&mut self, sep: impl Display, f: &mut Formatter<'_>) -> fmt::Result;
 }
 
 impl<I, T> Joined for I
 where
-    I: IntoIterator<Item = T>,
+    I: Iterator<Item = T>,
     T: Display,
 {
-    fn joined(self, sep: impl Display, f: &mut Formatter<'_>) -> fmt::Result {
-        let mut iter = self.into_iter();
-        let Some(first) = iter.next() else { return Ok(()) };
+    fn joined(&mut self, sep: impl Display, f: &mut Formatter<'_>) -> fmt::Result {
+        let Some(first) = self.next() else { return Ok(()) };
         first.fmt(f)?;
-        for item in iter {
+        for item in self {
             sep.fmt(f)?;
             item.fmt(f)?;
         }
diff --git a/src/librustdoc/html/format.rs b/src/librustdoc/html/format.rs
@@ -17,10 +17,10 @@ use rustc_abi::ExternAbi;
 use rustc_ast::join_path_syms;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir as hir;
-use rustc_hir::def::DefKind;
+use rustc_hir::def::{DefKind, MacroKinds};
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
 use rustc_hir::{ConstStability, StabilityLevel, StableSince};
-use rustc_metadata::creader::{CStore, LoadedMacro};
+use rustc_metadata::creader::CStore;
 use rustc_middle::ty::{self, TyCtxt, TypingMode};
 use rustc_span::symbol::kw;
 use rustc_span::{Symbol, sym};
@@ -349,47 +349,56 @@ pub(crate) enum HrefError {
     UnnamableItem,
 }
 
+/// Type representing information of an `href` attribute.
+pub(crate) struct HrefInfo {
+    /// URL to the item page.
+    pub(crate) url: String,
+    /// Kind of the item (used to generate the `title` attribute).
+    pub(crate) kind: ItemType,
+    /// Rust path to the item (used to generate the `title` attribute).
+    pub(crate) rust_path: Vec<Symbol>,
+}
+
 /// This function is to get the external macro path because they are not in the cache used in
 /// `href_with_root_path`.
 fn generate_macro_def_id_path(
     def_id: DefId,
     cx: &Context<'_>,
     root_path: Option<&str>,
-) -> Result<(String, ItemType, Vec<Symbol>), HrefError> {
+) -> Result<HrefInfo, HrefError> {
     let tcx = cx.tcx();
     let crate_name = tcx.crate_name(def_id.krate);
     let cache = cx.cache();
 
-    let fqp = clean::inline::item_relative_path(tcx, def_id);
-    let mut relative = fqp.iter().copied();
     let cstore = CStore::from_tcx(tcx);
     // We need this to prevent a `panic` when this function is used from intra doc links...
     if !cstore.has_crate_data(def_id.krate) {
         debug!("No data for crate {crate_name}");
         return Err(HrefError::NotInExternalCache);
     }
-    // Check to see if it is a macro 2.0 or built-in macro.
-    // More information in <https://rust-lang.github.io/rfcs/1584-macros.html>.
-    let is_macro_2 = match cstore.load_macro_untracked(def_id, tcx) {
-        // If `def.macro_rules` is `true`, then it's not a macro 2.0.
-        LoadedMacro::MacroDef { def, .. } => !def.macro_rules,
-        _ => false,
+    let DefKind::Macro(kinds) = tcx.def_kind(def_id) else {
+        unreachable!();
     };
-
-    let mut path = if is_macro_2 {
-        once(crate_name).chain(relative).collect()
+    let item_type = if kinds == MacroKinds::DERIVE {
+        ItemType::ProcDerive
+    } else if kinds == MacroKinds::ATTR {
+        ItemType::ProcAttribute
     } else {
-        vec![crate_name, relative.next_back().unwrap()]
+        ItemType::Macro
     };
+    let mut path = clean::inline::get_item_path(tcx, def_id, item_type);
     if path.len() < 2 {
         // The minimum we can have is the crate name followed by the macro name. If shorter, then
         // it means that `relative` was empty, which is an error.
         debug!("macro path cannot be empty!");
         return Err(HrefError::NotInExternalCache);
     }
 
-    if let Some(last) = path.last_mut() {
-        *last = Symbol::intern(&format!("macro.{last}.html"));
+    // FIXME: Try to use `iter().chain().once()` instead.
+    let mut prev = None;
+    if let Some(last) = path.pop() {
+        path.push(Symbol::intern(&format!("{}.{last}.html", item_type.as_str())));
+        prev = Some(last);
     }
 
     let url = match cache.extern_locations[&def_id.krate] {
@@ -410,7 +419,11 @@ fn generate_macro_def_id_path(
             return Err(HrefError::NotInExternalCache);
         }
     };
-    Ok((url, ItemType::Macro, fqp))
+    if let Some(prev) = prev {
+        path.pop();
+        path.push(prev);
+    }
+    Ok(HrefInfo { url, kind: item_type, rust_path: path })
 }
 
 fn generate_item_def_id_path(
@@ -419,7 +432,7 @@ fn generate_item_def_id_path(
     cx: &Context<'_>,
     root_path: Option<&str>,
     original_def_kind: DefKind,
-) -> Result<(String, ItemType, Vec<Symbol>), HrefError> {
+) -> Result<HrefInfo, HrefError> {
     use rustc_middle::traits::ObligationCause;
     use rustc_trait_selection::infer::TyCtxtInferExt;
     use rustc_trait_selection::traits::query::normalize::QueryNormalizeExt;
@@ -455,7 +468,7 @@ fn generate_item_def_id_path(
         let kind = ItemType::from_def_kind(original_def_kind, Some(def_kind));
         url_parts = format!("{url_parts}#{kind}.{}", tcx.item_name(original_def_id))
     };
-    Ok((url_parts, shortty, fqp))
+    Ok(HrefInfo { url: url_parts, kind: shortty, rust_path: fqp })
 }
 
 /// Checks if the given defid refers to an item that is unnamable, such as one defined in a const block.
@@ -530,7 +543,7 @@ pub(crate) fn href_with_root_path(
     original_did: DefId,
     cx: &Context<'_>,
     root_path: Option<&str>,
-) -> Result<(String, ItemType, Vec<Symbol>), HrefError> {
+) -> Result<HrefInfo, HrefError> {
     let tcx = cx.tcx();
     let def_kind = tcx.def_kind(original_did);
     let did = match def_kind {
@@ -596,14 +609,14 @@ pub(crate) fn href_with_root_path(
             }
         }
     };
-    let url_parts = make_href(root_path, shortty, url_parts, fqp, is_remote);
-    Ok((url_parts, shortty, fqp.clone()))
+    Ok(HrefInfo {
+        url: make_href(root_path, shortty, url_parts, fqp, is_remote),
+        kind: shortty,
+        rust_path: fqp.clone(),
+    })
 }
 
-pub(crate) fn href(
-    did: DefId,
-    cx: &Context<'_>,
-) -> Result<(String, ItemType, Vec<Symbol>), HrefError> {
+pub(crate) fn href(did: DefId, cx: &Context<'_>) -> Result<HrefInfo, HrefError> {
     href_with_root_path(did, cx, None)
 }
 
@@ -690,12 +703,12 @@ fn resolved_path(
     } else {
         let path = fmt::from_fn(|f| {
             if use_absolute {
-                if let Ok((_, _, fqp)) = href(did, cx) {
+                if let Ok(HrefInfo { rust_path, .. }) = href(did, cx) {
                     write!(
                         f,
                         "{path}::{anchor}",
-                        path = join_path_syms(&fqp[..fqp.len() - 1]),
-                        anchor = print_anchor(did, *fqp.last().unwrap(), cx)
+                        path = join_path_syms(&rust_path[..rust_path.len() - 1]),
+                        anchor = print_anchor(did, *rust_path.last().unwrap(), cx)
                     )
                 } else {
                     write!(f, "{}", last.name)
@@ -824,12 +837,11 @@ fn print_higher_ranked_params_with_space(
 
 pub(crate) fn print_anchor(did: DefId, text: Symbol, cx: &Context<'_>) -> impl Display {
     fmt::from_fn(move |f| {
-        let parts = href(did, cx);
-        if let Ok((url, short_ty, fqp)) = parts {
+        if let Ok(HrefInfo { url, kind, rust_path }) = href(did, cx) {
             write!(
                 f,
-                r#"<a class="{short_ty}" href="{url}" title="{short_ty} {path}">{text}</a>"#,
-                path = join_path_syms(fqp),
+                r#"<a class="{kind}" href="{url}" title="{kind} {path}">{text}</a>"#,
+                path = join_path_syms(rust_path),
                 text = EscapeBodyText(text.as_str()),
             )
         } else {
@@ -1056,14 +1068,14 @@ fn print_qpath_data(qpath_data: &clean::QPathData, cx: &Context<'_>) -> impl Dis
                 None => self_type.def_id(cx.cache()).and_then(|did| href(did, cx).ok()),
             };
 
-            if let Some((url, _, path)) = parent_href {
+            if let Some(HrefInfo { url, rust_path, .. }) = parent_href {
                 write!(
                     f,
                     "<a class=\"associatedtype\" href=\"{url}#{shortty}.{name}\" \
                                 title=\"type {path}::{name}\">{name}</a>",
                     shortty = ItemType::AssocType,
                     name = assoc.name,
-                    path = join_path_syms(path),
+                    path = join_path_syms(rust_path),
                 )
             } else {
                 write!(f, "{}", assoc.name)
diff --git a/src/librustdoc/html/highlight.rs b/src/librustdoc/html/highlight.rs
@@ -21,6 +21,7 @@ use super::format;
 use crate::clean::PrimitiveType;
 use crate::display::Joined as _;
 use crate::html::escape::EscapeBodyText;
+use crate::html::format::HrefInfo;
 use crate::html::macro_expansion::ExpandedCode;
 use crate::html::render::span_map::{DUMMY_SP, Span};
 use crate::html::render::{Context, LinkFromSrc};
@@ -1357,19 +1358,19 @@ fn generate_link_to_def(
                     LinkFromSrc::External(def_id) => {
                         format::href_with_root_path(*def_id, context, Some(href_context.root_path))
                             .ok()
-                            .map(|(url, _, _)| url)
+                            .map(|HrefInfo { url, .. }| url)
                     }
                     LinkFromSrc::Primitive(prim) => format::href_with_root_path(
                         PrimitiveType::primitive_locations(context.tcx())[prim],
                         context,
                         Some(href_context.root_path),
                     )
                     .ok()
-                    .map(|(url, _, _)| url),
+                    .map(|HrefInfo { url, .. }| url),
                     LinkFromSrc::Doc(def_id) => {
                         format::href_with_root_path(*def_id, context, Some(href_context.root_path))
                             .ok()
-                            .map(|(doc_link, _, _)| doc_link)
+                            .map(|HrefInfo { url, .. }| url)
                     }
                 }
             })
diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs
@@ -74,7 +74,7 @@ use crate::formats::cache::Cache;
 use crate::formats::item_type::ItemType;
 use crate::html::escape::Escape;
 use crate::html::format::{
-    Ending, HrefError, PrintWithSpace, full_print_fn_decl, href, print_abi_with_space,
+    Ending, HrefError, HrefInfo, PrintWithSpace, full_print_fn_decl, href, print_abi_with_space,
     print_constness_with_space, print_default_space, print_generic_bounds, print_generics,
     print_impl, print_path, print_type, print_where_clause, visibility_print_with_space,
 };
@@ -982,7 +982,7 @@ fn assoc_href_attr(
             };
 
             match href(did.expect_def_id(), cx) {
-                Ok((url, ..)) => Href::Url(url, item_type),
+                Ok(HrefInfo { url, .. }) => Href::Url(url, item_type),
                 // The link is broken since it points to an external crate that wasn't documented.
                 // Do not create any link in such case. This is better than falling back to a
                 // dummy anchor like `#{item_type}.{name}` representing the `id` of *this* impl item
diff --git a/tests/rustdoc/jump-to-def/assoc-items.rs b/tests/rustdoc/jump-to-def/assoc-items.rs
@@ -26,8 +26,9 @@ impl C {
     pub fn wat() {}
 }
 
-//@ has - '//a[@href="{{channel}}/core/fmt/macros/macro.Debug.html"]' 'Debug'
-//@ has - '//a[@href="{{channel}}/core/cmp/macro.PartialEq.html"]' 'PartialEq'
+// These two links must not change and in particular must contain `/derive.`!
+//@ has - '//a[@href="{{channel}}/core/fmt/macros/derive.Debug.html"]' 'Debug'
+//@ has - '//a[@href="{{channel}}/core/cmp/derive.PartialEq.html"]' 'PartialEq'
 #[derive(Debug, PartialEq)]
 pub struct Bar;
 impl Trait for Bar {
diff --git a/tests/rustdoc/jump-to-def/derive-macro.rs b/tests/rustdoc/jump-to-def/derive-macro.rs
@@ -0,0 +1,24 @@
+// This test ensures that the same link is generated in both intra-doc links
+// and in jump to def links.
+
+//@ compile-flags: -Zunstable-options --generate-link-to-definition
+
+#![crate_name = "foo"]
+
+// First we check intra-doc links.
+//@ has 'foo/struct.Bar.html'
+//@ has - '//a[@href="{{channel}}/core/fmt/macros/derive.Debug.html"]' 'Debug'
+//@ has - '//a[@href="{{channel}}/core/cmp/derive.PartialEq.html"]' 'PartialEq'
+
+// We also check the "title" attributes.
+//@ has - '//a[@href="{{channel}}/core/fmt/macros/derive.Debug.html"]/@title' 'derive core::fmt::macros::Debug'
+//@ has - '//a[@href="{{channel}}/core/cmp/derive.PartialEq.html"]/@title' 'derive core::cmp::PartialEq'
+
+// Then we check that they are the same in jump to def.
+
+/// [Debug][derive@Debug] and [PartialEq][derive@PartialEq]
+//@ has 'src/foo/derive-macro.rs.html'
+//@ has - '//a[@href="{{channel}}/core/fmt/macros/derive.Debug.html"]' 'Debug'
+//@ has - '//a[@href="{{channel}}/core/cmp/derive.PartialEq.html"]' 'PartialEq'
+#[derive(Debug, PartialEq)]
+pub struct Bar;
