Auto merge of #12412 - yue4u:fix/visibility-completion, r=Veykril

fix: Retrigger visibility completion after parentheses

close #12390

This PR add `(` to trigger_characters as discussed in original issue.

Some questions:

1. Is lsp's `ctx.trigger_character` from `params.context` is the same as `ctx.original_token` inside actually completions?
    1. If not what's the difference?
    2. if they are the same, it's unnecessary to pass it down from handler at all.
    3.  if they are the same, maybe we could parse it from fixture directly instead of using the `check_with_trigger_character` I added.
2. Some completion fixtures written as `($0)` ( https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-completion/src/tests/fn_param.rs#L105 as an example), If I understand correctly they are not invoked outside tests at all?
    1. using `ctx.original_token` directly would break these tests as well as parsing trigger_character from fixture for now.
    2. I think it make sense to allow `(` triggering these cases?
3. I hope this line up with #12144

9 files changed, 103 insertions, 57 deletions

diff --git a/crates/ide-completion/src/lib.rs b/crates/ide-completion/src/lib.rs
index 991ab6a4b86..2bbdc48e639 100644
--- a/crates/ide-completion/src/lib.rs
+++ b/crates/ide-completion/src/lib.rs
@@ -143,36 +143,40 @@ pub fn completions(
     db: &RootDatabase,
     config: &CompletionConfig,
     position: FilePosition,
+    trigger_character: Option<&str>,
 ) -> Option<Completions> {
     let ctx = &CompletionContext::new(db, position, config)?;
     let mut acc = Completions::default();
 
     {
         let acc = &mut acc;
-        completions::attribute::complete_attribute(acc, ctx);
-        completions::attribute::complete_derive(acc, ctx);
-        completions::attribute::complete_known_attribute_input(acc, ctx);
-        completions::dot::complete_dot(acc, ctx);
-        completions::expr::complete_expr_path(acc, ctx);
-        completions::extern_abi::complete_extern_abi(acc, ctx);
-        completions::flyimport::import_on_the_fly(acc, ctx);
-        completions::fn_param::complete_fn_param(acc, ctx);
-        completions::format_string::format_string(acc, ctx);
-        completions::item_list::complete_item_list(acc, ctx);
-        completions::keyword::complete_expr_keyword(acc, ctx);
-        completions::lifetime::complete_label(acc, ctx);
-        completions::lifetime::complete_lifetime(acc, ctx);
-        completions::mod_::complete_mod(acc, ctx);
-        completions::pattern::complete_pattern(acc, ctx);
-        completions::postfix::complete_postfix(acc, ctx);
-        completions::record::complete_record_literal(acc, ctx);
-        completions::record::complete_record(acc, ctx);
-        completions::snippet::complete_expr_snippet(acc, ctx);
-        completions::snippet::complete_item_snippet(acc, ctx);
-        completions::trait_impl::complete_trait_impl(acc, ctx);
-        completions::r#type::complete_type_path(acc, ctx);
-        completions::r#type::complete_inferred_type(acc, ctx);
-        completions::use_::complete_use_tree(acc, ctx);
+        // prevent `(` from triggering unwanted completion noise
+        if trigger_character != Some("(") {
+            completions::attribute::complete_attribute(acc, ctx);
+            completions::attribute::complete_derive(acc, ctx);
+            completions::attribute::complete_known_attribute_input(acc, ctx);
+            completions::dot::complete_dot(acc, ctx);
+            completions::expr::complete_expr_path(acc, ctx);
+            completions::extern_abi::complete_extern_abi(acc, ctx);
+            completions::flyimport::import_on_the_fly(acc, ctx);
+            completions::fn_param::complete_fn_param(acc, ctx);
+            completions::format_string::format_string(acc, ctx);
+            completions::item_list::complete_item_list(acc, ctx);
+            completions::keyword::complete_expr_keyword(acc, ctx);
+            completions::lifetime::complete_label(acc, ctx);
+            completions::lifetime::complete_lifetime(acc, ctx);
+            completions::mod_::complete_mod(acc, ctx);
+            completions::pattern::complete_pattern(acc, ctx);
+            completions::postfix::complete_postfix(acc, ctx);
+            completions::record::complete_record_literal(acc, ctx);
+            completions::record::complete_record(acc, ctx);
+            completions::snippet::complete_expr_snippet(acc, ctx);
+            completions::snippet::complete_item_snippet(acc, ctx);
+            completions::trait_impl::complete_trait_impl(acc, ctx);
+            completions::r#type::complete_type_path(acc, ctx);
+            completions::r#type::complete_inferred_type(acc, ctx);
+            completions::use_::complete_use_tree(acc, ctx);
+        }
         completions::vis::complete_vis_path(acc, ctx);
     }
 
diff --git a/crates/ide-completion/src/render.rs b/crates/ide-completion/src/render.rs
index f5092183857..d51bc517d65 100644
--- a/crates/ide-completion/src/render.rs
+++ b/crates/ide-completion/src/render.rs
@@ -410,7 +410,7 @@ mod tests {
 
     #[track_caller]
     fn check_relevance_for_kinds(ra_fixture: &str, kinds: &[CompletionItemKind], expect: Expect) {
-        let mut actual = get_all_items(TEST_CONFIG, ra_fixture);
+        let mut actual = get_all_items(TEST_CONFIG, ra_fixture, None);
         actual.retain(|it| kinds.contains(&it.kind()));
         actual.sort_by_key(|it| cmp::Reverse(it.relevance().score()));
         check_relevance_(actual, expect);
@@ -418,7 +418,7 @@ mod tests {
 
     #[track_caller]
     fn check_relevance(ra_fixture: &str, expect: Expect) {
-        let mut actual = get_all_items(TEST_CONFIG, ra_fixture);
+        let mut actual = get_all_items(TEST_CONFIG, ra_fixture, None);
         actual.retain(|it| it.kind() != CompletionItemKind::Snippet);
         actual.retain(|it| it.kind() != CompletionItemKind::Keyword);
         actual.retain(|it| it.kind() != CompletionItemKind::BuiltinType);
diff --git a/crates/ide-completion/src/tests.rs b/crates/ide-completion/src/tests.rs
index 742368ce81c..7625018058c 100644
--- a/crates/ide-completion/src/tests.rs
+++ b/crates/ide-completion/src/tests.rs
@@ -79,20 +79,28 @@ pub(crate) const TEST_CONFIG: CompletionConfig = CompletionConfig {
 };
 
 pub(crate) fn completion_list(ra_fixture: &str) -> String {
-    completion_list_with_config(TEST_CONFIG, ra_fixture, true)
+    completion_list_with_config(TEST_CONFIG, ra_fixture, true, None)
 }
 
 pub(crate) fn completion_list_no_kw(ra_fixture: &str) -> String {
-    completion_list_with_config(TEST_CONFIG, ra_fixture, false)
+    completion_list_with_config(TEST_CONFIG, ra_fixture, false, None)
+}
+
+pub(crate) fn completion_list_with_trigger_character(
+    ra_fixture: &str,
+    trigger_character: Option<&str>,
+) -> String {
+    completion_list_with_config(TEST_CONFIG, ra_fixture, true, trigger_character)
 }
 
 fn completion_list_with_config(
     config: CompletionConfig,
     ra_fixture: &str,
     include_keywords: bool,
+    trigger_character: Option<&str>,
 ) -> String {
     // filter out all but one builtintype completion for smaller test outputs
-    let items = get_all_items(config, ra_fixture);
+    let items = get_all_items(config, ra_fixture, trigger_character);
     let mut bt_seen = false;
     let items = items
         .into_iter()
@@ -126,7 +134,7 @@ pub(crate) fn do_completion_with_config(
     code: &str,
     kind: CompletionItemKind,
 ) -> Vec<CompletionItem> {
-    get_all_items(config, code)
+    get_all_items(config, code, None)
         .into_iter()
         .filter(|c| c.kind() == kind)
         .sorted_by(|l, r| l.label().cmp(r.label()))
@@ -173,7 +181,7 @@ pub(crate) fn check_edit_with_config(
     let ra_fixture_after = trim_indent(ra_fixture_after);
     let (db, position) = position(ra_fixture_before);
     let completions: Vec<CompletionItem> =
-        crate::completions(&db, &config, position).unwrap().into();
+        crate::completions(&db, &config, position, None).unwrap().into();
     let (completion,) = completions
         .iter()
         .filter(|it| it.lookup() == what)
@@ -214,9 +222,14 @@ pub(crate) fn check_pattern_is_applicable(code: &str, check: impl FnOnce(SyntaxE
     assert!(check(NodeOrToken::Token(token)));
 }
 
-pub(crate) fn get_all_items(config: CompletionConfig, code: &str) -> Vec<CompletionItem> {
+pub(crate) fn get_all_items(
+    config: CompletionConfig,
+    code: &str,
+    trigger_character: Option<&str>,
+) -> Vec<CompletionItem> {
     let (db, position) = position(code);
-    let res = crate::completions(&db, &config, position).map_or_else(Vec::default, Into::into);
+    let res = crate::completions(&db, &config, position, trigger_character)
+        .map_or_else(Vec::default, Into::into);
     // validate
     res.iter().for_each(|it| {
         let sr = it.source_range();
diff --git a/crates/ide-completion/src/tests/fn_param.rs b/crates/ide-completion/src/tests/fn_param.rs
index f8e145c7470..a08f3f6e88d 100644
--- a/crates/ide-completion/src/tests/fn_param.rs
+++ b/crates/ide-completion/src/tests/fn_param.rs
@@ -1,12 +1,17 @@
 use expect_test::{expect, Expect};
 
-use crate::tests::completion_list;
+use crate::tests::{completion_list, completion_list_with_trigger_character};
 
 fn check(ra_fixture: &str, expect: Expect) {
     let actual = completion_list(ra_fixture);
     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);
+    expect.assert_eq(&actual)
+}
+
 #[test]
 fn only_param() {
     check(
@@ -114,6 +119,17 @@ fn outer(text: &str) {
 }
 
 #[test]
+fn trigger_by_l_paren() {
+    check_with_trigger_character(
+        r#"
+fn foo($0)
+"#,
+        Some("("),
+        expect![[]],
+    )
+}
+
+#[test]
 fn shows_non_ident_pat_param() {
     check(
         r#"
diff --git a/crates/ide-completion/src/tests/visibility.rs b/crates/ide-completion/src/tests/visibility.rs
index b77c595eeec..64bbb444bf4 100644
--- a/crates/ide-completion/src/tests/visibility.rs
+++ b/crates/ide-completion/src/tests/visibility.rs
@@ -1,20 +1,26 @@
 //! Completion tests for visibility modifiers.
 use expect_test::{expect, Expect};
 
-use crate::tests::completion_list;
+use crate::tests::{completion_list, completion_list_with_trigger_character};
 
 fn check(ra_fixture: &str, expect: Expect) {
     let actual = completion_list(ra_fixture);
     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);
+    expect.assert_eq(&actual)
+}
+
 #[test]
 fn empty_pub() {
     cov_mark::check!(kw_completion_in);
-    check(
+    check_with_trigger_character(
         r#"
 pub($0)
 "#,
+        Some("("),
         expect![[r#"
             kw crate
             kw in
diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs
index e02a6919203..00b8c057e46 100644
--- a/crates/ide/src/lib.rs
+++ b/crates/ide/src/lib.rs
@@ -547,8 +547,11 @@ impl Analysis {
         &self,
         config: &CompletionConfig,
         position: FilePosition,
+        trigger_character: Option<&str>,
     ) -> Cancellable<Option<Vec<CompletionItem>>> {
-        self.with_db(|db| ide_completion::completions(db, config, position).map(Into::into))
+        self.with_db(|db| {
+            ide_completion::completions(db, config, position, trigger_character).map(Into::into)
+        })
     }
 
     /// Resolves additional completion data at the position given.
diff --git a/crates/rust-analyzer/src/caps.rs b/crates/rust-analyzer/src/caps.rs
index 58b1f29df54..7bff6a8b0e3 100644
--- a/crates/rust-analyzer/src/caps.rs
+++ b/crates/rust-analyzer/src/caps.rs
@@ -29,7 +29,12 @@ pub fn server_capabilities(config: &Config) -> ServerCapabilities {
         hover_provider: Some(HoverProviderCapability::Simple(true)),
         completion_provider: Some(CompletionOptions {
             resolve_provider: completions_resolve_provider(config.caps()),
-            trigger_characters: Some(vec![":".to_string(), ".".to_string(), "'".to_string()]),
+            trigger_characters: Some(vec![
+                ":".to_string(),
+                ".".to_string(),
+                "'".to_string(),
+                "(".to_string(),
+            ]),
             all_commit_characters: None,
             completion_item: None,
             work_done_progress_options: WorkDoneProgressOptions { work_done_progress: None },
diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs
index 6567157bcbb..a8e3e93b8f5 100644
--- a/crates/rust-analyzer/src/handlers.rs
+++ b/crates/rust-analyzer/src/handlers.rs
@@ -796,27 +796,26 @@ pub(crate) fn handle_completion(
     let _p = profile::span("handle_completion");
     let text_document_position = params.text_document_position.clone();
     let position = from_proto::file_position(&snap, params.text_document_position)?;
-    let completion_triggered_after_single_colon = {
-        let mut res = false;
-        if let Some(ctx) = params.context {
-            if ctx.trigger_character.as_deref() == Some(":") {
-                let source_file = snap.analysis.parse(position.file_id)?;
-                let left_token =
-                    source_file.syntax().token_at_offset(position.offset).left_biased();
-                match left_token {
-                    Some(left_token) => res = left_token.kind() == T![:],
-                    None => res = true,
-                }
-            }
+    let completion_trigger_character = params.context.and_then(|ctx| ctx.trigger_character);
+
+    if Some(":") == completion_trigger_character.as_deref() {
+        let source_file = snap.analysis.parse(position.file_id)?;
+        let left_token = source_file.syntax().token_at_offset(position.offset).left_biased();
+        let completion_triggered_after_single_colon = match left_token {
+            Some(left_token) => left_token.kind() == T![:],
+            None => true,
+        };
+        if completion_triggered_after_single_colon {
+            return Ok(None);
         }
-        res
-    };
-    if completion_triggered_after_single_colon {
-        return Ok(None);
     }
 
     let completion_config = &snap.config.completion();
-    let items = match snap.analysis.completions(completion_config, position)? {
+    let items = match snap.analysis.completions(
+        completion_config,
+        position,
+        completion_trigger_character.as_deref(),
+    )? {
         None => return Ok(None),
         Some(items) => items,
     };
diff --git a/crates/rust-analyzer/src/integrated_benchmarks.rs b/crates/rust-analyzer/src/integrated_benchmarks.rs
index 5f110274b2e..47cdd8dfc75 100644
--- a/crates/rust-analyzer/src/integrated_benchmarks.rs
+++ b/crates/rust-analyzer/src/integrated_benchmarks.rs
@@ -148,7 +148,7 @@ fn integrated_completion_benchmark() {
         };
         let position =
             FilePosition { file_id, offset: TextSize::try_from(completion_offset).unwrap() };
-        analysis.completions(&config, position).unwrap();
+        analysis.completions(&config, position, None).unwrap();
     }
 
     let completion_offset = {
@@ -185,7 +185,7 @@ fn integrated_completion_benchmark() {
         };
         let position =
             FilePosition { file_id, offset: TextSize::try_from(completion_offset).unwrap() };
-        analysis.completions(&config, position).unwrap();
+        analysis.completions(&config, position, None).unwrap();
     }
 }