map_try_insert changes

[malezjaa]

Made changes according to #82766 (comment).
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.

r? @tgross35

[rustbot]

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

[tgross35]

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.

[malezjaa]

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

[malezjaa]

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]

malezjaa/hashbrown@82a45ea

[rust-log-analyzer]

The job aarch64-gnu-llvm-21-1 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

##[endgroup]
Executing "/scripts/stage_2_test_set1.sh"
+ /scripts/stage_2_test_set1.sh
+ '[' 1 == 1 ']'
+ echo 'PR_CI_JOB set; skipping tidy'
+ SKIP_TIDY='--skip tidy'
+ ../x.py --stage 2 test --skip tidy --skip compiler --skip src
PR_CI_JOB set; skipping tidy
##[group]Building bootstrap
    Finished `dev` profile [unoptimized] target(s) in 0.04s
##[endgroup]
---
   Compiling addr2line v0.25.1
[RUSTC-TIMING] addr2line test:false 0.732
[RUSTC-TIMING] gimli test:false 5.838
[RUSTC-TIMING] object test:false 8.368
error[E0599]: no method named `uninserted_key` found for struct `RustcOccupiedEntry<'a, K, V, A>` in the current scope
    --> library/std/src/collections/hash/map.rs:1358:38
     |
1358 |                 let key = entry.base.uninserted_key().take().expect("should exist");
     |                                      ^^^^^^^^^^^^^^ method not found in `RustcOccupiedEntry<'_, K, V, A>`

For more information about this error, try `rustc --explain E0599`.
[RUSTC-TIMING] std test:false 6.190
error: could not compile `std` (lib) due to 1 previous error
Bootstrap failed while executing `--stage 2 test --skip tidy --skip compiler --skip src`

[tgross35]

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.

[malezjaa]

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?