[WIP] Distinguish between links on inner and outer attributes

Normally, rustdoc will resolve `//!` docs in the scope inside the module
and `///` in the scope of the parent. But if there are modules with
inner docs, it would previously resolve _all_ of the links inside the module.

This now distinguishes between links that came from inside the module
and links that came from outside. The intra-doc links part works, but
unfortunately the rest of rustdoc assumes there is only one canonical
resolution for a link and won't look for more than one on the same item.

- Store the attribute style on each DocFragment
- Don't combine attributes with different styles
- Calculate the parent module in only one place
- Remove outdated and fixed FIXME comments

Author: jyn514

compiler/rustc_ast/src/ast.rs

@@ -2388,7 +2388,7 @@ impl UseTree {
 /// Distinguishes between `Attribute`s that decorate items and Attributes that
 /// are contained as statements within items. These two cases need to be
 /// distinguished for pretty-printing.
-#[derive(Clone, PartialEq, Encodable, Decodable, Debug, Copy, HashStable_Generic)]
+#[derive(Clone, PartialEq, Eq, Hash, Encodable, Decodable, Debug, Copy, HashStable_Generic)]
 pub enum AttrStyle {
     Outer,
     Inner,

src/librustdoc/clean/types.rs

@@ -385,6 +385,7 @@ pub struct DocFragment {
     pub parent_module: Option<DefId>,
     pub doc: String,
     pub kind: DocFragmentKind,
+    pub style: AttrStyle,
 }
 
 #[derive(Clone, PartialEq, Eq, Debug, Hash)]
@@ -421,7 +422,6 @@ pub struct Attributes {
     pub span: Option<rustc_span::Span>,
     /// map from Rust paths to resolved defs and potential URL fragments
     pub links: Vec<ItemLink>,
-    pub inner_docs: bool,
 }
 
 #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)]
@@ -559,6 +559,7 @@ impl Attributes {
                     doc: value,
                     kind,
                     parent_module,
+                    style: attr.style,
                 });
 
                 if sp.is_none() {
@@ -584,6 +585,7 @@ impl Attributes {
                                 doc: contents,
                                 kind: DocFragmentKind::Include { filename },
                                 parent_module: parent_module,
+                                style: attr.style,
                             });
                         }
                     }
@@ -618,18 +620,12 @@ impl Attributes {
             }
         }
 
-        let inner_docs = attrs
-            .iter()
-            .find(|a| a.doc_str().is_some())
-            .map_or(true, |a| a.style == AttrStyle::Inner);
-
         Attributes {
             doc_strings,
             other_attrs,
             cfg: if cfg == Cfg::True { None } else { Some(Arc::new(cfg)) },
             span: sp,
             links: vec![],
-            inner_docs,
         }
     }
 

src/librustdoc/passes/collapse_docs.rs

@@ -39,7 +39,9 @@ fn collapse(doc_strings: &mut Vec<DocFragment>) {
             if matches!(*curr_kind, DocFragmentKind::Include { .. })
                 || curr_kind != new_kind
                 || curr_frag.parent_module != frag.parent_module
+                || curr_frag.style != frag.style
             {
+                // Keep these as two separate attributes
                 if *curr_kind == DocFragmentKind::SugaredDoc
                     || *curr_kind == DocFragmentKind::RawDoc
                 {
@@ -49,6 +51,7 @@ fn collapse(doc_strings: &mut Vec<DocFragment>) {
                 docs.push(curr_frag);
                 last_frag = Some(frag);
             } else {
+                // Combine both attributes into one
                 curr_frag.doc.push('\n');
                 curr_frag.doc.push_str(&frag.doc);
                 curr_frag.span = curr_frag.span.to(frag.span);

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -737,6 +737,7 @@ fn is_derive_trait_collision<T>(ns: &PerNS<Result<(Res, T), ResolutionFailure<'_
 
 impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
     fn fold_item(&mut self, mut item: Item) -> Option<Item> {
+        use rustc_ast::AttrStyle;
         use rustc_middle::ty::DefIdTree;
 
         let parent_node = if item.is_fake() {
@@ -773,7 +774,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
 
         let current_item = match item.inner {
             ModuleItem(..) => {
-                if item.attrs.inner_docs {
+                // FIXME: this will be wrong if there are both inner and outer docs.
+                if item.attrs.doc_strings.iter().any(|attr| attr.style == AttrStyle::Inner) {
                     if item.def_id.is_top_level_module() { item.name.clone() } else { None }
                 } else {
                     match parent_node.or(self.mod_ids.last().copied()) {
@@ -798,10 +800,6 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
             _ => item.name.clone(),
         };
 
-        if item.is_mod() && item.attrs.inner_docs {
-            self.mod_ids.push(item.def_id);
-        }
-
         // find item's parent to resolve `Self` in item's docs below
         let parent_name = self.cx.as_local_hir_id(item.def_id).and_then(|item_hir| {
             let parent_hir = self.cx.tcx.hir().get_parent_item(item_hir);
@@ -839,6 +837,10 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
             }
         });
 
+        // If there are both inner and outer docs, we want to only resolve the inner docs
+        // within the module.
+        let mut seen_inner_docs = false;
+
         // We want to resolve in the lexical scope of the documentation.
         // In the presence of re-exports, this is not the same as the module of the item.
         // Rather than merging all documentation into one, resolve it one attribute at a time
@@ -849,10 +851,14 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
             // we want `///` and `#[doc]` to count as the same attribute,
             // but currently it will treat them as separate.
             // As a workaround, combine all attributes with the same parent module into the same attribute.
+            // NOTE: this can combine attributes across different spans,
+            // for example both inside and outside a crate.
             let mut combined_docs = attr.doc.clone();
             loop {
                 match attrs.peek() {
-                    Some(next) if next.parent_module == attr.parent_module => {
+                    Some(next)
+                        if next.parent_module == attr.parent_module && next.style == attr.style =>
+                    {
                         combined_docs.push('\n');
                         combined_docs.push_str(&attrs.next().unwrap().doc);
                     }
@@ -868,15 +874,39 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
                 trace!("no parent found for {:?}", attr.doc);
                 (item.def_id.krate, parent_node)
             };
+
+            // In order to correctly resolve intra-doc-links we need to
+            // pick a base AST node to work from.  If the documentation for
+            // this module came from an inner comment (//!) then we anchor
+            // our name resolution *inside* the module.  If, on the other
+            // hand it was an outer comment (///) then we anchor the name
+            // resolution in the parent module on the basis that the names
+            // used are more likely to be intended to be parent names.  For
+            // this, we set base_node to None for inner comments since
+            // we've already pushed this node onto the resolution stack but
+            // for outer comments we explicitly try and resolve against the
+            // parent_node first.
+
+            // NOTE: there is an implicit assumption here that outer docs will always come
+            // before inner docs.
+            let base_node = if !seen_inner_docs && item.is_mod() && attr.style == AttrStyle::Inner {
+                // FIXME(jynelson): once `Self` handling is cleaned up I think we can get rid
+                // of `mod_ids` altogether
+                self.mod_ids.push(item.def_id);
+                seen_inner_docs = true;
+                Some(item.def_id)
+            } else {
+                parent_node
+            };
+
             // NOTE: if there are links that start in one crate and end in another, this will not resolve them.
             // This is a degenerate case and it's not supported by rustdoc.
-            // FIXME: this will break links that start in `#[doc = ...]` and end as a sugared doc. Should this be supported?
             for (ori_link, link_range) in markdown_links(&combined_docs) {
                 let link = self.resolve_link(
                     &item,
                     &combined_docs,
                     &current_item,
-                    parent_node,
+                    base_node,
                     &parent_name,
                     krate,
                     ori_link,
@@ -888,11 +918,10 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
             }
         }
 
-        if item.is_mod() && !item.attrs.inner_docs {
-            self.mod_ids.push(item.def_id);
-        }
-
         if item.is_mod() {
+            if !seen_inner_docs {
+                self.mod_ids.push(item.def_id);
+            }
             let ret = self.fold_item_recur(item);
 
             self.mod_ids.pop();
@@ -910,7 +939,7 @@ impl LinkCollector<'_, '_> {
         item: &Item,
         dox: &str,
         current_item: &Option<String>,
-        parent_node: Option<DefId>,
+        base_node: Option<DefId>,
         parent_name: &Option<String>,
         krate: CrateNum,
         ori_link: String,
@@ -968,23 +997,6 @@ impl LinkCollector<'_, '_> {
                 .map(|d| d.display_for(path_str))
                 .unwrap_or_else(|| path_str.to_owned());
 
-            // In order to correctly resolve intra-doc-links we need to
-            // pick a base AST node to work from.  If the documentation for
-            // this module came from an inner comment (//!) then we anchor
-            // our name resolution *inside* the module.  If, on the other
-            // hand it was an outer comment (///) then we anchor the name
-            // resolution in the parent module on the basis that the names
-            // used are more likely to be intended to be parent names.  For
-            // this, we set base_node to None for inner comments since
-            // we've already pushed this node onto the resolution stack but
-            // for outer comments we explicitly try and resolve against the
-            // parent_node first.
-            let base_node = if item.is_mod() && item.attrs.inner_docs {
-                self.mod_ids.last().copied()
-            } else {
-                parent_node
-            };
-
             let mut module_id = if let Some(id) = base_node {
                 id
             } else {
@@ -1185,6 +1197,8 @@ impl LinkCollector<'_, '_> {
         ori_link: &str,
         link_range: Option<Range<usize>>,
     ) -> Option<(Res, Option<String>)> {
+        debug!("resolving {} relative to {:?}", path_str, base_node);
+
         match disambiguator.map(Disambiguator::ns) {
             Some(ns @ (ValueNS | TypeNS)) => {
                 match self.resolve(path_str, ns, &current_item, base_node, &extra_fragment) {

src/librustdoc/passes/mod.rs

@@ -179,7 +179,9 @@ crate fn source_span_for_markdown_range(
         return None;
     }
 
+    trace!("markdown is {}", markdown);
     let snippet = cx.sess().source_map().span_to_snippet(span_of_attrs(attrs)?).ok()?;
+    trace!("snippet is {}", snippet);
 
     let starting_line = markdown[..md_range.start].matches('\n').count();
     let ending_line = starting_line + markdown[md_range.start..md_range.end].matches('\n').count();
@@ -196,7 +198,9 @@ crate fn source_span_for_markdown_range(
 
     'outer: for (line_no, md_line) in md_lines.enumerate() {
         loop {
-            let source_line = src_lines.next().expect("could not find markdown in source");
+            let source_line = src_lines.next().unwrap_or_else(|| {
+                panic!("could not find markdown range {:?} in source", md_range)
+            });
             match source_line.find(md_line) {
                 Some(offset) => {
                     if line_no == starting_line {

src/test/rustdoc-ui/intra-link-inner-outer.rs

@@ -0,0 +1,12 @@
+// check-pass
+
+pub enum A {}
+
+/// Links to [outer][A] and [outer][B]
+//~^ WARNING: unresolved link to `B`
+pub mod M {
+    //! Links to [inner][A] and [inner][B]
+    //~^ WARNING: unresolved link to `A`
+
+    pub struct B;
+}

src/test/rustdoc/intra-link-inner-outer.rs

@@ -0,0 +1,24 @@
+#![crate_name = "foo"]
+
+pub enum A {}
+
+/// Links to [outer A][A] and [outer B][B]
+// @has foo/M/index.html '//*[@href="../foo/enum.A.html"]' 'outer A'
+// @!has foo/M/index.html '//*[@href="../foo/struct.B.html"]' 'outer B'
+// doesn't resolve unknown links
+pub mod M {
+    //! Links to [inner A][A] and [inner B][B]
+    // @!has foo/M/index.html '//*[@href="../foo/enum.A.html"]' 'inner A'
+    // @has foo/M/index.html '//*[@href="../foo/struct.B.html"]' 'inner B'
+    pub struct B;
+}
+
+// distinguishes between links to inner and outer attributes
+/// Links to [outer A][A]
+// @has foo/N/index.html '//*[@href="../foo/enum.A.html"]' 'outer A'
+pub mod N {
+    //! Links to [inner A][A]
+    // @has foo/N/index.html '//*[@href="../foo/struct.A.html"]' 'inner A'
+
+    pub struct A;
+}