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>

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 parent_kind = token_before_ws.parent().map_or(SyntaxKind::EOF, |it| it.kind());
-    let impl_item_offset = match token.kind() {
+    if token.parent().map(|n| n.kind()) == Some(SyntaxKind::ASSOC_ITEM_LIST)
-        // `impl .. { const $0 }`
+        && matches!(
-        // ERROR      0
+            token_before_ws.kind(),
-        //   CONST_KW <- *
+            SyntaxKind::SEMICOLON | SyntaxKind::R_CURLY | SyntaxKind::L_CURLY
-        T![const] => 0,
+        )
-        // `impl .. { fn/type $0 }`
+    {
-        // FN/TYPE_ALIAS  0
+        let impl_def = ast::Impl::cast(token.parent()?.parent()?)?;
-        //   FN_KW        <- *
+        let kind = ImplCompletionKind::All;
-        T![fn] | T![type] => 0,
+        let replacement_range = TextRange::empty(ctx.position.offset);
-        // `impl .. { fn/type/const foo$0 }`
+        Some((kind, replacement_range, impl_def))
-        // FN/TYPE_ALIAS/CONST  1
+    } else {
-        //  NAME                0
+        let impl_item_offset = match token_before_ws.kind() {
-        //    IDENT             <- *
+            // `impl .. { const $0 }`
-        SyntaxKind::IDENT if parent_kind == SyntaxKind::NAME => 1,
+            // ERROR      0
-        // `impl .. { foo$0 }`
+            //   CONST_KW <- *
-        // MACRO_CALL       3
+            T![const] => 0,
-        //  PATH            2
+            // `impl .. { fn/type $0 }`
-        //    PATH_SEGMENT  1
+            // FN/TYPE_ALIAS  0
-        //      NAME_REF    0
+            //   FN_KW        <- *
-        //        IDENT     <- *
+            T![fn] | T![type] => 0,
-        SyntaxKind::IDENT if parent_kind == SyntaxKind::NAME_REF => 3,
+            // `impl .. { fn/type/const foo$0 }`
-        _ => return None,
+            // 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)?;
+        let impl_item = token_before_ws.ancestors().nth(impl_item_offset)?;
-    // Must directly belong to an impl block.
+        // Must directly belong to an impl block.
-    // IMPL
+        // IMPL
-    //   ASSOC_ITEM_LIST
+        //   ASSOC_ITEM_LIST
-    //     <item>
+        //     <item>
-    let impl_def = ast::Impl::cast(impl_item.parent()?.parent()?)?;
+        let impl_def = ast::Impl::cast(impl_item.parent()?.parent()?)?;
-    let kind = match impl_item.kind() {
+        let kind = match impl_item.kind() {
-        // `impl ... { const $0 fn/type/const }`
+            // `impl ... { const $0 fn/type/const }`
-        _ if token.kind() == T![const] => ImplCompletionKind::Const,
+            _ if token_before_ws.kind() == T![const] => ImplCompletionKind::Const,
-        SyntaxKind::CONST | SyntaxKind::ERROR => ImplCompletionKind::Const,
+            SyntaxKind::CONST | SyntaxKind::ERROR => ImplCompletionKind::Const,
-        SyntaxKind::TYPE_ALIAS => ImplCompletionKind::TypeAlias,
+            SyntaxKind::TYPE_ALIAS => ImplCompletionKind::TypeAlias,
-        SyntaxKind::FN => ImplCompletionKind::Fn,
+            SyntaxKind::FN => ImplCompletionKind::Fn,
-        SyntaxKind::MACRO_CALL => ImplCompletionKind::All,
+            SyntaxKind::MACRO_CALL => ImplCompletionKind::All,
-        _ => return None,
+            _ => return None,
-    };
+        };
-    Some((kind, impl_item, impl_def))
+
+        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, replacement_range, label);
-    let mut item = CompletionItem::new(completion_kind, range, label);
+    item.lookup_by(fn_name)
-    item.lookup_by(fn_name).set_documentation(func.docs(ctx.db));
+        .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, replacement_range, label);
-    let mut item = CompletionItem::new(SymbolKind::TypeAlias, range, &snippet);
+    item.text_edit(TextEdit::replace(replacement_range, snippet))
-    item.text_edit(TextEdit::replace(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, replacement_range, label);
-                let mut item = CompletionItem::new(SymbolKind::Const, range, &snippet);
+                item.text_edit(TextEdit::replace(replacement_range, snippet))
-                item.text_edit(TextEdit::replace(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()
+        "#]],
+        );
+    }
 }

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;
         }

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,

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
         "#]],
     );
 }

crates/ide_db/src/traits.rs

@@ -3,10 +3,7 @@
 use crate::RootDatabase;
 use hir::Semantics;
 use rustc_hash::FxHashSet;
-use syntax::{
+use syntax::{ast, AstNode};
-    ast::{self, HasName},
-    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 imp.items(sema.db) {
-        for item in item_list.assoc_items() {
+        match item {
-            match item {
+            hir::AssocItem::Function(it) => {
-                ast::AssocItem::Fn(f) => {
+                impl_fns_consts.insert(it.name(sema.db).to_string());
-                    if let Some(n) = f.name() {
+            }
-                        impl_fns_consts.insert(n.syntax().to_string());
+            hir::AssocItem::Const(it) => {
-                    }
+                if let Some(name) = it.name(sema.db) {
+                    impl_fns_consts.insert(name.to_string());
                 }
-
+            }
-                ast::AssocItem::TypeAlias(t) => {
+            hir::AssocItem::TypeAlias(it) => {
-                    if let Some(n) = t.name() {
+                impl_type.insert(it.name(sema.db).to_string());
-                        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());
-                    }
-                }
-                ast::AssocItem::MacroCall(_) => (),
             }
         }
     }
@@ -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#""#]],
+        );
     }
 }