Skip to content

Commit

Permalink
Auto merge of rust-lang#12412 - yue4u:fix/visibility-completion, r=Ve…
Browse files Browse the repository at this point in the history
…ykril

fix: Retrigger visibility completion after parentheses

close rust-lang#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 rust-lang#12144
  • Loading branch information
bors committed May 30, 2022
2 parents f94fa62 + 1b5f046 commit e4ead8a
Show file tree
Hide file tree
Showing 9 changed files with 103 additions and 57 deletions.
52 changes: 28 additions & 24 deletions crates/ide-completion/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions crates/ide-completion/src/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,15 +410,15 @@ 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);
}

#[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);
Expand Down
27 changes: 20 additions & 7 deletions crates/ide-completion/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()))
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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();
Expand Down
18 changes: 17 additions & 1 deletion crates/ide-completion/src/tests/fn_param.rs
Original file line number Diff line number Diff line change
@@ -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(
Expand Down Expand Up @@ -113,6 +118,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(
Expand Down
10 changes: 8 additions & 2 deletions crates/ide-completion/src/tests/visibility.rs
Original file line number Diff line number Diff line change
@@ -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
Expand Down
5 changes: 4 additions & 1 deletion crates/ide/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 6 additions & 1 deletion crates/rust-analyzer/src/caps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down
33 changes: 16 additions & 17 deletions crates/rust-analyzer/src/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down
4 changes: 2 additions & 2 deletions crates/rust-analyzer/src/integrated_benchmarks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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();
}
}

Expand Down

0 comments on commit e4ead8a

Please sign in to comment.