From a9612666f82718380048895f07b35bd9f3c95a04 Mon Sep 17 00:00:00 2001 From: Eduardo Canellas Date: Tue, 4 Jan 2022 15:03:46 -0300 Subject: [PATCH 1/4] fix(completions): improve fn_param - insert commas around when necessary - only suggest `self` completions when param list is empty - stop suggesting completions for identifiers which are already on the param list --- .../src/completions/fn_param.rs | 79 ++++++++++++++----- crates/ide_completion/src/tests/fn_param.rs | 20 +++-- 2 files changed, 75 insertions(+), 24 deletions(-) diff --git a/crates/ide_completion/src/completions/fn_param.rs b/crates/ide_completion/src/completions/fn_param.rs index 5acda46498..e73e39d136 100644 --- a/crates/ide_completion/src/completions/fn_param.rs +++ b/crates/ide_completion/src/completions/fn_param.rs @@ -3,7 +3,7 @@ use rustc_hash::FxHashMap; use syntax::{ ast::{self, HasModuleItem}, - match_ast, AstNode, + match_ast, AstNode, SyntaxKind, }; use crate::{ @@ -16,24 +16,22 @@ use crate::{ /// `spam: &mut Spam` insert text/label and `spam` lookup string will be /// suggested. pub(crate) fn complete_fn_param(acc: &mut Completions, ctx: &CompletionContext) -> Option<()> { - if !matches!(ctx.pattern_ctx, Some(PatternContext { is_param: Some(ParamKind::Function), .. })) - { + let param_of_fn = + matches!(ctx.pattern_ctx, Some(PatternContext { is_param: Some(ParamKind::Function), .. })); + + if !param_of_fn { return None; } - let mut params = FxHashMap::default(); + let mut file_params = FxHashMap::default(); - let me = ctx.token.ancestors().find_map(ast::Fn::cast); - let mut process_fn = |func: ast::Fn| { - if Some(&func) == me.as_ref() { - return; - } - func.param_list().into_iter().flat_map(|it| it.params()).for_each(|param| { + let mut extract_params = |f: ast::Fn| { + f.param_list().into_iter().flat_map(|it| it.params()).for_each(|param| { if let Some(pat) = param.pat() { // FIXME: We should be able to turn these into SmolStr without having to allocate a String - let text = param.syntax().text().to_string(); - let lookup = pat.syntax().text().to_string(); - params.entry(text).or_insert(lookup); + let whole_param = param.syntax().text().to_string(); + let binding = pat.syntax().text().to_string(); + file_params.entry(whole_param).or_insert(binding); } }); }; @@ -44,32 +42,77 @@ pub(crate) fn complete_fn_param(acc: &mut Completions, ctx: &CompletionContext) ast::SourceFile(it) => it.items().filter_map(|item| match item { ast::Item::Fn(it) => Some(it), _ => None, - }).for_each(&mut process_fn), + }).for_each(&mut extract_params), ast::ItemList(it) => it.items().filter_map(|item| match item { ast::Item::Fn(it) => Some(it), _ => None, - }).for_each(&mut process_fn), + }).for_each(&mut extract_params), ast::AssocItemList(it) => it.assoc_items().filter_map(|item| match item { ast::AssocItem::Fn(it) => Some(it), _ => None, - }).for_each(&mut process_fn), + }).for_each(&mut extract_params), _ => continue, } }; } + let function = ctx.token.ancestors().find_map(ast::Fn::cast)?; + let param_list = function.param_list()?; + + remove_duplicated(&mut file_params, param_list.params())?; + let self_completion_items = ["self", "&self", "mut self", "&mut self"]; - if ctx.impl_def.is_some() && me?.param_list()?.params().next().is_none() { + if should_add_self_completions(ctx, param_list) { self_completion_items.into_iter().for_each(|self_item| { add_new_item_to_acc(ctx, acc, self_item.to_string(), self_item.to_string()) }); } - params.into_iter().for_each(|(label, lookup)| add_new_item_to_acc(ctx, acc, label, lookup)); + file_params.into_iter().try_for_each(|(whole_param, binding)| { + Some(add_new_item_to_acc(ctx, acc, surround_with_commas(ctx, whole_param)?, binding)) + })?; Some(()) } +fn remove_duplicated( + file_params: &mut FxHashMap, + mut fn_params: ast::AstChildren, +) -> Option<()> { + fn_params.try_for_each(|param| { + let whole_param = param.syntax().text().to_string(); + file_params.remove(&whole_param); + + let binding = param.pat()?.syntax().text().to_string(); + + file_params.retain(|_, v| v != &binding); + Some(()) + }) +} + +fn should_add_self_completions(ctx: &CompletionContext, param_list: ast::ParamList) -> bool { + let inside_impl = ctx.impl_def.is_some(); + let no_params = param_list.params().next().is_none() && param_list.self_param().is_none(); + + inside_impl && no_params +} + +fn surround_with_commas(ctx: &CompletionContext, param: String) -> Option { + let end_of_param_list = matches!(ctx.token.next_token()?.kind(), SyntaxKind::R_PAREN); + let trailing = if end_of_param_list { "" } else { "," }; + + let previous_token = if matches!(ctx.token.kind(), SyntaxKind::IDENT | SyntaxKind::WHITESPACE) { + ctx.previous_token.as_ref()? + } else { + &ctx.token + }; + + let needs_leading = !matches!(previous_token.kind(), SyntaxKind::L_PAREN | SyntaxKind::COMMA); + let leading = if needs_leading { ", " } else { "" }; + + Some(format!("{}{}{}", leading, param, trailing)) +} + fn add_new_item_to_acc( ctx: &CompletionContext, acc: &mut Completions, diff --git a/crates/ide_completion/src/tests/fn_param.rs b/crates/ide_completion/src/tests/fn_param.rs index 8a07aefafe..940cecf395 100644 --- a/crates/ide_completion/src/tests/fn_param.rs +++ b/crates/ide_completion/src/tests/fn_param.rs @@ -46,7 +46,20 @@ fn bar(file_id: usize) {} fn baz(file$0 id: u32) {} "#, expect![[r#" - bn file_id: usize + bn file_id: usize, + kw mut + "#]], + ); +} + +#[test] +fn repeated_param_name() { + check( + r#" +fn foo(file_id: usize) {} +fn bar(file_id: u32, $0) {} +"#, + expect![[r#" kw mut "#]], ); @@ -126,7 +139,6 @@ impl A { #[test] fn in_impl_after_self() { - // FIXME: self completions should not be here check( r#" struct A {} @@ -137,10 +149,6 @@ impl A { } "#, expect![[r#" - bn self - bn &self - bn mut self - bn &mut self bn file_id: usize kw mut sp Self From 838944b3870deefce57950d1604de9307d61d828 Mon Sep 17 00:00:00 2001 From: Eduardo Canellas Date: Tue, 4 Jan 2022 15:30:30 -0300 Subject: [PATCH 2/4] improve logic for trailing comma addition --- crates/ide_completion/src/completions/fn_param.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/ide_completion/src/completions/fn_param.rs b/crates/ide_completion/src/completions/fn_param.rs index e73e39d136..2fb5336f87 100644 --- a/crates/ide_completion/src/completions/fn_param.rs +++ b/crates/ide_completion/src/completions/fn_param.rs @@ -98,8 +98,18 @@ fn should_add_self_completions(ctx: &CompletionContext, param_list: ast::ParamLi } fn surround_with_commas(ctx: &CompletionContext, param: String) -> Option { - let end_of_param_list = matches!(ctx.token.next_token()?.kind(), SyntaxKind::R_PAREN); - let trailing = if end_of_param_list { "" } else { "," }; + let next_token = { + let t = ctx.token.next_token()?; + if !matches!(t.kind(), SyntaxKind::WHITESPACE) { + t + } else { + t.next_token()? + } + }; + + let trailing_comma_missing = matches!(next_token.kind(), SyntaxKind::IDENT); + let trailing = if trailing_comma_missing { "," } else { "" }; + dbg!(&ctx.token.next_token()?); let previous_token = if matches!(ctx.token.kind(), SyntaxKind::IDENT | SyntaxKind::WHITESPACE) { ctx.previous_token.as_ref()? From 30b7e92afaa6dc6b276d60b8e7b47485ca7c2ee3 Mon Sep 17 00:00:00 2001 From: Eduardo Canellas Date: Tue, 4 Jan 2022 15:37:10 -0300 Subject: [PATCH 3/4] remove forgotten dbg macro --- crates/ide_completion/src/completions/fn_param.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/ide_completion/src/completions/fn_param.rs b/crates/ide_completion/src/completions/fn_param.rs index 2fb5336f87..3dd4aca9ce 100644 --- a/crates/ide_completion/src/completions/fn_param.rs +++ b/crates/ide_completion/src/completions/fn_param.rs @@ -109,7 +109,6 @@ fn surround_with_commas(ctx: &CompletionContext, param: String) -> Option Date: Tue, 4 Jan 2022 19:30:57 -0300 Subject: [PATCH 4/4] refactor: apply review suggestions --- .../src/completions/fn_param.rs | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/crates/ide_completion/src/completions/fn_param.rs b/crates/ide_completion/src/completions/fn_param.rs index 3dd4aca9ce..bb4ce0a24d 100644 --- a/crates/ide_completion/src/completions/fn_param.rs +++ b/crates/ide_completion/src/completions/fn_param.rs @@ -59,7 +59,7 @@ pub(crate) fn complete_fn_param(acc: &mut Completions, ctx: &CompletionContext) let function = ctx.token.ancestors().find_map(ast::Fn::cast)?; let param_list = function.param_list()?; - remove_duplicated(&mut file_params, param_list.params())?; + remove_duplicated(&mut file_params, param_list.params()); let self_completion_items = ["self", "&self", "mut self", "&mut self"]; if should_add_self_completions(ctx, param_list) { @@ -69,7 +69,7 @@ pub(crate) fn complete_fn_param(acc: &mut Completions, ctx: &CompletionContext) } file_params.into_iter().try_for_each(|(whole_param, binding)| { - Some(add_new_item_to_acc(ctx, acc, surround_with_commas(ctx, whole_param)?, binding)) + Some(add_new_item_to_acc(ctx, acc, surround_with_commas(ctx, whole_param), binding)) })?; Some(()) @@ -77,16 +77,16 @@ pub(crate) fn complete_fn_param(acc: &mut Completions, ctx: &CompletionContext) fn remove_duplicated( file_params: &mut FxHashMap, - mut fn_params: ast::AstChildren, -) -> Option<()> { - fn_params.try_for_each(|param| { + fn_params: ast::AstChildren, +) { + fn_params.for_each(|param| { let whole_param = param.syntax().text().to_string(); file_params.remove(&whole_param); - let binding = param.pat()?.syntax().text().to_string(); - - file_params.retain(|_, v| v != &binding); - Some(()) + if let Some(pattern) = param.pat() { + let binding = pattern.syntax().text().to_string(); + file_params.retain(|_, v| v != &binding); + } }) } @@ -97,13 +97,20 @@ fn should_add_self_completions(ctx: &CompletionContext, param_list: ast::ParamLi inside_impl && no_params } -fn surround_with_commas(ctx: &CompletionContext, param: String) -> Option { +fn surround_with_commas(ctx: &CompletionContext, param: String) -> String { + match fallible_surround_with_commas(ctx, ¶m) { + Some(surrounded) => surrounded, + // fallback to the original parameter + None => param, + } +} + +fn fallible_surround_with_commas(ctx: &CompletionContext, param: &str) -> Option { let next_token = { let t = ctx.token.next_token()?; - if !matches!(t.kind(), SyntaxKind::WHITESPACE) { - t - } else { - t.next_token()? + match t.kind() { + SyntaxKind::WHITESPACE => t.next_token()?, + _ => t, } };