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

std::ops::Range does not implement std::hash::Hash #34170

Closed
ntninja opened this issue Jun 9, 2016 · 9 comments · Fixed by #34180
Closed

std::ops::Range does not implement std::hash::Hash #34170

ntninja opened this issue Jun 9, 2016 · 9 comments · Fixed by #34180

Comments

@ntninja
Copy link

ntninja commented Jun 9, 2016

Range should implement Hash if the value it holds implements Hash.

Something like:

impl<T> core::hash::Hash for core::ops::Range<T> where T: core::hash::Hash {
    fn hash<H>(&self, state: &mut H) where H: core::hash::Hasher {
        core::hash::Hash::hash(self.start, state);
        core::hash::Hash::hash(self.end, state);
    }
}
@ntninja
Copy link
Author

ntninja commented Jun 9, 2016

The same goes for RangeStart, RangeEnd and RangeFull of course.

@durka
Copy link
Contributor

durka commented Jun 9, 2016

Can we just derive it?

@ntninja
Copy link
Author

ntninja commented Jun 9, 2016

If we'd derive it, then all type parameters would be required to implement Hash if IIRC.

@sfackler
Copy link
Member

sfackler commented Jun 9, 2016

Yep, but that's the behavior we'd want anyway, yes?

@durka
Copy link
Contributor

durka commented Jun 9, 2016

Yes. Not sure how you'd hash a range with unhashable endpoints.
On Jun 9, 2016 8:35 AM, "Steven Fackler" notifications@github.com wrote:

Yep, but that's the behavior we'd want anyway, yes?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34170 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAC3n-SZhTHSXx14WG6yVFG1NcikobLdks5qKAh-gaJpZM4Ixhqh
.

@ntninja
Copy link
Author

ntninja commented Jun 9, 2016

@sfackler No, if the thing that the range is variant over is not hashable, then the Range itself shouldn't be hashable either. Doing #[derive(Hash)] would require the type parameter to always be hashable, which I don't see any reason for (besides the fact that it would break compatibility).

@sfackler
Copy link
Member

sfackler commented Jun 9, 2016

@Alexander255 I don't believe that's how #[derive(Hash)] works - all it will do is generate a hash impl that looks identical to the one in your original post. I does not modify bounds on the struct definition itself.

@ntninja
Copy link
Author

ntninja commented Jun 9, 2016

@sflacker You're right:
Example with manual impl: https://is.gd/Jj6KRL
Example with derive: https://is.gd/W4O8ai

I was under the strong impression that #[derive(Hash)] will abort the compilation, requiring you to bound the type parameters appropriately (but apparently it's smarter than that).

durka added a commit to durka/rust that referenced this issue Jun 9, 2016
bors added a commit that referenced this issue Jun 15, 2016
derive Hash (and not Copy) for ranges

Fixes #34170.

Also, `RangeInclusive` was `Copy` by mistake -- fix that, which is a [breaking-change] to that unstable type.
@ntninja
Copy link
Author

ntninja commented Jun 15, 2016

@durka Thanks!

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

Successfully merging a pull request may close this issue.

4 participants