Skip to content

[rustdoc] Fix invalid jump to def macro link generation #148080

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+118 −71
Open

[rustdoc] Fix invalid jump to def macro link generation #148080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04f798b
Fix invalid jump to def macro link generation
GuillaumeGomez Oct 24, 2025
dacabcd
Update jump to def macro link generation test
GuillaumeGomez Oct 24, 2025
f4e1ffc
Add missing documentation
GuillaumeGomez Nov 6, 2025
4d3d3b3
Improve code
GuillaumeGomez Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 27 additions & 18 deletions src/librustdoc/clean/inline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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));
}
}

Expand Down
9 changes: 5 additions & 4 deletions src/librustdoc/clean/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down
11 changes: 5 additions & 6 deletions src/librustdoc/display.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
}
Expand Down
84 changes: 48 additions & 36 deletions src/librustdoc/html/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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);
}
Copy link
Contributor

@lolbinarycat lolbinarycat Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a bit excessive, but the clone isn't actually needed since we only need path to be an iterator.

we could do something like let path = fqp[..fqp.len()-1].iter().chain(once(Symbol::intern(&format("{}.{last}.html"))).

no idea if this would be worth it, but it is possible.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is surprisingly complex to do because of from_fn so for now I used pop and push on path and added a FIXME.


let url = match cache.extern_locations[&def_id.krate] {
Expand All @@ -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(
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 4 additions & 3 deletions src/librustdoc/html/highlight.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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)
}
}
})
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/html/render/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions tests/rustdoc/jump-to-def/assoc-items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.`!
Copy link
Contributor

@lolbinarycat lolbinarycat Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why must they not change? Seems like an odd thing to say without explanation considering this PR is changing them.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they are valid as is and since it's derive macros, "derive" must be in the URL.

Copy link
Contributor

@lolbinarycat lolbinarycat Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, they're obviously the best thing to do currently, but that's the case for most things in tests, "must not" seems a bit strong for "this is the current best"

Also, does changing this break any existing links to these pages? It doesn't look like there are redirects.

//@ 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 {
Expand Down
24 changes: 24 additions & 0 deletions tests/rustdoc/jump-to-def/derive-macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// This test ensures that the same link is generated in both intra-doc links
Copy link
Contributor

@lolbinarycat lolbinarycat Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this file in the existing tests/rustdoc/jump-to-def dir. (created #148548 about moving existing files that should also be in there)

Also, I'm slightly confused as to how this test works, and what it's testing, as the PR description says "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.", but that's not what's happening here, and trying to run this test against current nightly actually results in an ICE, not an invalid link.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there is no intra-doc link linking to Debug and PartialEq derive proc macro in the docs, which is what we're testing here.

Also, that might be a newer change because originally, it didn't ICE but generated invalid links.

Copy link
Contributor

@lolbinarycat lolbinarycat Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean "there's no intra-doc link linking to Debug and PartialEq in core"? Because line 10 and 11 literally test that a link exists. Or are you not counting those as intra-doc because they aren't written as a doc comment? If that's the case, you should probably remove the comment on line 9 that describes them as doc comments.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arf, I'm so bad at explaining.

So:

//! [PartialEq] [Debug]

#[derive(Debug, PartialEq)]
pub struct Foo;

Before this PR, if the //! [PartialEq] [Debug] doc comment was not present, in "jump to def", the links for Debug and PartialEq would be wrongly generated there (in the source code pages). Because it wasn't in the cache and so the path computation was wrong.

Copy link
Contributor

@lolbinarycat lolbinarycat Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense, it's just that the PR description and the code comments in the test have conflicting definitions of "intra-doc link", which was very confusing at first.

// 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;
Loading