From fbb9d69758b71b6eb1f9e039c968d9ad9f11370b Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 4 Jul 2021 16:50:02 +0300 Subject: [PATCH 1/2] feat: always prefer postfix snippets if there's exact textual match Note that, while we don't currently have a fuzzy-matching score, it makes sense to special-case postfix templates -- it's very annoying when `.not()` gets sorted before `.not`. We might want to move this infra to fuzzy matching, once we have that! --- .../ide_completion/src/completions/postfix.rs | 8 +- crates/ide_completion/src/item.rs | 20 ++++- crates/ide_completion/src/render.rs | 85 +++++++++++++++---- crates/rust-analyzer/src/to_proto.rs | 15 +++- 4 files changed, 105 insertions(+), 23 deletions(-) diff --git a/crates/ide_completion/src/completions/postfix.rs b/crates/ide_completion/src/completions/postfix.rs index 4e20ec003f4..aaa346eeae9 100644 --- a/crates/ide_completion/src/completions/postfix.rs +++ b/crates/ide_completion/src/completions/postfix.rs @@ -15,7 +15,7 @@ context::CompletionContext, item::{Builder, CompletionKind}, patterns::ImmediateLocation, - CompletionItem, CompletionItemKind, Completions, + CompletionItem, CompletionItemKind, CompletionRelevance, Completions, }; pub(crate) fn complete_postfix(acc: &mut Completions, ctx: &CompletionContext) { @@ -299,6 +299,12 @@ fn postfix_snippet( }; let mut item = CompletionItem::new(CompletionKind::Postfix, ctx.source_range(), label); item.detail(detail).kind(CompletionItemKind::Snippet).snippet_edit(cap, edit); + if ctx.original_token.text() == label { + let mut relevance = CompletionRelevance::default(); + relevance.exact_postfix_snippet_match = true; + item.set_relevance(relevance); + } + item } diff --git a/crates/ide_completion/src/item.rs b/crates/ide_completion/src/item.rs index 9f808e7628a..96d3fcf5916 100644 --- a/crates/ide_completion/src/item.rs +++ b/crates/ide_completion/src/item.rs @@ -144,6 +144,15 @@ pub struct CompletionRelevance { /// } /// ``` pub is_local: bool, + /// This is set in cases like these: + /// + /// ``` + /// (a > b).not$0 + /// ``` + /// + /// Basically, we want to guarantee that postfix snippets always takes + /// precedence over everything else. + pub exact_postfix_snippet_match: bool, } #[derive(Debug, Clone, Copy, Eq, PartialEq)] @@ -194,7 +203,9 @@ pub fn score(&self) -> u32 { if self.is_local { score += 1; } - + if self.exact_postfix_snippet_match { + score += 100; + } score } @@ -598,6 +609,13 @@ fn relevance_score() { exact_name_match: true, type_match: Some(CompletionRelevanceTypeMatch::Exact), is_local: true, + ..CompletionRelevance::default() + }], + vec![CompletionRelevance { + exact_name_match: false, + type_match: None, + is_local: false, + exact_postfix_snippet_match: true, }], ]; diff --git a/crates/ide_completion/src/render.rs b/crates/ide_completion/src/render.rs index 1a9b6212af5..3ee6831dc8b 100644 --- a/crates/ide_completion/src/render.rs +++ b/crates/ide_completion/src/render.rs @@ -339,32 +339,22 @@ mod tests { CompletionKind, CompletionRelevance, }; + #[track_caller] fn check(ra_fixture: &str, expect: Expect) { let actual = do_completion(ra_fixture, CompletionKind::Reference); expect.assert_debug_eq(&actual); } + #[track_caller] fn check_relevance(ra_fixture: &str, expect: Expect) { - fn display_relevance(relevance: CompletionRelevance) -> String { - let relevance_factors = vec![ - (relevance.type_match == Some(CompletionRelevanceTypeMatch::Exact), "type"), - ( - relevance.type_match == Some(CompletionRelevanceTypeMatch::CouldUnify), - "type_could_unify", - ), - (relevance.exact_name_match, "name"), - (relevance.is_local, "local"), - ] - .into_iter() - .filter_map(|(cond, desc)| if cond { Some(desc) } else { None }) - .join("+"); - - format!("[{}]", relevance_factors) - } + check_relevance_for_kinds(&[CompletionKind::Reference], ra_fixture, expect) + } + #[track_caller] + fn check_relevance_for_kinds(kinds: &[CompletionKind], ra_fixture: &str, expect: Expect) { let actual = get_all_items(TEST_CONFIG, ra_fixture) .into_iter() - .filter(|it| it.completion_kind == CompletionKind::Reference) + .filter(|it| kinds.contains(&it.completion_kind)) .flat_map(|it| { let mut items = vec![]; @@ -384,6 +374,24 @@ fn display_relevance(relevance: CompletionRelevance) -> String { .collect::(); expect.assert_eq(&actual); + + fn display_relevance(relevance: CompletionRelevance) -> String { + let relevance_factors = vec![ + (relevance.type_match == Some(CompletionRelevanceTypeMatch::Exact), "type"), + ( + relevance.type_match == Some(CompletionRelevanceTypeMatch::CouldUnify), + "type_could_unify", + ), + (relevance.exact_name_match, "name"), + (relevance.is_local, "local"), + (relevance.exact_postfix_snippet_match, "snippet"), + ] + .into_iter() + .filter_map(|(cond, desc)| if cond { Some(desc) } else { None }) + .join("+"); + + format!("[{}]", relevance_factors) + } } #[test] @@ -528,6 +536,7 @@ fn main() { let _: m::Spam = S$0 } Exact, ), is_local: false, + exact_postfix_snippet_match: false, }, trigger_call_info: true, }, @@ -556,6 +565,7 @@ fn main() { let _: m::Spam = S$0 } Exact, ), is_local: false, + exact_postfix_snippet_match: false, }, }, CompletionItem { @@ -649,6 +659,7 @@ fn foo() { A { the$0 } } CouldUnify, ), is_local: false, + exact_postfix_snippet_match: false, }, }, ] @@ -1339,4 +1350,44 @@ fn foo() [] "#]], ); } + + #[test] + fn postfix_completion_relevance() { + check_relevance_for_kinds( + &[CompletionKind::Postfix, CompletionKind::Magic], + r#" +mod ops { + pub trait Not { + type Output; + fn not(self) -> Self::Output; + } + + impl Not for bool { + type Output = bool; + fn not(self) -> bool { if self { false } else { true }} + } +} + +fn main() { + let _: bool = (9 > 2).not$0; +} +"#, + expect![[r#" + sn if [] + sn while [] + sn not [snippet] + sn ref [] + sn refm [] + sn match [] + sn box [] + sn ok [] + sn err [] + sn some [] + sn dbg [] + sn dbgr [] + sn call [] + me not() (ops::Not) [type_could_unify] + "#]], + ); + } } diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs index 9bd671e36c1..7b951b4e93c 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -195,6 +195,7 @@ pub(crate) fn completion_items( tdpp: lsp_types::TextDocumentPositionParams, items: Vec, ) -> Vec { + let max_relevance = items.iter().map(|it| it.relevance().score()).max().unwrap_or_default(); let mut res = Vec::with_capacity(items.len()); for item in items { completion_item( @@ -203,6 +204,7 @@ pub(crate) fn completion_items( enable_imports_on_the_fly, line_index, &tdpp, + max_relevance, item, ) } @@ -215,6 +217,7 @@ fn completion_item( enable_imports_on_the_fly: bool, line_index: &LineIndex, tdpp: &lsp_types::TextDocumentPositionParams, + max_relevance: u32, item: CompletionItem, ) { let mut additional_text_edits = Vec::new(); @@ -259,7 +262,7 @@ fn completion_item( ..Default::default() }; - set_score(&mut lsp_item, item.relevance()); + set_score(&mut lsp_item, max_relevance, item.relevance()); if item.deprecated() { lsp_item.tags = Some(vec![lsp_types::CompletionItemTag::Deprecated]) @@ -288,7 +291,7 @@ fn completion_item( if let Some((mutability, relevance)) = item.ref_match() { let mut lsp_item_with_ref = lsp_item.clone(); - set_score(&mut lsp_item_with_ref, relevance); + set_score(&mut lsp_item_with_ref, max_relevance, relevance); lsp_item_with_ref.label = format!("&{}{}", mutability.as_keyword_for_ref(), lsp_item_with_ref.label); if let Some(it) = &mut lsp_item_with_ref.text_edit { @@ -304,8 +307,12 @@ fn completion_item( acc.push(lsp_item); - fn set_score(res: &mut lsp_types::CompletionItem, relevance: CompletionRelevance) { - if relevance.is_relevant() { + fn set_score( + res: &mut lsp_types::CompletionItem, + max_relevance: u32, + relevance: CompletionRelevance, + ) { + if relevance.is_relevant() && relevance.score() == max_relevance { res.preselect = Some(true); } // The relevance needs to be inverted to come up with a sort score From 9b3292541c74b8de48a0dc11b3ce56800d812f86 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 4 Jul 2021 16:54:30 +0300 Subject: [PATCH 2/2] internal: improve feedback for relevance tests --- crates/ide_completion/src/render.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/crates/ide_completion/src/render.rs b/crates/ide_completion/src/render.rs index 3ee6831dc8b..f1ce3aa7130 100644 --- a/crates/ide_completion/src/render.rs +++ b/crates/ide_completion/src/render.rs @@ -330,6 +330,8 @@ fn compute_ref_match( #[cfg(test)] mod tests { + use std::cmp; + use expect_test::{expect, Expect}; use itertools::Itertools; @@ -352,9 +354,12 @@ fn check_relevance(ra_fixture: &str, expect: Expect) { #[track_caller] fn check_relevance_for_kinds(kinds: &[CompletionKind], ra_fixture: &str, expect: Expect) { - let actual = get_all_items(TEST_CONFIG, ra_fixture) + let mut actual = get_all_items(TEST_CONFIG, ra_fixture); + actual.retain(|it| kinds.contains(&it.completion_kind)); + actual.sort_by_key(|it| cmp::Reverse(it.relevance().score())); + + let actual = actual .into_iter() - .filter(|it| kinds.contains(&it.completion_kind)) .flat_map(|it| { let mut items = vec![]; @@ -921,9 +926,9 @@ fn test(bar: u32) { } fn foo(s: S) { test(s.$0) } "#, expect![[r#" - fd foo [] fd bar [type+name] fd baz [type] + fd foo [] "#]], ); } @@ -937,9 +942,9 @@ struct B { x: (), y: f32, bar: u32 } fn foo(a: A) { B { bar: a.$0 }; } "#, expect![[r#" - fd foo [] fd bar [type+name] fd baz [type] + fd foo [] "#]], ) } @@ -967,9 +972,9 @@ fn f(foo: i64) { } fn foo(a: A) { f(B { bar: a.$0 }); } "#, expect![[r#" - fd foo [] fd bar [type+name] fd baz [type] + fd foo [] "#]], ); } @@ -1015,9 +1020,9 @@ fn bar() -> u8 { 0 } fn f() { A { bar: b$0 }; } "#, expect![[r#" + fn bar() [type+name] fn baz() [type] st A [] - fn bar() [type+name] fn f() [] "#]], ); @@ -1340,9 +1345,9 @@ fn foo() { } "#, expect![[r#" + lc foo [type+local] ev Foo::A(…) [type_could_unify] ev Foo::B [type_could_unify] - lc foo [type+local] en Foo [] fn baz() [] fn bar() [] @@ -1373,9 +1378,10 @@ fn main() { } "#, expect![[r#" + sn not [snippet] + me not() (ops::Not) [type_could_unify] sn if [] sn while [] - sn not [snippet] sn ref [] sn refm [] sn match [] @@ -1386,7 +1392,6 @@ fn main() { sn dbg [] sn dbgr [] sn call [] - me not() (ops::Not) [type_could_unify] "#]], ); }