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

Test and fix Send and Sync traits of BTreeMap artefacts #76722

Merged
merged 1 commit into from
Sep 20, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Sep 14, 2020

Fixes #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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 14, 2020
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 14, 2020
Comment on lines 19 to 20
unsafe impl<'a, T: Sync> Sync for DormantMutRef<'a, T> {}
unsafe impl<'a, T: Send> Send for DormantMutRef<'a, T> {}
Copy link
Member

Choose a reason for hiding this comment

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

Don't you want &mut T to be Send, not T?

Suggested change
unsafe impl<'a, T: Sync> Sync for DormantMutRef<'a, T> {}
unsafe impl<'a, T: Send> Send for DormantMutRef<'a, T> {}
unsafe impl<'a, T> Sync for DormantMutRef<'a, T> where &'a mut T: Sync {}
unsafe impl<'a, T> Send for DormantMutRef<'a, T> where &'a mut T: Send {}

Copy link
Member

Choose a reason for hiding this comment

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

Is there any tangible difference? That implementation is already:

unsafe impl<T: Send + ?Sized> Send for &mut T {}

And I believe Sync for &mut T is auto-implemented for T: Sync.

Copy link
Member

Choose a reason for hiding this comment

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

You could have unsafe impl<T> Send for &mut T {}, even if T is not Send.

Copy link
Member

Choose a reason for hiding this comment

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

And in general this is conceptually a &mut T so it makes sense to use the same rules as &mut T for it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think they might match up in practice, but I didn't want to replicate impls when we could just use this.

Copy link
Member

Choose a reason for hiding this comment

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

I guess core could pull that trick, but nobody else:

struct Foo(*const i32);

unsafe impl Send for &mut Foo {}
error[E0119]: conflicting implementations of trait `std::marker::Send` for type `&mut Foo`:
 --> src/lib.rs:3:1
  |
3 | unsafe impl Send for &mut Foo {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: conflicting implementation in crate `core`:
          - impl<T> std::marker::Send for &mut T
            where T: std::marker::Send, T: ?Sized;

error[E0321]: cross-crate traits with a default impl, like `std::marker::Send`, can only be implemented for a struct/enum type, not `&mut Foo`
 --> src/lib.rs:3:1
  |
3 | unsafe impl Send for &mut Foo {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't implement cross-crate trait with a default impl for non-struct/enum type

@Mark-Simulacrum
Copy link
Member

@jyn514 is correct, I think, that we want these in terms of &mut T.

r=me with that change made

@ssomers
Copy link
Contributor Author

ssomers commented Sep 15, 2020

Also got more fine grained testing to compile and added newcomers into_keys and into_values.

r= @Mark-Simulacrum

@jyn514
Copy link
Member

jyn514 commented Sep 15, 2020

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Sep 15, 2020

📌 Commit 176956c has been approved by Mark-Simulacrum

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request 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
RalfJung added a commit to RalfJung/rust that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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
Copy link
Member

@bors rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2020
Rollup of 15 pull requests

Successful merges:

 - rust-lang#76722 (Test and fix Send and Sync traits of BTreeMap artefacts)
 - rust-lang#76766 (Extract some intrinsics out of rustc_codegen_llvm)
 - rust-lang#76800 (Don't generate bootstrap usage unless it's needed)
 - rust-lang#76809 (simplfy condition in ItemLowerer::with_trait_impl_ref())
 - rust-lang#76815 (Fix wording in mir doc)
 - rust-lang#76818 (Don't compile regex at every function call.)
 - rust-lang#76821 (Remove redundant nightly features)
 - rust-lang#76823 (black_box: silence unused_mut warning when building with cfg(miri))
 - rust-lang#76825 (use `array_windows` instead of `windows` in the compiler)
 - rust-lang#76827 (fix array_windows docs)
 - rust-lang#76828 (use strip_prefix over starts_with and manual slicing based on pattern length (clippy::manual_strip))
 - rust-lang#76840 (Move to intra doc links in core/src/future)
 - rust-lang#76845 (Use intra docs links in core::{ascii, option, str, pattern, hash::map})
 - rust-lang#76853 (Use intra-doc links in library/core/src/task/wake.rs)
 - rust-lang#76871 (support panic=abort in Miri)

Failed merges:

r? `@ghost`
@bors bors merged commit f5e19a3 into rust-lang:master Sep 20, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 20, 2020
@ssomers ssomers deleted the btree_send_sync branch September 21, 2020 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

btree_map::OccupiedEntry: Send regression
8 participants