tgross35 is currently at their maximum review capacity.
They may take a while to respond.

The only thing I wasn't sure about was adding Clone bound to key, even though I think this is the only way to do that, because .entry() consumes the key.

Clone shouldn't be added here, that defeats the purpose of getting the original key back without paying the cost of duplication.

A PR to update https://github.com/rust-lang/hashbrown may be needed first to make rustc_entry also return the key if it's creating an occupied entry. @Amanieu might have suggestions here.

Okay, I will keep this PR a draft for now.

This is a test. It won't compile right now since I was using a local build of hashbrown. Let me know what you think.

malezjaa/hashbrown@82a45ea

Originally I was thinking that rustc_entry should have a return a struct containing RustcEntry and Option<K>, but the idea of putting an Option<K> in OccupiedEntry crossed my mind too because it could come in handy other places. We could bubble this up to std.

I guess I'll re-nominate for @rust-lang/libs-api because there's a tradeoff here. Question: is there any interest in something like this as an alternative to putting the key in the OccupiedError?

pub struct OccupiedEntry<...> {
    hash: u64,
    elem: Bucket<(K, V)>,
    table: &'a mut HashMap<K, V, S, A>,
    original_key: Option<K>, // new field
}

impl<...> OccupiedEntry<...> {
    fn original_key(&mut self) -> &mut Option<K>;
}

The advantage is that users can get the unused owned K back, both for this new try_insert method and for the stable map.entry(key). One disadvantage is that the API shape feels somewhat clunky; it's going to need .take().unwrap() in places where the key is known to exist.

@rustbot label +I-libs-api-nominated

@malezjaa in the meantime feel free to open the HashBrown PR, and please add tests that the key is actually present when expected.

Personally storing the key on OccupiedEntry seems like an easier solution, but that's something that should be decided by the libs team.

It's true that .take().unwrap() is ugly, maybe I could add a method like take_original_key to OccupiedEntry to abstract this?

I don't think that OccupiedEntry should hold an Option<K>, it just doesn't make sense for a type that is supposed to represent an entry in the map. However what we could do is modify RustcEntry to have 2 fields for the occupied variant: it could return the original key in addition to the OccupiedEntry.

r? Amanieu

Agreed that it's not very optimal to hold the key in the entry so I'll unnominate.

The job x86_64-gnu-gcc failed! Check out the build log: (web) (plain enhanced) (plain)

     |                                  ^^^^^  ^^^ expected 1 field, found 2
     |
    ::: /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/hashbrown-0.17.0/src/rustc_entry.rs:67:14
     |
  67 |     Occupied(RustcOccupiedEntry<'a, K, V, A>),
     |              ------------------------------- tuple variant has 1 field

error[E0023]: this pattern has 2 fields, but the corresponding tuple variant has 1 field
    --> library/std/src/collections/hash/map.rs:3003:36
     |
3003 |         base::RustcEntry::Occupied(base, _) => Entry::Occupied(OccupiedEntry { base }),
     |                                    ^^^^  ^ expected 1 field, found 2
     |
    ::: /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/hashbrown-0.17.0/src/rustc_entry.rs:67:14
     |
  67 |     Occupied(RustcOccupiedEntry<'a, K, V, A>),
     |              ------------------------------- tuple variant has 1 field

For more information about this error, try `rustc --explain E0023`.
[RUSTC-TIMING] std test:false 3.550
error: could not compile `std` (lib) due to 2 previous errors

For more information how to resolve CI failures of this job, visit this link.

Maybe I should keep this as a draft until new version of hashbrown gets released?