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

Unnecessary trait bounds in HashMap (and BTreeMap) #44777

Closed
Rufflewind opened this issue Sep 22, 2017 · 3 comments · Fixed by #67642
Closed

Unnecessary trait bounds in HashMap (and BTreeMap) #44777

Rufflewind opened this issue Sep 22, 2017 · 3 comments · Fixed by #67642
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Rufflewind
Copy link
Contributor

Rufflewind commented Sep 22, 2017

There's a lot of unnecessary trait bounds in HashMap that could be pruned.

Examples:

  • impl Default for HashMap<K, V, S> really only needs S: Default
  • impl Debug for HashMap<K, V, S> really only needs K: Debug and V: Debug
  • Many of the methods only require one out of K: Eq + Hash or S: BuildHasher, or sometimes neither.

Most of this isn't a big deal, but for Default and Debug this is a nuisance because you can't use #[derive(Debug, Default)] on unconstrained types containing HashMap, e.g.

// won't compile
#[derive(Debug, Default)]
struct MyStruct<K>(HashMap<K>);

// your options are to either:
//
//   - needlessly constrain `K` directly on the struct itself,
//     which then infects everything involving MyStruct
//   - define Debug and Default manually with the redundant constraints;
//     downstream types that contain MyStruct are still screwed

One might argue that this would limit potential implementations, but for a lot them like Default or .len(), no sensible implementation should ever use K: Eq + Hash at all.

This also applies to BTreeMap to some extent (Default doesn't need Ord, for example).

Patch: Rufflewind@a1587a1

@bluss
Copy link
Member

bluss commented Sep 23, 2017

See also the Implied bounds RFC rust-lang/rfcs/pull/2089 which addresses this, if viewed in a slightly different perspective.

@kennytm
Copy link
Member

kennytm commented Sep 23, 2017

I believe implied bounds is not going to fix HashMap and BTreeMap, since changing them to require K: Ord in the struct itself will break stability.

@ollie27
Copy link
Member

ollie27 commented Sep 23, 2017

Why doesn't HashMap have K: Eq + Hash and S: BuildHasher on the struct definition itself?

@TimNN TimNN added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 24, 2017
@dtolnay dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 14, 2017
@bors bors closed this as completed in 2a20133 Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants