hashbrown icon indicating copy to clipboard operation
hashbrown copied to clipboard

Feature: `HashTable::try_insert_unique_within_capacity`

Open morrisonlevi opened this issue 10 months ago • 4 comments

I'm experimenting with no_panic in FFI calls which use HashTable. The goal is to "prove" that the extern "C" function cannot panic, which is a nice goal because it's not allowed to.

I can use HashTable::try_reserve to avoid the panic in practice. However, that's not good enough for no_panic; if the code for panics exists when linking, then no_panic will cause the crate to fail to build. The problem specifically with insert_unique is that it calls RawTable::insert which calls self.reserve(1, hasher), which will panic out of necessity if allocation fails.

So, I propose add HashTable::try_insert_unique_within_capacity which looks like:

    pub fn try_insert_unique_within_capacity(
        &mut self,
        hash: u64,
        value: T,
    ) -> Result<OccupiedEntry<'_, T, A>, T>

~And then we remove the feature gate of #[cfg(feature = "rustc-internal-api")] from RawTable::insert_no_grow, or create an equivalent method that's only pub(crate) or something.~ Edit: reworked to be safe and return Result instead.

This doesn't seem difficult at all--is there maybe a reason it doesn't already exist?

morrisonlevi avatar Apr 22 '25 00:04 morrisonlevi

HashTable has mostly avoided unsafe methods, except for get_many_unchecked_mut.

Maybe we can add something that returns Result<OccupiedEntry<'_, T, A>, T> instead? Then you can decide what to do with the error case that gave the value back -- you can hint::unreachable_unchecked() if you're really sure.

cuviper avatar May 02 '25 18:05 cuviper

FWIW, indexmap could use this idea for a cleaner panic here: https://github.com/indexmap-rs/indexmap/blob/1818d4140d86aeef18c515f1b060a3fa68da2708/src/map/core.rs#L86

(but I do want to stick to safe APIs as much as possible)

cuviper avatar May 02 '25 18:05 cuviper

I think the idea to keep a safe API and use hint::unreachable_unchecked() in the caller is a good one. I've opened a PR and verified that it's no-panic compatible.

morrisonlevi avatar May 04 '25 20:05 morrisonlevi

I think it should be renamed try_insert_unique_within_capacity. I'll update the PR too.

morrisonlevi avatar May 06 '25 18:05 morrisonlevi