Merge pull request #19507 from Hmikihiro/fix_module_doc_links

fix: resolve doc path from parent module if outer comments exist on module

Author: Veykril

crates/hir-expand/src/attrs.rs

@@ -1,4 +1,5 @@
 //! A higher level attributes based on TokenTree, with also some shortcuts.
+use std::iter;
 use std::{borrow::Cow, fmt, ops};
 
 use base_db::Crate;
@@ -122,16 +123,15 @@ impl RawAttrs {
             (None, entries @ Some(_)) => Self { entries },
             (Some(entries), None) => Self { entries: Some(entries.clone()) },
             (Some(a), Some(b)) => {
-                let last_ast_index = a.slice.last().map_or(0, |it| it.id.ast_index() + 1) as u32;
+                let last_ast_index = a.slice.last().map_or(0, |it| it.id.ast_index() + 1);
                 let items = a
                     .slice
                     .iter()
                     .cloned()
                     .chain(b.slice.iter().map(|it| {
                         let mut it = it.clone();
-                        it.id.id = (it.id.ast_index() as u32 + last_ast_index)
-                            | ((it.id.cfg_attr_index().unwrap_or(0) as u32)
-                                << AttrId::AST_INDEX_BITS);
+                        let id = it.id.ast_index() + last_ast_index;
+                        it.id = AttrId::new(id, it.id.is_inner_attr());
                         it
                     }))
                     .collect::<Vec<_>>();
@@ -175,25 +175,20 @@ pub struct AttrId {
 // FIXME: This only handles a single level of cfg_attr nesting
 // that is `#[cfg_attr(all(), cfg_attr(all(), cfg(any())))]` breaks again
 impl AttrId {
-    const CFG_ATTR_BITS: usize = 7;
-    const AST_INDEX_MASK: usize = 0x00FF_FFFF;
-    const AST_INDEX_BITS: usize = Self::AST_INDEX_MASK.count_ones() as usize;
-    const CFG_ATTR_SET_BITS: u32 = 1 << 31;
+    const INNER_ATTR_SET_BIT: u32 = 1 << 31;
 
-    pub fn ast_index(&self) -> usize {
-        self.id as usize & Self::AST_INDEX_MASK
+    pub fn new(id: usize, is_inner: bool) -> Self {
+        assert!(id <= !Self::INNER_ATTR_SET_BIT as usize);
+        let id = id as u32;
+        Self { id: if is_inner { id | Self::INNER_ATTR_SET_BIT } else { id } }
     }
 
-    pub fn cfg_attr_index(&self) -> Option<usize> {
-        if self.id & Self::CFG_ATTR_SET_BITS == 0 {
-            None
-        } else {
-            Some(self.id as usize >> Self::AST_INDEX_BITS)
-        }
+    pub fn ast_index(&self) -> usize {
+        (self.id & !Self::INNER_ATTR_SET_BIT) as usize
     }
 
-    pub fn with_cfg_attr(self, idx: usize) -> AttrId {
-        AttrId { id: self.id | ((idx as u32) << Self::AST_INDEX_BITS) | Self::CFG_ATTR_SET_BITS }
+    pub fn is_inner_attr(&self) -> bool {
+        self.id & Self::INNER_ATTR_SET_BIT != 0
     }
 }
 
@@ -333,10 +328,7 @@ impl Attr {
             None => return smallvec![self.clone()],
         };
         let index = self.id;
-        let attrs = parts
-            .enumerate()
-            .take(1 << AttrId::CFG_ATTR_BITS)
-            .filter_map(|(idx, attr)| Attr::from_tt(db, attr, index.with_cfg_attr(idx)));
+        let attrs = parts.filter_map(|attr| Attr::from_tt(db, attr, index));
 
         let cfg = TopSubtree::from_token_trees(subtree.top_subtree().delimiter, cfg);
         let cfg = CfgExpr::parse(&cfg);
@@ -467,13 +459,18 @@ fn unescape(s: &str) -> Option<Cow<'_, str>> {
 pub fn collect_attrs(
     owner: &dyn ast::HasAttrs,
 ) -> impl Iterator<Item = (AttrId, Either<ast::Attr, ast::Comment>)> {
-    let inner_attrs = inner_attributes(owner.syntax()).into_iter().flatten();
-    let outer_attrs =
-        ast::AttrDocCommentIter::from_syntax_node(owner.syntax()).filter(|el| match el {
+    let inner_attrs =
+        inner_attributes(owner.syntax()).into_iter().flatten().zip(iter::repeat(true));
+    let outer_attrs = ast::AttrDocCommentIter::from_syntax_node(owner.syntax())
+        .filter(|el| match el {
             Either::Left(attr) => attr.kind().is_outer(),
             Either::Right(comment) => comment.is_outer(),
-        });
-    outer_attrs.chain(inner_attrs).enumerate().map(|(id, attr)| (AttrId { id: id as u32 }, attr))
+        })
+        .zip(iter::repeat(false));
+    outer_attrs
+        .chain(inner_attrs)
+        .enumerate()
+        .map(|(id, (attr, is_inner))| (AttrId::new(id, is_inner), attr))
 }
 
 fn inner_attributes(

crates/hir/src/attrs.rs

@@ -105,21 +105,31 @@ impl HasAttrs for crate::Crate {
 /// Resolves the item `link` points to in the scope of `def`.
 pub fn resolve_doc_path_on(
     db: &dyn HirDatabase,
-    def: impl HasAttrs,
+    def: impl HasAttrs + Copy,
     link: &str,
     ns: Option<Namespace>,
+    is_inner_doc: bool,
 ) -> Option<DocLinkDef> {
-    resolve_doc_path_on_(db, link, def.attr_id(), ns)
+    resolve_doc_path_on_(db, link, def.attr_id(), ns, is_inner_doc)
 }
 
 fn resolve_doc_path_on_(
     db: &dyn HirDatabase,
     link: &str,
     attr_id: AttrDefId,
     ns: Option<Namespace>,
+    is_inner_doc: bool,
 ) -> Option<DocLinkDef> {
     let resolver = match attr_id {
-        AttrDefId::ModuleId(it) => it.resolver(db),
+        AttrDefId::ModuleId(it) => {
+            if is_inner_doc {
+                it.resolver(db)
+            } else if let Some(parent) = Module::from(it).parent(db) {
+                parent.id.resolver(db)
+            } else {
+                it.resolver(db)
+            }
+        }
         AttrDefId::FieldId(it) => it.parent.resolver(db),
         AttrDefId::AdtId(it) => it.resolver(db),
         AttrDefId::FunctionId(it) => it.resolver(db),

crates/ide-db/src/defs.rs

@@ -6,7 +6,7 @@
 // FIXME: this badly needs rename/rewrite (matklad, 2020-02-06).
 
 use crate::RootDatabase;
-use crate::documentation::{Documentation, HasDocs};
+use crate::documentation::{DocsRangeMap, Documentation, HasDocs};
 use crate::famous_defs::FamousDefs;
 use arrayvec::ArrayVec;
 use either::Either;
@@ -21,7 +21,7 @@ use hir::{
 use span::Edition;
 use stdx::{format_to, impl_from};
 use syntax::{
-    SyntaxKind, SyntaxNode, SyntaxToken,
+    SyntaxKind, SyntaxNode, SyntaxToken, TextSize,
     ast::{self, AstNode},
     match_ast,
 };
@@ -210,29 +210,40 @@ impl Definition {
         famous_defs: Option<&FamousDefs<'_, '_>>,
         display_target: DisplayTarget,
     ) -> Option<Documentation> {
+        self.docs_with_rangemap(db, famous_defs, display_target).map(|(docs, _)| docs)
+    }
+
+    pub fn docs_with_rangemap(
+        &self,
+        db: &RootDatabase,
+        famous_defs: Option<&FamousDefs<'_, '_>>,
+        display_target: DisplayTarget,
+    ) -> Option<(Documentation, Option<DocsRangeMap>)> {
         let docs = match self {
-            Definition::Macro(it) => it.docs(db),
-            Definition::Field(it) => it.docs(db),
-            Definition::Module(it) => it.docs(db),
-            Definition::Crate(it) => it.docs(db),
-            Definition::Function(it) => it.docs(db),
-            Definition::Adt(it) => it.docs(db),
-            Definition::Variant(it) => it.docs(db),
-            Definition::Const(it) => it.docs(db),
-            Definition::Static(it) => it.docs(db),
-            Definition::Trait(it) => it.docs(db),
-            Definition::TraitAlias(it) => it.docs(db),
+            Definition::Macro(it) => it.docs_with_rangemap(db),
+            Definition::Field(it) => it.docs_with_rangemap(db),
+            Definition::Module(it) => it.docs_with_rangemap(db),
+            Definition::Crate(it) => it.docs_with_rangemap(db),
+            Definition::Function(it) => it.docs_with_rangemap(db),
+            Definition::Adt(it) => it.docs_with_rangemap(db),
+            Definition::Variant(it) => it.docs_with_rangemap(db),
+            Definition::Const(it) => it.docs_with_rangemap(db),
+            Definition::Static(it) => it.docs_with_rangemap(db),
+            Definition::Trait(it) => it.docs_with_rangemap(db),
+            Definition::TraitAlias(it) => it.docs_with_rangemap(db),
             Definition::TypeAlias(it) => {
-                it.docs(db).or_else(|| {
+                it.docs_with_rangemap(db).or_else(|| {
                     // docs are missing, try to fall back to the docs of the aliased item.
                     let adt = it.ty(db).as_adt()?;
-                    let docs = adt.docs(db)?;
-                    let docs = format!(
-                        "*This is the documentation for* `{}`\n\n{}",
-                        adt.display(db, display_target),
-                        docs.as_str()
+                    let (docs, range_map) = adt.docs_with_rangemap(db)?;
+                    let header_docs = format!(
+                        "*This is the documentation for* `{}`\n\n",
+                        adt.display(db, display_target)
                     );
-                    Some(Documentation::new(docs))
+                    let offset = TextSize::new(header_docs.len() as u32);
+                    let range_map = range_map.shift_docstring_line_range(offset);
+                    let docs = header_docs + docs.as_str();
+                    Some((Documentation::new(docs), range_map))
                 })
             }
             Definition::BuiltinType(it) => {
@@ -241,17 +252,17 @@ impl Definition {
                     let primitive_mod =
                         format!("prim_{}", it.name().display(fd.0.db, display_target.edition));
                     let doc_owner = find_std_module(fd, &primitive_mod, display_target.edition)?;
-                    doc_owner.docs(fd.0.db)
+                    doc_owner.docs_with_rangemap(fd.0.db)
                 })
             }
             Definition::BuiltinLifetime(StaticLifetime) => None,
             Definition::Local(_) => None,
             Definition::SelfType(impl_def) => {
-                impl_def.self_ty(db).as_adt().map(|adt| adt.docs(db))?
+                impl_def.self_ty(db).as_adt().map(|adt| adt.docs_with_rangemap(db))?
             }
             Definition::GenericParam(_) => None,
             Definition::Label(_) => None,
-            Definition::ExternCrateDecl(it) => it.docs(db),
+            Definition::ExternCrateDecl(it) => it.docs_with_rangemap(db),
 
             Definition::BuiltinAttr(it) => {
                 let name = it.name(db);
@@ -276,7 +287,8 @@ impl Definition {
                         name_value_str
                     );
                 }
-                Some(Documentation::new(docs.replace('*', "\\*")))
+
+                return Some((Documentation::new(docs.replace('*', "\\*")), None));
             }
             Definition::ToolModule(_) => None,
             Definition::DeriveHelper(_) => None,
@@ -291,8 +303,9 @@ impl Definition {
             let trait_ = assoc.implemented_trait(db)?;
             let name = Some(assoc.name(db)?);
             let item = trait_.items(db).into_iter().find(|it| it.name(db) == name)?;
-            item.docs(db)
+            item.docs_with_rangemap(db)
         })
+        .map(|(docs, range_map)| (docs, Some(range_map)))
     }
 
     pub fn label(&self, db: &RootDatabase, display_target: DisplayTarget) -> String {