Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

btree_map::OccupiedEntry: Send regression #76686

Closed
ghost opened this issue Sep 13, 2020 · 4 comments · Fixed by #76722
Closed

btree_map::OccupiedEntry: Send regression #76686

ghost opened this issue Sep 13, 2020 · 4 comments · Fixed by #76722
Assignees
Labels
A-collections Area: std::collections. C-bug Category: This is a bug. P-critical Critical priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Milestone

Comments

@ghost
Copy link

ghost commented Sep 13, 2020

The code below does not compile on new versions of rustc (bisect below)

use std::collections::{btree_map::Entry, BTreeMap};
use std::thread::spawn;

fn main() {
    let map = BTreeMap::<i32, i32>::new();
    let map = Box::leak(Box::new(map));

    match map.entry(1) {
        Entry::Occupied(entry) => {
            spawn(move || {
                entry.get();
            });
        }
        _ => (),
    }
}
error[E0277]: `NonNull<BTreeMap<i32, i32>>` cannot be sent between threads safely
   --> src/main.rs:10:13
    |
10  |               spawn(move || {
    |  _____________^^^^^_-
    | |             |
    | |             `NonNull<BTreeMap<i32, i32>>` cannot be sent between threads safely
11  | |                 entry.get();
12  | |             });
    | |_____________- within this `[closure@src/main.rs:10:19: 12:14 entry:std::collections::btree_map::OccupiedEntry<'_, i32, i32>]`
    | 
   ::: /home/antek/.local/opt/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/mod.rs:596:8
    |
596 |       F: Send + 'static,
    |          ---- required by this bound in `spawn`
    |
    = help: within `[closure@src/main.rs:10:19: 12:14 entry:std::collections::btree_map::OccupiedEntry<'_, i32, i32>]`, the trait `Send` is not implemented for `NonNull<BTreeMap<i32, i32>>`
    = note: required because it appears within the type `alloc::collections::btree::borrow::DormantMutRef<'_, BTreeMap<i32, i32>>`
    = note: required because it appears within the type `std::collections::btree_map::OccupiedEntry<'_, i32, i32>`
    = note: required because it appears within the type `[closure@src/main.rs:10:19: 12:14 entry:std::collections::btree_map::OccupiedEntry<'_, i32, i32>]`

Bisect

searched nightlies: from nightly-2020-09-11 to nightly-2020-09-12
regressed nightly: nightly-2020-09-12
searched commits: from a1947b3 to 9911160
regressed commit: ee04f9a

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start 2020-09-11 
@ghost ghost added the C-bug Category: This is a bug. label Sep 13, 2020
@jonas-schievink jonas-schievink added A-collections Area: std::collections. T-libs Relevant to the library team, which will review and decide on the PR/issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Sep 13, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 13, 2020
@jonas-schievink
Copy link
Contributor

cc @ssomers @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

We probably just need to unsafe impl<T> Send for DormantMutRef<T> where &mut T: Send {} and likewise for Sync. Conceptually it's no different to a &mut T, so that should be reasonable.

@Mark-Simulacrum Mark-Simulacrum added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 13, 2020
@Mark-Simulacrum Mark-Simulacrum added this to the 1.48.0 milestone Sep 13, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 16, 2020
…ulacrum

Test and fix Send and Sync traits of BTreeMap artefacts

Fixes rust-lang#76686.

I'm not quite sure what all this implies. E.g. comparing with the definitions for `NodeRef` in node.rs,  maybe an extra bound `T: 'a` is useful for something. The test compiles on stable/beta (apart from `drain_filter`) so I bet `Sync` is equally desirable.

r? @Mark-Simulacrum
@spastorino
Copy link
Member

@ssommers can we assign the issue to you given that you've already provided a PR?

@Mark-Simulacrum Mark-Simulacrum self-assigned this Sep 16, 2020
@Mark-Simulacrum
Copy link
Member

I'm going to self-assign, I don't expect to hit any roadblocks with the PR though, it's already bors-ready.

RalfJung added a commit to RalfJung/rust that referenced this issue Sep 19, 2020
…ulacrum

Test and fix Send and Sync traits of BTreeMap artefacts

Fixes rust-lang#76686.

I'm not quite sure what all this implies. E.g. comparing with the definitions for `NodeRef` in node.rs,  maybe an extra bound `T: 'a` is useful for something. The test compiles on stable/beta (apart from `drain_filter`) so I bet `Sync` is equally desirable.

r? @Mark-Simulacrum
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
…ulacrum

Test and fix Send and Sync traits of BTreeMap artefacts

Fixes rust-lang#76686.

I'm not quite sure what all this implies. E.g. comparing with the definitions for `NodeRef` in node.rs,  maybe an extra bound `T: 'a` is useful for something. The test compiles on stable/beta (apart from `drain_filter`) so I bet `Sync` is equally desirable.

r? @Mark-Simulacrum
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
…ulacrum

Test and fix Send and Sync traits of BTreeMap artefacts

Fixes rust-lang#76686.

I'm not quite sure what all this implies. E.g. comparing with the definitions for `NodeRef` in node.rs,  maybe an extra bound `T: 'a` is useful for something. The test compiles on stable/beta (apart from `drain_filter`) so I bet `Sync` is equally desirable.

r? @Mark-Simulacrum
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 20, 2020
…ulacrum

Test and fix Send and Sync traits of BTreeMap artefacts

Fixes rust-lang#76686.

I'm not quite sure what all this implies. E.g. comparing with the definitions for `NodeRef` in node.rs,  maybe an extra bound `T: 'a` is useful for something. The test compiles on stable/beta (apart from `drain_filter`) so I bet `Sync` is equally desirable.

r? @Mark-Simulacrum
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 20, 2020
…ulacrum

Test and fix Send and Sync traits of BTreeMap artefacts

Fixes rust-lang#76686.

I'm not quite sure what all this implies. E.g. comparing with the definitions for `NodeRef` in node.rs,  maybe an extra bound `T: 'a` is useful for something. The test compiles on stable/beta (apart from `drain_filter`) so I bet `Sync` is equally desirable.

r? @Mark-Simulacrum
@bors bors closed this as completed in f5e19a3 Sep 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: std::collections. C-bug Category: This is a bug. P-critical Critical priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants