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.

Sign up for GitHub

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

Improve {BTreeMap,HashMap}::get_key_value docs. #132758

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Improve {BTreeMap,HashMap}::get_key_value docs. #132758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 42 additions & 4 deletions library/alloc/src/collections/btree/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,20 +677,58 @@ impl<K, V, A: Allocator + Clone> BTreeMap<K, V, A> {
}
}

/// Returns the key-value pair corresponding to the supplied key.
/// Returns the key-value pair corresponding to the supplied key. This is
/// potentially useful:
/// - for key types where non-identical keys can be considered equal;
/// - for getting the `&K` stored key value from a borrowed `&Q` lookup key; or
/// - for getting a reference to a key with the same lifetime as the collection.
///
/// The supplied key may be any borrowed form of the map's key type, but the ordering
/// on the borrowed form *must* match the ordering on the key type.
///
/// # Examples
///
/// ```
/// use std::cmp::Ordering;
/// use std::collections::BTreeMap;
///
/// #[derive(Clone, Copy, Debug)]
/// struct S {
/// id: u32,
/// # #[allow(unused)] // prevents a "field `name` is never read" error
/// name: &'static str, // ignored by equality and ordering operations
/// }
///
/// impl PartialEq for S {
/// fn eq(&self, other: &S) -> bool {
/// self.id == other.id
/// }
/// }
///
/// impl Eq for S {}
///
/// impl PartialOrd for S {
/// fn partial_cmp(&self, other: &S) -> Option<Ordering> {
/// self.id.partial_cmp(&other.id)
/// }
/// }
///
/// impl Ord for S {
/// fn cmp(&self, other: &S) -> Ordering {
/// self.id.cmp(&other.id)
/// }
/// }
///
/// let j_a = S { id: 1, name: "Jessica" };
/// let j_b = S { id: 1, name: "Jess" };
/// let p = S { id: 2, name: "Paul" };
/// assert_eq!(j_a, j_b);
///
/// let mut map = BTreeMap::new();
/// map.insert(1, "a");
/// assert_eq!(map.get_key_value(&1), Some((&1, &"a")));
/// assert_eq!(map.get_key_value(&2), None);
/// map.insert(j_a, "Paris");
/// assert_eq!(map.get_key_value(&j_a), Some((&j_a, &"Paris")));
/// assert_eq!(map.get_key_value(&j_b), Some((&j_a, &"Paris"))); // the notable case
/// assert_eq!(map.get_key_value(&p), None);
Copy link
Member

Choose a reason for hiding this comment

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

Muad'Dib knew that every experience carries its lesson.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Static typing is better than dynamic typing.

- Winston Churchill

/// ```
#[stable(feature = "map_get_key_value", since = "1.40.0")]
pub fn get_key_value<Q: ?Sized>(&self, k: &Q) -> Option<(&K, &V)>
Expand Down
40 changes: 36 additions & 4 deletions library/std/src/collections/hash/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,11 @@ where
self.base.get(k)
}

/// Returns the key-value pair corresponding to the supplied key.
/// Returns the key-value pair corresponding to the supplied key. This is
Copy link
Contributor

@Kobzol Kobzol Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When you index a hashmap, you don't actually use K for indexing, but you use Q that upholds K: Borrow<Q>. So another use-case could be that you want to inspect &K when you only have &Q, right? Like if you have &str as an indexing key for HashMap<String, T>, but you want to inspect e.g. the capacity of the &String key.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think the K/Q difference would be more interesting to demonstrate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, let me explain how this PR came about. This method has one genuine use in the compiler, in rustc_resolve. (There are three call sites which are all extremely similar.) I had never heard of this method, so I looked up the docs and found them unhelpful. After more digging I discovered that the method was being used in the way I described/demonstrated in this PR: with a type where one of the fields was being ignored.

So while I can see that the &K/&Q case is possible, the very limited real-world experience I have suggests the "ignored field" case is more interesting. "you want to inspect e.g. the capacity of the &String key" feels not that useful.

Also, I just looked through the version control history. The get_key_value methods were added in #49346, implementing the suggestion from #43143. The only justification there is "sometimes you need a reference to a key with the lifetime of the collection", which is a third motivation, but there are no actual examples and surprisingly little discussion :(

So now my inclination is to expand the text description of the methods to include the &K/&Q case and the "reference to a key with the lifetime of the collection" case, but keep the examples on the "ignore a field" case. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good to me. I agree that your use-case is probably the most useful one.

/// potentially useful:
/// - for key types where non-identical keys can be considered equal;
/// - for getting the `&K` stored key value from a borrowed `&Q` lookup key; or
/// - for getting a reference to a key with the same lifetime as the collection.
///
/// The supplied key may be any borrowed form of the map's key type, but
/// [`Hash`] and [`Eq`] on the borrowed form *must* match those for
Expand All @@ -890,11 +894,39 @@ where
///
/// ```
/// use std::collections::HashMap;
/// use std::hash::{Hash, Hasher};
///
/// #[derive(Clone, Copy, Debug)]
/// struct S {
/// id: u32,
/// # #[allow(unused)] // prevents a "field `name` is never read" error
/// name: &'static str, // ignored by equality and hashing operations
/// }
///
/// impl PartialEq for S {
/// fn eq(&self, other: &S) -> bool {
/// self.id == other.id
/// }
/// }
///
/// impl Eq for S {}
///
/// impl Hash for S {
/// fn hash<H: Hasher>(&self, state: &mut H) {
/// self.id.hash(state);
/// }
/// }
///
/// let j_a = S { id: 1, name: "Jessica" };
/// let j_b = S { id: 1, name: "Jess" };
/// let p = S { id: 2, name: "Paul" };
/// assert_eq!(j_a, j_b);
///
/// let mut map = HashMap::new();
/// map.insert(1, "a");
/// assert_eq!(map.get_key_value(&1), Some((&1, &"a")));
/// assert_eq!(map.get_key_value(&2), None);
/// map.insert(j_a, "Paris");
/// assert_eq!(map.get_key_value(&j_a), Some((&j_a, &"Paris")));
/// assert_eq!(map.get_key_value(&j_b), Some((&j_a, &"Paris"))); // the notable case
/// assert_eq!(map.get_key_value(&p), None);
/// ```
#[inline]
#[stable(feature = "map_get_key_value", since = "1.40.0")]
Expand Down
Loading