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

Implement Hash on IndexMap and IndexSet #75

Closed
lucab opened this issue Jun 15, 2018 · 7 comments
Closed

Implement Hash on IndexMap and IndexSet #75

lucab opened this issue Jun 15, 2018 · 7 comments

Comments

@lucab
Copy link
Contributor

lucab commented Jun 15, 2018

I'm currently looking into embedding an IndexMap into a larger recursive enum (which looks pretty similar to this). However I hit a blocker on my journey, as the type-checker complains that IndexMap does not implement Hash (duh).

While I think in theory it is quite unlikely to have a usecase where an IndexMap is used as an hashable key, the type system is technically correct on this. Would a PR to derive Hash on all the relevant types up to IndexMap+IndexSet be welcome here? For reference, within stdlib BTreeMap implements it but HashMap doesn't.

@cuviper
Copy link
Member

cuviper commented Jun 15, 2018

See the closed #67 -- the main challenge is that Hash needs to be consistent with Eq.

@lucab lucab changed the title Derive Hash on IndexMap and IndexSet Implement Hash on IndexMap and IndexSet Jun 15, 2018
@lucab
Copy link
Contributor Author

lucab commented Jun 15, 2018

Meh, I should have searched better. Also, I was too naïf so I have now retitled this.

@bluss
Copy link
Member

bluss commented Sep 1, 2019

I'm going to close this as there is no use case proposed for hashing the indexmap nor an algorithm that it should use. If I read the original report correctly, the map is embedded into a bigger type and it's entirely possible for you to define a domain specific hash and equality for that specific case.

@bluss bluss closed this as completed Sep 1, 2019
@lucab
Copy link
Contributor Author

lucab commented Sep 1, 2019

@bluss glad that you are back! As an afterthought related to this, I was wondering if PartialEq should behave like the BTreeSet one instead, by taking item-order into account too?

@cuviper
Copy link
Member

cuviper commented Sep 1, 2019

You can also use Iterator::eq to compare in order.

@lucab
Copy link
Contributor Author

lucab commented Sep 1, 2019

Sure, but I am talking about the commonly-used comparison, which is a potential hazard for anybody actually relying on insertion order for semantics.
As a quite common usecase for an example, let's take "de-duplicated ordered policy rules":

extern crate indexmap; // 1.0.2

// Policy: subject - verb - object
type Policies = indexmap::IndexSet<(&'static str, &'static str, &'static str)>;

fn main() {
    let mut rules1 = Policies::new();
    rules1.insert(("user", "allow", "ssh"));
    rules1.insert(("user", "deny", "ssh"));

    let mut rules2 = Policies::new();
    rules2.insert(("user", "deny", "ssh"));
    rules2.insert(("user", "allow", "ssh"));

    // This does not panic right now.
    assert_eq!(rules1, rules2)
}

@bluss
Copy link
Member

bluss commented Sep 12, 2019

@lucab What you're saying makes sense, but a guiding tenet here has been to be a drop-in HashMap replacement, and this would break with that. I think this means that by default, indexmap will not implement your suggestion. You could use indexmap to implement it for your own policies collection, and we could provide an ordered comparison method.

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

3 participants