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

RFC: get collection keys #1175

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
6 participants
@seppo0010
Copy link

seppo0010 commented Jun 24, 2015

@alexcrichton alexcrichton added the T-libs label Jun 24, 2015

@apasel422

This comment has been minimized.

Copy link
Member

apasel422 commented Jun 25, 2015

For clarity, it would be great if this RFC could include the signatures for the new methods, even if the names are still being bikeshedded.

CC rust-lang/rust#26531.

@apasel422

This comment has been minimized.

Copy link
Member

apasel422 commented Jun 25, 2015

And could this also mention the following potential additions to the OccupiedEntry API (modulo naming):

impl<'a, K, V> OccupiedEntry<'a, K, V> {
    fn key(&self) -> &K;
    fn keyed_remove(self) -> (K, V);
    fn keyed_get_mut(&mut self) -> (&K, &mut V);
    fn keyed_get(&self) -> (&K, &V);
}

The last method isn't strictly necessary, but is provided for consistency with keyed_get_mut. keyed_get_mut is necessary because you cannot borrow self mutably and immutably at the same time. For completion, it might also be worthwhile to have

impl<'a, K, V> OccupiedEntry<'a, K, V> {
    fn keyed_into_mut(self) -> (&'a K, &'a mut V);
}

@Gankro Gankro self-assigned this Jun 25, 2015

consistency is unclear since it would only apply for `usize`:

* std::collections::VecMap
* std::collections::BitSet

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

These types are slated for deprecation. Regardless, this RFC is proposing a standard map/set API, and as such all maps/sets should implement them unless infeasible.

pub fn remove_item<Q: ?Sized>(&mut self, value: &Q) -> Option<T>
where T: Borrow<Q>, Q: Hash + Eq;
pub fn insert_item(&mut self, value: T) -> Option<T>;
}

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

I'd nix the Hash* instances. Describe the generic API.

// ...
pub fn keyed_get<Q: ?Sized>(&self, key: &Q) -> Option<(&K, &V)> where K: Borrow<Q>, Q: Ord;
pub fn keyed_get_mut<Q: ?Sized>(&mut self, key: &Q) -> Option<(&K, &mut V)> where K: Borrow<Q>, Q: Ord;

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

Arguably you can yield &mut K for the hypothetical desire to mutate things in the key that don't affect hash/eq/ord. This of course allows you to mess up hash/eq/ord but the collection can't possibly care since that can already be true thanks to:

  • Interior mutability
  • Bad implementations of hash/eq/ord

The collection is already locked up yielding the &mut V, so it's not an ownership problem. In fact the &K is also mutably borrowing the map because of how lifetimes work. Obviously mutating a key in place is super sketchy, and I'm not clear that anyone can reasonably suggest they want this. Still... you could.

This comment has been minimized.

@apasel422

apasel422 Jul 1, 2015

Member

I'm not sure I'm in favor of allowing key mutation, but if we were to expose &mut K, how would that affect IterMut types? For backwards compatibility, we would be unable to yield (&mut K, &mut V) without introducing a new mode of iteration, right?

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

This is a good point. &mut K was honestly more of a musing; I ultimately probably wouldn't go for it, if only as a lint for "you shouldn't be mutating this".


There are currently four operations that could have a new function to them:
`get`, `get_mut`, `insert` and `remove`. All of those methods receive a reference
to a key type and return reference to the value or a the value itself.

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

We generally voice RFCs as "do this thing" and not "we could do this thing".

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

Also this sentence is troubled. Might I suggest "None of these methods provide access to the key" or something.

```

The existing `OccupiedEntry` could benefit from these new methods and provide
its own pretty API on top:

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

Either propose this for certain or add it as an alternative or unresolved question.

# Alternatives

Keep the current design, since keys are immutables once in the collection use
an Rc.

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

This doesn't solve the Set usecase.

an Rc.

Add methods to only fetch the keys, but most of the users will need both or
can easily discard the value.

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

This is a bit strange...

Removing a key from a HashMap or an item from a HashSet will drop the item.
This is usually acceptable for small clonable objects like numbers and strings,
but they are not always clonable, and even if they are it might be an expensive
operation.

This comment has been minimized.

@Gankro

Gankro Jul 1, 2015

Contributor

This needs to be massively fleshed out. I would argue that there are two major motivators:

  • Sets
  • Entries

Entries would primarily like to be able to reflect on the key to complete the construction of the value. Potentially also to decide whether to really do the insertion, though I don't expect this much or at all.

Sets on the other hand would like to be able to talk about the key at all. In particular you might use a Set as a cache by indexing on only a small fragment of the key (e.g. an integer id). For technical reasons it may be infeasible or just ineffecient to split the key into a proper (key, value) pair.

The rest of Maps are for the most part, I think, just a nice-to-have. Largely the extended Map API is just needed to implement the new Set API on top of (since Sets are generally trivial Map wrappers).

@apasel422

This comment has been minimized.

Copy link
Member

apasel422 commented Jul 1, 2015

It's really too bad that we can't change

fn get<Q>(&self, key: &Q) -> Option<&V>;

to

fn get<Q>(&self, key: &Q) -> Option<(&K, &V)>;

due to backwards compatibility concerns. Instead, we're going to end up with an explosion of method variants when this change would subsume all uses. I guess a future Map trait could solve this (i.e. we could wait to add these until then, and only then on the trait impl itself, not as inherent methods), but then the naming would be inconsistent between the concrete types and the traits.

@seppo0010

This comment has been minimized.

Copy link
Author

seppo0010 commented Jul 1, 2015

Thanks for the feedback, @Gankro. I'll update the PR when I get a chance.

One thing I'm not certain is the Entry API. The Entry receives a new Key and the HashMap may already have a Key that's equal to the new one. In that case, which key should be exposed, the existing one or the one used to create the Entry?

@seppo0010

This comment has been minimized.

Copy link
Author

seppo0010 commented Jul 3, 2015

I don't think I can make this happen. Someone else should take care of this if desired.

@seppo0010 seppo0010 closed this Jul 3, 2015

@apasel422

This comment has been minimized.

Copy link
Member

apasel422 commented Jul 3, 2015

I may be interested in taking this over.

@apasel422

This comment has been minimized.

@dhardy

This comment has been minimized.

Copy link
Contributor

dhardy commented Oct 16, 2015

What's the state of this? Does it still need revision with @Gankro 's comments? @apasel422, your link is broken.

Completeness appears to demand a lot of extra functions. I don't know how far it's worth pursuing completeness. For what it's worth, all I want is get_key and remove_key.

@apasel422

This comment has been minimized.

Copy link
Member

apasel422 commented Oct 16, 2015

This was partially addressed by RFC 1194.

@vitiral

This comment has been minimized.

Copy link

vitiral commented Aug 3, 2016

I would also like this, although my use case is mostly "as a newbie I'm confused why this doesn't exist." I am trying to use Rc and want to clone the key for correctness, but can't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.