Skip to content

Make the Entry API of HashMap<K, V> Sync and Send #58369

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 9, 2019
Merged

Make the Entry API of HashMap<K, V> Sync and Send #58369

merged 1 commit into from
Mar 9, 2019

Conversation

nox
Copy link
Contributor

@nox nox commented Feb 11, 2019 •
edited by Mark-Simulacrum
Loading

Fixes #45219

@rust-highfive

This comment has been minimized.

# to view
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 11, 2019
@rust-highfive

This comment has been minimized.

# to view
@nox nox force-pushed the sync-hash-map-entry branch from 5b84f65 to fafe8ce Compare February 11, 2019 11:03
@rust-highfive

This comment has been minimized.

# to view
@nox nox force-pushed the sync-hash-map-entry branch from fafe8ce to 1bcb6b3 Compare February 11, 2019 11:21
@Mark-Simulacrum
Copy link
Member

r? @alexcrichton

@alexcrichton
Copy link
Member

Would it be possible to place these impls at the root cause of why they're not send/sync today? Placing them so high up looks pretty tricky and error prone

@nox
Copy link
Contributor Author

nox commented Feb 11, 2019

Not really, see my comment in the issue:

If we make table::RawBucket Send and Sync itself, then table::Iter will have an auto implementation of Send where K: Send, V: Send, which is incorrect. I would rather have additional impls on the leafs (here OccupiedEntry and VacantEntry) than implementation at the root which can then cause error-prone auto implementations on other leaves.

@nox
Copy link
Contributor Author

nox commented Feb 11, 2019

BTreeMap avoids this issue through a marker on NodeRef, but hash tables lack such a concept.

rust/src/liballoc/collections/btree/node.rs

Lines 348 to 351 in 57d7cfc

unsafe impl<'a, K: Sync + 'a, V: Sync + 'a, Type> Send
for NodeRef<marker::Immut<'a>, K, V, Type> { }
unsafe impl<'a, K: Send + 'a, V: Send + 'a, Type> Send
for NodeRef<marker::Mut<'a>, K, V, Type> { }

@alexcrichton
Copy link
Member

@Amanieu having likely looked at the internals here more recently than I, would you be willing to review this?

@Amanieu
Copy link
Member

Amanieu commented Feb 12, 2019

Entry is already Send+Sync in hashbrown, so this change looks fine to me.

Do we want to gate this behind a feature or should we just make it immediately stable? I don't think this change is very controversial.

@alexcrichton
Copy link
Member

We don't have a way to gate impls like these currently, so it'll be insta-stable. In that case though I'll...

r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Feb 13, 2019

@nox Can you remove the unstable feature and just mark the Send/Sync impls as stable?

@Amanieu
Copy link
Member

Amanieu commented Feb 25, 2019

ping @nox

@nox
Copy link
Contributor Author

nox commented Feb 26, 2019

What version number should I put in the attribute?

@Amanieu
Copy link
Member

Amanieu commented Feb 26, 2019

Since this really should have been done since 1.0, just use the same as the Entry API itself:

#[stable(feature = "rust1", since = "1.0.0")]

@Mark-Simulacrum Mark-Simulacrum changed the title Make the Entry API of HashMap<K, V> Sync and Send (fixes #45219) Make the Entry API of HashMap<K, V> Sync and Send Feb 27, 2019
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2019
@nox nox force-pushed the sync-hash-map-entry branch from 1bcb6b3 to 1fec8c2 Compare March 4, 2019 09:21
@nox
Copy link
Contributor Author

nox commented Mar 4, 2019

Fixed!

@Amanieu
Copy link
Member

Amanieu commented Mar 4, 2019

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Mar 4, 2019

📌 Commit 1fec8c2 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2019
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Mar 8, 2019
@pietroalbini
Rollup merge of rust-lang#58369 - nox:sync-hash-map-entry, r=Amanieu
55dc386 
Make the Entry API of HashMap<K, V> Sync and Send

Fixes rust-lang#45219
@pietroalbini pietroalbini mentioned this pull request Mar 8, 2019
Merged
bors added a commit that referenced this pull request Mar 9, 2019
@bors
Auto merge of #59012 - pietroalbini:rollup, r=pietroalbini
c9f8304 
Rollup of 24 pull requests

Successful merges:

 - #58080 (Add FreeBSD armv6 and armv7 targets)
 - #58204 (On return type `impl Trait` for block with no expr point at last semi)
 - #58269 (Add librustc and libsyntax to rust-src distribution.)
 - #58369 (Make the Entry API of HashMap<K, V> Sync and Send)
 - #58861 (Expand where negative supertrait specific error is shown)
 - #58877 (Suggest removal of `&` when borrowing macro and appropriate)
 - #58883 (Suggest appropriate code for unused field when destructuring pattern)
 - #58891 (Remove stray ` in the docs for the FromIterator implementation for Option)
 - #58893 (race condition in thread local storage example)
 - #58906 (Monomorphize generator field types for debuginfo)
 - #58911 (Regression test for #58435.)
 - #58912 (Regression test for #58813)
 - #58916 (Fix release note problems noticed after merging.)
 - #58918 (Regression test added for an async ICE.)
 - #58921 (Add an explicit test for issue #50582)
 - #58926 (Make the lifetime parameters of tcx consistent.)
 - #58931 (Elide invalid method receiver error when it contains TyErr)
 - #58940 (Remove JSBackend from config.toml)
 - #58950 (Add self to mailmap)
 - #58961 (On incorrect cfg literal/identifier, point at the right span)
 - #58963 (libstd: implement Error::source for io::Error)
 - #58970 (delay_span_bug in wfcheck's ty.lift_to_tcx unwrap)
 - #58984 (Teach `-Z treat-err-as-bug` to take a number of errors to emit)
 - #59007 (Add a test for invalid const arguments)

Failed merges:

 - #58959 (Add release notes for PR #56243)

r? @ghost
@bors bors merged commit 1fec8c2 into rust-lang:master Mar 9, 2019
@Centril Centril added this to the 1.35 milestone Apr 27, 2019
@Centril Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 27, 2019
@Elarnon
Copy link

Elarnon commented May 1, 2019

Won't using since = "1.0.0" be confusing for people trying to figure out why sharing entries doesn't work with old versions of Rust when the official docs says it should? I know I would be confused had I not stumbled on this issue by accident.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 31, 2019
@jperkin
rust: Update to 1.35.0.
6e0b39c 
Version 1.35.0 (2019-05-23)
==========================

Language
--------
- [`FnOnce`, `FnMut`, and the `Fn` traits are now implemented for `Box<FnOnce>`,
  `Box<FnMut>`, and `Box<Fn>` respectively.][59500]
- [You can now coerce closures into unsafe function pointers.][59580] e.g.
  ```rust
  unsafe fn call_unsafe(func: unsafe fn()) {
      func()
  }

  pub fn main() {
      unsafe { call_unsafe(|| {}); }
  }
  ```


Compiler
--------
- [Added the `armv6-unknown-freebsd-gnueabihf` and
  `armv7-unknown-freebsd-gnueabihf` targets.][58080]
- [Added the `wasm32-unknown-wasi` target.][59464]


Libraries
---------
- [`Thread` will now show its ID in `Debug` output.][59460]
- [`StdinLock`, `StdoutLock`, and `StderrLock` now implement `AsRawFd`.][59512]
- [`alloc::System` now implements `Default`.][59451]
- [Expanded `Debug` output (`{:#?}`) for structs now has a trailing comma on the
  last field.][59076]
- [`char::{ToLowercase, ToUppercase}` now
  implement `ExactSizeIterator`.][58778]
- [All `NonZero` numeric types now implement `FromStr`.][58717]
- [Removed the `Read` trait bounds
  on the `BufReader::{get_ref, get_mut, into_inner}` methods.][58423]
- [You can now call the `dbg!` macro without any parameters to print the file
  and line where it is called.][57847]
- [In place ASCII case conversions are now up to 4× faster.][59283]
  e.g. `str::make_ascii_lowercase`
- [`hash_map::{OccupiedEntry, VacantEntry}` now implement `Sync`
  and `Send`.][58369]

Stabilized APIs
---------------
- [`f32::copysign`]
- [`f64::copysign`]
- [`RefCell::replace_with`]
- [`RefCell::map_split`]
- [`ptr::hash`]
- [`Range::contains`]
- [`RangeFrom::contains`]
- [`RangeTo::contains`]
- [`RangeInclusive::contains`]
- [`RangeToInclusive::contains`]
- [`Option::copied`]

Cargo
-----
- [You can now set `cargo:rustc-cdylib-link-arg` at build time to pass custom
  linker arguments when building a `cdylib`.][cargo/6298] Its usage is highly
  platform specific.

Misc
----
- [The Rust toolchain is now available natively for musl based distros.][58575]

[59460]: rust-lang/rust#59460
[59464]: rust-lang/rust#59464
[59500]: rust-lang/rust#59500
[59512]: rust-lang/rust#59512
[59580]: rust-lang/rust#59580
[59283]: rust-lang/rust#59283
[59451]: rust-lang/rust#59451
[59076]: rust-lang/rust#59076
[58778]: rust-lang/rust#58778
[58717]: rust-lang/rust#58717
[58369]: rust-lang/rust#58369
[58423]: rust-lang/rust#58423
[58080]: rust-lang/rust#58080
[57847]: rust-lang/rust#57847
[58575]: rust-lang/rust#58575
[cargo/6298]: rust-lang/cargo#6298
[`f32::copysign`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.copysign
[`f64::copysign`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.copysign
[`RefCell::replace_with`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.replace_with
[`RefCell::map_split`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.map_split
[`ptr::hash`]: https://doc.rust-lang.org/stable/std/ptr/fn.hash.html
[`Range::contains`]: https://doc.rust-lang.org/std/ops/struct.Range.html#method.contains
[`RangeFrom::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeFrom.html#method.contains
[`RangeTo::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeTo.html#method.contains
[`RangeInclusive::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeInclusive.html#method.contains
[`RangeToInclusive::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeToInclusive.html#method.contains
[`Option::copied`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.copied
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.35
Development

Successfully merging this pull request may close these issues.

std::collections::hash_map::{Entry, OccupiedEntry, VacantEntry} are never Send/Sync
8 participants
@nox @rust-highfive @Mark-Simulacrum @alexcrichton @Amanieu @bors @Elarnon @Centril