-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Relax bounds on HashMap/HashSet #67642
Conversation
No functional changes are made, and all APIs are moved to strictly less restrictive bounds. These APIs changed from the old bound listed to no trait bounds: K: Hash + Eq * new * with_capacity K: Eq + Hash, S: BuildHasher * with_hasher * with_capacity_and_hasher * hasher K: Eq + Hash + Debug -> K: Debug S: BuildHasher -> S <HashMap as Debug> K: Eq + Hash -> K S: BuildHasher + Default -> S: Default <HashMap as Default>
LGTM @rfcbot fcp merge |
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
This comment has been minimized.
This comment has been minimized.
Test failure appears to be because it is now possible to create a HashSet from |
469a8da
to
5514ead
Compare
This comment has been minimized.
This comment has been minimized.
5514ead
to
515f642
Compare
What's the rationale for allowing the creation of maps that can't be used? |
I think the primary rationale from my perspective (I am not the one who filed the original issue, nor do I have a use case myself) is to allow wrapping HashMap/HashSet in types that don't necessarily want to constrain themselves to using those always. I am not sure that I have a concrete use case -- most things I can come up with would know the type parameters (i.e., not actually be generic). My loose idea is a type that wants to construct a HashSet/Map eagerly but may not use it except for some use cases (e.g., some graph data structure that caches nodes/edges if you call some methods, but can be used without those methods in some cases). |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
No functional changes are made, and all APIs are moved to strictly less restrictive bounds. These APIs changed from the old bound listed to the new bound: T: Hash + Eq -> T * new * with_capacity T: Eq + Hash, S: BuildHasher -> T * with_hasher * with_capacity_and_hasher * hasher T: Eq + Hash + Debug -> T: Debug S: BuildHasher -> S <HashSet as Debug> T: Eq + Hash -> T S: BuildHasher + Default -> S: Default <HashSet as Default>
515f642
to
48859db
Compare
@bors r+ |
📌 Commit 48859db has been approved by |
@bors r- Oops, wrong window. |
IIRC this is a general guideline that we've used for libstd historically, only requiring bounds when absolutely necessary. While there's not much use in creating defunkt objects relaxing the bounds can often make generic programming much easier since you don't have to thread through so many bounds in so many places. For example Now that being said there's also the point to be made of being conservative. I suspect these bounds were actually used at some point and refactorings have since enabled us to not need them. By removing them we're never going to be able to design a hash map where they're required for these methods. That being said the current hash table is quite good, so I suspect we won't have to worry about this too too much. |
We already provide The construction methods are the only ones which are a bit up in the air -- I could see us wanting to use some property of the hasher to decide how much to allocate in the future (e.g., how many "spare bits" we need or so), but I don't think it's a huge concern as I'm not aware of cases where HashMaps specialize for the hasher being used in such a way. |
Since no one has brought anything up, I've checked my box, don't block on my vague concern. |
@bors r+ |
📌 Commit 48859db has been approved by |
…nieu Relax bounds on HashMap/HashSet These APIs changed from the old bound listed to the new bound (possibly empty): K: Hash + Eq -> K * new * with_capacity K: Eq + Hash, S: BuildHasher -> K, S * with_hasher * with_capacity_and_hasher * hasher K: Eq + Hash + Debug -> K: Debug S: BuildHasher -> S HashMap as Debug K: Eq + Hash -> K S: BuildHasher + Default -> S: Default HashMap as Default Resolves rust-lang#44777.
Rollup of 9 pull requests Successful merges: - #67642 (Relax bounds on HashMap/HashSet) - #68848 (Hasten macro parsing) - #69008 (Properly use parent generics for opaque types) - #69048 (Suggestion when encountering assoc types from hrtb) - #69049 (Optimize image sizes) - #69050 (Micro-optimize the heck out of LEB128 reading and writing.) - #69068 (Make the SGX arg cleanup implementation a NOP) - #69082 (When expecting `BoxFuture` and using `async {}`, suggest `Box::pin`) - #69104 (bootstrap: Configure cmake when building sanitizer runtimes) Failed merges: r? @ghost
For posterity, one specific case where we don't want the K: Hash + Eq bound on struct Span { lo: u32, hi: u32 }
struct Interner {
interned: HashSet<Span>, // equality & hash is determined by `&self.storage[span]`
storage: String,
} |
Damn, this ship has sailed, but I'm going to bring the argument about why this wasn't that great idea and hope to avoid such thing in the future. Consider error messages for this program in different versions: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=38b035ac5545bc6fe2d7caa805571067 old: I think the old message was much clearer than the new one. :( |
That would be a diagnostics bug. The program calls extend on a HashMap<K, V>. HashMap has an extend method but it requires K: Eq. Diagnostic should say so. Please file an issue to follow up. |
OK, I filed the bug. Just note that these kinds of things become worse when combinators are used, as the resulting type gets complicated and the code that passed a wrong type is far away from the place at which rustc found the issue. This is also the reason why error messages from futures are so difficult to understand. That's why I opened rust-lang/api-guidelines#217 I don't think that there's a general solution to such error messages. If there was, it might be a good reason to stop requiring types in (non-public) function signatures too. |
Pkgsrc changes: * Bump rust bootstrap version to 1.42.0, except for Darwin/i686 where the bootstrap is not (yet?) available. Upstream changes: Version 1.43.0 (2020-04-23) ========================== Language -------- - [Fixed using binary operations with `&{number}` (e.g. `&1.0`) not having the type inferred correctly.][68129] - [Attributes such as `#[cfg()]` can now be used on `if` expressions.][69201] **Syntax only changes** - [Allow `type Foo: Ord` syntactically.][69361] - [Fuse associated and extern items up to defaultness.][69194] - [Syntactically allow `self` in all `fn` contexts.][68764] - [Merge `fn` syntax + cleanup item parsing.][68728] - [`item` macro fragments can be interpolated into `trait`s, `impl`s, and `extern` blocks.][69366] For example, you may now write: ```rust macro_rules! mac_trait { ($i:item) => { trait T { $i } } } mac_trait! { fn foo() {} } ``` These are still rejected *semantically*, so you will likely receive an error but these changes can be seen and parsed by macros and conditional compilation. Compiler -------- - [You can now pass multiple lint flags to rustc to override the previous flags.][67885] For example; `rustc -D unused -A unused-variables` denies everything in the `unused` lint group except `unused-variables` which is explicitly allowed. However, passing `rustc -A unused-variables -D unused` denies everything in the `unused` lint group **including** `unused-variables` since the allow flag is specified before the deny flag (and therefore overridden). - [rustc will now prefer your system MinGW libraries over its bundled libraries if they are available on `windows-gnu`.][67429] - [rustc now buffers errors/warnings printed in JSON.][69227] Libraries --------- - [`Arc<[T; N]>`, `Box<[T; N]>`, and `Rc<[T; N]>`, now implement `TryFrom<Arc<[T]>>`,`TryFrom<Box<[T]>>`, and `TryFrom<Rc<[T]>>` respectively.][69538] **Note** These conversions are only available when `N` is `0..=32`. - [You can now use associated constants on floats and integers directly, rather than having to import the module.][68952] e.g. You can now write `u32::MAX` or `f32::NAN` with no imports. - [`u8::is_ascii` is now `const`.][68984] - [`String` now implements `AsMut<str>`.][68742] - [Added the `primitive` module to `std` and `core`.][67637] This module reexports Rust's primitive types. This is mainly useful in macros where you want avoid these types being shadowed. - [Relaxed some of the trait bounds on `HashMap` and `HashSet`.][67642] - [`string::FromUtf8Error` now implements `Clone + Eq`.][68738] Stabilized APIs --------------- - [`Once::is_completed`] - [`f32::LOG10_2`] - [`f32::LOG2_10`] - [`f64::LOG10_2`] - [`f64::LOG2_10`] - [`iter::once_with`] Cargo ----- - [You can now set config `[profile]`s in your `.cargo/config`, or through your environment.][cargo/7823] - [Cargo will now set `CARGO_BIN_EXE_<name>` pointing to a binary's executable path when running integration tests or benchmarks.][cargo/7697] `<name>` is the name of your binary as-is e.g. If you wanted the executable path for a binary named `my-program`you would use `env!("CARGO_BIN_EXE_my-program")`. Misc ---- - [Certain checks in the `const_err` lint were deemed unrelated to const evaluation][69185], and have been moved to the `unconditional_panic` and `arithmetic_overflow` lints. Compatibility Notes ------------------- - [Having trailing syntax in the `assert!` macro is now a hard error.][69548] This has been a warning since 1.36.0. - [Fixed `Self` not having the correctly inferred type.][69340] This incorrectly led to some instances being accepted, and now correctly emits a hard error. [69340]: rust-lang/rust#69340 Internal Only ------------- These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of `rustc` and related tools. - [All components are now built with `opt-level=3` instead of `2`.][67878] - [Improved how rustc generates drop code.][67332] - [Improved performance from `#[inline]`-ing certain hot functions.][69256] - [traits: preallocate 2 Vecs of known initial size][69022] - [Avoid exponential behaviour when relating types][68772] - [Skip `Drop` terminators for enum variants without drop glue][68943] - [Improve performance of coherence checks][68966] - [Deduplicate types in the generator witness][68672] - [Invert control in struct_lint_level.][68725] [67332]: rust-lang/rust#67332 [67429]: rust-lang/rust#67429 [67637]: rust-lang/rust#67637 [67642]: rust-lang/rust#67642 [67878]: rust-lang/rust#67878 [67885]: rust-lang/rust#67885 [68129]: rust-lang/rust#68129 [68672]: rust-lang/rust#68672 [68725]: rust-lang/rust#68725 [68728]: rust-lang/rust#68728 [68738]: rust-lang/rust#68738 [68742]: rust-lang/rust#68742 [68764]: rust-lang/rust#68764 [68772]: rust-lang/rust#68772 [68943]: rust-lang/rust#68943 [68952]: rust-lang/rust#68952 [68966]: rust-lang/rust#68966 [68984]: rust-lang/rust#68984 [69022]: rust-lang/rust#69022 [69185]: rust-lang/rust#69185 [69194]: rust-lang/rust#69194 [69201]: rust-lang/rust#69201 [69227]: rust-lang/rust#69227 [69548]: rust-lang/rust#69548 [69256]: rust-lang/rust#69256 [69361]: rust-lang/rust#69361 [69366]: rust-lang/rust#69366 [69538]: rust-lang/rust#69538 [cargo/7823]: rust-lang/cargo#7823 [cargo/7697]: rust-lang/cargo#7697 [`Once::is_completed`]: https://doc.rust-lang.org/std/sync/struct.Once.html#method.is_completed [`f32::LOG10_2`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG10_2.html [`f32::LOG2_10`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG2_10.html [`f64::LOG10_2`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG10_2.html [`f64::LOG2_10`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG2_10.html [`iter::once_with`]: https://doc.rust-lang.org/std/iter/fn.once_with.html
Relax bounds on HashSet constructors Relax bounds on `HashSet::with_hasher` and `with_capacity_and_hasher` to match the bounds on `HashMap` and on `std::collections::HashSet`. This will allow `std::collections::HashSet` to be based on `hashbrown::HashSet`. See also rust-lang/rust#67642
These APIs changed from the old bound listed to the new bound (possibly empty):
K: Hash + Eq -> K
K: Eq + Hash, S: BuildHasher -> K, S
K: Eq + Hash + Debug -> K: Debug
S: BuildHasher -> S
HashMap as Debug
K: Eq + Hash -> K
S: BuildHasher + Default -> S: Default
HashMap as Default
Resolves #44777.