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

std: Ensure AssertRecoverSafe indeed is more often #30513

Merged
merged 1 commit into from
Dec 25, 2015

Conversation

alexcrichton
Copy link
Member

Types like &AssertRecoverSafe<T> and Rc<AssertRecoverSafe<T>> were
mistakenly not considered recover safe, but the point of the assertion wrapper
is that it indeed is! This was caused by an interaction between the
RecoverSafe and NoUnsafeCell marker traits, and this is updated by adding an
impl of the NoUnsafeCell marker trait for AssertRecoverSafe to ensure that
it never interacts with the other negative impls of RecoverSafe.

cc #30510

@alexcrichton
Copy link
Member Author

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned brson Dec 21, 2015
@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@aturon
Copy link
Member

aturon commented Dec 21, 2015

This makes me slightly nervous given the current naming. NoUnsafeCell could actually be a pretty useful trait in general (though would probably need to be marked unsafe if you really wanted to take advantage). But this change makes it misleading.

I'd be happy to r+ if we can think of a better name for this use. Maybe NoVisibleInternalMutability :P

@alexcrichton
Copy link
Member Author

Hm yes indeed, although we do have an alternative of instead having a manual impl:

impl<T> RecoverSafe for &AssertRecoverSafe<T> {}
impl<T> RecoverSafe for Rc<AssertRecoverSafe<T>> {}
// ...

That just has the downside of being a little repetitive (e.g. needing a positive for every negative impl) and tough to remember (e.g. external crates with a negative RecoverSafe impl may forget the positive for AssertRecoverSafe).

Perhaps a name could be RecoverSafeViaRef, RecoverSafeRef, or RefRecoverSafe? That's kinda what's in play here really.

@aturon
Copy link
Member

aturon commented Dec 21, 2015

RefRcoverSafe SGTM. Thanks!

Types like `&AssertRecoverSafe<T>` and `Rc<AssertRecoverSafe<T>>` were
mistakenly not considered recover safe, but the point of the assertion wrapper
is that it indeed is! This was caused by an interaction between the
`RecoverSafe` and `NoUnsafeCell` marker traits, and this is updated by adding an
impl of the `NoUnsafeCell` marker trait for `AssertRecoverSafe` to ensure that
it never interacts with the other negative impls of `RecoverSafe`.

cc rust-lang#30510
@alexcrichton
Copy link
Member Author

Updated, re-r?

@aturon
Copy link
Member

aturon commented Dec 21, 2015

@bors: r+

@bors
Copy link
Contributor

bors commented Dec 21, 2015

📌 Commit cb3826d has been approved by aturon

@abonander
Copy link
Contributor

If it can be snuck in before bors merges it, can AssertRecoverSafe derive Copy and Clone for better ergonomics?

@bors
Copy link
Contributor

bors commented Dec 22, 2015

⌛ Testing commit cb3826d with merge 5c41d5e...

@bors
Copy link
Contributor

bors commented Dec 22, 2015

💔 Test failed - auto-linux-32-nopt-t

@bluss
Copy link
Member

bluss commented Dec 22, 2015

@bors: retry

@bluss
Copy link
Member

bluss commented Dec 22, 2015

I'm very happy the trait is called RecoverSafe! 😄

@bors
Copy link
Contributor

bors commented Dec 22, 2015

⌛ Testing commit cb3826d with merge 3df4b3e...

@bors
Copy link
Contributor

bors commented Dec 22, 2015

💔 Test failed - auto-mac-64-opt

@alexcrichton
Copy link
Member Author

@bors: retry

On Tue, Dec 22, 2015 at 3:49 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/7497


Reply to this email directly or view it on GitHub
#30513 (comment).

@alexcrichton
Copy link
Member Author

@bors: retry force

@bors
Copy link
Contributor

bors commented Dec 23, 2015

⌛ Testing commit cb3826d with merge bd13175...

@bors
Copy link
Contributor

bors commented Dec 23, 2015

💔 Test failed - auto-linux-64-nopt-t

@sfackler
Copy link
Member

@bors retry

Manishearth added a commit to Manishearth/rust that referenced this pull request Dec 25, 2015
Types like `&AssertRecoverSafe<T>` and `Rc<AssertRecoverSafe<T>>` were
mistakenly not considered recover safe, but the point of the assertion wrapper
is that it indeed is! This was caused by an interaction between the
`RecoverSafe` and `NoUnsafeCell` marker traits, and this is updated by adding an
impl of the `NoUnsafeCell` marker trait for `AssertRecoverSafe` to ensure that
it never interacts with the other negative impls of `RecoverSafe`.

cc rust-lang#30510
bors added a commit that referenced this pull request Dec 25, 2015
@bors bors merged commit cb3826d into rust-lang:master Dec 25, 2015
@alexcrichton alexcrichton deleted the assert-is-safe branch January 21, 2016 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants