Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Retrigger visibility completion after parentheses #12412

Merged
merged 1 commit into from
May 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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("(") {
Copy link
Contributor Author

@yue4u yue4u May 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could put this down to individual tests but I think it's harder to understand and will just mess the codebase more. Current approach does not scale very well if we want to allow ( to trigger more completions though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye I think putting it here for now is fine, depending on the final state of the completion architecture we might wanna move this somewhere else.

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![
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there some reasons for this not being a Vec<char>?

Copy link
Member

@Veykril Veykril May 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

json has no notion of characters, so strings are used

":".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(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think trigger_character is a state as the same as position and putting it into config would be weird.

)? {
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