Better handling of paths in link to def feature

Author: GuillaumeGomez

src/librustdoc/html/highlight.rs

@@ -1059,6 +1059,64 @@ fn string<T: Display, W: Write>(
     }
 }
 
+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
+        && 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.
@@ -1088,10 +1146,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>",
@@ -1104,58 +1166,24 @@ 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
-        && let Some(href) = href_context.context.shared.span_correspondence_map.get(&def_span)
-        && let Some(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();
-            }
-        }
+    if !added_links && generate_link_to_def(out, &text_s, klass, href_context, def_span, open_tag) {
         return Some("</a>");
     }
     if !open_tag {

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,24 +77,36 @@ 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) => {
                 // FIXME: Doesn't handle "path-like" primitives like arrays or tuples.
@@ -200,37 +212,41 @@ 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) {
                     let tcx = self.tcx;
-                    let hir = tcx.hir();
-                    let body_id = hir.enclosing_body_owner(id);
-                    let typeck_results = tcx.typeck_body(hir.body_owned_by(body_id).id());
+                    let body_id = tcx.hir_enclosing_body_owner(id);
+                    let typeck_results = tcx.typeck_body(tcx.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));
+                if let Some(qself) = qself.try_as_ambig_ty() {
+                    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);
 
+                let maybe_qself = maybe_qself.and_then(|maybe_qself| maybe_qself.try_as_ambig_ty());
                 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/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/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/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",
         }
     }

tests/rustdoc/jump-to-def/jump-to-non-local-method.rs

@@ -21,9 +21,10 @@ pub fn bar2<T: Read>(readable: T) {
 }
 
 pub fn bar() {
-    //@ has - '//a[@href="{{channel}}/core/sync/atomic/struct.AtomicIsize.html#method.new"]' 'AtomicIsize::new'
+    //@ has - '//a[@href="{{channel}}/core/sync/atomic/struct.AtomicIsize.html"]' 'AtomicIsize'
+    //@ has - '//a[@href="{{channel}}/core/sync/atomic/struct.AtomicIsize.html#method.new"]' 'new'
     let _ = AtomicIsize::new(0);
-    //@ has - '//a[@href="#48"]' 'local_private'
+    //@ has - '//a[@href="#49"]' 'local_private'
     local_private();
 }
 
@@ -39,7 +40,7 @@ pub fn macro_call() -> Result<(), ()> {
 }
 
 pub fn variant() {
-    //@ has - '//a[@href="{{channel}}/core/cmp/enum.Ordering.html#variant.Less"]' 'Ordering::Less'
+    //@ has - '//a[@href="{{channel}}/core/cmp/enum.Ordering.html#variant.Less"]' 'Less'
     let _ = Ordering::Less;
     //@ has - '//a[@href="{{channel}}/core/marker/struct.PhantomData.html"]' 'PhantomData'
     let _: PhantomData::<usize> = PhantomData;