From 9349769363624795db79a860b5a4b46fec9b0930 Mon Sep 17 00:00:00 2001 From: Max Heller Date: Thu, 27 Jul 2023 19:33:00 -0400 Subject: [PATCH 1/6] exclude non-identifier aliases from completion filtering text --- Cargo.lock | 1 + crates/ide-completion/Cargo.toml | 1 + crates/ide-completion/src/item.rs | 34 +++++++++++++++++++++++++++++-- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f8806794979e..f07c08a77bcc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -715,6 +715,7 @@ dependencies = [ "syntax", "test-utils", "text-edit", + "unicode-ident", ] [[package]] diff --git a/crates/ide-completion/Cargo.toml b/crates/ide-completion/Cargo.toml index 092fb303668f..c06ac55aae33 100644 --- a/crates/ide-completion/Cargo.toml +++ b/crates/ide-completion/Cargo.toml @@ -17,6 +17,7 @@ itertools = "0.10.5" once_cell = "1.17.0" smallvec.workspace = true +unicode-ident = "1.0.0" # local deps diff --git a/crates/ide-completion/src/item.rs b/crates/ide-completion/src/item.rs index e850f7bfdf3f..d5713db0a36b 100644 --- a/crates/ide-completion/src/item.rs +++ b/crates/ide-completion/src/item.rs @@ -427,9 +427,21 @@ impl Builder { let insert_text = self.insert_text.unwrap_or_else(|| label.to_string()); if !self.doc_aliases.is_empty() { - let doc_aliases = self.doc_aliases.into_iter().join(", "); + let doc_aliases = self.doc_aliases.iter().join(", "); label = SmolStr::from(format!("{label} (alias {doc_aliases})")); - lookup = SmolStr::from(format!("{lookup} {doc_aliases}")); + let lookup_doc_aliases = (self.doc_aliases.iter()) + // Don't include aliases in `lookup` that aren't valid identifiers as including + // them results in weird completion filtering behavior e.g. `Partial>` matching + // `PartialOrd` because it has an alias of ">". + .filter(|alias| { + let mut chars = alias.chars(); + chars.next().map(unicode_ident::is_xid_start).unwrap_or(false) + && chars.all(unicode_ident::is_xid_continue) + }) + .join(", "); + if !lookup_doc_aliases.is_empty() { + lookup = SmolStr::from(format!("{lookup} {lookup_doc_aliases}")); + } } if let [import_edit] = &*self.imports_to_add { // snippets can have multiple imports, but normal completions only have up to one @@ -553,9 +565,12 @@ impl Builder { #[cfg(test)] mod tests { + use ide_db::SymbolKind; use itertools::Itertools; use test_utils::assert_eq_text; + use crate::{CompletionItem, CompletionItemKind}; + use super::{ CompletionRelevance, CompletionRelevancePostfixMatch, CompletionRelevanceTypeMatch, }; @@ -630,4 +645,19 @@ mod tests { check_relevance_score_ordered(expected_relevance_order); } + + #[test] + fn exclude_non_identifier_aliases_from_lookup() { + let mut item = CompletionItem::new( + CompletionItemKind::SymbolKind(SymbolKind::Trait), + Default::default(), + "PartialOrd", + ); + let aliases = [">", "<", "<=", ">="]; + item.doc_aliases(aliases.iter().map(|&alias| alias.into()).collect()); + let item = item.build(&Default::default()); + for alias in aliases { + assert!(!item.lookup().contains(alias)); + } + } } From 047bc47ecd5f0769b5eaa4e56001b59bdfc5cdf1 Mon Sep 17 00:00:00 2001 From: Max Heller Date: Fri, 28 Jul 2023 09:22:22 -0400 Subject: [PATCH 2/6] Cleanup Co-authored-by: LowR --- crates/ide-completion/src/item.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ide-completion/src/item.rs b/crates/ide-completion/src/item.rs index d5713db0a36b..40e51201987b 100644 --- a/crates/ide-completion/src/item.rs +++ b/crates/ide-completion/src/item.rs @@ -435,7 +435,7 @@ impl Builder { // `PartialOrd` because it has an alias of ">". .filter(|alias| { let mut chars = alias.chars(); - chars.next().map(unicode_ident::is_xid_start).unwrap_or(false) + chars.next().is_some_and(unicode_ident::is_xid_start) && chars.all(unicode_ident::is_xid_continue) }) .join(", "); From bc2b70d678f0a99484b9353cf2fdb6a0e25a3db5 Mon Sep 17 00:00:00 2001 From: Max Heller Date: Fri, 28 Jul 2023 09:23:05 -0400 Subject: [PATCH 3/6] formatting --- crates/ide-completion/src/item.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/ide-completion/src/item.rs b/crates/ide-completion/src/item.rs index 40e51201987b..c7879d7bf3ad 100644 --- a/crates/ide-completion/src/item.rs +++ b/crates/ide-completion/src/item.rs @@ -429,7 +429,9 @@ impl Builder { if !self.doc_aliases.is_empty() { let doc_aliases = self.doc_aliases.iter().join(", "); label = SmolStr::from(format!("{label} (alias {doc_aliases})")); - let lookup_doc_aliases = (self.doc_aliases.iter()) + let lookup_doc_aliases = self + .doc_aliases + .iter() // Don't include aliases in `lookup` that aren't valid identifiers as including // them results in weird completion filtering behavior e.g. `Partial>` matching // `PartialOrd` because it has an alias of ">". From 4bb7702833f9c172694579adb53c830d5ba9ca2d Mon Sep 17 00:00:00 2001 From: Max Heller Date: Sat, 29 Jul 2023 11:39:59 -0400 Subject: [PATCH 4/6] `check_edit` test --- crates/ide-completion/src/tests.rs | 28 ++++++++++++++++++---- crates/ide-completion/src/tests/special.rs | 25 +++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/crates/ide-completion/src/tests.rs b/crates/ide-completion/src/tests.rs index 2464e8d5f817..63c7f789d3b6 100644 --- a/crates/ide-completion/src/tests.rs +++ b/crates/ide-completion/src/tests.rs @@ -23,6 +23,8 @@ mod type_pos; mod use_tree; mod visibility; +use std::ops::ControlFlow; + use expect_test::Expect; use hir::PrefixKind; use ide_db::{ @@ -185,11 +187,29 @@ pub(crate) fn check_edit_with_config( let (db, position) = position(ra_fixture_before); let completions: Vec = crate::completions(&db, &config, position, None).unwrap(); - let (completion,) = completions + let matching = completions .iter() - .filter(|it| it.lookup() == what) - .collect_tuple() - .unwrap_or_else(|| panic!("can't find {what:?} completion in {completions:#?}")); + // Match IDE behavior by considering completions as matching if `what` is a subsequence + // of the completion's lookup text. + .filter(|it| { + let mut lookup = it.lookup().chars(); + what.chars().all(|c| lookup.contains(&c)) + }) + // Select the first exact match if one exists, or the first subsequence match if not + .try_fold(None, |first_match, completion| { + let exact_match = completion.lookup() == what; + if exact_match { + ControlFlow::Break(completion) + } else { + ControlFlow::Continue(first_match.or(Some(completion))) + } + }); + let completion = match matching { + ControlFlow::Continue(first_match) => first_match + .unwrap_or_else(|| panic!("can't find {what:?} completion in {completions:#?}")), + ControlFlow::Break(exact_match) => exact_match, + }; + let mut actual = db.file_text(position.file_id).to_string(); let mut combined_edit = completion.text_edit.clone(); diff --git a/crates/ide-completion/src/tests/special.rs b/crates/ide-completion/src/tests/special.rs index 3824720839eb..a50184de533c 100644 --- a/crates/ide-completion/src/tests/special.rs +++ b/crates/ide-completion/src/tests/special.rs @@ -1280,3 +1280,28 @@ fn here_we_go() { "#]], ); } + +#[test] +fn completion_filtering_excludes_non_identifier_aliases() { + // Catch panic instead of using `#[should_panic]` as style check bans + // `#[should_panic]`. Making `check_edit` return a result would require + // a lot of test changes. + std::panic::catch_unwind(|| { + check_edit( + "Partial>", + r#" +#[doc(alias = ">")] +trait PartialOrd {} + +struct Foo Date: Sun, 30 Jul 2023 15:58:20 -0400 Subject: [PATCH 5/6] update tests --- crates/ide-completion/src/item.rs | 25 ++++--------------- crates/ide-completion/src/tests.rs | 28 ++++------------------ crates/ide-completion/src/tests/special.rs | 22 ++++++++--------- 3 files changed, 19 insertions(+), 56 deletions(-) diff --git a/crates/ide-completion/src/item.rs b/crates/ide-completion/src/item.rs index c7879d7bf3ad..92782d1e807d 100644 --- a/crates/ide-completion/src/item.rs +++ b/crates/ide-completion/src/item.rs @@ -440,9 +440,12 @@ impl Builder { chars.next().is_some_and(unicode_ident::is_xid_start) && chars.all(unicode_ident::is_xid_continue) }) - .join(", "); + // Deliberately concatenated without separators as adding separators e.g. + // `alias1, alias2` results in LSP clients continuing to display the completion even + // after typing a comma or space. + .join(""); if !lookup_doc_aliases.is_empty() { - lookup = SmolStr::from(format!("{lookup} {lookup_doc_aliases}")); + lookup = SmolStr::from(format!("{lookup}{lookup_doc_aliases}")); } } if let [import_edit] = &*self.imports_to_add { @@ -567,12 +570,9 @@ impl Builder { #[cfg(test)] mod tests { - use ide_db::SymbolKind; use itertools::Itertools; use test_utils::assert_eq_text; - use crate::{CompletionItem, CompletionItemKind}; - use super::{ CompletionRelevance, CompletionRelevancePostfixMatch, CompletionRelevanceTypeMatch, }; @@ -647,19 +647,4 @@ mod tests { check_relevance_score_ordered(expected_relevance_order); } - - #[test] - fn exclude_non_identifier_aliases_from_lookup() { - let mut item = CompletionItem::new( - CompletionItemKind::SymbolKind(SymbolKind::Trait), - Default::default(), - "PartialOrd", - ); - let aliases = [">", "<", "<=", ">="]; - item.doc_aliases(aliases.iter().map(|&alias| alias.into()).collect()); - let item = item.build(&Default::default()); - for alias in aliases { - assert!(!item.lookup().contains(alias)); - } - } } diff --git a/crates/ide-completion/src/tests.rs b/crates/ide-completion/src/tests.rs index 63c7f789d3b6..2464e8d5f817 100644 --- a/crates/ide-completion/src/tests.rs +++ b/crates/ide-completion/src/tests.rs @@ -23,8 +23,6 @@ mod type_pos; mod use_tree; mod visibility; -use std::ops::ControlFlow; - use expect_test::Expect; use hir::PrefixKind; use ide_db::{ @@ -187,29 +185,11 @@ pub(crate) fn check_edit_with_config( let (db, position) = position(ra_fixture_before); let completions: Vec = crate::completions(&db, &config, position, None).unwrap(); - let matching = completions + let (completion,) = completions .iter() - // Match IDE behavior by considering completions as matching if `what` is a subsequence - // of the completion's lookup text. - .filter(|it| { - let mut lookup = it.lookup().chars(); - what.chars().all(|c| lookup.contains(&c)) - }) - // Select the first exact match if one exists, or the first subsequence match if not - .try_fold(None, |first_match, completion| { - let exact_match = completion.lookup() == what; - if exact_match { - ControlFlow::Break(completion) - } else { - ControlFlow::Continue(first_match.or(Some(completion))) - } - }); - let completion = match matching { - ControlFlow::Continue(first_match) => first_match - .unwrap_or_else(|| panic!("can't find {what:?} completion in {completions:#?}")), - ControlFlow::Break(exact_match) => exact_match, - }; - + .filter(|it| it.lookup() == what) + .collect_tuple() + .unwrap_or_else(|| panic!("can't find {what:?} completion in {completions:#?}")); let mut actual = db.file_text(position.file_id).to_string(); let mut combined_edit = completion.text_edit.clone(); diff --git a/crates/ide-completion/src/tests/special.rs b/crates/ide-completion/src/tests/special.rs index a50184de533c..e80a289049f1 100644 --- a/crates/ide-completion/src/tests/special.rs +++ b/crates/ide-completion/src/tests/special.rs @@ -1282,26 +1282,24 @@ fn here_we_go() { } #[test] -fn completion_filtering_excludes_non_identifier_aliases() { - // Catch panic instead of using `#[should_panic]` as style check bans - // `#[should_panic]`. Making `check_edit` return a result would require - // a lot of test changes. - std::panic::catch_unwind(|| { - check_edit( - "Partial>", - r#" +fn completion_filtering_excludes_non_identifier_doc_aliases() { + check_edit( + "PartialOrdcmporder", + r#" #[doc(alias = ">")] +#[doc(alias = "cmp")] +#[doc(alias = "order")] trait PartialOrd {} struct Foo Date: Tue, 1 Aug 2023 18:18:12 -0400 Subject: [PATCH 6/6] remove unicode-ident dependency --- Cargo.lock | 1 - crates/ide-completion/Cargo.toml | 1 - crates/ide-completion/src/item.rs | 4 ++-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f07c08a77bcc..f8806794979e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -715,7 +715,6 @@ dependencies = [ "syntax", "test-utils", "text-edit", - "unicode-ident", ] [[package]] diff --git a/crates/ide-completion/Cargo.toml b/crates/ide-completion/Cargo.toml index c06ac55aae33..092fb303668f 100644 --- a/crates/ide-completion/Cargo.toml +++ b/crates/ide-completion/Cargo.toml @@ -17,7 +17,6 @@ itertools = "0.10.5" once_cell = "1.17.0" smallvec.workspace = true -unicode-ident = "1.0.0" # local deps diff --git a/crates/ide-completion/src/item.rs b/crates/ide-completion/src/item.rs index 92782d1e807d..0309952c29a8 100644 --- a/crates/ide-completion/src/item.rs +++ b/crates/ide-completion/src/item.rs @@ -437,8 +437,8 @@ impl Builder { // `PartialOrd` because it has an alias of ">". .filter(|alias| { let mut chars = alias.chars(); - chars.next().is_some_and(unicode_ident::is_xid_start) - && chars.all(unicode_ident::is_xid_continue) + chars.next().is_some_and(char::is_alphabetic) + && chars.all(|c| c.is_alphanumeric() || c == '_') }) // Deliberately concatenated without separators as adding separators e.g. // `alias1, alias2` results in LSP clients continuing to display the completion even