Merge #11938

11938: feat: improve assoc. item completion in trait impls r=jonas-schievink a=jonas-schievink

Account for macro-generated items, increase the score of these completions since they're very relevant, and allow them to trigger when the cursor is directly in the assoc. item list without requiring further input.

![screenshot-2022-04-08-18:12:06](https://user-images.githubusercontent.com/1786438/162481277-2a0d2f21-dc20-4452-804d-6370766216b6.png)

Part of https://github.com/rust-analyzer/rust-analyzer/issues/11860

bors r+

Co-authored-by: Jonas Schievink <jonas.schievink@ferrous-systems.com>

5 files changed, 173 insertions, 91 deletions

diff --git a/crates/ide_completion/src/completions/trait_impl.rs b/crates/ide_completion/src/completions/trait_impl.rs
index 9c557aa5244..13e05049e2f 100644
--- a/crates/ide_completion/src/completions/trait_impl.rs
+++ b/crates/ide_completion/src/completions/trait_impl.rs
@@ -36,11 +36,13 @@ use ide_db::{path_transform::PathTransform, traits::get_missing_assoc_items, Sym
 use syntax::{
     ast::{self, edit_in_place::AttrsOwnerEdit},
     display::function_declaration,
-    AstNode, SyntaxElement, SyntaxKind, SyntaxNode, SyntaxToken, TextRange, T,
+    AstNode, SyntaxElement, SyntaxKind, SyntaxNode, TextRange, T,
 };
 use text_edit::TextEdit;
 
-use crate::{CompletionContext, CompletionItem, CompletionItemKind, Completions};
+use crate::{
+    CompletionContext, CompletionItem, CompletionItemKind, CompletionRelevance, Completions,
+};
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 enum ImplCompletionKind {
@@ -51,22 +53,22 @@ enum ImplCompletionKind {
 }
 
 pub(crate) fn complete_trait_impl(acc: &mut Completions, ctx: &CompletionContext) {
-    if let Some((kind, trigger, impl_def)) = completion_match(ctx.token.clone()) {
+    if let Some((kind, replacement_range, impl_def)) = completion_match(ctx) {
         if let Some(hir_impl) = ctx.sema.to_def(&impl_def) {
             get_missing_assoc_items(&ctx.sema, &impl_def).into_iter().for_each(|item| {
                 match (item, kind) {
                     (
                         hir::AssocItem::Function(fn_item),
                         ImplCompletionKind::All | ImplCompletionKind::Fn,
-                    ) => add_function_impl(acc, ctx, &trigger, fn_item, hir_impl),
+                    ) => add_function_impl(acc, ctx, replacement_range, fn_item, hir_impl),
                     (
                         hir::AssocItem::TypeAlias(type_item),
                         ImplCompletionKind::All | ImplCompletionKind::TypeAlias,
-                    ) => add_type_alias_impl(acc, ctx, &trigger, type_item),
+                    ) => add_type_alias_impl(acc, ctx, replacement_range, type_item),
                     (
                         hir::AssocItem::Const(const_item),
                         ImplCompletionKind::All | ImplCompletionKind::Const,
-                    ) => add_const_impl(acc, ctx, &trigger, const_item, hir_impl),
+                    ) => add_const_impl(acc, ctx, replacement_range, const_item, hir_impl),
                     _ => {}
                 }
             });
@@ -74,61 +76,79 @@ pub(crate) fn complete_trait_impl(acc: &mut Completions, ctx: &CompletionContext
     }
 }
 
-fn completion_match(mut token: SyntaxToken) -> Option<(ImplCompletionKind, SyntaxNode, ast::Impl)> {
+fn completion_match(ctx: &CompletionContext) -> Option<(ImplCompletionKind, TextRange, ast::Impl)> {
+    let token = ctx.token.clone();
+
     // For keyword without name like `impl .. { fn $0 }`, the current position is inside
     // the whitespace token, which is outside `FN` syntax node.
     // We need to follow the previous token in this case.
+    let mut token_before_ws = token.clone();
     if token.kind() == SyntaxKind::WHITESPACE {
-        token = token.prev_token()?;
+        token_before_ws = token.prev_token()?;
     }
 
-    let parent_kind = token.parent().map_or(SyntaxKind::EOF, |it| it.kind());
-    let impl_item_offset = match token.kind() {
-        // `impl .. { const $0 }`
-        // ERROR      0
-        //   CONST_KW <- *
-        T![const] => 0,
-        // `impl .. { fn/type $0 }`
-        // FN/TYPE_ALIAS  0
-        //   FN_KW        <- *
-        T![fn] | T![type] => 0,
-        // `impl .. { fn/type/const foo$0 }`
-        // FN/TYPE_ALIAS/CONST  1
-        //  NAME                0
-        //    IDENT             <- *
-        SyntaxKind::IDENT if parent_kind == SyntaxKind::NAME => 1,
-        // `impl .. { foo$0 }`
-        // MACRO_CALL       3
-        //  PATH            2
-        //    PATH_SEGMENT  1
-        //      NAME_REF    0
-        //        IDENT     <- *
-        SyntaxKind::IDENT if parent_kind == SyntaxKind::NAME_REF => 3,
-        _ => return None,
-    };
+    let parent_kind = token_before_ws.parent().map_or(SyntaxKind::EOF, |it| it.kind());
+    if token.parent().map(|n| n.kind()) == Some(SyntaxKind::ASSOC_ITEM_LIST)
+        && matches!(
+            token_before_ws.kind(),
+            SyntaxKind::SEMICOLON | SyntaxKind::R_CURLY | SyntaxKind::L_CURLY
+        )
+    {
+        let impl_def = ast::Impl::cast(token.parent()?.parent()?)?;
+        let kind = ImplCompletionKind::All;
+        let replacement_range = TextRange::empty(ctx.position.offset);
+        Some((kind, replacement_range, impl_def))
+    } else {
+        let impl_item_offset = match token_before_ws.kind() {
+            // `impl .. { const $0 }`
+            // ERROR      0
+            //   CONST_KW <- *
+            T![const] => 0,
+            // `impl .. { fn/type $0 }`
+            // FN/TYPE_ALIAS  0
+            //   FN_KW        <- *
+            T![fn] | T![type] => 0,
+            // `impl .. { fn/type/const foo$0 }`
+            // FN/TYPE_ALIAS/CONST  1
+            //  NAME                0
+            //    IDENT             <- *
+            SyntaxKind::IDENT if parent_kind == SyntaxKind::NAME => 1,
+            // `impl .. { foo$0 }`
+            // MACRO_CALL       3
+            //  PATH            2
+            //    PATH_SEGMENT  1
+            //      NAME_REF    0
+            //        IDENT     <- *
+            SyntaxKind::IDENT if parent_kind == SyntaxKind::NAME_REF => 3,
+            _ => return None,
+        };
 
-    let impl_item = token.ancestors().nth(impl_item_offset)?;
-    // Must directly belong to an impl block.
-    // IMPL
-    //   ASSOC_ITEM_LIST
-    //     <item>
-    let impl_def = ast::Impl::cast(impl_item.parent()?.parent()?)?;
-    let kind = match impl_item.kind() {
-        // `impl ... { const $0 fn/type/const }`
-        _ if token.kind() == T![const] => ImplCompletionKind::Const,
-        SyntaxKind::CONST | SyntaxKind::ERROR => ImplCompletionKind::Const,
-        SyntaxKind::TYPE_ALIAS => ImplCompletionKind::TypeAlias,
-        SyntaxKind::FN => ImplCompletionKind::Fn,
-        SyntaxKind::MACRO_CALL => ImplCompletionKind::All,
-        _ => return None,
-    };
-    Some((kind, impl_item, impl_def))
+        let impl_item = token_before_ws.ancestors().nth(impl_item_offset)?;
+        // Must directly belong to an impl block.
+        // IMPL
+        //   ASSOC_ITEM_LIST
+        //     <item>
+        let impl_def = ast::Impl::cast(impl_item.parent()?.parent()?)?;
+        let kind = match impl_item.kind() {
+            // `impl ... { const $0 fn/type/const }`
+            _ if token_before_ws.kind() == T![const] => ImplCompletionKind::Const,
+            SyntaxKind::CONST | SyntaxKind::ERROR => ImplCompletionKind::Const,
+            SyntaxKind::TYPE_ALIAS => ImplCompletionKind::TypeAlias,
+            SyntaxKind::FN => ImplCompletionKind::Fn,
+            SyntaxKind::MACRO_CALL => ImplCompletionKind::All,
+            _ => return None,
+        };
+
+        let replacement_range = replacement_range(ctx, &impl_item);
+
+        Some((kind, replacement_range, impl_def))
+    }
 }
 
 fn add_function_impl(
     acc: &mut Completions,
     ctx: &CompletionContext,
-    fn_def_node: &SyntaxNode,
+    replacement_range: TextRange,
     func: hir::Function,
     impl_def: hir::Impl,
 ) {
@@ -146,9 +166,10 @@ fn add_function_impl(
         CompletionItemKind::SymbolKind(SymbolKind::Function)
     };
 
-    let range = replacement_range(ctx, fn_def_node);
-    let mut item = CompletionItem::new(completion_kind, range, label);
-    item.lookup_by(fn_name).set_documentation(func.docs(ctx.db));
+    let mut item = CompletionItem::new(completion_kind, replacement_range, label);
+    item.lookup_by(fn_name)
+        .set_documentation(func.docs(ctx.db))
+        .set_relevance(CompletionRelevance { is_item_from_trait: true, ..Default::default() });
 
     if let Some(source) = ctx.sema.source(func) {
         let assoc_item = ast::AssocItem::Fn(source.value);
@@ -162,11 +183,11 @@ fn add_function_impl(
             match ctx.config.snippet_cap {
                 Some(cap) => {
                     let snippet = format!("{} {{\n    $0\n}}", function_decl);
-                    item.snippet_edit(cap, TextEdit::replace(range, snippet));
+                    item.snippet_edit(cap, TextEdit::replace(replacement_range, snippet));
                 }
                 None => {
                     let header = format!("{} {{", function_decl);
-                    item.text_edit(TextEdit::replace(range, header));
+                    item.text_edit(TextEdit::replace(replacement_range, header));
                 }
             };
             item.add_to(acc);
@@ -201,25 +222,26 @@ fn get_transformed_assoc_item(
 fn add_type_alias_impl(
     acc: &mut Completions,
     ctx: &CompletionContext,
-    type_def_node: &SyntaxNode,
+    replacement_range: TextRange,
     type_alias: hir::TypeAlias,
 ) {
     let alias_name = type_alias.name(ctx.db).to_smol_str();
 
+    let label = format!("type {} =", alias_name);
     let snippet = format!("type {} = ", alias_name);
 
-    let range = replacement_range(ctx, type_def_node);
-    let mut item = CompletionItem::new(SymbolKind::TypeAlias, range, &snippet);
-    item.text_edit(TextEdit::replace(range, snippet))
+    let mut item = CompletionItem::new(SymbolKind::TypeAlias, replacement_range, label);
+    item.text_edit(TextEdit::replace(replacement_range, snippet))
         .lookup_by(alias_name)
-        .set_documentation(type_alias.docs(ctx.db));
+        .set_documentation(type_alias.docs(ctx.db))
+        .set_relevance(CompletionRelevance { is_item_from_trait: true, ..Default::default() });
     item.add_to(acc);
 }
 
 fn add_const_impl(
     acc: &mut Completions,
     ctx: &CompletionContext,
-    const_def_node: &SyntaxNode,
+    replacement_range: TextRange,
     const_: hir::Const,
     impl_def: hir::Impl,
 ) {
@@ -234,13 +256,17 @@ fn add_const_impl(
                     _ => unreachable!(),
                 };
 
-                let snippet = make_const_compl_syntax(&transformed_const);
+                let label = make_const_compl_syntax(&transformed_const);
+                let snippet = format!("{} ", label);
 
-                let range = replacement_range(ctx, const_def_node);
-                let mut item = CompletionItem::new(SymbolKind::Const, range, &snippet);
-                item.text_edit(TextEdit::replace(range, snippet))
+                let mut item = CompletionItem::new(SymbolKind::Const, replacement_range, label);
+                item.text_edit(TextEdit::replace(replacement_range, snippet))
                     .lookup_by(const_name)
-                    .set_documentation(const_.docs(ctx.db));
+                    .set_documentation(const_.docs(ctx.db))
+                    .set_relevance(CompletionRelevance {
+                        is_item_from_trait: true,
+                        ..Default::default()
+                    });
                 item.add_to(acc);
             }
         }
@@ -267,7 +293,7 @@ fn make_const_compl_syntax(const_: &ast::Const) -> String {
 
     let syntax = const_.syntax().text().slice(range).to_string();
 
-    format!("{} = ", syntax.trim_end())
+    format!("{} =", syntax.trim_end())
 }
 
 fn replacement_range(ctx: &CompletionContext, item: &SyntaxNode) -> TextRange {
@@ -987,4 +1013,38 @@ where Self: SomeTrait<u32> {
 "#,
         )
     }
+
+    #[test]
+    fn works_directly_in_impl() {
+        check(
+            r#"
+trait Tr {
+    fn required();
+}
+
+impl Tr for () {
+    $0
+}
+"#,
+            expect![[r#"
+            fn fn required()
+        "#]],
+        );
+        check(
+            r#"
+trait Tr {
+    fn provided() {}
+    fn required();
+}
+
+impl Tr for () {
+    fn provided() {}
+    $0
+}
+"#,
+            expect![[r#"
+            fn fn required()
+        "#]],
+        );
+    }
 }
diff --git a/crates/ide_completion/src/item.rs b/crates/ide_completion/src/item.rs
index 8c73bcaab2d..2fa8f772642 100644
--- a/crates/ide_completion/src/item.rs
+++ b/crates/ide_completion/src/item.rs
@@ -132,6 +132,8 @@ pub struct CompletionRelevance {
     /// }
     /// ```
     pub is_local: bool,
+    /// This is set when trait items are completed in an impl of that trait.
+    pub is_item_from_trait: bool,
     /// Set for method completions of the `core::ops` and `core::cmp` family.
     pub is_op_method: bool,
     /// Set for item completions that are private but in the workspace.
@@ -197,6 +199,7 @@ impl CompletionRelevance {
             exact_name_match,
             type_match,
             is_local,
+            is_item_from_trait,
             is_op_method,
             is_private_editable,
             postfix_match,
@@ -228,6 +231,9 @@ impl CompletionRelevance {
         if is_local {
             score += 1;
         }
+        if is_item_from_trait {
+            score += 1;
+        }
         if is_definite {
             score += 10;
         }
diff --git a/crates/ide_completion/src/render.rs b/crates/ide_completion/src/render.rs
index 70ae6399f4d..5b0257f6b40 100644
--- a/crates/ide_completion/src/render.rs
+++ b/crates/ide_completion/src/render.rs
@@ -624,6 +624,7 @@ fn main() { let _: m::Spam = S$0 }
                                 Exact,
                             ),
                             is_local: false,
+                            is_item_from_trait: false,
                             is_op_method: false,
                             is_private_editable: false,
                             postfix_match: None,
@@ -646,6 +647,7 @@ fn main() { let _: m::Spam = S$0 }
                                 Exact,
                             ),
                             is_local: false,
+                            is_item_from_trait: false,
                             is_op_method: false,
                             is_private_editable: false,
                             postfix_match: None,
@@ -734,6 +736,7 @@ fn foo() { A { the$0 } }
                                 CouldUnify,
                             ),
                             is_local: false,
+                            is_item_from_trait: false,
                             is_op_method: false,
                             is_private_editable: false,
                             postfix_match: None,
diff --git a/crates/ide_completion/src/tests/item_list.rs b/crates/ide_completion/src/tests/item_list.rs
index 82824fd3932..0e60f748790 100644
--- a/crates/ide_completion/src/tests/item_list.rs
+++ b/crates/ide_completion/src/tests/item_list.rs
@@ -241,11 +241,14 @@ impl Test for () {
             kw fn
             kw const
             kw type
+            ta type Type1 =
+            ct const CONST1: () =
+            fn fn function1()
             kw self
             kw super
             kw crate
             md module
-            ma makro!(…)  macro_rules! makro
+            ma makro!(…)          macro_rules! makro
         "#]],
     );
 }
diff --git a/crates/ide_db/src/traits.rs b/crates/ide_db/src/traits.rs
index c8cb1a26a87..0fbfd869921 100644
--- a/crates/ide_db/src/traits.rs
+++ b/crates/ide_db/src/traits.rs
@@ -3,10 +3,7 @@
 use crate::RootDatabase;
 use hir::Semantics;
 use rustc_hash::FxHashSet;
-use syntax::{
-    ast::{self, HasName},
-    AstNode,
-};
+use syntax::{ast, AstNode};
 
 /// Given the `impl` block, attempts to find the trait this `impl` corresponds to.
 pub fn resolve_target_trait(
@@ -28,32 +25,28 @@ pub fn get_missing_assoc_items(
     sema: &Semantics<RootDatabase>,
     impl_def: &ast::Impl,
 ) -> Vec<hir::AssocItem> {
+    let imp = match sema.to_def(impl_def) {
+        Some(it) => it,
+        None => return vec![],
+    };
+
     // Names must be unique between constants and functions. However, type aliases
     // may share the same name as a function or constant.
     let mut impl_fns_consts = FxHashSet::default();
     let mut impl_type = FxHashSet::default();
 
-    if let Some(item_list) = impl_def.assoc_item_list() {
-        for item in item_list.assoc_items() {
-            match item {
-                ast::AssocItem::Fn(f) => {
-                    if let Some(n) = f.name() {
-                        impl_fns_consts.insert(n.syntax().to_string());
-                    }
-                }
-
-                ast::AssocItem::TypeAlias(t) => {
-                    if let Some(n) = t.name() {
-                        impl_type.insert(n.syntax().to_string());
-                    }
-                }
-
-                ast::AssocItem::Const(c) => {
-                    if let Some(n) = c.name() {
-                        impl_fns_consts.insert(n.syntax().to_string());
-                    }
+    for item in imp.items(sema.db) {
+        match item {
+            hir::AssocItem::Function(it) => {
+                impl_fns_consts.insert(it.name(sema.db).to_string());
+            }
+            hir::AssocItem::Const(it) => {
+                if let Some(name) = it.name(sema.db) {
+                    impl_fns_consts.insert(name.to_string());
                 }
-                ast::AssocItem::MacroCall(_) => (),
+            }
+            hir::AssocItem::TypeAlias(it) => {
+                impl_type.insert(it.name(sema.db).to_string());
             }
         }
     }
@@ -219,5 +212,22 @@ impl Foo {
 }"#,
             expect![[r#""#]],
         );
+
+        check_missing_assoc(
+            r#"
+trait Tr {
+    fn required();
+}
+macro_rules! m {
+    () => { fn required() {} };
+}
+impl Tr for () {
+    m!();
+    $0
+}
+
+            "#,
+            expect![[r#""#]],
+        );
     }
 }