From 3679821eea94f30f2582ea7bca7569dad3ca31be Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 11 Mar 2021 19:11:14 -0800 Subject: add completion relevance score --- crates/ide/src/lib.rs | 4 +-- crates/ide_completion/src/item.rs | 55 ++++++++++++++++++++++++++++-------- crates/ide_completion/src/lib.rs | 4 +-- crates/ide_completion/src/render.rs | 22 ++++++++------- crates/rust-analyzer/src/to_proto.rs | 34 ++++++++++++++-------- 5 files changed, 82 insertions(+), 37 deletions(-) diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs index d1a250d48..16302eb2f 100644 --- a/crates/ide/src/lib.rs +++ b/crates/ide/src/lib.rs @@ -87,8 +87,8 @@ pub use crate::{ pub use hir::{Documentation, Semantics}; pub use ide_assists::{Assist, AssistConfig, AssistId, AssistKind}; pub use ide_completion::{ - CompletionConfig, CompletionItem, CompletionItemKind, CompletionScore, ImportEdit, - InsertTextFormat, + CompletionConfig, CompletionItem, CompletionItemKind, CompletionRelevance, CompletionScore, + ImportEdit, InsertTextFormat, }; pub use ide_db::{ base_db::{ diff --git a/crates/ide_completion/src/item.rs b/crates/ide_completion/src/item.rs index cf1aaa131..9a4dc915c 100644 --- a/crates/ide_completion/src/item.rs +++ b/crates/ide_completion/src/item.rs @@ -70,7 +70,7 @@ pub struct CompletionItem { /// Note that Relevance ignores fuzzy match score. We compute Relevance for /// all possible items, and then separately build an ordered completion list /// based on relevance and fuzzy matching with the already typed identifier. - relevance: Relevance, + relevance: CompletionRelevance, /// Indicates that a reference or mutable reference to this variable is a /// possible match. @@ -107,9 +107,11 @@ impl fmt::Debug for CompletionItem { if self.deprecated { s.field("deprecated", &true); } - if self.relevance.is_relevant() { + + if self.relevance != CompletionRelevance::default() { s.field("relevance", &self.relevance); } + if let Some(mutability) = &self.ref_match { s.field("ref_match", &format!("&{}", mutability.as_keyword_for_ref())); } @@ -129,7 +131,7 @@ pub enum CompletionScore { } #[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq, Default)] -pub struct Relevance { +pub struct CompletionRelevance { /// This is set in cases like these: /// /// ``` @@ -152,9 +154,34 @@ pub struct Relevance { pub exact_type_match: bool, } -impl Relevance { +impl CompletionRelevance { + /// Provides a relevance score. Higher values are more relevant. + /// + /// The absolute value of the relevance score is not meaningful, for + /// example a value of 0 doesn't mean "not relevant", rather + /// it means "least relevant". The score value should only be used + /// for relative ordering. + /// + /// See is_relevant if you need to make some judgement about score + /// in an absolute sense. + pub fn score(&self) -> u8 { + let mut score = 0; + + if self.exact_name_match { + score += 1; + } + if self.exact_type_match { + score += 1; + } + + score + } + + /// Returns true when the score for this threshold is above + /// some threshold such that we think it is especially likely + /// to be relevant. pub fn is_relevant(&self) -> bool { - self != &Relevance::default() + self.score() > 0 } } @@ -249,7 +276,7 @@ impl CompletionItem { text_edit: None, deprecated: false, trigger_call_info: None, - relevance: Relevance::default(), + relevance: CompletionRelevance::default(), ref_match: None, import_to_add: None, } @@ -292,7 +319,7 @@ impl CompletionItem { self.deprecated } - pub fn relevance(&self) -> Relevance { + pub fn relevance(&self) -> CompletionRelevance { self.relevance } @@ -300,8 +327,14 @@ impl CompletionItem { self.trigger_call_info } - pub fn ref_match(&self) -> Option { - self.ref_match + pub fn ref_match(&self) -> Option<(Mutability, CompletionRelevance)> { + // Relevance of the ref match should be the same as the original + // match, but with exact type match set because self.ref_match + // is only set if there is an exact type match. + let mut relevance = self.relevance; + relevance.exact_type_match = true; + + self.ref_match.map(|mutability| (mutability, relevance)) } pub fn import_to_add(&self) -> Option<&ImportEdit> { @@ -349,7 +382,7 @@ pub(crate) struct Builder { text_edit: Option, deprecated: bool, trigger_call_info: Option, - relevance: Relevance, + relevance: CompletionRelevance, ref_match: Option, } @@ -457,7 +490,7 @@ impl Builder { self.deprecated = deprecated; self } - pub(crate) fn set_relevance(&mut self, relevance: Relevance) -> &mut Builder { + pub(crate) fn set_relevance(&mut self, relevance: CompletionRelevance) -> &mut Builder { self.relevance = relevance; self } diff --git a/crates/ide_completion/src/lib.rs b/crates/ide_completion/src/lib.rs index d46f521a0..21e489755 100644 --- a/crates/ide_completion/src/lib.rs +++ b/crates/ide_completion/src/lib.rs @@ -24,8 +24,8 @@ use crate::{completions::Completions, context::CompletionContext, item::Completi pub use crate::{ config::CompletionConfig, item::{ - CompletionItem, CompletionItemKind, CompletionScore, ImportEdit, InsertTextFormat, - Relevance, + CompletionItem, CompletionItemKind, CompletionRelevance, CompletionScore, ImportEdit, + InsertTextFormat, }, }; diff --git a/crates/ide_completion/src/render.rs b/crates/ide_completion/src/render.rs index f7f9084d9..db31896e5 100644 --- a/crates/ide_completion/src/render.rs +++ b/crates/ide_completion/src/render.rs @@ -20,7 +20,7 @@ use ide_db::{ use syntax::TextRange; use crate::{ - item::{ImportEdit, Relevance}, + item::{CompletionRelevance, ImportEdit}, CompletionContext, CompletionItem, CompletionItemKind, CompletionKind, }; @@ -322,9 +322,9 @@ impl<'a> Render<'a> { } } -fn compute_relevance(ctx: &RenderContext, ty: &Type, name: &str) -> Option { +fn compute_relevance(ctx: &RenderContext, ty: &Type, name: &str) -> Option { let (expected_name, expected_type) = ctx.expected_name_and_type()?; - let mut res = Relevance::default(); + let mut res = CompletionRelevance::default(); res.exact_type_match = ty == &expected_type; res.exact_name_match = name == &expected_name; Some(res) @@ -338,7 +338,7 @@ mod tests { use crate::{ test_utils::{check_edit, do_completion, get_all_items, TEST_CONFIG}, - CompletionKind, Relevance, + CompletionKind, CompletionRelevance, }; fn check(ra_fixture: &str, expect: Expect) { @@ -347,12 +347,14 @@ mod tests { } fn check_relevance(ra_fixture: &str, expect: Expect) { - fn display_relevance(relevance: Relevance) -> &'static str { + fn display_relevance(relevance: CompletionRelevance) -> &'static str { match relevance { - Relevance { exact_type_match: true, exact_name_match: true } => "[type+name]", - Relevance { exact_type_match: true, exact_name_match: false } => "[type]", - Relevance { exact_type_match: false, exact_name_match: true } => "[name]", - Relevance { exact_type_match: false, exact_name_match: false } => "[]", + CompletionRelevance { exact_type_match: true, exact_name_match: true } => { + "[type+name]" + } + CompletionRelevance { exact_type_match: true, exact_name_match: false } => "[type]", + CompletionRelevance { exact_type_match: false, exact_name_match: true } => "[name]", + CompletionRelevance { exact_type_match: false, exact_name_match: false } => "[]", } } @@ -975,7 +977,7 @@ fn main() { Local, ), detail: "S", - relevance: Relevance { + relevance: CompletionRelevance { exact_name_match: true, exact_type_match: false, }, diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs index a9846fa70..a467bc685 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -6,9 +6,10 @@ use std::{ use ide::{ Annotation, AnnotationKind, Assist, AssistKind, CallInfo, CompletionItem, CompletionItemKind, - Documentation, FileId, FileRange, FileSystemEdit, Fold, FoldKind, Highlight, HlMod, HlPunct, - HlRange, HlTag, Indel, InlayHint, InlayKind, InsertTextFormat, Markup, NavigationTarget, - ReferenceAccess, RenameError, Runnable, Severity, SourceChange, TextEdit, TextRange, TextSize, + CompletionRelevance, Documentation, FileId, FileRange, FileSystemEdit, Fold, FoldKind, + Highlight, HlMod, HlPunct, HlRange, HlTag, Indel, InlayHint, InlayKind, InsertTextFormat, + Markup, NavigationTarget, ReferenceAccess, RenameError, Runnable, Severity, SourceChange, + TextEdit, TextRange, TextSize, }; use ide_db::SymbolKind; use itertools::Itertools; @@ -213,12 +214,22 @@ pub(crate) fn completion_item( ..Default::default() }; - if item.relevance().is_relevant() { - lsp_item.preselect = Some(true); - // HACK: sort preselect items first - lsp_item.sort_text = Some(format!(" {}", item.label())); + fn set_score(res: &mut lsp_types::CompletionItem, relevance: CompletionRelevance) { + if relevance.is_relevant() { + res.preselect = Some(true); + } + // The relevance needs to be inverted to come up with a sort score + // because the client will sort ascending. + let sort_score = relevance.score() ^ 0xFF; + // Zero pad the string to ensure values are sorted numerically + // even though the client is sorting alphabetically. Three + // characters is enough to fit the largest u8, which is the + // type of the relevance score. + res.sort_text = Some(format!("{:03}", sort_score)); } + set_score(&mut lsp_item, item.relevance()); + if item.deprecated() { lsp_item.tags = Some(vec![lsp_types::CompletionItemTag::Deprecated]) } @@ -228,10 +239,9 @@ pub(crate) fn completion_item( } let mut res = match item.ref_match() { - Some(mutability) => { + Some((mutability, relevance)) => { let mut lsp_item_with_ref = lsp_item.clone(); - lsp_item.preselect = Some(true); - lsp_item.sort_text = Some(format!(" {}", item.label())); + set_score(&mut lsp_item_with_ref, relevance); lsp_item_with_ref.label = format!("&{}{}", mutability.as_keyword_for_ref(), lsp_item_with_ref.label); if let Some(lsp_types::CompletionTextEdit::Edit(it)) = &mut lsp_item_with_ref.text_edit @@ -1107,13 +1117,13 @@ mod tests { ( "&arg", Some( - " arg", + "253", ), ), ( "arg", Some( - " arg", + "254", ), ), ] -- cgit v1.2.3 From 9ee3914c61f0476d28d3f74f046f487900e2a6fb Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 11 Mar 2021 19:13:27 -0800 Subject: remove unused CompletionScore enum --- crates/ide/src/lib.rs | 4 ++-- crates/ide_completion/src/item.rs | 8 -------- crates/ide_completion/src/lib.rs | 5 +---- 3 files changed, 3 insertions(+), 14 deletions(-) diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs index 16302eb2f..0a493d2f3 100644 --- a/crates/ide/src/lib.rs +++ b/crates/ide/src/lib.rs @@ -87,8 +87,8 @@ pub use crate::{ pub use hir::{Documentation, Semantics}; pub use ide_assists::{Assist, AssistConfig, AssistId, AssistKind}; pub use ide_completion::{ - CompletionConfig, CompletionItem, CompletionItemKind, CompletionRelevance, CompletionScore, - ImportEdit, InsertTextFormat, + CompletionConfig, CompletionItem, CompletionItemKind, CompletionRelevance, ImportEdit, + InsertTextFormat, }; pub use ide_db::{ base_db::{ diff --git a/crates/ide_completion/src/item.rs b/crates/ide_completion/src/item.rs index 9a4dc915c..30317842d 100644 --- a/crates/ide_completion/src/item.rs +++ b/crates/ide_completion/src/item.rs @@ -122,14 +122,6 @@ impl fmt::Debug for CompletionItem { } } -#[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq)] -pub enum CompletionScore { - /// If only type match - TypeMatch, - /// If type and name match - TypeAndNameMatch, -} - #[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq, Default)] pub struct CompletionRelevance { /// This is set in cases like these: diff --git a/crates/ide_completion/src/lib.rs b/crates/ide_completion/src/lib.rs index 21e489755..263554ecf 100644 --- a/crates/ide_completion/src/lib.rs +++ b/crates/ide_completion/src/lib.rs @@ -23,10 +23,7 @@ use crate::{completions::Completions, context::CompletionContext, item::Completi pub use crate::{ config::CompletionConfig, - item::{ - CompletionItem, CompletionItemKind, CompletionRelevance, CompletionScore, ImportEdit, - InsertTextFormat, - }, + item::{CompletionItem, CompletionItemKind, CompletionRelevance, ImportEdit, InsertTextFormat}, }; //FIXME: split the following feature into fine-grained features. -- cgit v1.2.3 From 10fb065b146d7d03a65bbc73cdb34d32523dafa1 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 11 Mar 2021 20:39:25 -0800 Subject: add relevance score test --- crates/ide_completion/src/item.rs | 60 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/crates/ide_completion/src/item.rs b/crates/ide_completion/src/item.rs index 30317842d..352640e3b 100644 --- a/crates/ide_completion/src/item.rs +++ b/crates/ide_completion/src/item.rs @@ -499,3 +499,63 @@ impl Builder { self } } + +#[cfg(test)] +mod tests { + use itertools::Itertools; + use test_utils::assert_eq_text; + + use super::CompletionRelevance; + + /// Check that these are CompletionRelevance are sorted in ascending order + /// by their relevance score. + /// + /// We want to avoid making assertions about the absolute score of any + /// item, but we do want to assert whether each is >, <, or == to the + /// others. + /// + /// If provided vec![vec![a], vec![b, c], vec![d]], then this will assert: + /// a.score < b.score == c.score < d.score + fn check_relevance_score_ordered(expected_relevance_order: Vec>) { + let expected = format!("{:#?}", &expected_relevance_order); + + let actual_relevance_order = expected_relevance_order + .into_iter() + .flatten() + .map(|r| (r.score(), r)) + .sorted_by_key(|(score, _r)| *score) + .fold( + (u8::MIN, vec![vec![]]), + |(mut currently_collecting_score, mut out), (score, r)| { + if currently_collecting_score == score { + out.last_mut().unwrap().push(r); + } else { + currently_collecting_score = score; + out.push(vec![r]); + } + (currently_collecting_score, out) + }, + ) + .1; + + let actual = format!("{:#?}", &actual_relevance_order); + + assert_eq_text!(&expected, &actual); + } + + #[test] + fn relevance_score() { + // This test asserts that the relevance score for these items is ascending, and + // that any items in the same vec have the same score. + let expected_relevance_order = vec![ + vec![CompletionRelevance::default()], + vec![ + CompletionRelevance { exact_name_match: true, ..CompletionRelevance::default() }, + CompletionRelevance { exact_type_match: true, ..CompletionRelevance::default() }, + ], + vec![CompletionRelevance { exact_name_match: true, exact_type_match: true }], + ]; + + check_relevance_score_ordered(expected_relevance_order); + } +} -- cgit v1.2.3 From acbe297fbd10250e0d99d1e3e98751dd6ef77adc Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 12 Mar 2021 06:08:07 -0800 Subject: update relevance score u8 -> u32 --- crates/ide_completion/src/item.rs | 4 ++-- crates/rust-analyzer/src/to_proto.rs | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/ide_completion/src/item.rs b/crates/ide_completion/src/item.rs index 352640e3b..3febab32b 100644 --- a/crates/ide_completion/src/item.rs +++ b/crates/ide_completion/src/item.rs @@ -156,7 +156,7 @@ impl CompletionRelevance { /// /// See is_relevant if you need to make some judgement about score /// in an absolute sense. - pub fn score(&self) -> u8 { + pub fn score(&self) -> u32 { let mut score = 0; if self.exact_name_match { @@ -525,7 +525,7 @@ mod tests { .map(|r| (r.score(), r)) .sorted_by_key(|(score, _r)| *score) .fold( - (u8::MIN, vec![vec![]]), + (u32::MIN, vec![vec![]]), |(mut currently_collecting_score, mut out), (score, r)| { if currently_collecting_score == score { out.last_mut().unwrap().push(r); diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs index a467bc685..1a8cdadad 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -220,12 +220,12 @@ pub(crate) fn completion_item( } // The relevance needs to be inverted to come up with a sort score // because the client will sort ascending. - let sort_score = relevance.score() ^ 0xFF; - // Zero pad the string to ensure values are sorted numerically - // even though the client is sorting alphabetically. Three - // characters is enough to fit the largest u8, which is the - // type of the relevance score. - res.sort_text = Some(format!("{:03}", sort_score)); + let sort_score = relevance.score() ^ 0xFF_FF_FF_FF; + // Zero pad the string to ensure values can be properly sorted + // by the client. Hex format is used because it is easier to + // visually compare very large values, which the sort text + // tends to be since it is the opposite of the score. + res.sort_text = Some(format!("{:08x}", sort_score)); } set_score(&mut lsp_item, item.relevance()); @@ -1117,13 +1117,13 @@ mod tests { ( "&arg", Some( - "253", + "fffffffd", ), ), ( "arg", Some( - "254", + "fffffffe", ), ), ] -- cgit v1.2.3