-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Hash completion items to properly match them during /resolve #18653
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
Changes from 6 commits
62d97d9
d348ffb
5906bda
b59b2fb
89c2aae
d8d35db
2529e9e
4169926
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ use triomphe::Arc; | |
use vfs::{AbsPath, AbsPathBuf, FileId, VfsPath}; | ||
|
||
use crate::{ | ||
completion_item_hash, | ||
config::{Config, RustfmtConfig, WorkspaceSymbolConfig}, | ||
diagnostics::convert_diagnostic, | ||
global_state::{FetchWorkspaceRequest, GlobalState, GlobalStateSnapshot}, | ||
|
@@ -1127,30 +1128,34 @@ pub(crate) fn handle_completion_resolve( | |
forced_resolve_completions_config.fields_to_resolve = CompletionFieldsToResolve::empty(); | ||
|
||
let position = FilePosition { file_id, offset }; | ||
let Some(resolved_completions) = snap.analysis.completions( | ||
let Some(completions) = snap.analysis.completions( | ||
&forced_resolve_completions_config, | ||
position, | ||
resolve_data.trigger_character, | ||
)? | ||
else { | ||
return Ok(original_completion); | ||
}; | ||
|
||
let Some(corresponding_completion) = completions.into_iter().find(|completion_item| { | ||
let hash = completion_item_hash(completion_item, resolve_data.for_ref); | ||
hash == resolve_data.hash | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think may be a good idea to first check if the labels are identical and only compute and compare the hash if they are:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice idea. It's a bit tricky because LSP completion items get their labels changed compared to the original ones, but luckily only by adding a suffix, so we can still reduce the computations with a |
||
}) else { | ||
return Ok(original_completion); | ||
}; | ||
|
||
let mut resolved_completions = to_proto::completion_items( | ||
&snap.config, | ||
&forced_resolve_completions_config.fields_to_resolve, | ||
&line_index, | ||
snap.file_version(position.file_id), | ||
resolve_data.position, | ||
resolve_data.trigger_character, | ||
resolved_completions, | ||
vec![corresponding_completion], | ||
); | ||
|
||
let mut resolved_completion = | ||
if resolved_completions.get(resolve_data.completion_item_index).is_some() { | ||
resolved_completions.swap_remove(resolve_data.completion_item_index) | ||
} else { | ||
return Ok(original_completion); | ||
}; | ||
let Some(mut resolved_completion) = resolved_completions.pop() else { | ||
return Ok(original_completion); | ||
}; | ||
|
||
if !resolve_data.imports.is_empty() { | ||
let additional_edits = snap | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -826,7 +826,8 @@ pub struct CompletionResolveData { | |
pub imports: Vec<CompletionImport>, | ||
pub version: Option<i32>, | ||
pub trigger_character: Option<char>, | ||
pub completion_item_index: usize, | ||
pub for_ref: bool, | ||
pub hash: [u8; 20], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can reduce the JSON size with https://github.com/cessen/tenthash/blob/159da57b120bf2abcdd42532d17a7ae2957f98b8/tenthash-rust/tests/test_vectors.rs#L17 but not sure how appropriate this conversion is, any ideas? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would do something like base64, that encoding is just a hex string which is pretty standard to use for hashes but not necessairily the most information dense (compared to base64) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, had applied Base64 and it does trim the character number by half: Before: After: Seems like a good thing to do. |
||
} | ||
|
||
#[derive(Debug, Serialize, Deserialize)] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be quite an odd change, but it was quite tempting to reuse existing
&str
producer for this large enum.As an alternative, I can copy this into the module with the hashing function, are there better ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just make
CompletionItemKind
andSymbolKind
Hash
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see,
tenthash
doesn't accept hash things, only byte slices...