Extend map with referring insertion #60142
Conversation
The motivation for this is that after inserting an association into map, it becomes impossible to do anything else to either key or the value, because the ownership is transferred to the map. It also is impossible to acquire these references without costly workarounds using the current API. There is a question on StackOverflow providing an extended discussion on the matter: https://stackoverflow.com/q/32401857/485115. Please notice that the implementation of the `insert_and_get_mut_key_value` function might not be optimal and requires further review. The names of the functions are absolutely up for discussion.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @rkruppe (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
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 |
There was a problem hiding this comment.
Note that the hashmap implementation is in the process of being replaced. You'll want to ensure that the new implementation can also support this feature.
| @@ -692,6 +692,39 @@ impl<K: Ord, V> BTreeMap<K, V> { | |||
| } | |||
| } | |||
|
|
|||
| /// Inserts a key-value pair into the map, | |||
| /// returning references to key and value. | |||
| pub fn insert_and_get_key_value(&mut self, key: K, value: V) -> (&K, &V) { | |||
There was a problem hiding this comment.
This function seems redundant; the references returned from _mut can be downgraded to immutable at the callsite.
There was a problem hiding this comment.
The same argument could be applied against get as there is get_mut.
There was a problem hiding this comment.
Not quite, as those take &self and &mut self, respectively, while the new methods always take &mut self
|
I think it would be a good idea to show your usecase for this. Since inserting requires a mutable reference, you can't do anything to the hashmap after you've used this method until you no longer hold the returned reference. That makes the function very limited.
There's a related question that asks about it for |
|
I have no strong opinion on this and adding a new API needs T-libs sign-off anyway, so punting to first libs person that comes to mind: r? @SimonSapin |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
T-libs-api
|
This looks like a specific ad-hoc API for a very narrow use case. How about something based on New: impl Entry<'a, K, V> {
fn insert(self, value: V) -> OccupiedEntry<'a, K, V> {…}
}Existing: impl OccupiedEntry<'a, K, V> {
fn key(&self) -> &K {…}
fn get(&self) -> &V {…}
fn get_mut(&mut self) -> &mut V {…}
fn into_mut(self) -> &'a mut V {…}
} |
Mark-Simulacrum
added
S-waiting-on-author
|
@nikita-volkov Can you modify this pull request to implement the |
|
I like the suggestion by @SimonSapin, but I doubt I'll be able to implement it soon. |
|
In that case I'm going to close this pull request for now. Feel free to reopen at a later point! |
Add RustcVacantEntry::insert_entry for rust-lang/rust#64656 See rust-lang/rust#64656. ~~This was based on v0.5.0 as that's what rustc uses; I can rebase onto master, but I'm not sure whether rustc wants v0.6.0 or if v0.6.0 is rustc-ready.~~ For context, this ultimately provides an API with this shape: ```rust impl Entry<'a, K, V> { fn insert(self, value: V) -> OccupiedEntry<'a, K, V> {…} } ``` to be used when one wants to insert a value without consuming the Entry, e.g. because one wants to keep a reference to the key around. There's more at the original (defunct) PR: rust-lang/rust#60142.
Implement (HashMap) Entry::insert as per rust-lang#60142 Implementation of `Entry::insert` as per @SimonSapin's comment on rust-lang#60142. This requires a patch to hashbrown: ```diff diff --git a/src/rustc_entry.rs b/src/rustc_entry.rs index fefa5c3..7de8300 100644 --- a/src/rustc_entry.rs +++ b/src/rustc_entry.rs @@ -546,6 +546,32 @@ impl<'a, K, V> RustcVacantEntry<'a, K, V> { let bucket = self.table.insert_no_grow(self.hash, (self.key, value)); unsafe { &mut bucket.as_mut().1 } } + + /// Sets the value of the entry with the RustcVacantEntry's key, + /// and returns a RustcOccupiedEntry. + /// + /// # Examples + /// + /// ``` + /// use hashbrown::HashMap; + /// use hashbrown::hash_map::RustcEntry; + /// + /// let mut map: HashMap<&str, u32> = HashMap::new(); + /// + /// if let RustcEntry::Vacant(v) = map.rustc_entry("poneyland") { + /// let o = v.insert_and_return(37); + /// assert_eq!(o.get(), &37); + /// } + /// ``` + #[inline] + pub fn insert_and_return(self, value: V) -> RustcOccupiedEntry<'a, K, V> { + let bucket = self.table.insert_no_grow(self.hash, (self.key, value)); + RustcOccupiedEntry { + key: None, + elem: bucket, + table: self.table + } + } } impl<K, V> IterMut<'_, K, V> { ``` This is also only an implementation for HashMap. I tried implementing for BTreeMap, but I don't really understand BTreeMap's internals and require more guidance on implementing the equivalent `VacantEntry::insert_and_return` such that it returns an `OccupiedEntry`. Notably, following the original PR's modifications I end up needing a `Handle<NodeRef<marker::Mut<'_>, _, _, marker::LeafOrInternal>, _>` while I only have a `Handle<NodeRef<marker::Mut<'_>, _, _, marker::Internal>, _>` and don't know how to proceed. (To be clear, I'm not asking for guidance right now; I'd be happy getting only the HashMap implementation — the subject of this PR — reviewed and ready, and leave the BTreeMap implementation for a latter PR.)
Rollup of 6 pull requests Successful merges: - #64656 (Implement (HashMap) Entry::insert as per #60142) - #64986 (Function pointers as const generic arguments) - #65037 (`#[track_caller]` feature gate (RFC 2091 1/N)) - #65166 (Suggest to add `move` keyword for generator capture) - #65175 (add more info in debug traces for gcu merging) - #65220 (Update LLVM for Emscripten exception handling support) Failed merges: r? @ghost
Implement (HashMap) Entry::insert as per rust-lang#60142 Implementation of `Entry::insert` as per @SimonSapin's comment on rust-lang#60142. This requires a patch to hashbrown: ```diff diff --git a/src/rustc_entry.rs b/src/rustc_entry.rs index fefa5c3..7de8300 100644 --- a/src/rustc_entry.rs +++ b/src/rustc_entry.rs @@ -546,6 +546,32 @@ impl<'a, K, V> RustcVacantEntry<'a, K, V> { let bucket = self.table.insert_no_grow(self.hash, (self.key, value)); unsafe { &mut bucket.as_mut().1 } } + + /// Sets the value of the entry with the RustcVacantEntry's key, + /// and returns a RustcOccupiedEntry. + /// + /// # Examples + /// + /// ``` + /// use hashbrown::HashMap; + /// use hashbrown::hash_map::RustcEntry; + /// + /// let mut map: HashMap<&str, u32> = HashMap::new(); + /// + /// if let RustcEntry::Vacant(v) = map.rustc_entry("poneyland") { + /// let o = v.insert_and_return(37); + /// assert_eq!(o.get(), &37); + /// } + /// ``` + #[inline] + pub fn insert_and_return(self, value: V) -> RustcOccupiedEntry<'a, K, V> { + let bucket = self.table.insert_no_grow(self.hash, (self.key, value)); + RustcOccupiedEntry { + key: None, + elem: bucket, + table: self.table + } + } } impl<K, V> IterMut<'_, K, V> { ``` This is also only an implementation for HashMap. I tried implementing for BTreeMap, but I don't really understand BTreeMap's internals and require more guidance on implementing the equivalent `VacantEntry::insert_and_return` such that it returns an `OccupiedEntry`. Notably, following the original PR's modifications I end up needing a `Handle<NodeRef<marker::Mut<'_>, _, _, marker::LeafOrInternal>, _>` while I only have a `Handle<NodeRef<marker::Mut<'_>, _, _, marker::Internal>, _>` and don't know how to proceed. (To be clear, I'm not asking for guidance right now; I'd be happy getting only the HashMap implementation — the subject of this PR — reviewed and ready, and leave the BTreeMap implementation for a latter PR.)
Rollup of 4 pull requests Successful merges: - #64656 (Implement (HashMap) Entry::insert as per #60142) - #65037 (`#[track_caller]` feature gate (RFC 2091 1/N)) - #65166 (Suggest to add `move` keyword for generator capture) - #65175 (add more info in debug traces for gcu merging) Failed merges: r? @ghost
The motivation for this is that after inserting an association into map, it becomes impossible to do anything else to either key or the value, because the ownership is transferred to the map. It also is impossible to acquire these references without costly workarounds using the current API. There is a question on StackOverflow providing an extended discussion on the matter: https://stackoverflow.com/q/32401857/485115.
Please notice that the implementation of the
insert_and_get_mut_key_valuefunction might not be optimal and requires further review.The names of the functions are absolutely up for discussion.