Hash completion items to properly match them during /resolve#18653
Conversation
rustbot
added
the
S-waiting-on-review
SomeoneToIgnore
left a comment
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
My only concern is how relatively fragile this is, if another field is added or some fields start to alter between text edits, but otherwise it seems to work on VSCode and Helix during my manual testing.
cc @Veykril as it would be good to use this for some while to see if that sporadic error is gone with this build.
| impl_from!(SymbolKind for CompletionItemKind); | ||
|
|
||
| impl CompletionItemKind { | ||
| #[cfg(test)] |
There was a problem hiding this comment.
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.
Can't we just make CompletionItemKind and SymbolKind Hash?
There was a problem hiding this comment.
Oh I see, tenthash doesn't accept hash things, only byte slices...
| 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.
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.
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.
Thanks, had applied Base64 and it does trim the character number by half:
Before:
"hash":[215,92,116,28,211,148,173,32,58,55,147,15,75,30,169,165,93,8,60,63]
After:
"hash":"11x0HNOUrSA6N5MPSx6ppV0IPD8="
Seems like a good thing to do.
| impl_from!(SymbolKind for CompletionItemKind); | ||
|
|
||
| impl CompletionItemKind { | ||
| #[cfg(test)] |
There was a problem hiding this comment.
Can't we just make CompletionItemKind and SymbolKind Hash?
| impl_from!(SymbolKind for CompletionItemKind); | ||
|
|
||
| impl CompletionItemKind { | ||
| #[cfg(test)] |
There was a problem hiding this comment.
Oh I see, tenthash doesn't accept hash things, only byte slices...
pascalkuthe
left a comment
pascalkuthe
left a comment
There was a problem hiding this comment.
this is a much more robust approach, thanks for the fast turnaround!
|
|
||
| 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.
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:
- this will reduce computational requirements a bit since hash is only computed for any realistic candidates
- as a bonus it makes collisons even more unlikely
There was a problem hiding this comment.
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 starts_with check.
| 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.
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)
* Exclude documentation field from hashing * Do less cloning during initial completion list generation
* Use Base64 to minify the hash representation in the JSON data * Do hash checks only for items with similar labels
|
Would it be possible to do a sync soonish? I don't think either of the fixes have made it to r-l/rust so nightly is still buggy. Edit: thanks Inicola, done in rust-lang/rust#134170 |
5 participants
Closes #18547
Closes #18640
Follows #18547 (comment) proposal and adds completion items hashing, using all their data possible, that does not change between the edits.