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

Rename std::ptr::Shared to NonNull and stabilize it #46952

Merged
merged 15 commits into from Jan 20, 2018

Conversation

@SimonSapin
Contributor

SimonSapin commented Dec 22, 2017

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:

    #[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.)

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 22, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented Dec 22, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 22, 2017

Contributor

That proposal so far has got a couple thumbs up and no negative comment or reaction. Maybe @rust-lang/libs can discuss and do a FCP in this thread?

Contributor

SimonSapin commented Dec 22, 2017

That proposal so far has got a couple thumbs up and no negative comment or reaction. Maybe @rust-lang/libs can discuss and do a FCP in this thread?

@leodasvacas

This comment has been minimized.

Show comment
Hide comment
@leodasvacas

leodasvacas Dec 22, 2017

Contributor

Bikeshed: nonnull is an eyesore.

Contributor

leodasvacas commented Dec 22, 2017

Bikeshed: nonnull is an eyesore.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 22, 2017

Contributor

@leodasvacas What’s your preferred shed color?

By your use of lower case I assume you mean in the Box methods. I’m very open to renaming them, do you have a suggestions? Ideally these might replace from_raw and into_raw, but those are already stable with *mut T.

Contributor

SimonSapin commented Dec 22, 2017

@leodasvacas What’s your preferred shed color?

By your use of lower case I assume you mean in the Box methods. I’m very open to renaming them, do you have a suggestions? Ideally these might replace from_raw and into_raw, but those are already stable with *mut T.

@leodasvacas

This comment has been minimized.

Show comment
Hide comment
@leodasvacas

leodasvacas Dec 22, 2017

Contributor

Sorry for being unclear, I just prefer non_null over nonnull in method names that's all.

Contributor

leodasvacas commented Dec 22, 2017

Sorry for being unclear, I just prefer non_null over nonnull in method names that's all.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 22, 2017

Contributor

Ok. So the methods would be Box::from_non_null_raw(…) and Box::into_non_null_raw(…). I don’t have a strong opinion either way, let’s see what other people think.

Contributor

SimonSapin commented Dec 22, 2017

Ok. So the methods would be Box::from_non_null_raw(…) and Box::into_non_null_raw(…). I don’t have a strong opinion either way, let’s see what other people think.

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Dec 22, 2017

Member

🚲🏡: since it's NonNull (not Nonnull), non_null makes sense over nonnull.

Member

scottmcm commented Dec 22, 2017

🚲🏡: since it's NonNull (not Nonnull), non_null makes sense over nonnull.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 22, 2017

Contributor

I’ve pushed a commit to rename *_nonnull_raw to *_non_null_raw. Note however that there is precedent in the name of the core::nonzero module.

Contributor

SimonSapin commented Dec 22, 2017

I’ve pushed a commit to rename *_nonnull_raw to *_non_null_raw. Note however that there is precedent in the name of the core::nonzero module.

@ExpHP

This comment has been minimized.

Show comment
Hide comment
@ExpHP

ExpHP Dec 23, 2017

Contributor

But core::nonzero is not stable, is it?

That said, riding in the 🚲 lane, non_zero certainly seems awkward. If I may suggest:

  • NotNull instead of NonNull.
  • Embrace "nonzero" as a single word.
use ::std::ptr::NotNull;
use ::core::nonzero::Nonzero;

fn foo() {
    let raw = NotNull::new(&0).unwrap();
    let _ = Box::from_not_null_raw(raw);
}

or leaning the other way, we could have ::std::ptr::Nonnull, but something about it just looks wrong to me...

Contributor

ExpHP commented Dec 23, 2017

But core::nonzero is not stable, is it?

That said, riding in the 🚲 lane, non_zero certainly seems awkward. If I may suggest:

  • NotNull instead of NonNull.
  • Embrace "nonzero" as a single word.
use ::std::ptr::NotNull;
use ::core::nonzero::Nonzero;

fn foo() {
    let raw = NotNull::new(&0).unwrap();
    let _ = Box::from_not_null_raw(raw);
}

or leaning the other way, we could have ::std::ptr::Nonnull, but something about it just looks wrong to me...

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 26, 2017

Contributor

Besides naming, this is also a good time to reconsider whether Box::from_non_null_raw and Box::into_non_null_raw should exist in the first place.

I added into_non_null_raw in order to remove a few unsafe blocks in rc, arc, and linked_list. They use Box to allocate but then only keep a NonNull<T> pointer. Without this function, they (and other users doing similar things) would need unsafe to assert that the result of Box::into_raw() is not null.

So I think there's value in keeping this conversion in some form. It could be an impl of the From trait, but we generally avoid methods (as opposed to associated functions) on generic smart pointers as these could shadow methods of the contained type.

Box::from_non_zero_raw however I think we can remove. I added it at the same time, for symmetry, but it's not used at all in the tree. It can be replaced with Box::from_raw(some_non_null.get_mut()), both versions of this are unsafe anyway.

Contributor

SimonSapin commented Dec 26, 2017

Besides naming, this is also a good time to reconsider whether Box::from_non_null_raw and Box::into_non_null_raw should exist in the first place.

I added into_non_null_raw in order to remove a few unsafe blocks in rc, arc, and linked_list. They use Box to allocate but then only keep a NonNull<T> pointer. Without this function, they (and other users doing similar things) would need unsafe to assert that the result of Box::into_raw() is not null.

So I think there's value in keeping this conversion in some form. It could be an impl of the From trait, but we generally avoid methods (as opposed to associated functions) on generic smart pointers as these could shadow methods of the contained type.

Box::from_non_zero_raw however I think we can remove. I added it at the same time, for symmetry, but it's not used at all in the tree. It can be replaced with Box::from_raw(some_non_null.get_mut()), both versions of this are unsafe anyway.

Show outdated Hide outdated src/libcore/ptr.rs Outdated
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fmt::Pointer::fmt(&self.as_ptr(), f)
}
}
#[unstable(feature = "shared", issue = "27730")]
impl<T: ?Sized> From<Unique<T>> for Shared<T> {
#[stable(feature = "nonnull", since = "1.24.0")]

This comment has been minimized.

@goodmanjonathan

goodmanjonathan Dec 27, 2017

Contributor

Shouldn't this be #[unstable(feature = "unique")]?

@goodmanjonathan

goodmanjonathan Dec 27, 2017

Contributor

Shouldn't this be #[unstable(feature = "unique")]?

This comment has been minimized.

@SimonSapin

SimonSapin Dec 27, 2017

Contributor

Maybe. Even if this impl is usable, it can’t be used if you can’t get a Unique<T> value in the first place on stable Rust. And anyway, IIRC stability attributes don’t work on impl blocks at the moment. (They’re insta-stable if the relevant traits and types are stable.)

@SimonSapin

SimonSapin Dec 27, 2017

Contributor

Maybe. Even if this impl is usable, it can’t be used if you can’t get a Unique<T> value in the first place on stable Rust. And anyway, IIRC stability attributes don’t work on impl blocks at the moment. (They’re insta-stable if the relevant traits and types are stable.)

This comment has been minimized.

@goodmanjonathan

goodmanjonathan Dec 27, 2017

Contributor

Right, I was just afraid it might have other effects (e.g. like showing up in rustdoc). If not, I'm fine either way.

@goodmanjonathan

goodmanjonathan Dec 27, 2017

Contributor

Right, I was just afraid it might have other effects (e.g. like showing up in rustdoc). If not, I'm fine either way.

This comment has been minimized.

@SimonSapin
@SimonSapin

SimonSapin Dec 27, 2017

Contributor

This comment has been minimized.

@goodmanjonathan

goodmanjonathan Dec 27, 2017

Contributor

Alright, then.

@goodmanjonathan

goodmanjonathan Dec 27, 2017

Contributor

Alright, then.

SimonSapin added a commit to SimonSapin/rust that referenced this pull request Dec 27, 2017

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 27, 2017

Contributor
  • Rebased on top of #46792
  • Removed Box::from_non_null_raw (can be replaced with Box::from_raw(x.get_mut()))
  • Renamed Box::into_non_null_raw to Box::into_raw_non_null
Contributor

SimonSapin commented Dec 27, 2017

  • Rebased on top of #46792
  • Removed Box::from_non_null_raw (can be replaced with Box::from_raw(x.get_mut()))
  • Renamed Box::into_non_null_raw to Box::into_raw_non_null
@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 29, 2017

Contributor

It could be an impl of the From trait, but we generally avoid methods (as opposed to associated functions) on generic smart pointers as these could shadow methods of the contained type.

However we already have impl<'a, T: ?Sized> From<&'a T> for NonNull<T> and impl<'a, T: ?Sized> From<&'a mut T> for NonNull<T> which (indirectly) add an .into() method on &T and &mut T. So maybe it’s not such a problem in practice?

Contributor

SimonSapin commented Dec 29, 2017

It could be an impl of the From trait, but we generally avoid methods (as opposed to associated functions) on generic smart pointers as these could shadow methods of the contained type.

However we already have impl<'a, T: ?Sized> From<&'a T> for NonNull<T> and impl<'a, T: ?Sized> From<&'a mut T> for NonNull<T> which (indirectly) add an .into() method on &T and &mut T. So maybe it’s not such a problem in practice?

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 29, 2017

Contributor

Pushed another commit (that we can remove from the PR if it turns out problematic) that replaces Box::into_raw_non_null with an impl of the Into trait. (Not From because coherence.)

Contributor

SimonSapin commented Dec 29, 2017

Pushed another commit (that we can remove from the PR if it turns out problematic) that replaces Box::into_raw_non_null with an impl of the Into trait. (Not From because coherence.)

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 29, 2017

Member

I don't think it's a good idea to reintroduce an Into impl, esp. when the corresponding From can't exist...

Member

kennytm commented Dec 29, 2017

I don't think it's a good idea to reintroduce an Into impl, esp. when the corresponding From can't exist...

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 29, 2017

Contributor

@kennytm What about the From<&T> and From<&mut T> impls?

Contributor

SimonSapin commented Dec 29, 2017

@kennytm What about the From<&T> and From<&mut T> impls?

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 29, 2017

Member

@SimonSapin Those sound fine to me. My concern is just about the Into impl. IIRC there was a PR that changed all Into impls to From impls, introducing a new Into impl is kinda regression in progress 😄.

Member

kennytm commented Dec 29, 2017

@SimonSapin Those sound fine to me. My concern is just about the Into impl. IIRC there was a PR that changed all Into impls to From impls, introducing a new Into impl is kinda regression in progress 😄.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 29, 2017

Contributor

Well, the reason both traits exist is because conversions between types from different crates can only be implemented with one or the other. I don’t see how adding a new Into impl is bad just because we removed (replaced) some other ones.

That said I’m not particularly attached to this one impl. It only avoid picking an awkward name for the Box associated function.

Contributor

SimonSapin commented Dec 29, 2017

Well, the reason both traits exist is because conversions between types from different crates can only be implemented with one or the other. I don’t see how adding a new Into impl is bad just because we removed (replaced) some other ones.

That said I’m not particularly attached to this one impl. It only avoid picking an awkward name for the Box associated function.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Dec 29, 2017

Contributor

I removed that commit since it caused a warning (an error, with #[deny(warnings)]) for #46205.

Contributor

SimonSapin commented Dec 29, 2017

I removed that commit since it caused a warning (an error, with #[deny(warnings)]) for #46205.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Jan 20, 2018

Contributor
Checking "basic.js" ... OK
Checking "enum-option.js" ... OK
Checking "fn-forget.js" ... OK
Checking "from_u.js" ... FAILED
==> Result not found in 'others': '{"path":"std::boxed::Box","name":"from_unique"}'
Checking "macro-print.js" ... OK
Checking "string-from_ut.js" ... OK
Checking "struct-vec.js" ... OK
command did not execute successfully: "C:\\Program Files (x86)\\nodejs\\node" "src/tools/rustdoc-js/tester.js" "x86_64-pc-windows-msvc"
expected success, got: exit code: 1

Sigh. This PR changes the expected result of an unrelated test (rustdoc seach results) that was added after the PR was created. Should be fixed now:

diff --git a/src/test/rustdoc-js/from_u.js b/src/test/rustdoc-js/from_u.js
index 920620a9ae..0296788f7a 100644
--- a/src/test/rustdoc-js/from_u.js
+++ b/src/test/rustdoc-js/from_u.js
@@ -15,7 +15,6 @@ const EXPECTED = {
         { 'path': 'std::char', 'name': 'from_u32' },
         { 'path': 'std::str', 'name': 'from_utf8' },
         { 'path': 'std::string::String', 'name': 'from_utf8' },
-        { 'path': 'std::boxed::Box', 'name': 'from_unique' },
         { 'path': 'std::i32', 'name': 'from_unsigned' },
         { 'path': 'std::i128', 'name': 'from_unsigned' },
     ],
Contributor

SimonSapin commented Jan 20, 2018

Checking "basic.js" ... OK
Checking "enum-option.js" ... OK
Checking "fn-forget.js" ... OK
Checking "from_u.js" ... FAILED
==> Result not found in 'others': '{"path":"std::boxed::Box","name":"from_unique"}'
Checking "macro-print.js" ... OK
Checking "string-from_ut.js" ... OK
Checking "struct-vec.js" ... OK
command did not execute successfully: "C:\\Program Files (x86)\\nodejs\\node" "src/tools/rustdoc-js/tester.js" "x86_64-pc-windows-msvc"
expected success, got: exit code: 1

Sigh. This PR changes the expected result of an unrelated test (rustdoc seach results) that was added after the PR was created. Should be fixed now:

diff --git a/src/test/rustdoc-js/from_u.js b/src/test/rustdoc-js/from_u.js
index 920620a9ae..0296788f7a 100644
--- a/src/test/rustdoc-js/from_u.js
+++ b/src/test/rustdoc-js/from_u.js
@@ -15,7 +15,6 @@ const EXPECTED = {
         { 'path': 'std::char', 'name': 'from_u32' },
         { 'path': 'std::str', 'name': 'from_utf8' },
         { 'path': 'std::string::String', 'name': 'from_utf8' },
-        { 'path': 'std::boxed::Box', 'name': 'from_unique' },
         { 'path': 'std::i32', 'name': 'from_unsigned' },
         { 'path': 'std::i128', 'name': 'from_unsigned' },
     ],
@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Jan 20, 2018

Member

@bors r=alexcrichton

Member

eddyb commented Jan 20, 2018

@bors r=alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 20, 2018

Contributor

📌 Commit 602a445 has been approved by alexcrichton

Contributor

bors commented Jan 20, 2018

📌 Commit 602a445 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 20, 2018

Contributor

⌛️ Testing commit 602a445 with merge bdda8d6...

Contributor

bors commented Jan 20, 2018

⌛️ Testing commit 602a445 with merge bdda8d6...

bors added a commit that referenced this pull request 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.)
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 20, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing bdda8d6 to master...

Contributor

bors commented Jan 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing bdda8d6 to master...

@bors bors merged commit 602a445 into rust-lang:master Jan 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Jan 20, 2018

Contributor

\o/ you did it!

Contributor

Gankro commented Jan 20, 2018

\o/ you did it!

@bluss bluss added the relnotes label Jan 20, 2018

@SimonSapin SimonSapin deleted the SimonSapin:nonnull branch Jan 20, 2018

@hawkw hawkw referenced this pull request Jan 20, 2018

Open

intruder_alarm stability #12

@joshlf

This comment has been minimized.

Show comment
Hide comment
@joshlf

joshlf Jan 21, 2018

Contributor

Stop me if I should bring this up in a new issue, but how do folks feel about adding a conversion from NonNull<T> to NonNull<U>? It wouldn't add any extra unsafety because you can already do NonNull::new(x.as_ptr() as *mut U).unwrap(), and would basically be a cleaner way of writing that.

Contributor

joshlf commented Jan 21, 2018

Stop me if I should bring this up in a new issue, but how do folks feel about adding a conversion from NonNull<T> to NonNull<U>? It wouldn't add any extra unsafety because you can already do NonNull::new(x.as_ptr() as *mut U).unwrap(), and would basically be a cleaner way of writing that.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Jan 21, 2018

Contributor

@joshlf It should probably be a new issue (or a PR ;)) but I’ve been thinking the same when considering whether every *mut u8 in return types in the Alloc trait should be NonNull<u8> instead. For now though I think the need is less pressing as long as there are few APIs that return NonNull<_>: if you create it from a raw pointer anyway, you can cast with as beforehand.

Contributor

SimonSapin commented Jan 21, 2018

@joshlf It should probably be a new issue (or a PR ;)) but I’ve been thinking the same when considering whether every *mut u8 in return types in the Alloc trait should be NonNull<u8> instead. For now though I think the need is less pressing as long as there are few APIs that return NonNull<_>: if you create it from a raw pointer anyway, you can cast with as beforehand.

@joshlf

This comment has been minimized.

Show comment
Hide comment
@joshlf

joshlf Jan 21, 2018

Contributor
Contributor

joshlf commented Jan 21, 2018

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Jan 21, 2018

Contributor

Oops, I forgot to comment here about it. Yeah, since I was making other changes… :)

Contributor

SimonSapin commented Jan 21, 2018

Oops, I forgot to comment here about it. Yeah, since I was making other changes… :)

rozbb added a commit to Aatch/ramp that referenced this pull request Jan 22, 2018

@declanvk declanvk referenced this pull request Feb 3, 2018

Open

Missing crates #23

bors added a commit that referenced this pull request 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 pull request 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment