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

Do not spawn redundant hints #6262

Merged
merged 5 commits into from
Oct 17, 2020
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
49 changes: 49 additions & 0 deletions crates/ide/src/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@ pub(crate) fn completions(
) -> Option<Completions> {
let ctx = CompletionContext::new(db, position, config)?;

if ctx.no_completion_required() {
// No work required here.
return None;
}

let mut acc = Completions::default();
complete_attribute::complete_attribute(&mut acc, &ctx);
complete_fn_param::complete_fn_param(&mut acc, &ctx);
Expand Down Expand Up @@ -157,6 +162,23 @@ mod tests {
panic!("completion detail not found: {}", expected.detail)
}

fn check_no_completion(ra_fixture: &str) {
let (analysis, position) = fixture::position(ra_fixture);
let config = CompletionConfig::default();
analysis.completions(&config, position).unwrap();

let completions: Option<Vec<String>> = analysis
.completions(&config, position)
.unwrap()
.and_then(|completions| if completions.is_empty() { None } else { Some(completions) })
.map(|completions| {
completions.into_iter().map(|completion| format!("{:?}", completion)).collect()
});

// `assert_eq` instead of `assert!(completions.is_none())` to get the list of completions if test will panic.
assert_eq!(completions, None, "Completions were generated, but weren't expected");
}

#[test]
fn test_completion_detail_from_macro_generated_struct_fn_doc_attr() {
check_detail_and_documentation(
Expand Down Expand Up @@ -208,4 +230,31 @@ mod tests {
DetailAndDocumentation { detail: "fn foo(&self)", documentation: " Do the foo" },
);
}

#[test]
fn test_no_completions_required() {
// There must be no hint for 'in' keyword.
check_no_completion(
r#"
fn foo() {
for i i<|>
}
"#,
);
// After 'in' keyword hints may be spawned.
check_detail_and_documentation(
r#"
/// Do the foo
fn foo() -> &'static str { "foo" }

fn bar() {
for c in fo<|>
}
"#,
DetailAndDocumentation {
detail: "fn foo() -> &'static str",
documentation: "Do the foo",
},
);
}
}
27 changes: 23 additions & 4 deletions crates/ide/src/completion/completion_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ use crate::{
call_info::ActiveParameter,
completion::{
patterns::{
has_bind_pat_parent, has_block_expr_parent, has_field_list_parent,
has_impl_as_prev_sibling, has_impl_parent, has_item_list_or_source_file_parent,
has_ref_parent, has_trait_as_prev_sibling, has_trait_parent, if_is_prev,
is_in_loop_body, is_match_arm, unsafe_is_prev,
fn_is_prev, for_is_prev2, has_bind_pat_parent, has_block_expr_parent,
has_field_list_parent, has_impl_as_prev_sibling, has_impl_parent,
has_item_list_or_source_file_parent, has_ref_parent, has_trait_as_prev_sibling,
has_trait_parent, if_is_prev, inside_impl_trait_block, is_in_loop_body, is_match_arm,
unsafe_is_prev,
},
CompletionConfig,
},
Expand Down Expand Up @@ -86,11 +87,14 @@ pub(crate) struct CompletionContext<'a> {
pub(super) in_loop_body: bool,
pub(super) has_trait_parent: bool,
pub(super) has_impl_parent: bool,
pub(super) inside_impl_trait_block: bool,
pub(super) has_field_list_parent: bool,
pub(super) trait_as_prev_sibling: bool,
pub(super) impl_as_prev_sibling: bool,
pub(super) is_match_arm: bool,
pub(super) has_item_list_or_source_file_parent: bool,
pub(super) for_is_prev2: bool,
pub(super) fn_is_prev: bool,
pub(super) locals: Vec<(String, Local)>,
}

Expand Down Expand Up @@ -168,12 +172,15 @@ impl<'a> CompletionContext<'a> {
block_expr_parent: false,
has_trait_parent: false,
has_impl_parent: false,
inside_impl_trait_block: false,
has_field_list_parent: false,
trait_as_prev_sibling: false,
impl_as_prev_sibling: false,
if_is_prev: false,
is_match_arm: false,
has_item_list_or_source_file_parent: false,
for_is_prev2: false,
fn_is_prev: false,
locals,
};

Expand Down Expand Up @@ -221,6 +228,15 @@ impl<'a> CompletionContext<'a> {
Some(ctx)
}

/// Checks whether completions in that particular case don't make much sense.
/// Examples:
/// - `fn <|>` -- we expect function name, it's unlikely that "hint" will be helpful.
/// Exception for this case is `impl Trait for Foo`, where we would like to hint trait method names.
/// - `for _ i<|>` -- obviously, it'll be "in" keyword.
pub(crate) fn no_completion_required(&self) -> bool {
(self.fn_is_prev && !self.inside_impl_trait_block) || self.for_is_prev2
}

/// The range of the identifier that is being completed.
pub(crate) fn source_range(&self) -> TextRange {
// check kind of macro-expanded token, but use range of original token
Expand All @@ -244,6 +260,7 @@ impl<'a> CompletionContext<'a> {
self.in_loop_body = is_in_loop_body(syntax_element.clone());
self.has_trait_parent = has_trait_parent(syntax_element.clone());
self.has_impl_parent = has_impl_parent(syntax_element.clone());
self.inside_impl_trait_block = inside_impl_trait_block(syntax_element.clone());
self.has_field_list_parent = has_field_list_parent(syntax_element.clone());
self.impl_as_prev_sibling = has_impl_as_prev_sibling(syntax_element.clone());
self.trait_as_prev_sibling = has_trait_as_prev_sibling(syntax_element.clone());
Expand All @@ -253,6 +270,8 @@ impl<'a> CompletionContext<'a> {
self.mod_declaration_under_caret =
find_node_at_offset::<ast::Module>(&file_with_fake_ident, offset)
.filter(|module| module.item_list().is_none());
self.for_is_prev2 = for_is_prev2(syntax_element.clone());
self.fn_is_prev = fn_is_prev(syntax_element.clone());
}

fn fill(
Expand Down
48 changes: 47 additions & 1 deletion crates/ide/src/completion/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use syntax::{
};

#[cfg(test)]
use crate::completion::test_utils::check_pattern_is_applicable;
use crate::completion::test_utils::{check_pattern_is_applicable, check_pattern_is_not_applicable};

pub(crate) fn has_trait_parent(element: SyntaxElement) -> bool {
not_same_range_ancestor(element)
Expand All @@ -34,6 +34,25 @@ pub(crate) fn has_impl_parent(element: SyntaxElement) -> bool {
fn test_has_impl_parent() {
check_pattern_is_applicable(r"impl A { f<|> }", has_impl_parent);
}

pub(crate) fn inside_impl_trait_block(element: SyntaxElement) -> bool {
// Here we search `impl` keyword up through the all ancestors, unlike in `has_impl_parent`,
// where we only check the first parent with different text range.
element
.ancestors()
.find(|it| it.kind() == IMPL)
.map(|it| ast::Impl::cast(it).unwrap())
.map(|it| it.trait_().is_some())
.unwrap_or(false)
}
#[test]
fn test_inside_impl_trait_block() {
check_pattern_is_applicable(r"impl Foo for Bar { f<|> }", inside_impl_trait_block);
check_pattern_is_applicable(r"impl Foo for Bar { fn f<|> }", inside_impl_trait_block);
check_pattern_is_not_applicable(r"impl A { f<|> }", inside_impl_trait_block);
check_pattern_is_not_applicable(r"impl A { fn f<|> }", inside_impl_trait_block);
}

pub(crate) fn has_field_list_parent(element: SyntaxElement) -> bool {
not_same_range_ancestor(element).filter(|it| it.kind() == RECORD_FIELD_LIST).is_some()
}
Expand Down Expand Up @@ -116,6 +135,33 @@ pub(crate) fn if_is_prev(element: SyntaxElement) -> bool {
.is_some()
}

pub(crate) fn fn_is_prev(element: SyntaxElement) -> bool {
element
.into_token()
.and_then(|it| previous_non_trivia_token(it))
.filter(|it| it.kind() == FN_KW)
.is_some()
}
#[test]
fn test_fn_is_prev() {
check_pattern_is_applicable(r"fn l<|>", fn_is_prev);
}

/// Check if the token previous to the previous one is `for`.
/// For example, `for _ i<|>` => true.
pub(crate) fn for_is_prev2(element: SyntaxElement) -> bool {
element
.into_token()
.and_then(|it| previous_non_trivia_token(it))
.and_then(|it| previous_non_trivia_token(it))
.filter(|it| it.kind() == FOR_KW)
.is_some()
}
#[test]
fn test_for_is_prev2() {
check_pattern_is_applicable(r"for i i<|>", for_is_prev2);
}

#[test]
fn test_if_is_prev() {
check_pattern_is_applicable(r"if l<|>", if_is_prev);
Expand Down
12 changes: 12 additions & 0 deletions crates/ide/src/completion/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,18 @@ pub(crate) fn check_pattern_is_applicable(code: &str, check: fn(SyntaxElement) -
.unwrap();
}

pub(crate) fn check_pattern_is_not_applicable(code: &str, check: fn(SyntaxElement) -> bool) {
let (analysis, pos) = fixture::position(code);
analysis
.with_db(|db| {
let sema = Semantics::new(db);
let original_file = sema.parse(pos.file_id);
let token = original_file.syntax().token_at_offset(pos.offset).left_biased().unwrap();
assert!(!check(NodeOrToken::Token(token)));
})
.unwrap();
}

pub(crate) fn get_all_completion_items(
config: CompletionConfig,
code: &str,
Expand Down