Repository: rust-analyzer/rust-analyzer
Pull request #11237 (2022-01-08 13:55:04)

fix: Fix outline modules spilling inner doc injections into their parent
Fixes another regression caused by https://github.com/rust-analyzer/rust-analyzer/pull/11225
bors r+


Patch:
diff --git a/crates/hir_def/src/attr.rs b/crates/hir_def/src/attr.rs
index 30642f6cc6..a244d32149 100644
--- a/crates/hir_def/src/attr.rs
+++ b/crates/hir_def/src/attr.rs
@@ -390,7 +390,9 @@ impl AttrsWithOwner {
                         if let InFile { file_id, value: ModuleSource::SourceFile(file) } =
                             mod_data.definition_source(db)
                         {
-                            map.merge(AttrSourceMap::new(InFile::new(file_id, &file)));
+                            map.append_module_inline_attrs(AttrSourceMap::new(InFile::new(
+                                file_id, &file,
+                            )));
                         }
                         return map;
                     }
@@ -552,6 +554,11 @@ fn inner_attributes(
 pub struct AttrSourceMap {
     source: Vec<Either<ast::Attr, ast::Comment>>,
     file_id: HirFileId,
+    /// If this map is for a module, this will be the [`HirFileId`] of the module's definition site,
+    /// while `file_id` will be the one of the module declaration site.
+    /// The usize is the index into `source` from which point on the entries reside in the def site
+    /// file.
+    mod_def_site_file_id: Option<(HirFileId, usize)>,
 }
 
 impl AttrSourceMap {
@@ -559,11 +566,19 @@ impl AttrSourceMap {
         Self {
             source: collect_attrs(owner.value).map(|(_, it)| it).collect(),
             file_id: owner.file_id,
+            mod_def_site_file_id: None,
         }
     }
 
-    fn merge(&mut self, other: Self) {
+    /// Append a second source map to this one, this is required for modules, whose outline and inline
+    /// attributes can reside in different files
+    fn append_module_inline_attrs(&mut self, other: Self) {
+        assert!(self.mod_def_site_file_id.is_none() && other.mod_def_site_file_id.is_none());
+        let len = self.source.len();
         self.source.extend(other.source);
+        if other.file_id != self.file_id {
+            self.mod_def_site_file_id = Some((other.file_id, len));
+        }
     }
 
     /// Maps the lowered `Attr` back to its original syntax node.
@@ -577,9 +592,15 @@ impl AttrSourceMap {
     }
 
     fn source_of_id(&self, id: AttrId) -> InFile<&Either<ast::Attr, ast::Comment>> {
+        let ast_idx = id.ast_index as usize;
+        let file_id = match self.mod_def_site_file_id {
+            Some((file_id, def_site_cut)) if def_site_cut <= ast_idx => file_id,
+            _ => self.file_id,
+        };
+
         self.source
-            .get(id.ast_index as usize)
-            .map(|it| InFile::new(self.file_id, it))
+            .get(ast_idx)
+            .map(|it| InFile::new(file_id, it))
             .unwrap_or_else(|| panic!("cannot find attr at index {:?}", id))
     }
 }
diff --git a/crates/ide_assists/src/handlers/add_missing_impl_members.rs b/crates/ide_assists/src/handlers/add_missing_impl_members.rs
index 3105b28911..a10eca10d1 100644
--- a/crates/ide_assists/src/handlers/add_missing_impl_members.rs
+++ b/crates/ide_assists/src/handlers/add_missing_impl_members.rs
@@ -942,366 +942,4 @@ impl FooB for Foo {
 "#,
         )
     }
-
-    #[test]
-    fn test_assoc_type_when_trait_with_same_name_in_scope() {
-        check_assist(
-            add_missing_impl_members,
-            r#"
-pub trait Foo {}
-
-pub trait Types {
-    type Foo;
-}
-
-pub trait Behavior<T: Types> {
-    fn reproduce(&self, foo: T::Foo);
-}
-
-pub struct Impl;
-
-impl<T: Types> Behavior<T> for Impl { $0 }"#,
-            r#"
-pub trait Foo {}
-
-pub trait Types {
-    type Foo;
-}
-
-pub trait Behavior<T: Types> {
-    fn reproduce(&self, foo: T::Foo);
-}
-
-pub struct Impl;
-
-impl<T: Types> Behavior<T> for Impl {
-    fn reproduce(&self, foo: <T as Types>::Foo) {
-        ${0:todo!()}
-    }
-}"#,
-        );
-    }
-
-    #[test]
-    fn test_assoc_type_on_concrete_type() {
-        check_assist(
-            add_missing_impl_members,
-            r#"
-pub trait Types {
-    type Foo;
-}
-
-impl Types for u32 {
-    type Foo = bool;
-}
-
-pub trait Behavior<T: Types> {
-    fn reproduce(&self, foo: T::Foo);
-}
-
-pub struct Impl;
-
-impl Behavior<u32> for Impl { $0 }"#,
-            r#"
-pub trait Types {
-    type Foo;
-}
-
-impl Types for u32 {
-    type Foo = bool;
-}
-
-pub trait Behavior<T: Types> {
-    fn reproduce(&self, foo: T::Foo);
-}
-
-pub struct Impl;
-
-impl Behavior<u32> for Impl {
-    fn reproduce(&self, foo: <u32 as Types>::Foo) {
-        ${0:todo!()}
-    }
-}"#,
-        );
-    }
-
-    #[test]
-    fn test_assoc_type_on_concrete_type_qualified() {
-        check_assist(
-            add_missing_impl_members,
-            r#"
-pub trait Types {
-    type Foo;
-}
-
-impl Types for std::string::String {
-    type Foo = bool;
-}
-
-pub trait Behavior<T: Types> {
-    fn reproduce(&self, foo: T::Foo);
-}
-
-pub struct Impl;
-
-impl Behavior<std::string::String> for Impl { $0 }"#,
-            r#"
-pub trait Types {
-    type Foo;
-}
-
-impl Types for std::string::String {
-    type Foo = bool;
-}
-
-pub trait Behavior<T: Types> {
-    fn reproduce(&self, foo: T::Foo);
-}
-
-pub struct Impl;
-
-impl Behavior<std::string::String> for Impl {
-    fn reproduce(&self, foo: <std::string::String as Types>::Foo) {
-        ${0:todo!()}
-    }
-}"#,
-        );
-    }
-
-    #[test]
-    fn test_assoc_type_on_concrete_type_multi_option_ambiguous() {
-        check_assist(
-            add_missing_impl_members,
-            r#"
-pub trait Types {
-    type Foo;
-}
-
-pub trait Types2 {
-    type Foo;
-}
-
-impl Types for u32 {
-    type Foo = bool;
-}
-
-impl Types2 for u32 {
-    type Foo = String;
-}
-
-pub trait Behavior<T: Types + Types2> {
-    fn reproduce(&self, foo: <T as Types2>::Foo);
-}
-
-pub struct Impl;
-
-impl Behavior<u32> for Impl { $0 }"#,
-            r#"
-pub trait Types {
-    type Foo;
-}
-
-pub trait Types2 {
-    type Foo;
-}
-
-impl Types for u32 {
-    type Foo = bool;
-}
-
-impl Types2 for u32 {
-    type Foo = String;
-}
-
-pub trait Behavior<T: Types + Types2> {
-    fn reproduce(&self, foo: <T as Types2>::Foo);
-}
-
-pub struct Impl;
-
-impl Behavior<u32> for Impl {
-    fn reproduce(&self, foo: <u32 as Types2>::Foo) {
-        ${0:todo!()}
-    }
-}"#,
-        );
-    }
-
-    #[test]
-    fn test_assoc_type_on_concrete_type_multi_option() {
-        check_assist(
-            add_missing_impl_members,
-            r#"
-pub trait Types {
-    type Foo;
-}
-
-pub trait Types2 {
-    type Bar;
-}
-
-impl Types for u32 {
-    type Foo = bool;
-}
-
-impl Types2 for u32 {
-    type Bar = String;
-}
-
-pub trait Behavior<T: Types + Types2> {
-    fn reproduce(&self, foo: T::Bar);
-}
-
-pub struct Impl;
-
-impl Behavior<u32> for Impl { $0 }"#,
-            r#"
-pub trait Types {
-    type Foo;
-}
-
-pub trait Types2 {
-    type Bar;
-}
-
-impl Types for u32 {
-    type Foo = bool;
-}
-
-impl Types2 for u32 {
-    type Bar = String;
-}
-
-pub trait Behavior<T: Types + Types2> {
-    fn reproduce(&self, foo: T::Bar);
-}
-
-pub struct Impl;
-
-impl Behavior<u32> for Impl {
-    fn reproduce(&self, foo: <u32 as Types2>::Bar) {
-        ${0:todo!()}
-    }
-}"#,
-        );
-    }
-
-    #[test]
-    fn test_assoc_type_on_concrete_type_multi_option_foreign() {
-        check_assist(
-            add_missing_impl_members,
-            r#"
-mod bar {
-    pub trait Types2 {
-        type Bar;
-    }
-}
-
-pub trait Types {
-    type Foo;
-}
-
-impl Types for u32 {
-    type Foo = bool;
-}
-
-impl bar::Types2 for u32 {
-    type Bar = String;
-}
-
-pub trait Behavior<T: Types + bar::Types2> {
-    fn reproduce(&self, foo: T::Bar);
-}
-
-pub struct Impl;
-
-impl Behavior<u32> for Impl { $0 }"#,
-            r#"
-mod bar {
-    pub trait Types2 {
-        type Bar;
-    }
-}
-
-pub trait Types {
-    type Foo;
-}
-
-impl Types for u32 {
-    type Foo = bool;
-}
-
-impl bar::Types2 for u32 {
-    type Bar = String;
-}
-
-pub trait Behavior<T: Types + bar::Types2> {
-    fn reproduce(&self, foo: T::Bar);
-}
-
-pub struct Impl;
-
-impl Behavior<u32> for Impl {
-    fn reproduce(&self, foo: <u32 as bar::Types2>::Bar) {
-        ${0:todo!()}
-    }
-}"#,
-        );
-    }
-
-    #[test]
-    fn test_transform_path_in_path_expr() {
-        check_assist(
-            add_missing_default_members,
-            r#"
-pub trait Const {
-    const FOO: u32;
-}
-
-pub trait Trait<T: Const> {
-    fn foo() -> bool {
-        match T::FOO {
-            0 => true,
-            _ => false,
-        }
-    }
-}
-
-impl Const for u32 {
-    const FOO: u32 = 1;
-}
-
-struct Impl;
-
-impl Trait<u32> for Impl { $0 }"#,
-            r#"
-pub trait Const {
-    const FOO: u32;
-}
-
-pub trait Trait<T: Const> {
-    fn foo() -> bool {
-        match T::FOO {
-            0 => true,
-            _ => false,
-        }
-    }
-}
-
-impl Const for u32 {
-    const FOO: u32 = 1;
-}
-
-struct Impl;
-
-impl Trait<u32> for Impl {
-    $0fn foo() -> bool {
-        match <u32 as Const>::FOO {
-            0 => true,
-            _ => false,
-        }
-    }
-}"#,
-        );
-    }
 }
