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

dwijnand · 2018-12-19T23:22:39Z

/cc @withoutboats who mentioned to me this might be worth adding to the standard library, in withoutboats/smart#4
/cc @kennytm who last year didn't love the idea of having an Into<NonNull> for Box, in #46952

rust-highfive · 2018-12-19T23:22:49Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2018-12-20T16:42:24Z

Thanks for the PR! This seems reasonable to me, but I'm double checking the rest of libstd for other idioms. It looks like this isn't implemented for Box, the other obvious candidate. The Box type, however, has an into_raw_non_null unstable method for doing the same thing (just not via a trait).

The Box::into_raw_non_null method was added in #46952 and looks like at least at one point it was From but ended up switching back. @SimonSapin do you recall why we opted for not having a From/Into impl between the NonNull and Box types?

I think that leaves us with two routes to go here:

Land these as stable, also removing Box::into_raw_non_null in favor of the same pattern here. Note that if we do this I think we'll want to use From impls instead of Into which are slightly more general.
Land these as unstable, renaming to Arc::into_raw_non_null to mirror Box.

I'd ideally like to hear from @SimonSapin first to see if they remember why we didn't do Into in the first place.

dwijnand · 2018-12-20T17:15:31Z

From is not possible because NonNull is in libcore and Arc/Rc is in liballoc, so according to the laws of coherence it can't be implemented.

So the options left are implementing Into or implementing it as an inherent method. (Or option 3, wait for that coherency rule to be lifted via RFC).

SimonSapin · 2018-12-20T17:17:19Z

do you recall why we opted for not having a From/Into impl between the NonNull and Box types?

#46952 (comment) mentions that we generally avoid methods that take self (rather than associated functions that take Self) on generic smart pointers like Box<T> because it could shadow a method of the same name on T.

Perhaps more importantly, #46952 (comment) says I undid that because of a incoherent_fundamental_impls warning: #46205

Box is #[fundamental] so adding impls for it of existing traits is a breaking change as far as I understand. #56955 proposes adding #[fundamental] to Arc and Rc.

we'll want to use From impls instead of Into which are slightly more general

IIRC coherence does not allow this, since NonNull is defined in libcore but Arc and Rc in liballoc.

withoutboats · 2018-12-20T18:03:07Z

Yes, for box its a potentially breaking change because someone could have implemented Into<NonNull<MyType>> for Box<MyType> because its fundamental. We'd need to at least do a crater run.

I personally prefer using standard conversion methods here, especially since we already do that for references, to avoid users having to look up methods with names like into_raw_non_null.

alexcrichton · 2018-12-20T19:57:18Z

I don't think coherence blocks this, so @dwijnand want to switch to From, add an impl for Box, and we can see what the fallout is on crater?

SimonSapin · 2018-12-20T20:20:42Z

It does when generics are involved: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=cff9ce9a3b1da269f92da22175236596

dwijnand · 2018-12-20T20:30:23Z

Yeah... Over on Zulip I've been pointed to https://github.com/rust-lang/rfcs/blob/master/text/2451-re-rebalancing-coherence.md, which points to #55437 and thus #56145. So if we want to use From and make Box work the same way, we could block this on that PR landing.

SimonSapin · 2018-12-20T20:46:48Z

we generally avoid methods that take self (rather than associated functions that take Self) on generic smart pointers like Box<T> because it could shadow a method of the same name on T.

I guess this is the remaining concern with an Into impl for Box (even through a From one). @rust-lang/libs, any thoughts on this?

alexcrichton · 2018-12-20T21:08:12Z

Oh oops, my mistake! @dwijnand want to switch to Into for Box and we can crater?

I'll hold of on formulating whether it's a good idea until we've tested it on crater :)

withoutboats · 2018-12-20T21:20:55Z

I'll also note that there's an active lang RFC to change coherence in a way that I think would allow From & we are generally inclined to merge, but I also think its backward compatible to switch the Into to From

SimonSapin · 2018-12-24T11:15:36Z

As far as I understand, found possibly newer version of crate can occur because of a bug in rustbuild and/or Cargo. One way to get out of that situation is ./x.py clean or cargo clean. (Unfortunately I don’t know a better way.)

As to the initial incoherent_fundamental_impls warning/error, this is what I mentioned before in #56998 (comment). Currently the language rules around #[fundamental] do not allow this impl. #56998 (comment) suggests that the language will be tweaked and it might be allowed later, but that has not landed yet.

dwijnand · 2018-12-24T16:46:56Z

Is it perhaps related to my usage of RUSTC_WRAPPER=sccache?

dwijnand · 2018-12-25T11:03:01Z

Anyways, thank you @SimonSapin for unblocking me, I've pushed a commit that seems to work for my test cases.

alexcrichton · 2019-01-02T18:16:05Z

@bors: try

bors · 2019-01-02T18:16:23Z

⌛ Trying commit 98b46ba with merge 185e079...

@withoutboats

Add Into<NonNull<T>> impls for Arc<T>/Rc<T> /cc @withoutboats who mentioned to me this might be worth adding to the standard library, in withoutboats/smart#4 /cc @kennytm who last year didn't love the idea of having an Into<NonNull<T>> for Box<T>, in #46952

alexcrichton · 2019-01-03T01:50:39Z

@bors: retry

kennytm · 2019-01-03T18:12:58Z

@craterbot run start=master#ec194646fef1a467073ad74b8b68f6f202cfce97 end=try#185e0799ac53f1168549e11c4fc64ebf18b4a2c1 mode=check-only

craterbot · 2019-01-03T18:13:00Z

👌 Experiment pr-56998 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

dwijnand · 2019-01-09T15:50:51Z

