Skip to content

Conversation

@clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Nov 29, 2025

Both of these would be useful for #666, and presumably other downstream hash table implementations.

First, the iter() methods make it possible to implement downstream Debug implementations more easily, by letting iterators like Drain be paused and viewed immutably.

Second, the new unsafe_iter method and corresponding UnsafeIter exists to make implementing hash_map::IterMut a bit more reasonable without being as unsafe. The struct docs should hopefully make its existence clear, but I would definitely love some extra scrutiny on that.

src/table.rs Outdated
Comment on lines 2614 to 2623
/// // Even though the key is mutably borrowed here, this is sound
/// // because we never actually mutate the key before yielding the
/// // immutable reference
/// let (ref key, ref mut val) = self.inner.next()?;
/// Some((key, val))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is just an example, and not part of the actual implementation, I chose to use a clearer implementation instead of the one we actually use.

@clarfonthey clarfonthey force-pushed the iter_iter branch 2 times, most recently from 3f35627 to e2a7538 Compare November 29, 2025 22:22
clarfonthey added a commit to clarfonthey/hashbrown that referenced this pull request Nov 29, 2025
@Amanieu
Copy link
Member

Amanieu commented Nov 30, 2025

Rather than adding a whole new iterator kind, would it make more sense to direct users that need custom variance to the bucket iterator instead?

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Nov 30, 2025

That wouldn't really work because you would then need to mutably borrow the entire table inside the iterator, alongside the bucket iterator, to make it work, which would then give the same variance as IterMut. The only other option would be to make IterMut normally unsafe and variant, which feels like a worse option because I'd imagine a lot of code is just using IterMut and expecting the right thing to happen.


Also because you might be confusing the raw bucket API with the one for HashTable: it returns indices, not pointers, so, it's impossible to convert them back into references without the original table. And, mutably borrowing the table inside an iterator would make the whole thing invariant again, defeating the purpose of the variant version of the iterator.

clarfonthey added a commit to clarfonthey/hashbrown that referenced this pull request Nov 30, 2025
clarfonthey added a commit to clarfonthey/hashbrown that referenced this pull request Nov 30, 2025
clarfonthey added a commit to clarfonthey/hashbrown that referenced this pull request Nov 30, 2025
@Amanieu
Copy link
Member

Amanieu commented Dec 12, 2025

I'm not super happy with this API, but I understand why it is necessary to implement hash_map::IterMut. Since this is such an unusual API, I would prefer if the word "variant" was somewhere in the name, to indicate that the unsafety is because of variance. So maybe IterUnsafeVariantMut? I don't mind it being long, we can say in the documentation that it is primarily intended to be used to implement hash_map::IterMut.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Dec 13, 2025

I think giving it a longer, wordier name would probably be okay; part of my thought process was that it was better to offer a variant mut iterator over just offering an iterator without any lifetimes (i.e., over pointers), but in hindsight, I don't really know if that's worth it, since a correct use requires adding markers anyway.

So, perhaps we could just convert this into an UnsafeIter which returns *mut T instead and adds no markers at all?


I've decided to adopt this new approach: it still has a lifetime, but only superficially.

@clarfonthey clarfonthey changed the title Add hash_table::IterUnsafeMut, iter() method to various iterators Add hash_table::UnsafeIter, iter() method to various iterators Dec 13, 2025
/// Any part of the returned elements which is mutated must be made invariant.
/// See the documentation for [`UnsafeIter`] for an example and further
/// explanation.
pub unsafe fn unsafe_iter(&mut self) -> UnsafeIter<'_, T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this now returns NonNull, I don't think this method needs to be unsafe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants