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

Extend map with referring insertion #60142

Closed
wants to merge 1 commit into from

Conversation

nikita-volkov
Copy link

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.

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.
@rust-highfive
Copy link
Collaborator

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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:1617d87e:start=1555791129192816899,finish=1555791129946635688,duration=753818789
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:04:48]    Compiling rustc-std-workspace-core v1.0.0 (/checkout/src/tools/rustc-std-workspace-core)
[00:04:50]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[00:04:50]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:04:50]    Compiling rustc-demangle v0.1.10
[00:04:53] error: item has missing stability attribute
[00:04:53]     |
[00:04:53]     |
[00:04:53] 697 | /     pub fn insert_and_get_key_value(&mut self, key: K, value: V) -> (&K, &V) {
[00:04:53] 698 | |         match self.entry(key) {
[00:04:53] 699 | |             Occupied(mut entry) => {
[00:04:53] 700 | |                 entry.insert(value);
[00:04:53] 708 | |         }
[00:04:53] 709 | |     }
[00:04:53]     | |_____^
[00:04:53] 
[00:04:53] 
[00:04:53] error: item has missing stability attribute
[00:04:53]     |
[00:04:53]     |
[00:04:53] 714 | /     pub fn insert_and_get_mut_key_value(&mut self, key: K, value: V) -> (&K, &mut V) {
[00:04:53] 715 | |         match self.entry(key) {
[00:04:53] 716 | |             Occupied(mut entry) => {
[00:04:53] 717 | |                 entry.insert(value);
[00:04:53] 725 | |         }
[00:04:53] 726 | |     }
[00:04:53]     | |_____^
[00:04:53] 
---
205992 ./obj/build/cache/2019-04-11
157492 ./obj/build/bootstrap/debug/incremental
156496 ./src/llvm-project/clang
142508 ./obj/build/bootstrap/debug/incremental/bootstrap-hfsog967tquc
142504 ./obj/build/bootstrap/debug/incremental/bootstrap-hfsog967tquc/s-fbhcnq774a-1ljttzl-3k7gym5px36ja
108532 ./src/llvm-project/lldb
100284 ./.git
97584 ./src/llvm-project/clang/test
89964 ./src/llvm-emscripten/test/CodeGen

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 @TimNN. (Feature Requests)

Copy link
Member

@shepmaster shepmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems redundant; the references returned from _mut can be downgraded to immutable at the callsite.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same argument could be applied against get as there is get_mut.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite, as those take &self and &mut self, respectively, while the new methods always take &mut self

@shepmaster
Copy link
Member

shepmaster commented Apr 20, 2019

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.

here is a question on StackOverflow providing an extended discussion on the matter

There's a related question that asks about it for HashSet as well — do you think we should add the equivalent function there?

@hanna-kruppe
Copy link
Contributor

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

@tesuji

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 20, 2019
@SimonSapin
Copy link
Contributor

This looks like a specific ad-hoc API for a very narrow use case. How about something based on Entry instead?

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 Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2019
@Mark-Simulacrum
Copy link
Member

@nikita-volkov Can you modify this pull request to implement the insert method suggested by @SimonSapin? We should also be sure to have this change on HashMap's entry as well as BTreeMap's, I think -- though I haven't investigated in detail whether it makes sense for both.

@nikita-volkov
Copy link
Author

I like the suggestion by @SimonSapin, but I doubt I'll be able to implement it soon.

@Mark-Simulacrum
Copy link
Member

In that case I'm going to close this pull request for now. Feel free to reopen at a later point!

bors added a commit to rust-lang/hashbrown that referenced this pull request Oct 4, 2019
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.
Centril added a commit to Centril/rust that referenced this pull request Oct 9, 2019
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.)
bors added a commit that referenced this pull request Oct 9, 2019
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
Centril added a commit to Centril/rust that referenced this pull request Oct 9, 2019
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.)
bors added a commit that referenced this pull request Oct 9, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants