map_try_insert changes

[malezjaa]

View all comments

Made changes according to #82766 (comment).

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

[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?

[Amanieu]

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.

[tgross35]

r? Amanieu

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

[malezjaa]

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

[cuviper]

hashbrown v0.17.1 now includes rustc_try_insert, so the implementation here should be able to just reconstruct the local error type from that.

[rustbot]

These commits modify the library/Cargo.lock file. Unintentional changes to library/Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[rustbot]

⚠️ Warning ⚠️

• This PR is based on an upstream commit that is older than 28 days.

  It's recommended to update your branch according to the rustc-dev-guide.

[cuviper]

Why did you add this?

View changes since the review

[malezjaa]

My bad, I messed up when rebasing.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.