-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Add item recovery collection APIs #1194
Merged
+106
−0
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
320ad8e
create collection recovery RFC
apasel422 e1a90c3
s/item/element/ and move `VacantEntry` stuff into details section
apasel422 ac347d1
s/functionailty/functionality/
apasel422 a195994
remove question of changing `insert`'s replacement behavior
apasel422 5442de0
update for libs team changes
apasel422 b8648e4
rename RFC
apasel422 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
- Feature Name: collection_recovery | ||
- Start Date: 2015-07-08 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Add item-recovery methods to the set types in `std`. Add key-recovery methods to the map types in | ||
`std` in order to facilitate this. | ||
|
||
# Motivation | ||
|
||
Sets are sometimes used as a cache keyed on a certain property of a type, but programs may need to | ||
access the type's other properties for efficiency or functionailty. The sets in `std` do not expose | ||
their items (by reference or by value), making this use-case impossible. | ||
|
||
Consider the following example: | ||
|
||
```rust | ||
use std::collections::HashSet; | ||
use std::hash::{Hash, Hasher}; | ||
|
||
// The `Widget` type has two fields that are inseparable. | ||
#[derive(PartialEq, Eq, Hash)] | ||
struct Widget { | ||
foo: Foo, | ||
bar: Bar, | ||
} | ||
|
||
#[derive(PartialEq, Eq, Hash)] | ||
struct Foo(&'static str); | ||
|
||
#[derive(PartialEq, Eq, Hash)] | ||
struct Bar(u32); | ||
|
||
// Widgets are normally considered equal if all their corresponding fields are equal, but we would | ||
// also like to maintain a set of widgets keyed only on their `bar` field. To this end, we create a | ||
// new type with custom `{PartialEq, Hash}` impls. | ||
struct MyWidget(Widget); | ||
|
||
impl PartialEq for MyWidget { | ||
fn eq(&self, other: &Self) -> bool { self.0.bar == other.0.bar } | ||
} | ||
|
||
impl Eq for MyWidget {} | ||
|
||
impl Hash for MyWidget { | ||
fn hash<H: Hasher>(&self, h: &mut H) { self.0.bar.hash(h); } | ||
} | ||
|
||
fn main() { | ||
// In our program, users are allowed to interactively query the set of widgets according to | ||
// their `bar` field, as well as insert, replace, and remove widgets. | ||
|
||
let mut widgets = HashSet::new(); | ||
|
||
// Add some default widgets. | ||
widgets.insert(MyWidget(Widget { foo: Foo("iron"), bar: Bar(1) })); | ||
widgets.insert(MyWidget(Widget { foo: Foo("nickel"), bar: Bar(2) })); | ||
widgets.insert(MyWidget(Widget { foo: Foo("copper"), bar: Bar(3) })); | ||
|
||
// At this point, the user enters commands and receives output like: | ||
// | ||
// ``` | ||
// > get 1 | ||
// Some(iron) | ||
// > get 4 | ||
// None | ||
// > remove 2 | ||
// removed nickel | ||
// > add 2 cobalt | ||
// added cobalt | ||
// > add 3 zinc | ||
// replaced copper with zinc | ||
// ``` | ||
// | ||
// However, `HashSet` does not expose its items via its `{contains, insert, remove}` methods, | ||
// instead providing only a boolean indicator of the item's presence in the set, preventing us | ||
// from implementing the desired functionality. | ||
} | ||
``` | ||
|
||
# Detailed design | ||
|
||
Add the following item-recovery methods to `std::collections::{BTreeSet, HashSet}`: | ||
|
||
```rust | ||
impl<T> Set<T> { | ||
// Like `contains`, but returns a reference to the item if the set contains it. | ||
fn item<Q: ?Sized>(&self, item: &Q) -> Option<&T>; | ||
|
||
// Like `remove`, but returns the item if the set contained it. | ||
fn remove_item<Q: ?Sized>(&mut self, item: &Q) -> Option<T>; | ||
|
||
// Like `insert`, but replaces the item with the given one and returns the previous item if the | ||
// set contained it. | ||
fn replace(&mut self, item: T) -> Option<T>; | ||
} | ||
``` | ||
|
||
In order to implement the above methods, add the following key-recovery methods to | ||
`std::collections::{BTreeMap, HashMap}`: | ||
|
||
```rust | ||
impl<K, V> Map<K, V> { | ||
// Like `get`, but additionally returns a reference to the entry's key. | ||
fn key_value<Q: ?Sized>(&self, key: &Q) -> Option<(&K, &V)>; | ||
|
||
// Like `get_mut`, but additionally returns a reference to the entry's key. | ||
fn key_value_mut<Q: ?Sized>(&mut self, key: &Q) -> Option<(&K, &mut V)>; | ||
|
||
// Like `remove`, but additionally returns the entry's key. | ||
fn remove_key_value<Q: ?Sized>(&mut self, key: &Q) -> Option<(K, V)>; | ||
|
||
// Like `insert`, but additionally replaces the key with the given one and returns the previous | ||
// key and value if the map contained it. | ||
fn replace(&mut self, key: K, value: V) -> Option<(K, V)>; | ||
} | ||
``` | ||
|
||
For completion, add the following key-recovery methods to | ||
`std::collections::{btree_map, hash_map}::OccupiedEntry`: | ||
|
||
```rust | ||
impl<'a, K, V> OccupiedEntry<'a, K, V> { | ||
// Like `get`, but additionally returns a reference to the entry's key. | ||
fn key_value(&self) -> (&K, &V); | ||
|
||
// Like `get_mut`, but additionally returns a reference to the entry's key. | ||
fn key_value_mut(&mut self) -> (&K, &mut V); | ||
|
||
// Like `into_mut`, but additionally returns a reference to the entry's key. | ||
fn into_key_value_mut(self) -> (&'a K, &'a mut V); | ||
|
||
// Like `remove`, but additionally returns the entry's key. | ||
fn remove_key_value(self) -> (K, V); | ||
} | ||
``` | ||
|
||
# Drawbacks | ||
|
||
This complicates the collection APIs. | ||
|
||
The distinction between `insert` and `replace` may be confusing. It would be more consistent to | ||
call `Set::replace` `Set::insert_item` and `Map::replace` `Map::insert_key_value`, but `BTreeMap` | ||
and `HashMap` do not replace equivalent keys in their `insert` methods, so rather than have | ||
`insert` and `insert_key_value` behave differently in that respect, `replace` is used instead. | ||
|
||
# Alternatives | ||
|
||
Do nothing. | ||
|
||
# Unresolved questions | ||
|
||
Are these the best method names? | ||
|
||
Should `std::collections::{btree_map, hash_map}::VacantEntry` provide methods like | ||
|
||
```rust | ||
impl<'a, K, V> VacantEntry<'a, K, V> { | ||
/// Returns a reference to the entry's key. | ||
fn key(&self) -> &K; | ||
|
||
// Like `insert`, but additionally returns a reference to the entry's key. | ||
fn insert_key_value(self, value: V) -> (&'a K, &'a mut V); | ||
|
||
// Returns the entry's key without inserting it into the map. | ||
fn into_key(self) -> K; | ||
} | ||
``` | ||
|
||
Should `{BTreeMap, HashMap}::insert` be changed to replace equivalent keys? This could break code | ||
relying on the old behavior, and would add an additional inconsistency to `OccupiedEntry::insert`. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hold on, to be clear: "Do nothing" here means "Do nothing and let users write such caches via, e.g.,
HashMap<T, ()>
" ... right?I don't particular mind adding the functionality described here to
HashSet
, but I'm also not sure its strictly necessary, unless I have missed something with howHashMap<T, ()>
would work.Update: Ah, re-reading the RFC, I now see that our current
HashMap
API would not support thatThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not possible to use
HashMap
that way, because it doesn't provide any methods that return&K
(orK
) other than via its iterators.