Auto merge of rust-lang#12418 - Veykril:completions, r=Veykril

internal: More precise completion filtering with existing item qualifiers

Now we are approaching the more complex cases for filtering completions

Author: bors

crates/ide-completion/src/completions/item_list.rs

@@ -8,21 +8,18 @@ use crate::{
 
 pub(crate) fn complete_item_list(acc: &mut Completions, ctx: &CompletionContext) {
     let _p = profile::span("complete_item_list");
-    if ctx.is_path_disallowed() || ctx.has_unfinished_impl_or_trait_prev_sibling() {
-        return;
-    }
 
-    let (&is_absolute_path, qualifier) = match ctx.path_context() {
+    let (&is_absolute_path, path_qualifier, _kind) = match ctx.path_context() {
         Some(PathCompletionCtx {
-            kind: PathKind::Item { .. },
+            kind: PathKind::Item { kind },
             is_absolute_path,
             qualifier,
             ..
-        }) => (is_absolute_path, qualifier),
+        }) => (is_absolute_path, qualifier, kind),
         _ => return,
     };
 
-    match qualifier {
+    match path_qualifier {
         Some(PathQualifierCtx { resolution, is_super_chain, .. }) => {
             if let Some(hir::PathResolution::Def(hir::ModuleDef::Module(module))) = resolution {
                 for (name, def) in module.scope(ctx.db, Some(ctx.module)) {
@@ -39,13 +36,14 @@ pub(crate) fn complete_item_list(acc: &mut Completions, ctx: &CompletionContext)
         None if is_absolute_path => {
             acc.add_crate_roots(ctx);
         }
-        None => {
+        None if ctx.qualifier_ctx.none() => {
             ctx.process_all_names(&mut |name, def| {
                 if let Some(def) = module_or_fn_macro(ctx.db, def) {
                     acc.add_resolution(ctx, name, def);
                 }
             });
             acc.add_nameref_keywords_with_colon(ctx);
         }
+        None => {}
     }
 }

crates/ide-completion/src/completions/keyword.rs

@@ -51,7 +51,7 @@ pub(crate) fn complete_expr_keyword(acc: &mut Completions, ctx: &CompletionConte
         return;
     }
 
-    if !ctx.has_visibility_prev_sibling()
+    if ctx.qualifier_ctx.vis_node.is_none()
         && (expects_item || ctx.expects_non_trait_assoc_item() || ctx.expect_field())
     {
         add_keyword("pub(crate)", "pub(crate)");
@@ -67,7 +67,7 @@ pub(crate) fn complete_expr_keyword(acc: &mut Completions, ctx: &CompletionConte
     }
 
     if expects_item || has_block_expr_parent {
-        if !ctx.has_visibility_prev_sibling() {
+        if ctx.qualifier_ctx.vis_node.is_none() {
             add_keyword("impl", "impl $1 {\n    $0\n}");
             add_keyword("extern", "extern $0");
         }

crates/ide-completion/src/completions/snippet.rs

@@ -2,7 +2,6 @@
 
 use hir::Documentation;
 use ide_db::{imports::insert_use::ImportScope, SnippetCap};
-use syntax::T;
 
 use crate::{
     context::{ItemListKind, PathCompletionCtx, PathKind},
@@ -52,10 +51,10 @@ pub(crate) fn complete_item_snippet(acc: &mut Completions, ctx: &CompletionConte
         }) => kind,
         _ => return,
     };
-    if ctx.previous_token_is(T![unsafe]) || ctx.has_unfinished_impl_or_trait_prev_sibling() {
+    if !ctx.qualifier_ctx.none() {
         return;
     }
-    if ctx.has_visibility_prev_sibling() {
+    if ctx.qualifier_ctx.vis_node.is_some() {
         return; // technically we could do some of these snippet completions if we were to put the
                 // attributes before the vis node.
     }

crates/ide-completion/src/context.rs

@@ -75,6 +75,18 @@ pub(super) enum ItemListKind {
     ExternBlock,
 }
 
+#[derive(Debug, Default)]
+pub(super) struct QualifierCtx {
+    pub(super) unsafe_tok: Option<SyntaxToken>,
+    pub(super) vis_node: Option<ast::Visibility>,
+}
+
+impl QualifierCtx {
+    pub(super) fn none(&self) -> bool {
+        self.unsafe_tok.is_none() && self.vis_node.is_none()
+    }
+}
+
 #[derive(Debug)]
 pub(crate) struct PathCompletionCtx {
     /// If this is a call with () already there (or {} in case of record patterns)
@@ -253,6 +265,7 @@ pub(crate) struct CompletionContext<'a> {
     pub(super) ident_ctx: IdentContext,
 
     pub(super) pattern_ctx: Option<PatternContext>,
+    pub(super) qualifier_ctx: QualifierCtx,
 
     pub(super) existing_derives: FxHashSet<hir::Macro>,
 
@@ -363,17 +376,13 @@ impl<'a> CompletionContext<'a> {
         matches!(self.prev_sibling, Some(ImmediatePrevSibling::ImplDefType))
     }
 
-    pub(crate) fn has_visibility_prev_sibling(&self) -> bool {
-        matches!(self.prev_sibling, Some(ImmediatePrevSibling::Visibility))
-    }
-
     pub(crate) fn after_if(&self) -> bool {
         matches!(self.prev_sibling, Some(ImmediatePrevSibling::IfExpr))
     }
 
     // FIXME: This shouldn't exist
     pub(crate) fn is_path_disallowed(&self) -> bool {
-        self.previous_token_is(T![unsafe])
+        !self.qualifier_ctx.none()
             || matches!(self.prev_sibling, Some(ImmediatePrevSibling::Visibility))
             || (matches!(self.name_ctx(), Some(NameContext { .. })) && self.pattern_ctx.is_none())
             || matches!(self.pattern_ctx, Some(PatternContext { record_pat: Some(_), .. }))
@@ -555,6 +564,7 @@ impl<'a> CompletionContext<'a> {
             // dummy value, will be overwritten
             ident_ctx: IdentContext::UnexpandedAttrTT { fake_attribute_under_caret: None },
             pattern_ctx: None,
+            qualifier_ctx: Default::default(),
             existing_derives: Default::default(),
             locals,
         };
@@ -865,7 +875,7 @@ impl<'a> CompletionContext<'a> {
         offset: TextSize,
         derive_ctx: Option<(SyntaxNode, SyntaxNode, TextSize, ast::Attr)>,
     ) -> Option<()> {
-        let fake_ident_token = file_with_fake_ident.token_at_offset(offset).right_biased().unwrap();
+        let fake_ident_token = file_with_fake_ident.token_at_offset(offset).right_biased()?;
         let syntax_element = NodeOrToken::Token(fake_ident_token);
         if is_in_token_of_for_loop(syntax_element.clone()) {
             // for pat $0
@@ -967,7 +977,49 @@ impl<'a> CompletionContext<'a> {
             ast::NameLike::NameRef(name_ref) => {
                 let parent = name_ref.syntax().parent()?;
                 let (nameref_ctx, pat_ctx) =
-                    Self::classify_name_ref(&self.sema, &original_file, name_ref, parent);
+                    Self::classify_name_ref(&self.sema, &original_file, name_ref, parent.clone());
+
+                // Extract qualifiers
+                if let Some(path_ctx) = &nameref_ctx.path_ctx {
+                    if path_ctx.qualifier.is_none() {
+                        let top = match path_ctx.kind {
+                            PathKind::Expr { in_block_expr: true, .. } => parent
+                                .ancestors()
+                                .find(|it| ast::PathExpr::can_cast(it.kind()))
+                                .and_then(|p| {
+                                    let parent = p.parent()?;
+                                    if ast::StmtList::can_cast(parent.kind()) {
+                                        Some(p)
+                                    } else if ast::ExprStmt::can_cast(parent.kind()) {
+                                        Some(parent)
+                                    } else {
+                                        None
+                                    }
+                                }),
+                            PathKind::Item { .. } => {
+                                parent.ancestors().find(|it| ast::MacroCall::can_cast(it.kind()))
+                            }
+                            _ => None,
+                        };
+                        if let Some(top) = top {
+                            if let Some(NodeOrToken::Node(error_node)) =
+                                syntax::algo::non_trivia_sibling(
+                                    top.into(),
+                                    syntax::Direction::Prev,
+                                )
+                            {
+                                if error_node.kind() == SyntaxKind::ERROR {
+                                    self.qualifier_ctx.unsafe_tok = error_node
+                                        .children_with_tokens()
+                                        .filter_map(NodeOrToken::into_token)
+                                        .find(|it| it.kind() == T![unsafe]);
+                                    self.qualifier_ctx.vis_node =
+                                        error_node.children().find_map(ast::Visibility::cast);
+                                }
+                            }
+                        }
+                    }
+                }
                 self.ident_ctx = IdentContext::NameRef(nameref_ctx);
                 self.pattern_ctx = pat_ctx;
             }
@@ -1145,12 +1197,54 @@ impl<'a> CompletionContext<'a> {
             }
         };
 
+        // We do not want to generate path completions when we are sandwiched between an item decl signature and its body.
+        // ex. trait Foo $0 {}
+        // in these cases parser recovery usually kicks in for our inserted identifier, causing it
+        // to either be parsed as an ExprStmt or a MacroCall, depending on whether it is in a block
+        // expression or an item list.
+        // The following code checks if the body is missing, if it is we either cut off the body
+        // from the item or it was missing in the first place
+        let inbetween_body_and_decl_check = |node: SyntaxNode| {
+            if let Some(NodeOrToken::Node(n)) =
+                syntax::algo::non_trivia_sibling(node.into(), syntax::Direction::Prev)
+            {
+                if let Some(item) = ast::Item::cast(n) {
+                    match item {
+                        ast::Item::Const(it) => it.body().is_none(),
+                        ast::Item::Enum(it) => it.variant_list().is_none(),
+                        ast::Item::ExternBlock(it) => it.extern_item_list().is_none(),
+                        ast::Item::Fn(it) => it.body().is_none(),
+                        ast::Item::Impl(it) => it.assoc_item_list().is_none(),
+                        ast::Item::Module(it) => it.item_list().is_none(),
+                        ast::Item::Static(it) => it.body().is_none(),
+                        ast::Item::Struct(it) => it.field_list().is_none(),
+                        ast::Item::Trait(it) => it.assoc_item_list().is_none(),
+                        ast::Item::TypeAlias(it) => it.ty().is_none(),
+                        ast::Item::Union(it) => it.record_field_list().is_none(),
+                        _ => false,
+                    }
+                } else {
+                    false
+                }
+            } else {
+                false
+            }
+        };
+
         let kind = path.syntax().ancestors().find_map(|it| {
             // using Option<Option<PathKind>> as extra controlflow
             let kind = match_ast! {
                 match it {
                     ast::PathType(_) => Some(PathKind::Type),
                     ast::PathExpr(it) => {
+                        if let Some(p) = it.syntax().parent() {
+                            if ast::ExprStmt::can_cast(p.kind()) {
+                                if inbetween_body_and_decl_check(p) {
+                                    return Some(None);
+                                }
+                            }
+                        }
+
                         fill_record_expr(it.syntax());
 
                         path_ctx.has_call_parens = it.syntax().parent().map_or(false, |it| ast::CallExpr::can_cast(it.kind()));
@@ -1173,6 +1267,10 @@ impl<'a> CompletionContext<'a> {
                         Some(PathKind::Pat)
                     },
                     ast::MacroCall(it) => {
+                        if inbetween_body_and_decl_check(it.syntax().clone()) {
+                            return Some(None);
+                        }
+
                         path_ctx.has_macro_bang = it.excl_token().is_some();
                         let parent = it.syntax().parent();
                         match parent.as_ref().map(|it| it.kind()) {

crates/ide-completion/src/lib.rs

@@ -143,15 +143,15 @@ pub fn completions(
     db: &RootDatabase,
     config: &CompletionConfig,
     position: FilePosition,
-    trigger_character: Option<&str>,
+    trigger_character: Option<char>,
 ) -> Option<Completions> {
     let ctx = &CompletionContext::new(db, position, config)?;
     let mut acc = Completions::default();
 
     {
         let acc = &mut acc;
         // prevent `(` from triggering unwanted completion noise
-        if trigger_character != Some("(") {
+        if trigger_character != Some('(') {
             completions::attribute::complete_attribute(acc, ctx);
             completions::attribute::complete_derive(acc, ctx);
             completions::attribute::complete_known_attribute_input(acc, ctx);

crates/ide-completion/src/tests.rs

@@ -88,7 +88,7 @@ pub(crate) fn completion_list_no_kw(ra_fixture: &str) -> String {
 
 pub(crate) fn completion_list_with_trigger_character(
     ra_fixture: &str,
-    trigger_character: Option<&str>,
+    trigger_character: Option<char>,
 ) -> String {
     completion_list_with_config(TEST_CONFIG, ra_fixture, true, trigger_character)
 }
@@ -97,7 +97,7 @@ fn completion_list_with_config(
     config: CompletionConfig,
     ra_fixture: &str,
     include_keywords: bool,
-    trigger_character: Option<&str>,
+    trigger_character: Option<char>,
 ) -> String {
     // filter out all but one builtintype completion for smaller test outputs
     let items = get_all_items(config, ra_fixture, trigger_character);
@@ -225,7 +225,7 @@ pub(crate) fn check_pattern_is_applicable(code: &str, check: impl FnOnce(SyntaxE
 pub(crate) fn get_all_items(
     config: CompletionConfig,
     code: &str,
-    trigger_character: Option<&str>,
+    trigger_character: Option<char>,
 ) -> Vec<CompletionItem> {
     let (db, position) = position(code);
     let res = crate::completions(&db, &config, position, trigger_character)

crates/ide-completion/src/tests/fn_param.rs

@@ -7,8 +7,8 @@ fn check(ra_fixture: &str, expect: Expect) {
     expect.assert_eq(&actual);
 }
 
-fn check_with_trigger_character(ra_fixture: &str, trigger_character: Option<&str>, expect: Expect) {
-    let actual = completion_list_with_trigger_character(ra_fixture, trigger_character);
+fn check_with_trigger_character(ra_fixture: &str, trigger_character: char, expect: Expect) {
+    let actual = completion_list_with_trigger_character(ra_fixture, Some(trigger_character));
     expect.assert_eq(&actual)
 }
 
@@ -124,7 +124,7 @@ fn trigger_by_l_paren() {
         r#"
 fn foo($0)
 "#,
-        Some("("),
+        '(',
         expect![[]],
     )
 }

crates/ide-completion/src/tests/item.rs

@@ -92,10 +92,7 @@ fn after_struct_name() {
     check(
         r"struct Struct $0",
         expect![[r#"
-            ma makro!(…)           macro_rules! makro
-            md module
             kw const
-            kw crate::
             kw enum
             kw extern
             kw fn
@@ -104,18 +101,13 @@ fn after_struct_name() {
             kw pub
             kw pub(crate)
             kw pub(super)
-            kw self::
             kw static
             kw struct
-            kw super::
             kw trait
             kw type
             kw union
             kw unsafe
             kw use
-            sn macro_rules
-            sn tfn (Test function)
-            sn tmod (Test module)
         "#]],
     );
 }
@@ -126,10 +118,7 @@ fn after_fn_name() {
     check(
         r"fn func() $0",
         expect![[r#"
-            ma makro!(…)           macro_rules! makro
-            md module
             kw const
-            kw crate::
             kw enum
             kw extern
             kw fn
@@ -138,18 +127,13 @@ fn after_fn_name() {
             kw pub
             kw pub(crate)
             kw pub(super)
-            kw self::
             kw static
             kw struct
-            kw super::
             kw trait
             kw type
             kw union
             kw unsafe
             kw use
-            sn macro_rules
-            sn tfn (Test function)
-            sn tmod (Test module)
         "#]],
     );
 }