Better handling of paths in link to def feature

Author: GuillaumeGomez

src/librustdoc/html/highlight.rs

@@ -944,6 +944,65 @@ fn string<T: Display>(
     }
 }
 
+fn generate_link_to_def(
+    out: &mut impl Write,
+    text_s: &str,
+    klass: Class,
+    href_context: &Option<HrefContext<'_, '_>>,
+    def_span: Span,
+    open_tag: bool,
+) -> bool {
+    if let Some(href_context) = href_context {
+        if let Some(href) =
+            href_context.context.shared.span_correspondence_map.get(&def_span).and_then(|href| {
+                let context = href_context.context;
+                // FIXME: later on, it'd be nice to provide two links (if possible) for all items:
+                // one to the documentation page and one to the source definition.
+                // FIXME: currently, external items only generate a link to their documentation,
+                // a link to their definition can be generated using this:
+                // https://github.com/rust-lang/rust/blob/60f1a2fc4b535ead9c85ce085fdce49b1b097531/src/librustdoc/html/render/context.rs#L315-L338
+                match href {
+                    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(|(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),
+                    LinkFromSrc::Doc(def_id) => {
+                        format::href_with_root_path(*def_id, context, Some(href_context.root_path))
+                            .ok()
+                            .map(|(doc_link, _, _)| doc_link)
+                    }
+                }
+            })
+        {
+            if !open_tag {
+                // We're already inside an element which has the same klass, no need to give it
+                // again.
+                write!(out, "<a href=\"{href}\">{text_s}").unwrap();
+            } else {
+                let klass_s = klass.as_html();
+                if klass_s.is_empty() {
+                    write!(out, "<a href=\"{href}\">{text_s}").unwrap();
+                } else {
+                    write!(out, "<a class=\"{klass_s}\" href=\"{href}\">{text_s}").unwrap();
+                }
+            }
+            return true;
+        }
+    }
+    false
+}
+
 /// This function writes `text` into `out` with some modifications depending on `klass`:
 ///
 /// * If `klass` is `None`, `text` is written into `out` with no modification.
@@ -973,10 +1032,14 @@ fn string_without_closing_tag<T: Display>(
         return Some("</span>");
     };
 
+    let mut added_links = false;
     let mut text_s = text.to_string();
     if text_s.contains("::") {
+        let mut span = def_span.with_hi(def_span.lo());
         text_s = text_s.split("::").intersperse("::").fold(String::new(), |mut path, t| {
+            span = span.with_hi(span.hi() + BytePos(t.len() as _));
             match t {
+                "::" => write!(&mut path, "::"),
                 "self" | "Self" => write!(
                     &mut path,
                     "<span class=\"{klass}\">{t}</span>",
@@ -989,60 +1052,25 @@ fn string_without_closing_tag<T: Display>(
                         klass = Class::KeyWord.as_html(),
                     )
                 }
-                t => write!(&mut path, "{t}"),
+                t => {
+                    if !t.is_empty()
+                        && generate_link_to_def(&mut path, t, klass, href_context, span, open_tag)
+                    {
+                        added_links = true;
+                        write!(&mut path, "</a>")
+                    } else {
+                        write!(&mut path, "{t}")
+                    }
+                }
             }
             .expect("Failed to build source HTML path");
+            span = span.with_lo(span.lo() + BytePos(t.len() as _));
             path
         });
     }
 
-    if let Some(href_context) = href_context {
-        if let Some(href) =
-            href_context.context.shared.span_correspondence_map.get(&def_span).and_then(|href| {
-                let context = href_context.context;
-                // FIXME: later on, it'd be nice to provide two links (if possible) for all items:
-                // one to the documentation page and one to the source definition.
-                // FIXME: currently, external items only generate a link to their documentation,
-                // a link to their definition can be generated using this:
-                // https://github.com/rust-lang/rust/blob/60f1a2fc4b535ead9c85ce085fdce49b1b097531/src/librustdoc/html/render/context.rs#L315-L338
-                match href {
-                    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(|(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),
-                    LinkFromSrc::Doc(def_id) => {
-                        format::href_with_root_path(*def_id, context, Some(href_context.root_path))
-                            .ok()
-                            .map(|(doc_link, _, _)| doc_link)
-                    }
-                }
-            })
-        {
-            if !open_tag {
-                // We're already inside an element which has the same klass, no need to give it
-                // again.
-                write!(out, "<a href=\"{href}\">{text_s}").unwrap();
-            } else {
-                let klass_s = klass.as_html();
-                if klass_s.is_empty() {
-                    write!(out, "<a href=\"{href}\">{text_s}").unwrap();
-                } else {
-                    write!(out, "<a class=\"{klass_s}\" href=\"{href}\">{text_s}").unwrap();
-                }
-            }
-            return Some("</a>");
-        }
+    if !added_links && generate_link_to_def(out, &text_s, klass, href_context, def_span, open_tag) {
+        return Some("</a>");
     }
     if !open_tag {
         write!(out, "{}", text_s).unwrap();

src/librustdoc/html/render/span_map.rs

@@ -65,7 +65,7 @@ struct SpanMapVisitor<'tcx> {
 
 impl SpanMapVisitor<'_> {
     /// This function is where we handle `hir::Path` elements and add them into the "span map".
-    fn handle_path(&mut self, path: &rustc_hir::Path<'_>) {
+    fn handle_path(&mut self, path: &rustc_hir::Path<'_>, only_use_last_segment: bool) {
         match path.res {
             // FIXME: For now, we handle `DefKind` if it's not a `DefKind::TyParam`.
             // Would be nice to support them too alongside the other `DefKind`
@@ -77,25 +77,38 @@ impl SpanMapVisitor<'_> {
                     LinkFromSrc::External(def_id)
                 };
                 // In case the path ends with generics, we remove them from the span.
-                let span = path
-                    .segments
-                    .last()
-                    .map(|last| {
-                        // In `use` statements, the included item is not in the path segments.
-                        // However, it doesn't matter because you can't have generics on `use`
-                        // statements.
-                        if path.span.contains(last.ident.span) {
-                            path.span.with_hi(last.ident.span.hi())
-                        } else {
-                            path.span
-                        }
-                    })
-                    .unwrap_or(path.span);
+                let span = if only_use_last_segment
+                    && let Some(path_span) = path.segments.last().map(|segment| segment.ident.span)
+                {
+                    path_span
+                } else {
+                    path.segments
+                        .last()
+                        .map(|last| {
+                            // In `use` statements, the included item is not in the path segments.
+                            // However, it doesn't matter because you can't have generics on `use`
+                            // statements.
+                            if path.span.contains(last.ident.span) {
+                                path.span.with_hi(last.ident.span.hi())
+                            } else {
+                                path.span
+                            }
+                        })
+                        .unwrap_or(path.span)
+                };
                 self.matches.insert(span, link);
             }
             Res::Local(_) => {
                 if let Some(span) = self.tcx.hir().res_span(path.res) {
-                    self.matches.insert(path.span, LinkFromSrc::Local(clean::Span::new(span)));
+                    let path_span = if only_use_last_segment
+                        && let Some(path_span) =
+                            path.segments.last().map(|segment| segment.ident.span)
+                    {
+                        path_span
+                    } else {
+                        path.span
+                    };
+                    self.matches.insert(path_span, LinkFromSrc::Local(clean::Span::new(span)));
                 }
             }
             Res::PrimTy(p) => {
@@ -202,11 +215,11 @@ impl<'tcx> Visitor<'tcx> for SpanMapVisitor<'tcx> {
         if self.handle_macro(path.span) {
             return;
         }
-        self.handle_path(path);
+        self.handle_path(path, false);
         intravisit::walk_path(self, path);
     }
 
-    fn visit_qpath(&mut self, qpath: &QPath<'tcx>, id: HirId, span: Span) {
+    fn visit_qpath(&mut self, qpath: &QPath<'tcx>, id: HirId, _span: Span) {
         match *qpath {
             QPath::TypeRelative(qself, path) => {
                 if matches!(path.res, Res::Err) {
@@ -216,23 +229,25 @@ impl<'tcx> Visitor<'tcx> for SpanMapVisitor<'tcx> {
                     let typeck_results = tcx.typeck_body(hir.body_owned_by(body_id).id());
                     let path = rustc_hir::Path {
                         // We change the span to not include parens.
-                        span: span.with_hi(path.ident.span.hi()),
+                        span: path.ident.span,
                         res: typeck_results.qpath_res(qpath, id),
                         segments: &[],
                     };
-                    self.handle_path(&path);
+                    self.handle_path(&path, false);
                 } else {
-                    self.infer_id(path.hir_id, Some(id), span);
+                    self.infer_id(path.hir_id, Some(id), path.ident.span);
                 }
 
                 rustc_ast::visit::try_visit!(self.visit_ty(qself));
                 self.visit_path_segment(path);
             }
             QPath::Resolved(maybe_qself, path) => {
-                self.handle_path(path);
+                self.handle_path(path, true);
 
                 rustc_ast::visit::visit_opt!(self, visit_ty, maybe_qself);
-                self.visit_path(path, id)
+                if !self.handle_macro(path.span) {
+                    intravisit::walk_path(self, path);
+                }
             }
             _ => {}
         }

tests/rustdoc/check-source-code-urls-to-def.rs

@@ -33,7 +33,7 @@ fn babar() {}
 //@ has - '//pre[@class="rust"]//a/@href' '/primitive.str.html'
 //@ count - '//pre[@class="rust"]//a[@href="#23"]' 5
 //@ has - '//pre[@class="rust"]//a[@href="../../source_code/struct.SourceCode.html"]' \
-//        'source_code::SourceCode'
+//        'SourceCode'
 pub fn foo(a: u32, b: &str, c: String, d: Foo, e: bar::Bar, f: source_code::SourceCode) {
     let x = 12;
     let y: Foo = Foo;

tests/rustdoc/jump-to-def-assoc-items.rs

@@ -1,15 +1,54 @@
+// This test ensures that patterns also get a link generated.
+
 //@ compile-flags: -Zunstable-options --generate-link-to-definition
 
 #![crate_name = "foo"]
 
-pub enum Ty {
-    Var,
+//@ has 'src/foo/jump-to-def-assoc-items.rs.html'
+
+pub trait Trait {
+    type T;
+}
+pub trait Another {
+    type T;
+    const X: u32;
 }
 
-//@ has 'src/foo/jump-to-def-assoc-items.rs.html'
-//@ has - '//a[@href="#6"]' 'Self::Var'
-impl Ty {
-    fn f() {
-        let _ = Self::Var;
+pub struct Foo;
+
+impl Foo {
+    pub fn new() -> Self { Foo }
+}
+
+pub struct C;
+
+impl C {
+    pub fn wat() {}
+}
+
+pub struct Bar;
+impl Trait for Bar {
+    type T = Foo;
+}
+impl Another for Bar {
+    type T = C;
+    const X: u32 = 12;
+}
+
+pub fn bar() {
+    //@ has - '//a[@href="#20"]' 'new'
+    <Bar as Trait>::T::new();
+    //@ has - '//a[@href="#26"]' 'wat'
+    <Bar as Another>::T::wat();
+
+    match 12u32 {
+        //@ has - '//a[@href="#14"]' 'X'
+        <Bar as Another>::X => {}
+        _ => {}
     }
 }
+
+pub struct Far {
+        //@ has - '//a[@href="#10"]' 'T'
+    x: <Bar as Trait>::T,
+}

tests/rustdoc/jump-to-def-doc-links-calls.rs

@@ -8,15 +8,15 @@
 pub struct Bar;
 
 impl std::default::Default for Bar {
-    //@ has - '//a[@href="#20-22"]' 'Self::new'
+    //@ has - '//a[@href="#20-22"]' 'new'
     fn default() -> Self {
         Self::new()
     }
 }
 
 //@ has - '//a[@href="#8"]' 'Bar'
 impl Bar {
-     //@ has - '//a[@href="#24-26"]' 'Self::bar'
+     //@ has - '//a[@href="#24-26"]' 'bar'
      pub fn new()-> Self {
          Self::bar()
      }

tests/rustdoc/jump-to-def-pats.rs

@@ -30,13 +30,13 @@ pub fn foo() -> Result<(), ()> {
 impl<T, E> fmt::Display for MyEnum<T, E> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            //@ has - '//a[@href="#12"]' 'Self::Ok'
+            //@ has - '//a[@href="#12"]' 'Ok'
             Self::Ok(_) => f.write_str("MyEnum::Ok"),
-            //@ has - '//a[@href="#13"]' 'MyEnum::Err'
+            //@ has - '//a[@href="#13"]' 'Err'
             MyEnum::Err(_) => f.write_str("MyEnum::Err"),
-            //@ has - '//a[@href="#14"]' 'Self::Some'
+            //@ has - '//a[@href="#14"]' 'Some'
             Self::Some(_) => f.write_str("MyEnum::Some"),
-            //@ has - '//a[@href="#15"]' 'Self::None'
+            //@ has - '//a[@href="#15"]' 'None'
             Self::None => f.write_str("MyEnum::None"),
         }
     }
@@ -45,7 +45,7 @@ impl<T, E> fmt::Display for MyEnum<T, E> {
 impl X {
     fn p(&self) -> &str {
         match self {
-            //@ has - '//a[@href="#19"]' 'Self::A'
+            //@ has - '//a[@href="#19"]' 'A'
             Self::A => "X::A",
         }
     }