Skip to content
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

Fix #19071: ensure completion_item_hash serializes items uniquely #19072

Merged
merged 3 commits into from
Jan 30, 2025

Conversation

cessen
Copy link
Contributor

@cessen cessen commented Jan 29, 2025

Previously it may have been possible for different completion items to produce colliding hashes, not because of the hash but because of how the items were serialized into byte streams for hashing. See #19071 for details.

The chances of that happening were low, if it was actually possible at all. Nevertheless, this commit ensures that it definitely can't happen.

This commit uses a handful of techniques used to fix this, but they all boil down to "ensure this could be re-parsed". If it's possible to parse to recreate the original item, then by construction there is no chance of two different items getting serialized to identical byte streams.

…niquely

Previously it may have been possible for different completion items to
produce colliding hashes, not because of the hash but because of how
the items were serialized into byte streams for hashing. See rust-lang#19071
for details.

The chances of that happening were low, if it was actually possible at
all. Nevertheless, this commit ensures that it definitely can't happen.

This commit uses a handful of techniques used to fix this, but they all
boil down to "ensure this could be re-parsed". If it's possible to parse
to recreate the original item, then by construction there is no chance
of two different items getting serialized to identical byte streams.
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 29, 2025
@lnicola
Copy link
Member

lnicola commented Jan 29, 2025

r? @SomeoneToIgnore

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Jan 29, 2025

Looks nice to me, thank you for improving this.

The only question I have, after reading the issue: are 0u8, 1u8 and 2u8 really better than their string counterparts?
The issue mentions a + bc and ab + c uniqueness concern, but here I see that multiple different strings are mapped to the same digits (could_unify, non_exact and "&mut " were different but now are all 1u8) — is this really an improvement?

(should have read the comments too)

@lnicola
Copy link
Member

lnicola commented Jan 29, 2025

I think it's an improvement in the sense we're hashing slightly less stuff. We're not going to get collisions for using the same discriminants (because we keep track of each field), but also I don't think it we could have gotten collisions before.

@lnicola lnicola added this pull request to the merge queue Jan 30, 2025
Merged via the queue into rust-lang:master with commit 3c2aca1 Jan 30, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants