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

Tracking issue for `NonZero`/`Unique`/`Shared` stabilization #27730

Closed
aturon opened this Issue Aug 12, 2015 · 114 comments

Comments

Projects
None yet
@aturon
Member

aturon commented Aug 12, 2015

We currently have three internal types, NonZero, Unique and Shared, that are very useful when working with unsafe Rust code. Ultimately, the hope is to have a clear vision for semi-high-level, but unsafe programming using types like these.

None of the current APIs have gone through the RFC process, and a comprehensive RFC should be written before any stabilization is done.

cc @Gankro

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Aug 12, 2015

Contributor

I'm not a fan of NonZero. I would like a more robust system that can hook into the things described in rust-lang/rfcs#1230.

Unique is great, but I would like to rename it to Owned to better reflect what its actual semantics are (e.g. it's fine to have other pointers into it just like it's fine for Box).

I would like to add a Shared equivalent for Rc/Arc to use. This is blocked by #26905

Contributor

Gankro commented Aug 12, 2015

I'm not a fan of NonZero. I would like a more robust system that can hook into the things described in rust-lang/rfcs#1230.

Unique is great, but I would like to rename it to Owned to better reflect what its actual semantics are (e.g. it's fine to have other pointers into it just like it's fine for Box).

I would like to add a Shared equivalent for Rc/Arc to use. This is blocked by #26905

@huonw huonw changed the title from Tracking issue for `NonZero`/`Unique` stabilization to Tracking issue for `NonZero`/`Unique`/`Shared` stabilization Oct 20, 2015

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Oct 20, 2015

Member

I expanded this issue to cover the new Shared type (#29110).

Member

huonw commented Oct 20, 2015

I expanded this issue to cover the new Shared type (#29110).

huonw added a commit to huonw/rust that referenced this issue Oct 20, 2015

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Oct 20, 2015

Contributor

CC @SimonSapin

Does Shared/Unique cover all your NonZero usecases?

Contributor

Gankro commented Oct 20, 2015

CC @SimonSapin

Does Shared/Unique cover all your NonZero usecases?

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Oct 20, 2015

Member

I commented about this in #29110, reposting here for visibility:

The Deref impls on these types are bad interfaces because they remove the optimizations NonZero can trigger, specifically the control-flow ones (not the layout optimizations, which are unaffected).

They also allow writing *p where p is a ptr::Unique<T> or a ptr::Shared<T> to obtain a *mut T, which might make unsafe code more confusing (you need **p to get at the T lvalue).

NonZero and wrappers such as Unique and Shared, should use a Cell-like API (.get() and .set() - or just .get(), really), allowing LLVM to reason about their non-nullability.

Member

eddyb commented Oct 20, 2015

I commented about this in #29110, reposting here for visibility:

The Deref impls on these types are bad interfaces because they remove the optimizations NonZero can trigger, specifically the control-flow ones (not the layout optimizations, which are unaffected).

They also allow writing *p where p is a ptr::Unique<T> or a ptr::Shared<T> to obtain a *mut T, which might make unsafe code more confusing (you need **p to get at the T lvalue).

NonZero and wrappers such as Unique and Shared, should use a Cell-like API (.get() and .set() - or just .get(), really), allowing LLVM to reason about their non-nullability.

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Nov 7, 2015

Contributor

@eddyb alternatively, we could just promote these types to compiler primitives, right? Have them coerce to raw pointers or summat? This may also improve debug perf, as there's less abstraction in the way if it's really "just" a raw pointer, and not a Foo(Bar(*const T)) that is accessed through two levels of methods.

Contributor

Gankro commented Nov 7, 2015

@eddyb alternatively, we could just promote these types to compiler primitives, right? Have them coerce to raw pointers or summat? This may also improve debug perf, as there's less abstraction in the way if it's really "just" a raw pointer, and not a Foo(Bar(*const T)) that is accessed through two levels of methods.

@alexreg

This comment has been minimized.

Show comment
Hide comment
@alexreg

alexreg Feb 22, 2016

Contributor

I agree with @eddyb's comment about using a Cell-like API for NonZero (and other types). It makes semantic sense.

Contributor

alexreg commented Feb 22, 2016

I agree with @eddyb's comment about using a Cell-like API for NonZero (and other types). It makes semantic sense.

@dhardy dhardy referenced this issue Feb 23, 2016

Open

Rust issues #18

4 of 12 tasks complete
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 29, 2016

Member

The libs team decided to not move these APIs into FCP this cycle because the NonZero type is at the heart of all of them, and we're the least sure about NonZero.

Member

alexcrichton commented Apr 29, 2016

The libs team decided to not move these APIs into FCP this cycle because the NonZero type is at the heart of all of them, and we're the least sure about NonZero.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin May 22, 2016

Contributor

CC @SimonSapin

Does Shared/Unique cover all your NonZero usecases?

(Answering with a small delay… :))

Unfortunately no. We would like to use NonZero<u64> instead of u64 in string_cache::Atom, but we can’t at the moment because NonZero’s field being private prevents matching on it in a pattern: https://internals.rust-lang.org/t/matching-on-struct-patterns-with-private-read-only-fields/3499

Contributor

SimonSapin commented May 22, 2016

CC @SimonSapin

Does Shared/Unique cover all your NonZero usecases?

(Answering with a small delay… :))

Unfortunately no. We would like to use NonZero<u64> instead of u64 in string_cache::Atom, but we can’t at the moment because NonZero’s field being private prevents matching on it in a pattern: https://internals.rust-lang.org/t/matching-on-struct-patterns-with-private-read-only-fields/3499

@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Jun 1, 2016

I don't know if this is the right place to comment, but I'm binding some C code and Unique appears perfect - except that it exposes the unsafe rather than that being internal - meaning that I need a further wrapper around it - since I know the C ptr in question isn't going to be freed randomly, and I know that its not null, I want to hide the unsafe entirely from further out users.

rbtcollins commented Jun 1, 2016

I don't know if this is the right place to comment, but I'm binding some C code and Unique appears perfect - except that it exposes the unsafe rather than that being internal - meaning that I need a further wrapper around it - since I know the C ptr in question isn't going to be freed randomly, and I know that its not null, I want to hide the unsafe entirely from further out users.

@DemiMarie

This comment has been minimized.

Show comment
Hide comment
@DemiMarie

DemiMarie Jul 9, 2016

Contributor

@rbtcollins My suggestion would be to write a wrapper struct that has a Unique as a member, and that frees the pointer in the Unique in its Drop impl.

Contributor

DemiMarie commented Jul 9, 2016

@rbtcollins My suggestion would be to write a wrapper struct that has a Unique as a member, and that frees the pointer in the Unique in its Drop impl.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jul 21, 2016

Contributor

N.B. *move from rust-lang/rfcs#1646 would almost surely replace this.

Contributor

Ericson2314 commented Jul 21, 2016

N.B. *move from rust-lang/rfcs#1646 would almost surely replace this.

@dtolnay

This comment has been minimized.

Show comment
Hide comment
@dtolnay

dtolnay Dec 30, 2016

Member

If we keep NonZero or something like it, I would love a safe (checked) way to construct one. Currently the single line of unsafe code in Serde is constructing a NonZero.

if value == Zero::zero() {
    return Err(Error::invalid_value("expected a non-zero value"))
}
unsafe {
    Ok(NonZero::new(value))
}
Member

dtolnay commented Dec 30, 2016

If we keep NonZero or something like it, I would love a safe (checked) way to construct one. Currently the single line of unsafe code in Serde is constructing a NonZero.

if value == Zero::zero() {
    return Err(Error::invalid_value("expected a non-zero value"))
}
unsafe {
    Ok(NonZero::new(value))
}
@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Apr 4, 2017

Contributor

I'm making changes to this API in this PR #41064

Contributor

Gankro commented Apr 4, 2017

I'm making changes to this API in this PR #41064

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Jan 6, 2018

Member

Zeroable should be unsafe trait FWIW, but transmute is checked too early for this, I guess.

Member

eddyb commented Jan 6, 2018

Zeroable should be unsafe trait FWIW, but transmute is checked too early for this, I guess.

@HyeonuPark

This comment has been minimized.

Show comment
Hide comment
@HyeonuPark

HyeonuPark Jan 6, 2018

Ah yeah that's the point, I think Zeroable should be unsafe trait, and also auto-impl.

As I understand, Option<NonZero<Foo>> has same size as Foo and Option<NonZero<Foo>>::None is identical with std::mem::zeroed(), as NonZero is used by compiler to perform NPO(Please tell me if this is wrong). Then something like this can be possible.

struct Foo(usize);

impl Zeroable for Foo {
    fn is_zero(&self) -> bool {
        self.0 != 0 // really!
    }
}

fn main() {
    let zero = Foo(0);
    let one = Foo(1);

    assert_eq(NonZero::new(zero), None); // as underlying memory is all zero
    assert_eq(NonZero::new(one), None);  // as `one.is_zero()` is true
}

Does this code makes sense?

HyeonuPark commented Jan 6, 2018

Ah yeah that's the point, I think Zeroable should be unsafe trait, and also auto-impl.

As I understand, Option<NonZero<Foo>> has same size as Foo and Option<NonZero<Foo>>::None is identical with std::mem::zeroed(), as NonZero is used by compiler to perform NPO(Please tell me if this is wrong). Then something like this can be possible.

struct Foo(usize);

impl Zeroable for Foo {
    fn is_zero(&self) -> bool {
        self.0 != 0 // really!
    }
}

fn main() {
    let zero = Foo(0);
    let one = Foo(1);

    assert_eq(NonZero::new(zero), None); // as underlying memory is all zero
    assert_eq(NonZero::new(one), None);  // as `one.is_zero()` is true
}

Does this code makes sense?

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Jan 6, 2018

Member

We should maybe abuse trait privacy (if we can, at all) to make it impossible to implement the trait outside of libcore - we don't really support more types than primitives, in NonZero.

Member

eddyb commented Jan 6, 2018

We should maybe abuse trait privacy (if we can, at all) to make it impossible to implement the trait outside of libcore - we don't really support more types than primitives, in NonZero.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Jan 6, 2018

Contributor

I was hoping to land #46952 and get Shared / NonNull out of the way first, but my plan is to propose removing the Zeroable trait and replace NonZero with 12 types like NonZeroUsize and NonZeroI32, similar to atomic integer types. That way we avoid the questions of making the trait an auto trait or attempting to prevent more impls of it, since there is no trait.

Contributor

SimonSapin commented Jan 6, 2018

I was hoping to land #46952 and get Shared / NonNull out of the way first, but my plan is to propose removing the Zeroable trait and replace NonZero with 12 types like NonZeroUsize and NonZeroI32, similar to atomic integer types. That way we avoid the questions of making the trait an auto trait or attempting to prevent more impls of it, since there is no trait.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Jan 6, 2018

Member

Assuming you mean via a hidden private NonZero, then you don't even need to change to compiler.

Member

eddyb commented Jan 6, 2018

Assuming you mean via a hidden private NonZero, then you don't even need to change to compiler.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Jan 6, 2018

Contributor

Right, I meant "remove" as far as the public API is concerned. We can have a private generic type (with or without a trait bound) in the implementation, or whatever’s easier to maintain for the compiler.

Contributor

SimonSapin commented Jan 6, 2018

Right, I meant "remove" as far as the public API is concerned. We can have a private generic type (with or without a trait bound) in the implementation, or whatever’s easier to maintain for the compiler.

SimonSapin added a commit to SimonSapin/rust that referenced this issue Jan 17, 2018

Rename std::ptr::Shared to NonNull
`Shared` is now a deprecated `type` alias.

CC rust-lang#27730 (comment)

bors added a commit that referenced this issue Jan 20, 2018

Auto merge of #46952 - SimonSapin:nonnull, r=alexcrichton
Rename std::ptr::Shared to NonNull and stabilize it

This implements the changes proposed at #27730 (comment):

> * Rename `Shared<T>` to `NonNull<T>` and stabilize it. (Being in the `ptr` module is enough to say that it’s a pointer. I’m not very attached to this specific name though.)
> * Rename `Box<T>` methods ~~`from_unique`~~/`into_unique` to ~~`from_nonnull`~~/`into_nonnull` (or whatever names are deemed appropriate), replace `Unique<T>` with `NonNull<T>` in their signatures, and stabilize them.
> *  Replace `Unique<T>` with `NonNull<T>` in the signatures of methods of the `Alloc` trait.
> * Mark `Unique` “permanently-unstable” by replacing remaining occurrences of `#[unstable(feature = "unique", issue = "27730")]` with:
>
>   ```rust
>   #[unstable(feature = "ptr_internals", issue = "0", reason = "\
>       use NonNull instead and consider PhantomData<T> (if you also use #[may_dangle]), \
>       Send, and/or Sync")]
>   ```
>
>   (Maybe the `reason` string is only useful on the struct definition.) Ideally it would be made private to some crate instead, but it needs to be used in both liballoc and libstd.
> * (Leave `NonZero` and `Zeroable` unstable for now, and subject to future bikeshedding.)

SimonSapin added a commit to SimonSapin/rust that referenced this issue Jan 20, 2018

Rename std::ptr::Shared to NonNull
`Shared` is now a deprecated `type` alias.

CC rust-lang#27730 (comment)

bors added a commit that referenced this issue Jan 20, 2018

Auto merge of #46952 - SimonSapin:nonnull, r=alexcrichton
Rename std::ptr::Shared to NonNull and stabilize it

This implements the changes proposed at #27730 (comment):

> * Rename `Shared<T>` to `NonNull<T>` and stabilize it. (Being in the `ptr` module is enough to say that it’s a pointer. I’m not very attached to this specific name though.)
> * Rename `Box<T>` methods ~~`from_unique`~~/`into_unique` to ~~`from_nonnull`~~/`into_nonnull` (or whatever names are deemed appropriate), replace `Unique<T>` with `NonNull<T>` in their signatures, and stabilize them.
> *  Replace `Unique<T>` with `NonNull<T>` in the signatures of methods of the `Alloc` trait.
> * Mark `Unique` “permanently-unstable” by replacing remaining occurrences of `#[unstable(feature = "unique", issue = "27730")]` with:
>
>   ```rust
>   #[unstable(feature = "ptr_internals", issue = "0", reason = "\
>       use NonNull instead and consider PhantomData<T> (if you also use #[may_dangle]), \
>       Send, and/or Sync")]
>   ```
>
>   (Maybe the `reason` string is only useful on the struct definition.) Ideally it would be made private to some crate instead, but it needs to be used in both liballoc and libstd.
> * (Leave `NonZero` and `Zeroable` unstable for now, and subject to future bikeshedding.)
@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Jan 21, 2018

Contributor

Shared is now renamed NonNull and stable; Unique is a permanently-unstable internal implementation detail of std: #47631

This only leaves NonZero for this tracking issue, which is now mostly only useful for integers.

I just published rust-lang/rfcs#2307 which propose adding 12 concrete types like NonZeroU32, deprecating NonZero, and later making it private. That last step would (finally!) close this tracking issue.

Contributor

SimonSapin commented Jan 21, 2018

Shared is now renamed NonNull and stable; Unique is a permanently-unstable internal implementation detail of std: #47631

This only leaves NonZero for this tracking issue, which is now mostly only useful for integers.

I just published rust-lang/rfcs#2307 which propose adding 12 concrete types like NonZeroU32, deprecating NonZero, and later making it private. That last step would (finally!) close this tracking issue.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Jan 21, 2018

Contributor

Also some NonNull follow up: #47631

Contributor

SimonSapin commented Jan 21, 2018

Also some NonNull follow up: #47631

cuviper pushed a commit to cuviper/rayon-hash that referenced this issue Jan 22, 2018

Rename std::ptr::Shared to NonNull
`Shared` is now a deprecated `type` alias.

CC rust-lang/rust#27730 (comment)

bors added a commit that referenced this issue Feb 7, 2018

Auto merge of #47631 - SimonSapin:nonnull, r=alexcrichton
Add some APIs to ptr::NonNull and fix `since` attributes

This is a follow-up to its stabilization in #46952. Tracking issue: #27730.

* These trait impls are insta-stable: `Hash`, `PartialEq`, `Eq`, `PartialOrd` and `Ord`.
* The new `cast<U>() -> NonNull<U>`  method is `#[unstable]`. It was proposed in #46952 (comment).

Manishearth added a commit to Manishearth/rust that referenced this issue Feb 7, 2018

Rollup merge of rust-lang#47631 - SimonSapin:nonnull, r=alexcrichton
Add some APIs to ptr::NonNull and fix `since` attributes

This is a follow-up to its stabilization in rust-lang#46952. Tracking issue: rust-lang#27730.

* These trait impls are insta-stable: `Hash`, `PartialEq`, `Eq`, `PartialOrd` and `Ord`.
* The new `cast<U>() -> NonNull<U>`  method is `#[unstable]`. It was proposed in rust-lang#46952 (comment).
@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Feb 16, 2018

Contributor

I published rust-lang/rfcs#2307 which propose adding 12 concrete types like NonZeroU32, deprecating NonZero, and later making it private. That last step would (finally!) close this tracking issue.

Implementation PR, ready to merge: #48265

  • Add new #[unstable] types in core::num and std::num
  • Deprecate core::nonzero

After whatever time is appropriate for the deprecation period of unstable features, we can land SimonSapin#1 to make core::nonzero private and leave just enough in it to support its public wrappers.

Contributor

SimonSapin commented Feb 16, 2018

I published rust-lang/rfcs#2307 which propose adding 12 concrete types like NonZeroU32, deprecating NonZero, and later making it private. That last step would (finally!) close this tracking issue.

Implementation PR, ready to merge: #48265

  • Add new #[unstable] types in core::num and std::num
  • Deprecate core::nonzero

After whatever time is appropriate for the deprecation period of unstable features, we can land SimonSapin#1 to make core::nonzero private and leave just enough in it to support its public wrappers.

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Feb 23, 2018

Member

Hm, this probably way too late, but are we sure that the name should be NonNull<T>? It seems to me that NonNull overemphasizes, well, non-nullness of this type, while in fact the real trick is, in my understanding, covariant *mut. At least, some folks on Reddit emphasize optimizations over variance: https://www.reddit.com/r/rust/comments/7zmfuu/when_should_nonnull_be_used/ :)

Would just a Ptr<T> be a better name?

EDIT: nvm, see that this already was discussed in another issue: #46952 (comment)

Member

matklad commented Feb 23, 2018

Hm, this probably way too late, but are we sure that the name should be NonNull<T>? It seems to me that NonNull overemphasizes, well, non-nullness of this type, while in fact the real trick is, in my understanding, covariant *mut. At least, some folks on Reddit emphasize optimizations over variance: https://www.reddit.com/r/rust/comments/7zmfuu/when_should_nonnull_be_used/ :)

Would just a Ptr<T> be a better name?

EDIT: nvm, see that this already was discussed in another issue: #46952 (comment)

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Feb 23, 2018

Contributor

The enum optimization is my motivation for pushing this feature to stabilization, FWIW.

I think you could already get covariance before by using *const T (or wrapping it in some other type outside of std). This aspect of ptr::NonNull is just library code, there is no compiler magic. Also it hasn’t happened to me to actively change some code to make it covariant. (Though maybe the stuff I used was already covariant and I just didn’t realize that was important?)

Contributor

SimonSapin commented Feb 23, 2018

The enum optimization is my motivation for pushing this feature to stabilization, FWIW.

I think you could already get covariance before by using *const T (or wrapping it in some other type outside of std). This aspect of ptr::NonNull is just library code, there is no compiler magic. Also it hasn’t happened to me to actively change some code to make it covariant. (Though maybe the stuff I used was already covariant and I just didn’t realize that was important?)

bors added a commit that referenced this issue Mar 21, 2018

Auto merge of #48265 - SimonSapin:nonzero, r=KodrAus
Add 12 num::NonZero* types for primitive integers, deprecate core::nonzero

RFC: rust-lang/rfcs#2307
Tracking issue: ~~#27730 #49137
Fixes #27730

frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 23, 2018

Rollup merge of rust-lang#48265 - SimonSapin:nonzero, r=KodrAus
Add 12 num::NonZero* types for primitive integers, deprecate core::nonzero

RFC: rust-lang/rfcs#2307
Tracking issue: ~~rust-lang#27730 rust-lang#49137
Fixes rust-lang#27730

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 23, 2018

Rollup merge of rust-lang#48265 - SimonSapin:nonzero, r=KodrAus
Add 12 num::NonZero* types for primitive integers, deprecate core::nonzero

RFC: rust-lang/rfcs#2307
Tracking issue: ~~rust-lang#27730 rust-lang#49137
Fixes rust-lang#27730
@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 23, 2018

Contributor

Tracking issue for the new num::NonZero* types: #49137

Contributor

SimonSapin commented Mar 23, 2018

Tracking issue for the new num::NonZero* types: #49137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment