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

many types don't derive Hash even though they can #245

Open
pickx opened this issue Jul 23, 2022 · 1 comment
Open

many types don't derive Hash even though they can #245

pickx opened this issue Jul 23, 2022 · 1 comment

Comments

@pickx
Copy link

pickx commented Jul 23, 2022

many types (for example: Position and Range) don't derive Hash. this makes it hard to use these types if a HashMap is needed, without using intermediate types.

is there a technical reason for this? if not, I can submit a PR to derive this trait for the more trivial types.

@ebkalderon
Copy link
Contributor

Not a maintainer nor a member of this organization, just a recurring contributor: I think most types could certainly derive Hash safely, though probably not indiscriminately. For example, serde_json::Value does not derive Hash because it may contain a number representable as an f32 or f64, neither of which are Hash either. I suspect there may also be a very slight forward compatibility hazard as well: should any struct potentially gain a non-hashable field in a new version of the spec, the Hash derivation will need to be removed on the entire thing and any other type that contains to it, which might frustrate downstream users.

With that said, I still think this is a good idea. One conservative approach you could take might be to derive Hash on common types like Position, Range, Location, and other items that you think are likely to be used as hashmap keys. I suspect that complex structs and enums, like the .*Params and .*Capabilit{y,ies} types especially, are probably poor candidates for Hash because they would be likely to change between LSP spec releases and could become non-hashable at any point in the future. I think you should go for opening a PR, if you want to!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants