From 6017689fa94bc36c26c3efd78f4d8ed4e02ed218 Mon Sep 17 00:00:00 2001 From: arferreira Date: Fri, 27 Feb 2026 09:54:43 -0500 Subject: [PATCH] Fix relative extern URL depth on source pages --- src/librustdoc/html/format.rs | 80 ++++++++++++------- src/librustdoc/html/highlight.rs | 30 ++++--- src/librustdoc/html/render/mod.rs | 2 +- src/librustdoc/html/sources.rs | 28 +++---- .../passes/collect_intra_doc_links.rs | 2 +- .../extern/extern-html-root-url-relative.rs | 6 +- 6 files changed, 89 insertions(+), 59 deletions(-) diff --git a/src/librustdoc/html/format.rs b/src/librustdoc/html/format.rs index 2070a24f60c81..8cb65d3e47fae 100644 --- a/src/librustdoc/html/format.rs +++ b/src/librustdoc/html/format.rs @@ -360,11 +360,11 @@ pub(crate) struct HrefInfo { } /// This function is to get the external macro path because they are not in the cache used in -/// `href_with_root_path`. +/// `href_with_jump_to_def_path_depth`. fn generate_macro_def_id_path( def_id: DefId, cx: &Context<'_>, - root_path: Option<&str>, + jump_to_def_path_depth: Option, ) -> Result { let tcx = cx.tcx(); let crate_name = tcx.crate_name(def_id.krate); @@ -403,15 +403,15 @@ fn generate_macro_def_id_path( let url = match cache.extern_locations[&def_id.krate] { ExternalLocation::Remote { ref url, is_absolute } => { - let mut prefix = remote_url_prefix(url, is_absolute, cx.current.len()); + let depth = jump_to_def_path_depth.unwrap_or(cx.current.len()); + let mut prefix = remote_url_prefix(url, is_absolute, depth); prefix.extend(path.iter().copied()); prefix.finish() } ExternalLocation::Local => { - // `root_path` always end with a `/`. format!( "{root_path}{path}", - root_path = root_path.unwrap_or(""), + root_path = "../".repeat(jump_to_def_path_depth.unwrap_or(0)), path = fmt::from_fn(|f| path.iter().joined("/", f)) ) } @@ -431,7 +431,7 @@ fn generate_item_def_id_path( mut def_id: DefId, original_def_id: DefId, cx: &Context<'_>, - root_path: Option<&str>, + jump_to_def_path_depth: Option, ) -> Result { use rustc_middle::traits::ObligationCause; use rustc_trait_selection::infer::TyCtxtInferExt; @@ -460,8 +460,9 @@ fn generate_item_def_id_path( let shortty = ItemType::from_def_id(def_id, tcx); let module_fqp = to_module_fqp(shortty, &fqp); - let (parts, is_absolute) = url_parts(cx.cache(), def_id, module_fqp, &cx.current)?; - let mut url = make_href(root_path, shortty, parts, &fqp, is_absolute); + let (parts, is_remote) = + url_parts(cx.cache(), def_id, module_fqp, &cx.current, jump_to_def_path_depth)?; + let mut url = make_href(jump_to_def_path_depth, shortty, parts, &fqp, is_remote); if def_id != original_def_id { let kind = ItemType::from_def_id(original_def_id, tcx); @@ -506,17 +507,33 @@ fn remote_url_prefix(url: &str, is_absolute: bool, depth: usize) -> UrlPartsBuil } } +/// Returns `(url_parts, is_remote)` where `is_remote` indicates the URL points to an +/// external location and should not use relative URLs. +/// +/// This function tries to generate minimal, relative URLs. When generating an intra-doc +/// link from one item page to another, it compares the paths of both items and generates +/// exactly the right number of "../"'s to reach the deepest common parent, then fills in +/// the rest of the path. This is done by comparing `module_fqp` with `relative_to`. +/// +/// When generating a link for jump-to-def across crates, that won't work, because sources are +/// stored under "src/[crate]/[path].rs". When linking jump-to-def for another crate, we just +/// write enough "../"'s to reach the doc root, then generate the entire path for `module_fqp`. +/// The right number of "../"'s is stored in `jump_to_def_path_depth`. fn url_parts( cache: &Cache, def_id: DefId, module_fqp: &[Symbol], relative_to: &[Symbol], + jump_to_def_path_depth: Option, ) -> Result<(UrlPartsBuilder, bool), HrefError> { match cache.extern_locations[&def_id.krate] { ExternalLocation::Remote { ref url, is_absolute } => { - let mut builder = remote_url_prefix(url, is_absolute, relative_to.len()); + let depth = jump_to_def_path_depth.unwrap_or(relative_to.len()); + let mut builder = remote_url_prefix(url, is_absolute, depth); builder.extend(module_fqp.iter().copied()); - Ok((builder, is_absolute)) + // Remote URLs (both absolute and relative) already encode the correct path; + // no additional depth prefix should be applied. + Ok((builder, true)) } ExternalLocation::Local => Ok((href_relative_parts(module_fqp, relative_to), false)), ExternalLocation::Unknown => Err(HrefError::DocumentationNotBuilt), @@ -524,16 +541,16 @@ fn url_parts( } fn make_href( - root_path: Option<&str>, + jump_to_def_path_depth: Option, shortty: ItemType, mut url_parts: UrlPartsBuilder, fqp: &[Symbol], - is_absolute: bool, + is_remote: bool, ) -> String { - // FIXME: relative extern URLs may break when prefixed with root_path - if !is_absolute && let Some(root_path) = root_path { - let root = root_path.trim_end_matches('/'); - url_parts.push_front(root); + // Remote URLs already have the correct depth via `remote_url_prefix`; + // only local items need `jump_to_def_path_depth` to bridge from source pages to the doc tree. + if !is_remote && let Some(depth) = jump_to_def_path_depth { + url_parts.push_front(&iter::repeat_n("..", depth).join("/")); } debug!(?url_parts); match shortty { @@ -548,10 +565,10 @@ fn make_href( url_parts.finish() } -pub(crate) fn href_with_root_path( +pub(crate) fn href_with_jump_to_def_path_depth( original_did: DefId, cx: &Context<'_>, - root_path: Option<&str>, + jump_to_def_path_depth: Option, ) -> Result { let tcx = cx.tcx(); let def_kind = tcx.def_kind(original_did); @@ -562,7 +579,13 @@ pub(crate) fn href_with_root_path( } // If this a constructor, we get the parent (either a struct or a variant) and then // generate the link for this item. - DefKind::Ctor(..) => return href_with_root_path(tcx.parent(original_did), cx, root_path), + DefKind::Ctor(..) => { + return href_with_jump_to_def_path_depth( + tcx.parent(original_did), + cx, + jump_to_def_path_depth, + ); + } DefKind::ExternCrate => { // Link to the crate itself, not the `extern crate` item. if let Some(local_did) = original_did.as_local() { @@ -582,7 +605,7 @@ pub(crate) fn href_with_root_path( if !original_did.is_local() { // If we are generating an href for the "jump to def" feature, then the only case we want // to ignore is if the item is `doc(hidden)` because we can't link to it. - if root_path.is_some() { + if jump_to_def_path_depth.is_some() { if tcx.is_doc_hidden(original_did) { return Err(HrefError::Private); } @@ -594,7 +617,7 @@ pub(crate) fn href_with_root_path( } } - let (fqp, shortty, url_parts, is_absolute) = match cache.paths.get(&did) { + let (fqp, shortty, url_parts, is_remote) = match cache.paths.get(&did) { Some(&(ref fqp, shortty)) => ( fqp, shortty, @@ -609,29 +632,30 @@ pub(crate) fn href_with_root_path( // Associated items are handled differently with "jump to def". The anchor is generated // directly here whereas for intra-doc links, we have some extra computation being // performed there. - let def_id_to_get = if root_path.is_some() { original_did } else { did }; + let def_id_to_get = if jump_to_def_path_depth.is_some() { original_did } else { did }; if let Some(&(ref fqp, shortty)) = cache.external_paths.get(&def_id_to_get) { let module_fqp = to_module_fqp(shortty, fqp); - let (parts, is_absolute) = url_parts(cache, did, module_fqp, relative_to)?; - (fqp, shortty, parts, is_absolute) + let (parts, is_remote) = + url_parts(cache, did, module_fqp, relative_to, jump_to_def_path_depth)?; + (fqp, shortty, parts, is_remote) } else if matches!(def_kind, DefKind::Macro(_)) { - return generate_macro_def_id_path(did, cx, root_path); + return generate_macro_def_id_path(did, cx, jump_to_def_path_depth); } else if did.is_local() { return Err(HrefError::Private); } else { - return generate_item_def_id_path(did, original_did, cx, root_path); + return generate_item_def_id_path(did, original_did, cx, jump_to_def_path_depth); } } }; Ok(HrefInfo { - url: make_href(root_path, shortty, url_parts, fqp, is_absolute), + url: make_href(jump_to_def_path_depth, shortty, url_parts, fqp, is_remote), kind: shortty, rust_path: fqp.clone(), }) } pub(crate) fn href(did: DefId, cx: &Context<'_>) -> Result { - href_with_root_path(did, cx, None) + href_with_jump_to_def_path_depth(did, cx, None) } /// Both paths should only be modules. diff --git a/src/librustdoc/html/highlight.rs b/src/librustdoc/html/highlight.rs index 1c162a79c4c44..bb5f3fe7f97df 100644 --- a/src/librustdoc/html/highlight.rs +++ b/src/librustdoc/html/highlight.rs @@ -33,7 +33,7 @@ pub(crate) struct HrefContext<'a, 'tcx> { pub(crate) file_span: Span, /// This field is used to know "how far" from the top of the directory we are to link to either /// documentation pages or other source pages. - pub(crate) root_path: &'a str, + pub(crate) jump_to_def_path_depth: usize, /// This field is used to calculate precise local URLs. pub(crate) current_href: String, } @@ -1396,23 +1396,27 @@ fn generate_link_to_def( LinkFromSrc::Local(span) => { context.href_from_span_relative(*span, &href_context.current_href) } - LinkFromSrc::External(def_id) => { - format::href_with_root_path(*def_id, context, Some(href_context.root_path)) - .ok() - .map(|HrefInfo { url, .. }| url) - } - LinkFromSrc::Primitive(prim) => format::href_with_root_path( + LinkFromSrc::External(def_id) => format::href_with_jump_to_def_path_depth( + *def_id, + context, + Some(href_context.jump_to_def_path_depth), + ) + .ok() + .map(|HrefInfo { url, .. }| url), + LinkFromSrc::Primitive(prim) => format::href_with_jump_to_def_path_depth( PrimitiveType::primitive_locations(context.tcx())[prim], context, - Some(href_context.root_path), + Some(href_context.jump_to_def_path_depth), + ) + .ok() + .map(|HrefInfo { url, .. }| url), + LinkFromSrc::Doc(def_id) => format::href_with_jump_to_def_path_depth( + *def_id, + context, + Some(href_context.jump_to_def_path_depth), ) .ok() .map(|HrefInfo { url, .. }| url), - LinkFromSrc::Doc(def_id) => { - format::href_with_root_path(*def_id, context, Some(href_context.root_path)) - .ok() - .map(|HrefInfo { url, .. }| url) - } } }) { diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 556d383a0e9fb..f3fdc01c728f7 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -2802,7 +2802,7 @@ fn render_call_locations( contents_subset, file_span, cx, - &cx.root_path(), + cx.current.len(), &highlight::DecorationInfo(decoration_info), &sources::SourceContext::Embedded(sources::ScrapedInfo { needs_expansion, diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index dda9b7c55351c..f66abad61dcbe 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -1,4 +1,4 @@ -use std::cell::RefCell; +use std::cell::{Cell, RefCell}; use std::ffi::OsStr; use std::path::{Component, Path, PathBuf}; use std::{fmt, fs}; @@ -193,31 +193,28 @@ impl SourceCollector<'_, '_> { let shared = &self.cx.shared; // Create the intermediate directories let cur = RefCell::new(PathBuf::new()); - let root_path = RefCell::new(PathBuf::new()); + // Tracks the extra directory depth accumulated during path traversal. + // Starts at 0; `PathBuf::pop()` on empty is a no-op, so we mirror that + // with `saturating_sub`. The base depth of 2 (for `src//`) is added after. + let extra_depth = Cell::new(0usize); clean_path( &shared.src_root, &p, |component| { cur.borrow_mut().push(component); - root_path.borrow_mut().push(".."); + extra_depth.set(extra_depth.get() + 1); }, || { cur.borrow_mut().pop(); - root_path.borrow_mut().pop(); + extra_depth.set(extra_depth.get().saturating_sub(1)); }, ); + let jump_to_def_path_depth = 2 + extra_depth.get(); + let src_fname = p.file_name().expect("source has no filename").to_os_string(); let mut fname = src_fname.clone(); - - let root_path = PathBuf::from("../../").join(root_path.into_inner()); - let mut root_path = root_path.to_string_lossy(); - if let Some(c) = root_path.as_bytes().last() - && *c != b'/' - { - root_path += "/"; - } let mut file_path = Path::new(&self.crate_name).join(&*cur.borrow()); file_path.push(&fname); fname.push(".html"); @@ -228,6 +225,7 @@ impl SourceCollector<'_, '_> { let title = format!("{} - source", src_fname.to_string_lossy()); let desc = format!("Source of the Rust file `{}`.", p.to_string_lossy()); + let root_path = "../".repeat(jump_to_def_path_depth); let page = layout::Page { title: &title, short_title: &src_fname.to_string_lossy(), @@ -251,7 +249,7 @@ impl SourceCollector<'_, '_> { contents, file_span, self.cx, - &root_path, + jump_to_def_path_depth, &highlight::DecorationInfo::default(), &source_context, ) @@ -330,7 +328,7 @@ pub(crate) fn print_src( s: &str, file_span: rustc_span::Span, context: &Context<'_>, - root_path: &str, + jump_to_def_path_depth: usize, decoration_info: &highlight::DecorationInfo, source_context: &SourceContext<'_>, ) -> fmt::Result { @@ -359,7 +357,7 @@ pub(crate) fn print_src( Some(highlight::HrefContext { context, file_span: file_span.into(), - root_path, + jump_to_def_path_depth, current_href, }), Some(decoration_info), diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 47a92be3ec6ef..bf55dd7d9a4b5 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1200,7 +1200,7 @@ impl LinkCollector<'_, '_> { /// Returns `true` if a link could be generated from the given intra-doc information. /// - /// This is a very light version of `format::href_with_root_path` since we're only interested + /// This is a very light version of `format::href_with_jump_to_def_path_depth` since we're only interested /// about whether we can generate a link to an item or not. /// /// * If `original_did` is local, then we check if the item is reexported or public. diff --git a/tests/rustdoc-html/extern/extern-html-root-url-relative.rs b/tests/rustdoc-html/extern/extern-html-root-url-relative.rs index ba2b50c6bf222..dedc0819ed577 100644 --- a/tests/rustdoc-html/extern/extern-html-root-url-relative.rs +++ b/tests/rustdoc-html/extern/extern-html-root-url-relative.rs @@ -21,7 +21,11 @@ pub mod nested { pub mod intra_doc_link { } -// link-to-definition +// link-to-definition: source pages live under src// so they need +// ../../ to reach the doc root. Previously these links were incorrectly generated +// as ../../../ (one level too deep) which passed the `has` check by substring match. //@ has src/extern_html_root_url_relative/extern-html-root-url-relative.rs.html //@ has - '//a/@href' '../../core/iter/index.html' //@ has - '//a/@href' '../../core/future/index.html' +//@ !has - '//a/@href' '../../../core/iter/index.html' +//@ !has - '//a/@href' '../../../core/future/index.html'