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

Add Into<NonNull<T>> impls for Box<T>/Arc<T>/Rc<T> #56998

Closed
wants to merge 14 commits into from
Closed
14 changes: 13 additions & 1 deletion src/liballoc/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ impl<T: ?Sized> Box<T> {
#[stable(feature = "box_raw", since = "1.4.0")]
#[inline]
pub fn into_raw(b: Box<T>) -> *mut T {
Box::into_raw_non_null(b).as_ptr()
let p: NonNull<T> = b.into();
p.as_ptr()
}

/// Consumes the `Box`, returning the wrapped pointer as `NonNull<T>`.
Expand All @@ -193,6 +194,7 @@ impl<T: ?Sized> Box<T> {
///
/// ```
/// #![feature(box_into_raw_non_null)]
/// #![allow(deprecated)]
///
/// fn main() {
/// let x = Box::new(5);
Expand All @@ -201,6 +203,7 @@ impl<T: ?Sized> Box<T> {
/// ```
#[unstable(feature = "box_into_raw_non_null", issue = "47336")]
#[inline]
#[rustc_deprecated(reason = "Use `.into()`", since = "1.32.0")]
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not to deprecated this. Inherent associated functions are more discoverable and avoid needing to guide type inference, as you see in the into_raw implementation above.

Box::into_raw_non_null(b).as_ptr()
let p: NonNull<T> = b.into();
p.as_ptr()

Copy link
Member Author

Choose a reason for hiding this comment

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

@SimonSapin suggested this. I'm happy either way.

Do we want one implementation to delegate to the other? And if yes, which way?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dtolnay Should every single impl of conversion traits have a corresponding inherent method? If not, what makes this one special?

Copy link
Member Author

@dwijnand dwijnand Jan 9, 2019

Choose a reason for hiding this comment

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

Perhaps the very fact that the From/Into traits are type conversions makes them special with regards to ergonomics and type inference. My understanding was that was one of the reasons for having both From and Into, i.e Foo::from avoids result type inference, while bar.into() method-syntax is nicer to use. Being forced by the coherence rules I think it might be worth keeping the associated function.

pub fn into_raw_non_null(b: Box<T>) -> NonNull<T> {
Box::into_unique(b).into()
}
Expand Down Expand Up @@ -479,6 +482,15 @@ impl From<Box<str>> for Box<[u8]> {
}
}

#[allow(incoherent_fundamental_impls)]
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment explaining the implications of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what they are. I'm happy to push comments if anyone has any suggestions!

Copy link
Contributor

Choose a reason for hiding this comment

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

incoherent_fundamental_impls is a C-future-compatibility warning; those are essentially hard errors that have been temporarily lowered into warnings to give people time to migrate. Tho in this case it doesn't seem clearly specified in the issue what the warning actually does. However, there's a PR up that is actually making it into a hard error #49799. If this impl is admitted into stable Rust (i.e. through #[allow(incoherent_fundamental_impls)]) then the warning cannot, AFAIK, be made into a hard error. That seems... bad.

cc @nikomatsakis @rust-lang/wg-traits

#[stable(feature = "box_into_non_null", since = "1.34.0")]
impl<T: ?Sized> Into<NonNull<T>> for Box<T> {
#[inline]
fn into(self) -> NonNull<T> {
Box::into_unique(self).into()
}
}

impl Box<dyn Any> {
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
8 changes: 8 additions & 0 deletions src/liballoc/boxed_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,3 +148,11 @@ fn boxed_slice_from_iter() {
assert_eq!(boxed.len(), 100);
assert_eq!(boxed[7], 7);
}

#[test]
fn to_nonnull() {
let boxed: Box<i32> = Box::from(0);
let ptr: std::ptr::NonNull<i32> = boxed.into();
let deref = unsafe { *ptr.as_ref() };
assert_eq!(deref, 0);
}
8 changes: 4 additions & 4 deletions src/liballoc/collections/linked_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ impl<T> LinkedList<T> {
unsafe {
node.next = self.head;
node.prev = None;
let node = Some(Box::into_raw_non_null(node));
let node = Some(node.into());

match self.head {
None => self.tail = node,
Expand Down Expand Up @@ -191,7 +191,7 @@ impl<T> LinkedList<T> {
unsafe {
node.next = None;
node.prev = self.tail;
let node = Some(Box::into_raw_non_null(node));
let node = Some(node.into());

match self.tail {
None => self.head = node,
Expand Down Expand Up @@ -933,11 +933,11 @@ impl<'a, T> IterMut<'a, T> {
Some(prev) => prev,
};

let node = Some(Box::into_raw_non_null(box Node {
let node = Some((box Node {
next: Some(head),
prev: Some(prev),
element,
}));
}).into());

prev.as_mut().next = node;
head.as_mut().prev = node;
Expand Down
12 changes: 10 additions & 2 deletions src/liballoc/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,11 +316,11 @@ impl<T> Rc<T> {
// pointers, which ensures that the weak destructor never frees
// the allocation while the strong destructor is running, even
// if the weak pointer is stored inside the strong one.
ptr: Box::into_raw_non_null(box RcBox {
ptr: (box RcBox {
strong: Cell::new(1),
weak: Cell::new(1),
value,
}),
}).into(),
phantom: PhantomData,
}
}
Expand Down Expand Up @@ -1179,6 +1179,14 @@ impl<T> From<Vec<T>> for Rc<[T]> {
}
}

#[stable(feature = "rc_into_nonnull", since = "1.34.0")]
impl<T: ?Sized> Into<NonNull<T>> for Rc<T> {
Copy link
Member

Choose a reason for hiding this comment

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

I would like to add an equivalent inherent function like we have for Box: Rc::into_raw_non_null and Arc::into_raw_non_null.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. What is the "raw" for? Also, between the Into impl and the inherent function, should one delegate to the other?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll wait for #56998 (comment) to be resolved first.

#[inline]
fn into(self) -> NonNull<T> {
unsafe { NonNull::new_unchecked(Rc::into_raw(self) as *mut _) }
}
}

/// `Weak` is a version of [`Rc`] that holds a non-owning reference to the
/// managed value. The value is accessed by calling [`upgrade`] on the `Weak`
/// pointer, which returns an [`Option`]`<`[`Rc`]`<T>>`.
Expand Down
10 changes: 9 additions & 1 deletion src/liballoc/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ impl<T> Arc<T> {
weak: atomic::AtomicUsize::new(1),
data,
};
Arc { ptr: Box::into_raw_non_null(x), phantom: PhantomData }
Arc { ptr: x.into(), phantom: PhantomData }
}

#[unstable(feature = "pin", issue = "49150")]
Expand Down Expand Up @@ -1574,6 +1574,14 @@ impl<T> From<Vec<T>> for Arc<[T]> {
}
}

#[stable(feature = "arc_into_nonnull", since = "1.34.0")]
impl<T: ?Sized> Into<NonNull<T>> for Arc<T> {
#[inline]
fn into(self) -> NonNull<T> {
unsafe { NonNull::new_unchecked(Arc::into_raw(self) as *mut _) }
}
}

#[cfg(test)]
mod tests {
use std::boxed::Box;
Expand Down
7 changes: 7 additions & 0 deletions src/liballoc/tests/arc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,10 @@ fn eq() {
assert!(!(x != x));
assert_eq!(*x.0.borrow(), 0);
}

#[test]
fn to_nonnull() {
let ptr: std::ptr::NonNull<i32> = Arc::new(0).into();
let deref = unsafe { *ptr.as_ref() };
assert_eq!(deref, 0);
}
7 changes: 7 additions & 0 deletions src/liballoc/tests/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,10 @@ fn eq() {
assert!(!(x != x));
assert_eq!(*x.0.borrow(), 0);
}

#[test]
fn to_nonnull() {
let ptr: std::ptr::NonNull<i32> = Rc::new(0).into();
let deref = unsafe { *ptr.as_ref() };
assert_eq!(deref, 0);
}