Fix relative extern URL depth on source pages

Author: arferreira

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<usize>,
 ) -> Result<HrefInfo, HrefError> {
     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<usize>,
 ) -> Result<HrefInfo, HrefError> {
     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,34 +507,50 @@ 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<usize>,
 ) -> 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),
     }
 }
 
 fn make_href(
-    root_path: Option<&str>,
+    jump_to_def_path_depth: Option<usize>,
     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<usize>,
 ) -> Result<HrefInfo, HrefError> {
     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<HrefInfo, HrefError> {
-    href_with_root_path(did, cx, None)
+    href_with_jump_to_def_path_depth(did, cx, None)
 }
 
 /// Both paths should only be modules.

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)
-                    }
                 }
             })
     {

src/librustdoc/html/render/mod.rs

@@ -2802,7 +2802,7 @@ fn render_call_locations<W: fmt::Write>(
             contents_subset,
             file_span,
             cx,
-            &cx.root_path(),
+            cx.current.len(),
             &highlight::DecorationInfo(decoration_info),
             &sources::SourceContext::Embedded(sources::ScrapedInfo {
                 needs_expansion,

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,23 @@ impl SourceCollector<'_, '_> {
         let shared = &self.cx.shared;
         // Create the intermediate directories
         let cur = RefCell::new(PathBuf::new());
-        let root_path = RefCell::new(PathBuf::new());
+        let jump_to_def_path_depth = Cell::new(2usize);
 
         clean_path(
             &shared.src_root,
             &p,
             |component| {
                 cur.borrow_mut().push(component);
-                root_path.borrow_mut().push("..");
+                jump_to_def_path_depth.set(jump_to_def_path_depth.get() + 1);
             },
             || {
                 cur.borrow_mut().pop();
-                root_path.borrow_mut().pop();
+                jump_to_def_path_depth.set(jump_to_def_path_depth.get() - 1);
             },
         );
 
         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 +220,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.get());
         let page = layout::Page {
             title: &title,
             short_title: &src_fname.to_string_lossy(),
@@ -251,7 +244,7 @@ impl SourceCollector<'_, '_> {
                     contents,
                     file_span,
                     self.cx,
-                    &root_path,
+                    jump_to_def_path_depth.get(),
                     &highlight::DecorationInfo::default(),
                     &source_context,
                 )
@@ -330,7 +323,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 +352,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),

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.

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/<crate_name>/ 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'