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.

@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 force-pushed the iter_iter branch 2 times, most recently from db9ea5d to 0b15891 Compare December 13, 2025 03:21
@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
@Amanieu Amanieu enabled auto-merge December 17, 2025 23:30
auto-merge was automatically disabled December 18, 2025 02:31

Head branch was pushed to by a user without write access

@clarfonthey
Copy link
Contributor Author

Everything should now properly compile. Got a bit lazy testing changes when dependencies were borked, but now they actually do work.

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