Add `Shared` pointer and have `{Arc, Rc}` use it #29110

apasel422 · 2015-10-16T21:54:56Z

apasel422 · 2015-10-16T21:57:20Z

If there isn't sufficient motivation to expose Shared at this point, then I can make it a type local to liballoc.

pnkfelix · 2015-10-16T22:29:39Z

src/test/run-pass/issue-29037.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+use std::rc::Rc;


please add a comment explaining what this is testing.

(I know it is testing that for each of the P<X> types, P is covariant in X, just write a comment saying as much, so that someone doesn't have to go looking up the issue.)

apasel422 · 2015-10-16T22:30:11Z

This may also count as a breaking change, though I'll wait for someone else to chime in before amending the commit message.

pnkfelix · 2015-10-16T22:33:08Z

lgtm, r=me after above comment is addressed.

I don't have much of an opinion on the Shared thing ... I don't mind adding it even if its only used in these two places. But if others (e.g. @gankro or @alexcrichton ) think that it is pre-mature abstraction, we can remove it.

It probably is a good idea to add the [breaking-change] marker, with a note pointing to the kind of example code that now is rejected. (Just point the reader at the test case that is rejected by dropck now, for example.)

This change has two consequences: 1. It makes `Arc<T>` and `Rc<T>` covariant in `T`. 2. It causes the compiler to reject code that was unsound with respect to dropck. See compile-fail/issue-29106.rs for an example of code that no longer compiles. Because of this, this is a [breaking-change]. Fixes #29037. Fixes #29106.

apasel422 · 2015-10-16T22:43:17Z

Updated to address @pnkfelix's comments.

alexcrichton · 2015-10-16T22:51:03Z

src/libstd/sync/mutex.rs

@@ -293,6 +293,7 @@ impl<T: ?Sized> Mutex<T> {

 #[stable(feature = "rust1", since = "1.0.0")]
 impl<T: ?Sized> Drop for Mutex<T> {
+    #[unsafe_destructor_blind_to_params]


Were this and the rwlock changes needed for this PR? I have a feeling this probably isn't too bad, but I just want to make sure that this wasn't something super subtle that came up when fixing Rc and Arc

Aha I see it was needed to make the tests pass, makes sense to me!

These were necessary to keep the run-pass/dropck_legal_cycles.rs test passing. From what I understand, it was an oversight that the attribute wasn't applied in these locations when the same changes were made to the collection types' destructors in #28861.

yeah they are needed; otherwise the dropck-legal-cycles test won't pass. @apasel422 mentioned this on the other ticket, but not here.

@apasel422 yeah the oversight was that I shouldn't have said "boy that was easy, didn't have to add that attribute in too many places"; I should have said "hmm, why are these tests passing currently without that attribute ... ooh, missing PhantomData"

alexcrichton · 2015-10-16T22:52:45Z

I'm fine with Shared as an unstable abstraction for now. Is there expected breakage for cases that are otherwise legitimate? In the examples here and in the issue they're trivially memory unsafe so it's fine to me if we break them, but we may also want to run a crater run on this if we're not sure that's the limit of the breakage.

pnkfelix · 2015-10-16T22:55:51Z

@alexcrichton I suspect that this fix won't actually break very much code in the wild. But I can wait to pull the r=me trigger in a bors msg if you want to wait for a crater run first. (However I have yet to successfully run crater myself -- niko had to do it for me the last time I had something to pass through it ... and that was before my machine started acting up on me :( )

alexcrichton · 2015-10-16T22:58:02Z

OK, in that case this seems like it pretty clearly falls into the category of "this is a soundness fix", so I'm fine letting this through. The standard crater runs should alert us to any breakage if it comes up.

I don't think this can be backported either, right? It looks like the support for the CoerceUnsized business may only be on nightly?

@bors: r+ d6bd8d8

Thanks @apasel422!

pnkfelix · 2015-10-16T23:05:54Z

@alexcrichton ~~we could backport just the addition of PhantomData to Rc and Arc, as a separate patch.~~

I'm on the fence about it, just because I don't like injecting too much divergence between the code bases. But they are unsound today without that, as one can see by running @apasel422 's test case from this ticket against stable. Or if you prefer seeing one that drives the point home a bit more obviously: http://is.gd/lvcTmN

Update: OH, yeah, the CoerceUnsized stuff, I forgot about that. Okay, yeah, then forget back-porting then.

bkoropoff · 2015-10-17T00:14:10Z

src/libcore/ptr.rs

+    type Target = *mut T;
+
+    #[inline]
+    fn deref(&self) -> &*mut T {


This interface feels a little off to me, but since this isn't a stable API it can be bikeshedded later.

It's the same interface exposed by Unique.

It's a bad interface because it removes the optimizations NonZero can trigger (the control-flow one, not the layout optimizations which are unaffected).
NonZero and wrappers such as Unique and Shared (which seem superfluous to me, btw, especially with all this extra boiler plating), should use a Cell-like API, allowing LLVM to reason about their non-nullability.

bkoropoff · 2015-10-17T00:17:09Z

Looks good, thanks for fixing this.

apasel422 · 2015-10-17T00:27:06Z

I'm wondering if this makes the two Weak<T> types overly conservative, as neither of them can drop a T in their destructor. Perhaps I should change them to use NonZero<*const T> instead?

apasel422 · 2015-10-17T00:28:40Z

Or maybe it doesn't matter, because there would be no way to obtain a Weak<T> that could make use of the greater flexibility?

bors · 2015-10-17T09:51:31Z

⌛ Testing commit d6bd8d8 with merge e1944b6...

@pnkfelix

Fixes #29037. Fixes #29106. r? @pnkfelix CC @gankro

eddyb · 2015-10-17T10:31:42Z

src/libcore/ptr.rs

+    //
+    // For details, see:
+    // https://github.com/rust-lang/rfcs/blob/master/text/0769-sound-generic-drop.md#phantom-data
+    _marker: PhantomData<T>,


How does this not affect variance? Or are you saying that it has the same effect owning *const T above has, therefore it doesn't have additional consequences for variance?

This is the same note that is in Unique above.

bors · 2015-10-17T11:38:09Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

SimonSapin · 2015-10-19T19:34:45Z

Is this more likely to be stabilized than NonZero (and could be a way to get the Option<NonZero<T>> layout optimization on stable Rust)?

Gankra · 2015-10-19T19:36:07Z

@SimonSapin yes, I would recommend this API for stabilization fast-track.

bstrie · 2015-10-21T06:41:59Z

@gankro before you nominate this API for stabilization, have you seen @eddyb's performance objections?

Gankra · 2015-10-21T13:44:35Z

@bstrie yes, eddyb made that comment after mine.

rust-highfive assigned pnkfelix Oct 16, 2015

pnkfelix reviewed Oct 16, 2015
View reviewed changes

alexcrichton reviewed Oct 16, 2015
View reviewed changes

bkoropoff reviewed Oct 17, 2015
View reviewed changes

bors added a commit that referenced this pull request Oct 17, 2015

Auto merge of #29110 - apasel422:shared, r=alexcrichton

e1944b6

Fixes #29037. Fixes #29106. r? @pnkfelix CC @gankro

eddyb reviewed Oct 17, 2015
View reviewed changes

bors merged commit d6bd8d8 into rust-lang:master Oct 17, 2015

apasel422 deleted the shared branch October 17, 2015 11:42

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 17, 2015

apasel422 mentioned this pull request Oct 17, 2015

RFC 1214 warning in libstd/thread/mod.rs #29128

Closed

huonw mentioned this pull request Oct 20, 2015

Tracking issue for NonZero/Unique/Shared stabilization #27730

Closed

cristicbz mentioned this pull request Feb 15, 2017

Tracking issue for Rc::into_raw, Rc::from_raw, Arc::into_raw, Arc::from_raw #37197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Shared` pointer and have `{Arc, Rc}` use it #29110

Add `Shared` pointer and have `{Arc, Rc}` use it #29110

apasel422 commented Oct 16, 2015

apasel422 commented Oct 16, 2015

pnkfelix Oct 16, 2015

apasel422 commented Oct 16, 2015

pnkfelix commented Oct 16, 2015

apasel422 commented Oct 16, 2015

alexcrichton Oct 16, 2015

alexcrichton Oct 16, 2015

apasel422 Oct 16, 2015

pnkfelix Oct 16, 2015

pnkfelix Oct 16, 2015

alexcrichton commented Oct 16, 2015

pnkfelix commented Oct 16, 2015

alexcrichton commented Oct 16, 2015

pnkfelix commented Oct 16, 2015

bkoropoff Oct 17, 2015

apasel422 Oct 17, 2015

eddyb Oct 17, 2015

bkoropoff commented Oct 17, 2015

apasel422 commented Oct 17, 2015

apasel422 commented Oct 17, 2015

bors commented Oct 17, 2015

eddyb Oct 17, 2015

apasel422 Oct 17, 2015

bors commented Oct 17, 2015

SimonSapin commented Oct 19, 2015

Gankra commented Oct 19, 2015

bstrie commented Oct 21, 2015

Gankra commented Oct 21, 2015

Add Shared pointer and have {Arc, Rc} use it #29110

Add Shared pointer and have {Arc, Rc} use it #29110

Conversation

apasel422 commented Oct 16, 2015

apasel422 commented Oct 16, 2015

Choose a reason for hiding this comment

apasel422 commented Oct 16, 2015

pnkfelix commented Oct 16, 2015

apasel422 commented Oct 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Oct 16, 2015

pnkfelix commented Oct 16, 2015

alexcrichton commented Oct 16, 2015

pnkfelix commented Oct 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkoropoff commented Oct 17, 2015

apasel422 commented Oct 17, 2015

apasel422 commented Oct 17, 2015

bors commented Oct 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 17, 2015

SimonSapin commented Oct 19, 2015

Gankra commented Oct 19, 2015

bstrie commented Oct 21, 2015

Gankra commented Oct 21, 2015

Add `Shared` pointer and have `{Arc, Rc}` use it #29110

Add `Shared` pointer and have `{Arc, Rc}` use it #29110