You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Tracking issue for Rc::into_raw, Rc::from_raw, Arc::into_raw, Arc::from_raw #37197
Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
@Amanieu could you elaborate on why you'd like to see these as a trait? I do think that'd be a breaking change to remove the inherent methods on Box (unless we add the trait to the prelude), but that doesn't meant we could also add a unifying trait later. We've just tended to avoid premature abstraction in libstd with extra traits.
Also on a technical level is accepting unsized types possible? Consider something like going from Rc<Trait> to *mut Trait. The vtable for *mut Trait has to come from somewhere, and the only one we have is the vtable for Rc<Trait>. Those methods expect the pointer being passed into them is *mut RcBox<T>, not *mut T. In that sense I don't think we could support unsized types?
@alexcrichton I'm building a safe interface for intrusive-collections, which works by having a collection "take ownership" of an owned pointer. This is done by calling into_raw before inserting an object into a collection, and calling from_raw when an object is removed from a collection. I currently define an IntrusivePointer trait myself which does this, so I guess a trait in the standard library isn't really needed.
Regarding vtables, I was under the impression that the vtable pointer in *mut RcBox<Trait> pointed to the same vtable as *mut Trait since the fat part of the point is only associated with the unsized part of a struct (Trait). This means that the compiler will first extract a pointer to the Trait before passing it to the vtable methods. On the other hand I am not very familiar with how vtables work in Rust, so I may be wrong here.
Unlike C++ we don't support splitting the allocation of the ref counts from
the allocation of the data, so that would be a bigger change. Not sure it
can be done backwards compatibly even without this functionality.
What use case is not covered by Rc::new(self)? (i. e. Moving into an Rc)
As I understand it (forgive me if I am mistaken in here somewhere), Shared holds a *const T, so it is covariant if T is. Thus it would be trivial to get a *mut T with a different lifetime just by coercing a Shared instance before calling deref. Since Shared is copy, it is trivial to store a value through pointer with a shorter lifetime and read it back out via the the pointer with a longer lifetime. This is bad! (Yes, this requires unsafe code, but it's a serious footgun, and I believe preventing this type of footgun is the reason *mut is considered by the compiler to be invariant.)
Further, since Shared is Copy (because it is designed to be shared), that means, like Rc itself, the data pointed to should not be considered mutable except for parts contained in an UnsafeCell.
Finally, neither Rc nor Arc have any need for Shared to hand out *mut pointers during active usage, since the counts themselves are already contained in UnsafeCell (as they must be for soundness, with Rc using Cell and Arc using AtomicUsize)
It seems to me, then, that Shared should also definitely be handing out *const and not *mut pointers. The data pointed to is meant to be shared and thus shouldn't be considered mutable; Shared is covariant, making handing out *mut pointers even more dangerous; and neither Rc nor Arc need *mut for their shared operations because they only mutate through UnsafeCell.
If I had to speculate, I would guess that Shared returns a *mut pointer because when the last reference is destroyed, Rc and Arc need a mutable pointer to execute drop. However, I think a cast from *const to *mutshould be necessary here as a way to assert, "I know I am currently the sole owner of this data."
EDIT: Rather, I would speculate that Shared returns a *mut because it was replacing NonZero<*mut RcBox>, and that that was originally *mut because that's what Box::into_raw returns and because that's what drop needs. That is to say, it seems like a historical accident, and I would argue that both Shared::deref and Rc::into_raw should return *const
So what is the consensus regarding support for DSTs? We could start by only implementing into_raw and from_raw for sized types, and later extend them to DSTs.
So #29110 added Shared specifically to make Rc and Arc covariant; looking at that PR it seems that @rkjnsn is right about *mut being due to it replacing a pointer obtained via Box::into_raw.
@brson What sort of tests would you like added? For the variance of Rc and Arc (define some noop fn-s like I did in the above playground link)? I can't think of any way to test it specifically around the new {into,from}_raw methods.
@Amanieu DST-wise yeah I thought that's sort-of where we left it. For DST-s it's a slightly bigger can of worms (see #37197 (comment)) and I'd be too worried about potential UB without an offsetof built into the compiler. Like you said, it will be trivially backwards compatible to extend this to DST-s when we're happy.
I'm not worried about variance here: since Rc is covariant, I feel like you can't end up being over-permissive.
I prefer *const T, but I don't massively care. Mutating the data behind the pointer obtained through into_raw is generally UB. If T is a cell, the as_ptr method on the cell is the correct way to get a mutable pointer.
I'd punt on DST-s until there's a built-in offsetof. They're easy to add later backwards-compatibly.
I agree that *const being covariant isn't an issue given that Rc is.
I am strongly opposed *mut here and think *const is clearly the way to go. *mut is dangerous both because Rc's covariance would make it too easy to get mut pointers to the same data with different lifetimes and because muting shared data without UnsafeCell is in violation of Rust‘s guarantees and likely to result in unsound behavior. In cases where it is guaranteed that ownership is exclusive, it's easy enough to cast to *mut (with a comment explaining why it's sound in that instance).
I agree that it makes sense to get this in for Sized types and then figure out DSTs.
Ah so to clarify the FCP ending here is just the standard one week period. In reality it'll "end" a week before the end of the cycle when we merge this.
I'm leaning towards changing to const, pending input from @brson.
Discussed during @rust-lang/libs triage today, the conclusion was to go with *const with these APIs and update Shared at the same time. If no one else gets around to it sooner I'll update when I do the blanket "stabilize everything" PR
Activity
Auto merge of #37192 - cristicbz:rust-rc-into-raw, r=brson
cristicbz commentedon Dec 21, 2016
@alexcrichton shouldn't this have a
B-unstablelabel? Don't know if it matters, but I figured I'd check.alexcrichton commentedon Dec 21, 2016
Oops, indeed!
alexcrichton commentedon Dec 21, 2016
And while we're at it, I'd like to stabilize this!
@rfcbot fcp merge
rfcbot commentedon Dec 21, 2016
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:
No concerns currently listed.
Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
Amanieu commentedon Dec 26, 2016
Is there any chance of putting
from_rawandinto_rawinto a trait or would that be a breaking change forBox?Amanieu commentedon Dec 26, 2016
Also I think
from_rawandinto_rawforRcandArcshould accept unsized types, just like the implementations forBox.alexcrichton commentedon Dec 26, 2016
@Amanieu could you elaborate on why you'd like to see these as a trait? I do think that'd be a breaking change to remove the inherent methods on
Box(unless we add the trait to the prelude), but that doesn't meant we could also add a unifying trait later. We've just tended to avoid premature abstraction in libstd with extra traits.Also on a technical level is accepting unsized types possible? Consider something like going from
Rc<Trait>to*mut Trait. The vtable for*mut Traithas to come from somewhere, and the only one we have is the vtable forRc<Trait>. Those methods expect the pointer being passed into them is*mut RcBox<T>, not*mut T. In that sense I don't think we could support unsized types?Amanieu commentedon Dec 27, 2016
@alexcrichton I'm building a safe interface for
intrusive-collections, which works by having a collection "take ownership" of an owned pointer. This is done by callinginto_rawbefore inserting an object into a collection, and callingfrom_rawwhen an object is removed from a collection. I currently define anIntrusivePointertrait myself which does this, so I guess a trait in the standard library isn't really needed.Regarding vtables, I was under the impression that the vtable pointer in
*mut RcBox<Trait>pointed to the same vtable as*mut Traitsince the fat part of the point is only associated with the unsized part of a struct (Trait). This means that the compiler will first extract a pointer to theTraitbefore passing it to the vtable methods. On the other hand I am not very familiar with how vtables work in Rust, so I may be wrong here.alexcrichton commentedon Dec 28, 2016
Hm yeah it's true I'm not entirely sure what the vtable would look like. Would be good to clarify before we support the functionality!
Amanieu commentedon Dec 28, 2016
&RcBox<Trait>and&Traithave the same vtable pointer: https://is.gd/QNfAIoishitatsuyuki commentedon Dec 29, 2016
I'm looking forward to have something like
std::shared_from_this(C++), thus constructing a new Arc from a reference (probably &self).cristicbz commentedon Dec 29, 2016
27 remaining items
rkjnsn commentedon Feb 15, 2017
As I understand it (forgive me if I am mistaken in here somewhere),
Sharedholds a*const T, so it is covariant ifTis. Thus it would be trivial to get a*mut Twith a different lifetime just by coercing aSharedinstance before callingderef. SinceSharedis copy, it is trivial to store a value through pointer with a shorter lifetime and read it back out via the the pointer with a longer lifetime. This is bad! (Yes, this requires unsafe code, but it's a serious footgun, and I believe preventing this type of footgun is the reason*mutis considered by the compiler to be invariant.)Further, since
SharedisCopy(because it is designed to be shared), that means, likeRcitself, the data pointed to should not be considered mutable except for parts contained in anUnsafeCell.Finally, neither
RcnorArchave any need forSharedto hand out*mutpointers during active usage, since the counts themselves are already contained inUnsafeCell(as they must be for soundness, withRcusingCellandArcusingAtomicUsize)It seems to me, then, that
Sharedshould also definitely be handing out*constand not*mutpointers. The data pointed to is meant to be shared and thus shouldn't be considered mutable;Sharedis covariant, making handing out*mutpointers even more dangerous; and neitherRcnorArcneed*mutfor their shared operations because they only mutate throughUnsafeCell.If I had to speculate, I would guess that
Sharedreturns a*mutpointer because when the last reference is destroyed,RcandArcneed a mutable pointer to executedrop. However, I think a cast from*constto*mutshould be necessary here as a way to assert, "I know I am currently the sole owner of this data."EDIT: Rather, I would speculate that
Sharedreturns a*mutbecause it was replacing NonZero<*mut RcBox>, and that that was originally*mutbecause that's whatBox::into_rawreturns and because that's whatdropneeds. That is to say, it seems like a historical accident, and I would argue that bothShared::derefandRc::into_rawshould return*constalexcrichton commentedon Feb 15, 2017
That seems like a reasonable explanation to me, and if we change
Sharedas well I think it makes sense to changeRcat the same time.@brson do you have other specific thoughts here?
Amanieu commentedon Feb 15, 2017
So what is the consensus regarding support for DSTs? We could start by only implementing
into_rawandfrom_rawfor sized types, and later extend them to DSTs.brson commentedon Feb 15, 2017
I'm not very enthusiastic about stabilizing this with so much confusion about the proper type.
The questions about variance worry me too. If there are variance concerns here let's add tests for them. This all feels risky.
cristicbz commentedon Feb 15, 2017
So #29110 added
Sharedspecifically to makeRcandArccovariant; looking at that PR it seems that @rkjnsn is right about*mutbeing due to it replacing a pointer obtained viaBox::into_raw.@brson What sort of tests would you like added? For the variance of
RcandArc(define some noop fn-s like I did in the above playground link)? I can't think of any way to test it specifically around the new{into,from}_rawmethods.@Amanieu DST-wise yeah I thought that's sort-of where we left it. For DST-s it's a slightly bigger can of worms (see #37197 (comment)) and I'd be too worried about potential UB without an
offsetofbuilt into the compiler. Like you said, it will be trivially backwards compatible to extend this to DST-s when we're happy.rfcbot commentedon Feb 23, 2017
The final comment period is now complete.
cristicbz commentedon Feb 23, 2017
Since the FCP is over, I'll sum up where I stand:
Rcis covariant, I feel like you can't end up being over-permissive.*const T, but I don't massively care. Mutating the data behind the pointer obtained throughinto_rawis generally UB. IfTis a cell, theas_ptrmethod on the cell is the correct way to get a mutable pointer.offsetof. They're easy to add later backwards-compatibly.rkjnsn commentedon Feb 25, 2017
My summary:
*constbeing covariant isn't an issue given thatRcis.*muthere and think*constis clearly the way to go.*mutis dangerous both becauseRc's covariance would make it too easy to get mut pointers to the same data with different lifetimes and because muting shared data withoutUnsafeCellis in violation of Rust‘s guarantees and likely to result in unsound behavior. In cases where it is guaranteed that ownership is exclusive, it's easy enough to cast to*mut(with a comment explaining why it's sound in that instance).Sizedtypes and then figure out DSTs.alexcrichton commentedon Feb 25, 2017
Ah so to clarify the FCP ending here is just the standard one week period. In reality it'll "end" a week before the end of the cycle when we merge this.
I'm leaning towards changing to
const, pending input from @brson.alexcrichton commentedon Feb 28, 2017
Discussed during @rust-lang/libs triage today, the conclusion was to go with
*constwith these APIs and updateSharedat the same time. If no one else gets around to it sooner I'll update when I do the blanket "stabilize everything" PRStabilize rc_raw feature, closes rust-lang#37197