diff --git a/crates/ide_db/src/path_transform.rs b/crates/ide_db/src/path_transform.rs
index 524af7fe8f..1b4f793a50 100644
--- a/crates/ide_db/src/path_transform.rs
+++ b/crates/ide_db/src/path_transform.rs
@@ -118,20 +118,14 @@ struct Ctx<'a> {
 
 impl<'a> Ctx<'a> {
     fn apply(&self, item: &SyntaxNode) {
-        // `transform_path` may update a node's parent and that would break the
-        // tree traversal. Thus all paths in the tree are collected into a vec
-        // so that such operation is safe.
-        let paths = item
-            .preorder()
-            .filter_map(|event| match event {
-                syntax::WalkEvent::Enter(_) => None,
-                syntax::WalkEvent::Leave(node) => Some(node),
-            })
-            .filter_map(ast::Path::cast)
-            .collect::<Vec<_>>();
-
-        for path in paths {
-            self.transform_path(path);
+        for event in item.preorder() {
+            let node = match event {
+                syntax::WalkEvent::Enter(_) => continue,
+                syntax::WalkEvent::Leave(it) => it,
+            };
+            if let Some(path) = ast::Path::cast(node.clone()) {
+                self.transform_path(path);
+            }
         }
     }
     fn transform_path(&self, path: ast::Path) -> Option<()> {
@@ -151,60 +145,10 @@ impl<'a> Ctx<'a> {
         match resolution {
             hir::PathResolution::TypeParam(tp) => {
                 if let Some(subst) = self.substs.get(&tp) {
-                    let parent = path.syntax().parent()?;
-                    if let Some(parent) = ast::Path::cast(parent.clone()) {
-                        // Path inside path means that there is an associated
-                        // type/constant on the type parameter. It is necessary
-                        // to fully qualify the type with `as Trait`. Even
-                        // though it might be unnecessary if `subst` is generic
-                        // type, always fully qualifying the path is safer
-                        // because of potential clash of associated types from
-                        // multiple traits
-
-                        let trait_ref = find_trait_for_assoc_item(
-                            self.source_scope,
-                            tp,
-                            parent.segment()?.name_ref()?,
-                        )
-                        .and_then(|trait_ref| {
-                            let found_path = self.target_module.find_use_path(
-                                self.source_scope.db.upcast(),
-                                hir::ModuleDef::Trait(trait_ref),
-                            )?;
-                            match ast::make::ty_path(mod_path_to_ast(&found_path)) {
-                                ast::Type::PathType(path_ty) => Some(path_ty),
-                                _ => None,
-                            }
-                        });
-
-                        let segment = ast::make::path_segment_ty(subst.clone(), trait_ref);
-                        let qualified =
-                            ast::make::path_from_segments(std::iter::once(segment), false);
-                        ted::replace(path.syntax(), qualified.clone_for_update().syntax());
-                    } else if let Some(path_ty) = ast::PathType::cast(parent) {
-                        ted::replace(
-                            path_ty.syntax(),
-                            subst.clone_subtree().clone_for_update().syntax(),
-                        );
-                    } else {
-                        ted::replace(
-                            path.syntax(),
-                            subst.clone_subtree().clone_for_update().syntax(),
-                        );
-                    }
+                    ted::replace(path.syntax(), subst.clone_subtree().clone_for_update().syntax())
                 }
             }
             hir::PathResolution::Def(def) => {
-                if let hir::ModuleDef::Trait(_) = def {
-                    if matches!(path.segment()?.kind()?, ast::PathSegmentKind::Type { .. }) {
-                        // `speculative_resolve` resolves segments like `<T as
-                        // Trait>` into `Trait`, but just the trait name should
-                        // not be used as the replacement of the original
-                        // segment.
-                        return None;
-                    }
-                }
-
                 let found_path =
                     self.target_module.find_use_path(self.source_scope.db.upcast(), def)?;
                 let res = mod_path_to_ast(&found_path).clone_for_update();
@@ -251,34 +195,3 @@ fn get_type_args_from_arg_list(generic_arg_list: ast::GenericArgList) -> Option<
 
     Some(result)
 }
-
-fn find_trait_for_assoc_item(
-    scope: &SemanticsScope,
-    type_param: hir::TypeParam,
-    assoc_item: ast::NameRef,
-) -> Option<hir::Trait> {
-    let db = scope.db;
-    let trait_bounds = type_param.trait_bounds(db);
-
-    let assoc_item_name = assoc_item.text();
-
-    for trait_ in trait_bounds {
-        let names = trait_.items(db).into_iter().filter_map(|item| match item {
-            hir::AssocItem::TypeAlias(ta) => Some(ta.name(db)),
-            hir::AssocItem::Const(cst) => cst.name(db),
-            _ => None,
-        });
-
-        for name in names {
-            if assoc_item_name.as_str() == name.as_text()?.as_str() {
-                // It is fine to return the first match because in case of
-                // multiple possibilities, the exact trait must be disambiguated
-                // in the definition of trait being implemented, so this search
-                // should not be needed.
-                return Some(trait_);
-            }
-        }
-    }
-
-    None
-}
diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs
index f80c5e382c..3575020975 100644
--- a/crates/syntax/src/ast/make.rs
+++ b/crates/syntax/src/ast/make.rs
@@ -188,14 +188,6 @@ pub fn path_segment(name_ref: ast::NameRef) -> ast::PathSegment {
     ast_from_text(&format!("use {};", name_ref))
 }
 
-pub fn path_segment_ty(type_ref: ast::Type, trait_ref: Option<ast::PathType>) -> ast::PathSegment {
-    let text = match trait_ref {
-        Some(trait_ref) => format!("fn f(x: <{} as {}>) {{}}", type_ref, trait_ref),
-        None => format!("fn f(x: <{}>) {{}}", type_ref),
-    };
-    ast_from_text(&text)
-}
-
 pub fn path_segment_self() -> ast::PathSegment {
     ast_from_text("use self;")
 }
@@ -226,9 +218,9 @@ pub fn path_from_segments(
 ) -> ast::Path {
     let segments = segments.into_iter().map(|it| it.syntax().clone()).join("::");
     ast_from_text(&if is_abs {
-        format!("fn f(x: ::{}) {{}}", segments)
+        format!("use ::{};", segments)
     } else {
-        format!("fn f(x: {}) {{}}", segments)
+        format!("use {};", segments)
     })
 }
 


Test patch:
diff --git a/crates/ide/src/syntax_highlighting/test_data/highlight_doctest.html b/crates/ide/src/syntax_highlighting/test_data/highlight_doctest.html
index 6e87147e8b..a04a211825 100644
--- a/crates/ide/src/syntax_highlighting/test_data/highlight_doctest.html
+++ b/crates/ide/src/syntax_highlighting/test_data/highlight_doctest.html
@@ -46,6 +46,8 @@ pre                 { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd
 <span class="comment documentation">//! </span><span class="keyword injected">fn</span><span class="none injected"> </span><span class="function declaration injected">test</span><span class="parenthesis injected">(</span><span class="parenthesis injected">)</span><span class="none injected"> </span><span class="brace injected">{</span><span class="brace injected">}</span>
 <span class="comment documentation">//! ```</span>
 
+<span class="keyword">mod</span> <span class="module declaration">outline_module</span><span class="semicolon">;</span>
+
 <span class="comment documentation">/// ```</span>
 <span class="comment documentation">/// </span><span class="keyword injected">let</span><span class="none injected"> </span><span class="punctuation injected">_</span><span class="none injected"> </span><span class="operator injected">=</span><span class="none injected"> </span><span class="string_literal injected">"early doctests should not go boom"</span><span class="semicolon injected">;</span>
 <span class="comment documentation">/// ```</span>
@@ -170,4 +172,6 @@ It is beyond me why you'd use these when you got ///
     ```
     </span><span class="function documentation injected intra_doc_link">[`block_comments`]</span><span class="comment documentation"> tests these without indentation
 */</span>
-<span class="keyword">pub</span> <span class="keyword">fn</span> <span class="function declaration public">block_comments2</span><span class="parenthesis">(</span><span class="parenthesis">)</span> <span class="brace">{</span><span class="brace">}</span></code></pre>
\ No newline at end of file
+<span class="keyword">pub</span> <span class="keyword">fn</span> <span class="function declaration public">block_comments2</span><span class="parenthesis">(</span><span class="parenthesis">)</span> <span class="brace">{</span><span class="brace">}</span>
+
+</code></pre>
\ No newline at end of file
diff --git a/crates/ide/src/syntax_highlighting/tests.rs b/crates/ide/src/syntax_highlighting/tests.rs
index c9c23318f5..4beab9909c 100644
--- a/crates/ide/src/syntax_highlighting/tests.rs
+++ b/crates/ide/src/syntax_highlighting/tests.rs
@@ -641,11 +641,14 @@ fn main() {
 fn test_highlight_doc_comment() {
     check_highlighting(
         r#"
+//- /main.rs
 //! This is a module to test doc injection.
 //! ```
 //! fn test() {}
 //! ```
 
+mod outline_module;
+
 /// ```
 /// let _ = "early doctests should not go boom";
 /// ```
@@ -771,6 +774,13 @@ pub fn block_comments() {}
     [`block_comments`] tests these without indentation
 */
 pub fn block_comments2() {}
+
+//- /outline_module.rs
+//! This is an outline module whose purpose is to test that its inline attribute injection does not
+//! spill into its parent.
+//! ```
+//! fn test() {}
+//! ```
 "#
         .trim(),
         expect_file!["./test_data/highlight_doctest.html"],