Travis CI is green but is showing yellow/pending.

Re-syncing...

dwijnand · 2019-01-09T15:51:38Z

Oh. Oops, didn't know rfcbot would do that :-/

SimonSapin · 2019-01-09T15:54:04Z

@rfcbot fcp merge

rfcbot · 2019-01-09T15:54:06Z

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

dtolnay · 2019-01-09T16:38:59Z

src/liballoc/boxed.rs

@@ -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")]


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()

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

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

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

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.

dtolnay · 2019-01-09T16:41:23Z

src/liballoc/boxed.rs

@@ -479,6 +482,15 @@ impl From<Box<str>> for Box<[u8]> {
    }
 }

+#[allow(incoherent_fundamental_impls)]


Please add a comment explaining the implications of this.

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

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

src/liballoc/boxed.rs

src/liballoc/rc.rs

dtolnay · 2019-01-09T16:44:10Z

src/liballoc/rc.rs

@@ -1179,6 +1179,14 @@ impl<T> From<Vec<T>> for Rc<[T]> {
    }
 }

+#[unstable(feature = "rc_into_nonnull", reason = "newly added", issue = "0")]
+impl<T: ?Sized> Into<NonNull<T>> for Rc<T> {


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.

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

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

Like Simon explained for Box into NonNull.

dwijnand · 2019-01-10T08:42:51Z

Yeah... Over on Zulip I've been pointed to RFC 2451 Re-Rebalancing Coherence, which points to #55437 and thus #56145. So if we want to use From and make Box work the same way, we could block this on that PR landing.

Well that PR landed, so maybe this can be implemented using From now?

dwijnand · 2019-01-10T09:13:14Z

maybe this can be implemented using From now?

No, it can't.

impl<T: ?Sized> From<Arc<T>> for NonNull<T>

Can't compile in liballoc because T is uncovered. And it can't be implemented with NonNull and From in libcore because Arc isn't available.

dwijnand · 2019-01-15T07:06:59Z

@Centril was this triaged at the meeting, or did you change your mind on nominating it?

Centril · 2019-01-15T07:17:50Z

@dwijnand It was. The gist of it is that you cannot do #[allow(incoherent_fundamental_impls)] for reasons mentioned in #56998 (comment). If elaboration is necessary, I'll redirect you to @nikomatsakis. :)

dwijnand · 2019-01-15T07:49:38Z

I see! Thanks for the info. I'll reformulate this proposal as Arc/Rc::into_raw_non_null then, and see how that goes.

dwijnand · 2019-01-27T17:26:28Z

I'll reformulate this proposal as Arc/Rc::into_raw_non_null then, and see how that goes.

Follow-up in #57934.

Add Into<NonNull<T>> impls for Arc<T>/Rc<T>

7a786d4

rust-highfive assigned alexcrichton Dec 19, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2018

Add assert_eq to Arc/Rc -> NonNull test

dd74e56

This comment has been minimized.

Sign in to view

Add an Into<NonNull<T>> impl for Box<T>

98b46ba

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 3, 2019

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 3, 2019

Remove usage of deprecated Box::into_raw_non_null

0761aa2

This comment has been minimized.

Sign in to view

Ashes to ashes, deprecated to deprecated

d52a14e

dwijnand closed this Jan 9, 2019

rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 9, 2019

dwijnand reopened this Jan 9, 2019

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 9, 2019

dtolnay reviewed Jan 9, 2019

View reviewed changes

dwijnand added 2 commits January 9, 2019 17:21

Fix target version in "since"

d7d526b

Mark Rc/Arc into NonNull as insta-stable

0d0f2fe

Like Simon explained for Box into NonNull.

Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed I-nominated labels Jan 10, 2019

dwijnand closed this Jan 15, 2019

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 15, 2019

dwijnand deleted the from-Arc/Rc-to-NonNull branch January 15, 2019 07:49

rfcbot removed the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Jan 15, 2019

dwijnand mentioned this pull request Apr 15, 2020

Tracking issue for Box::into_raw_non_null #47336

Closed

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

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

Conversation

dwijnand commented Dec 19, 2018

rust-highfive commented Dec 19, 2018

alexcrichton commented Dec 20, 2018

dwijnand commented Dec 20, 2018

SimonSapin commented Dec 20, 2018

withoutboats commented Dec 20, 2018

alexcrichton commented Dec 20, 2018

SimonSapin commented Dec 20, 2018

dwijnand commented Dec 20, 2018

SimonSapin commented Dec 20, 2018

alexcrichton commented Dec 20, 2018

withoutboats commented Dec 20, 2018

This comment has been minimized.

SimonSapin commented Dec 24, 2018

dwijnand commented Dec 24, 2018

dwijnand commented Dec 25, 2018

alexcrichton commented Jan 2, 2019

bors commented Jan 2, 2019

alexcrichton commented Jan 3, 2019

kennytm commented Jan 3, 2019

craterbot commented Jan 3, 2019

This comment has been minimized.

dwijnand commented Jan 9, 2019

dwijnand commented Jan 9, 2019

SimonSapin commented Jan 9, 2019

rfcbot commented Jan 9, 2019 • edited by dtolnay Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand Jan 9, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand commented Jan 10, 2019 • edited Loading

dwijnand commented Jan 10, 2019

dwijnand commented Jan 15, 2019

Centril commented Jan 15, 2019

dwijnand commented Jan 15, 2019

dwijnand commented Jan 27, 2019

rfcbot commented Jan 9, 2019 •

edited by dtolnay

Loading

dwijnand Jan 9, 2019 •

edited

Loading

dwijnand commented Jan 10, 2019 •

edited

Loading