Tracking for map_entry_recover_keys

[seanmonstar]

No description provided.

[alexcrichton]

Specifically covers:

• VacantEntry::into_key
• OccupiedEntry::remove_pair

for both HashMap and BTreeMap.

I would personally still hesitate a little on the name remove_pair (specifically the pair part) but otherwise seems good to me.

[alexcrichton]

🔔 This issue is now entering a cycle-long final comment period for stabilization 🔔

Note though that the libs team isn't entirely comfortable with the name remove_pair, so we'd like to consider other possibilities during this FCP!

[seanmonstar]

I somewhat like impl Into<(K, V)> for OccupiedEntry, so that it's then let mine = occupied.into();.

[alexcrichton]

Sounds like a great fit to me!

[Stebalien]

Is there any way to recover the key used in the entry from an occupied entry? That is, I'm trying to write a multimap like with the following insert definition:

        let mut key = (key, 0);
        loop {
            match self.values.entry(key) {
                Entry::Vacant(e) => {
                    let idx = e.key().1;
                    e.insert(value);
                    return idx;
                },
                Entry::Occupied(e) => {
                    key = e.??? /* What here? */
                    key.1 += 1;
                }
            }
        }

I don't want to remove the key/value pair from the hash-map, I just want my key back!

[alexcrichton]

@Stebalien I believe not currently, no, but it is the subject of an RFC or two

[Stebalien]

Got it. I just want to make sure the APIs play well together. Specifically, given that we want a way to to move keys out Entry, VacentEntry, and OccupiedEntry, I'd do the following:

1. Remove VacentEntry::into_key.
2. Rename remove_pair to remove or take as opposed to replacing it with an Into<(K, V)> implementation.
3. Add Into<K> implementations for converting entries back into their keys:

impl Into<K> for Entry {}
impl Into<K> for OccupiedEntry {}
impl Into<K> for VacantEntry {}

Note: I'm proposing (2) because having two different Into impls that do very different things (one mutates the map, the other doesn't is probably a bad idea.

[alexcrichton]

Hm yeah that's actually a good point about Into<(K, V)> having odd behavior by mutating the map. I also agree that if we have key recover it'd be good to be consistent across entry types. Thanks for the good points @Stebalien!

[Stebalien]

We can't actually write those Into impls due to coherence... 😢.

[alexcrichton]

The libs team discussed this recently and concluded:

• It does seem odd to use Into for removal from the map, so we'd prefer to avoid.
• Eventually adding Into seems good, but perhaps not necessary immediately
• VacantEntry::into_key seems good enough for now, we can always add Into<K> later if necessary.
• The remove method is unfortunately already taken
• The take method is used on HashSet, setting precedent, but the semantics are removal for a mutable container, not consumption of the container (e.g. Option::take as well)
• Another more plausible name for removal is remove_entry

As a result our conclusions were:

• Let's add into_key, looking to add Into<K> later if necessary
• Let's name the removal method remove_entry