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.

Sign up for GitHub

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

Jump to bottom

Improve Box<T> -> Pin<Box<T>> conversion #57313

Merged
merged 2 commits into from
Jan 5, 2019
Merged

Improve Box<T> -> Pin<Box<T>> conversion #57313

merged 2 commits into from
Jan 5, 2019

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jan 3, 2019

I found the From trait conversion for this very hard to find, having a named function for it is much more discoverable. Also fixes #56256 as I need that in the place I'm using this.

Has a placeholder tracking issue, will file an issue once I get feedback.

@rust-highfive
Copy link
Collaborator

r? @KodrAus

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 3, 2019
@Nemo157
Copy link
Member Author

Nemo157 commented Jan 3, 2019

r? @cramertj

@rust-highfive rust-highfive assigned cramertj and unassigned KodrAus Jan 3, 2019
@cramertj cramertj added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 3, 2019
@cramertj
Copy link
Member

cramertj commented Jan 3, 2019

I feel sort of odd about having Box::pin create a Pin<Box<_>> and using into_pin to convert a Box<_> into a Pin<Box<_>>, but it seems basically fine to me. Assigning to a T-libs member to FCP.

r? @alexcrichton

@alexcrichton
Copy link
Member

Since this is unstable it doesn't need an FCP, but should this also be added to Rc and Arc?

@cramertj
Copy link
Member

cramertj commented Jan 4, 2019

@alexcrichton No, this method wouldn't be safe on Rc and Arc because if you have one Rc<_> and one Pin<Rc<_>> pointing to the same value (which could be created by cloning an Rc and using the method you describe) you can use Pin<Rc<_>> to witness that the inner value would never be moved, drop the Pin<Rc<_>>, then use Rc::try_unwrap to move the value out of the remaining Rc<_>, which would be unsound.

@alexcrichton
Copy link
Member

Sure thing

@bors: r=cramertj

@bors
Copy link
Contributor

bors commented Jan 4, 2019

📌 Commit d1a42ea has been approved by cramertj

@bors 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 4, 2019
kennytm added a commit to kennytm/rust that referenced this pull request Jan 5, 2019
@kennytm
Rollup merge of rust-lang#57313 - Nemo157:box-to-pin, r=cramertj
676b0b0 
Improve Box<T> -> Pin<Box<T>> conversion

I found the `From` trait conversion for this very hard to find, having a named function for it is much more discoverable. Also fixes rust-lang#56256 as I need that in the place I'm using this.

Has a placeholder tracking issue, will file an issue once I get feedback.
@kennytm kennytm mentioned this pull request Jan 5, 2019
Merged
bors added a commit that referenced this pull request Jan 5, 2019
@bors
Auto merge of #57354 - kennytm:rollup, r=kennytm
68fe518 
Rollup of 17 pull requests

Successful merges:

 - #57219 (Remove some unused code)
 - #57229 (Fix #56806 by using `delay_span_bug` in object safety layout sanity checks)
 - #57233 (Rename and fix nolink-with-link-args test)
 - #57238 (Fix backtraces for inlined functions on Windows)
 - #57249 (Fix broken links to second edition TRPL.)
 - #57267 (src/jemalloc is gone, remove its mention from COPYRIGHT)
 - #57273 (Update the stdsimd submodule)
 - #57278 (Add Clippy to config.toml.example)
 - #57295 (Fix 'be be' constructs)
 - #57311 (VaList::copy should not require a mutable ref)
 - #57312 (`const fn` is no longer coming soon (const keyword docs))
 - #57313 (Improve Box<T> -> Pin<Box<T>> conversion)
 - #57314 (Fix repeated word typos)
 - #57326 (Doc rewording, use the same name `writer`)
 - #57338 (rustdoc: force binary filename for compiled doctests)
 - #57342 (librustc_mir: Make qualify_min_const_fn module public)
 - #57343 (Calculate privacy access only via query)

Failed merges:

 - #57340 (Use correct tracking issue for c_variadic)

r? @ghost
@bors bors merged commit d1a42ea into rust-lang:master Jan 5, 2019
@Nemo157 Nemo157 deleted the box-to-pin branch January 7, 2019 09:18
@hellow554
Copy link
Contributor

Funny enough, today I found this function and asked myself why the heck does it exist? It clearly states, that From already does the same, so why having another function doing the same? @Nemo157 what were your reaons apart from "it can be found easier"? Have you ever used it yet? ^^ I'm just curious, because to me it feels verbose and not necessary.

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 16, 2019

@hellow554 it's in use here: https://github.com/rust-lang-nursery/futures-rs/blob/8efde615a80aa23ece05d11edbe40db73f326f41/futures-util/src/future/join_all.rs#L119

@hellow554
Copy link
Contributor

I see, but elems.into() would have done the same. I see the explicitness though... :/ I'm indecisive.

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 16, 2019

Yep, but when implementing that originally I didn’t even think of elems.into() and just used an unsafe block around Pin::new_unchecked, I think I only noticed that From worked when adding this method.

@Centril Centril mentioned this pull request Jul 2, 2019
Closed
@czipperz
Copy link
Contributor

czipperz commented Jul 3, 2019

If you want to be explicit, you could just use Pin::from(x) rather than x.into_pin().

@Nemo157
Copy link
Member Author

Nemo157 commented Jul 4, 2019

@czipperz it's interesting that that works, but it's relying on the currently unique implementation of From<Box<_>> so could break if any more implementations are added (unless inference constrains the return type to be Pin<Box<_>>), e.g. playground.

@Nemo157 Nemo157 mentioned this pull request Jul 4, 2019
Closed
@czipperz
Copy link
Contributor

czipperz commented Jul 4, 2019

I misread the code as a method rather than a function. IE Pin::from(box) is equivalent to Box::into_pin(box) (as long as other implementations aren't in scope)

@czipperz
Copy link
Contributor

czipperz commented Jul 4, 2019

I don't think this is really a useful feature to specialize in this way. Can't you just use the hyper fish operator to disambiguate?

@czipperz
Copy link
Contributor

czipperz commented Jul 4, 2019

Like do people really implement this sort of functionality? It seems like there isn't really a reason for this feature to exist imo.

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

@alexcrichton alexcrichton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add T: ?Sized to the From<Box<T>> impl for Pin<Box<T>>
8 participants
@Nemo157 @rust-highfive @cramertj @alexcrichton @bors @hellow554 @czipperz @KodrAus