Send sync implementation on iterators

[GuillaumeGomez]

Part of #22709.
cc @Veedrac

r? @bluss

[bluss]

We'd only need Send + Sync on table::RawBuckets, and the rest should follow by itself. Is that feasible @gankro? (RawTable uses Unique and should be fine already).

I had another question for you, do you know why RawBucket is using PhantomData<(K, V)>? I don't think it owns a K, but I don't have it all clear like you.

[Gankra]

@bluss it ptr::read's out K,V pairs. Not sure if that necessitates it... this code has seen a lot of changes in the Phantom story.

RawBuckets should... basically be fine to impl for? Doesn't really matter since it's internal.

[GuillaumeGomez]

So... What should I add/remove/update ?

[bluss]

@gankro It matters if all the things using RawBuckets would automatically inherit Send + Sync

[Gankra]

Nothing in hashmap uses shared state/internal mutability so it's all trivially thread-safe, as far as I know.

[bluss]

@GuillaumeGomez If you impl Send + Sync for RawBuckets instead, all the rest should be inherited automatically.

[bluss]

Can you add the iterators to sync-send-iterators-in-libcollections.rs? (Or are they somewhere else?)

[GuillaumeGomez]

@bluss: sure !

[GuillaumeGomez]

I added the test.

[bluss]

Thanks! I'd prefer you squash the commits together to clean it up.

r=me with squash

[huonw]

Hm, it's not obvious to me that this is the right thing. Maybe this should be K: Sync, V: Sync? Basically, I'm asking what reference/value semantics does RawBuckets have with K and V?

[bluss]

Giving this issue to huon, who is much more experienced in the details of correct Sync and Send.

[bluss]

So, my plan make this easier by impl on RawBuckets was misguided, and this is because the different iterators do actually need different bounds. The Send/Sync impl for RawBuckets is not correct -- its behavior depends on use.

For reference, see this comment for the different cases. The guidelines should be directly applicable since the HashMap does not do any funny business with their contents during iteration.

We only need to impl Send / Sync for the basic building blocks. In this case this is table::{Iter, IterMut, IntoIter, Drain} inside src/libstd/collections/hash/table.rs. The HashMap and HashSet iterators will inherit Send / Sync from these.

• impl<'a, K: Sync, V: Sync> Sync for Iter<'a, K, V>
• impl<'a, K: Sync, V: Sync> Send for Iter<'a, K, V>
• impl<'a, K: Sync, V: Sync> Sync for IterMut<'a, K, V>
• impl<'a, K: Sync, V: Send> Send for IterMut<'a, K, V> // **Note**
• impl<K: Sync, V: Sync> Sync for IntoIter<K, V>
• impl<K: Send, V: Send> Send for IntoIter<K, V>
• impl<'a, K: Sync, V: Sync> Sync for Drain<'a, K, V>
• impl<'a, K: Send, V: Send> Send for Drain<'a, K, V>

[huonw]

This needs to be K: Sync, since Iter yields/behaves like &K.

[alexcrichton]

Can these be automatically inferred by applying the traits to the underlying Keys iterator?

[alexcrichton]

Er actually, all of the modifications in this file should not be necessary as they're built on top of the map iterators

[huonw]

@bors r+

[bors]

📌 Commit f2f4a5c has been approved by huonw

[bors]

⌛ Testing commit f2f4a5c with merge 542d56e...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt