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.

Sign up for GitHub

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

Jump to bottom

MutexGuard<T> may be Sync only if T is Sync #41624

Merged
merged 2 commits into from
May 3, 2017
Merged

MutexGuard<T> may be Sync only if T is Sync #41624

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
998a877
MutexGuard<T> may be Sync only if T is Sync
RalfJung Apr 29, 2017
23522f6
need to pick a new feature name
RalfJung Apr 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/libstd/sync/mutex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

use cell::UnsafeCell;
use fmt;
use marker;
use mem;
use ops::{Deref, DerefMut};
use ptr;
Expand Down Expand Up @@ -153,7 +152,9 @@ pub struct MutexGuard<'a, T: ?Sized + 'a> {
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<'a, T: ?Sized> !marker::Send for MutexGuard<'a, T> {}
impl<'a, T: ?Sized> !Send for MutexGuard<'a, T> { }
#[stable(feature = "rust1", since = "1.18.0")]
unsafe impl<'a, T: ?Sized + Sync> Sync for MutexGuard<'a, T> { }
Copy link
Member

@bluss bluss Apr 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

A brief test says that this is not enough to make T: !Sync → MutexGuard<T>: !Sync
Is a non-sync marker field the best way? Then opt in to Sync for this case.

Edit: Ok, sorry, that was of course a too simple test. Seems to work

Copy link
Member

@eddyb eddyb Apr 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So this is where I was arguing with @nikomatsakis that the current semantics for positive OIBIT impls don't make sense in terms of how they apply bounds and when, when compared with other impls.
That is, I'd expect to see these two impls:

impl<'a, T: ?Sized> !Sync for MutexGuard<'a, T> { }
unsafe impl<'a, T: ?Sized + Sync> Sync for MutexGuard<'a, T> { }

Carving out a Sync subset out of a more general !Sync case.
Indeed that is what a !Sync marker field would do.

EDIT: Okay, so it's not broken but it still bugs me how it's set up.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bluss: My compile-fail test seems to indicate this works, but I have to admit I don't know enough about OIBITs to say how this is supposed to be done.

@eddyb: I was not sure if that would work; are overlapping impls handed correctly? If they do, sure, I'm happy to change this. I will then also add a test checking that MutexGuard<i32> is Sync.

Copy link
Member

Choose a reason for hiding this comment

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

It fooled me, for one. The positive impls are stable though (and the negative ones are not).

Copy link
Member Author

Choose a reason for hiding this comment

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

@eddyb This doesn't seem to work...

error[E0119]: conflicting implementations of trait `core::marker::Sync` for type `sync::mutex::MutexGuard<'_, _>`:
   --> src/libstd/sync/mutex.rs:159:1
    |
157 | impl<'a, T: ?Sized> !Sync for MutexGuard<'a, T> { }
    | --------------------------------------------------- first implementation here
158 | #[stable(feature = "rust1", since = "1.18.0")]
159 | unsafe impl<'a, T: ?Sized + Sync> Sync for MutexGuard<'a, T> { }
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `sync::mutex::MutexGuard<'_, _>`

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean "stable" as in "a stable Rust feature" or "stable" as in "preserved by more impls being added elsewhere"?

Copy link
Member

Choose a reason for hiding this comment

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

No, it doesn't, but I would expect it to be the only way to achieve this result, and this is me trying to remember @nikomatsakis about a previous discussion on the matter.

Copy link
Member

Choose a reason for hiding this comment

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

The former, @RalfJung

Copy link
Contributor

@withoutboats withoutboats Apr 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@eddyb I argued the same thing with @nikomatsakis at RustConf, but he demonstrated an example in which that would result in adding a non-blanket impl of a non-OIBIT being a breaking change, something we've worked very hard to avoid.

In any event we do need an RFC to revise and clarify these rules because they're currently mostly unstated.


impl<T> Mutex<T> {
/// Creates a new mutex in an unlocked state ready for use.
Expand Down Expand Up @@ -459,9 +460,6 @@ mod tests {
#[derive(Eq, PartialEq, Debug)]
struct NonCopy(i32);

unsafe impl<T: Send> Send for Packet<T> {}
unsafe impl<T> Sync for Packet<T> {}

#[test]
fn smoke() {
let m = Mutex::new(());
Expand Down
22 changes: 22 additions & 0 deletions src/test/compile-fail/mutexguard-sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// MutexGuard<Cell<i32>> must not be Sync, that would be unsound.
use std::sync::Mutex;
use std::cell::Cell;

fn test_sync<T: Sync>(_t: T) {}

fn main()
{
let m = Mutex::new(Cell::new(0i32));
let guard = m.lock().unwrap();
test_sync(guard); //~ ERROR the trait bound
}