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

Require allocator to be static for boxed Pin-API #79327

Merged
merged 1 commit into from
Nov 29, 2020

Conversation

TimDiekmann
Copy link
Member

@TimDiekmann TimDiekmann commented Nov 23, 2020

Allocators has to retain their validity until the instance and all of its clones are dropped. When pinning a value, it must live forever, thus, the allocator requires a 'static lifetime for pinning a value. Example from reddit:

let alloc = MyAlloc(/* ... */);
let pinned = Box::pin_in(42, alloc);
mem::forget(pinned); // Now `value` must live forever
// Otherwise `Pin`'s invariants are violated, storage invalidated
// before Drop was called.
// borrow of `memory` can end here, there is no value keeping it.
drop(alloc); // Oh, value doesn't live forever.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Nov 23, 2020
@DutchGhost
Copy link
Contributor

Does this requirement also need to be on into_pin (https://github.com/rust-lang/rust/blob/master/library/alloc/src/boxed.rs#L805) ? And perhaps other places as well?

@Mark-Simulacrum
Copy link
Member

cc @withoutboats

@TimDiekmann TimDiekmann changed the title Require allocator to be static for Box::pin_in Require allocator to be static for boxed Pin-API Nov 23, 2020
@TimDiekmann
Copy link
Member Author

Does this requirement also need to be on into_pin (https://github.com/rust-lang/rust/blob/master/library/alloc/src/boxed.rs#L805) ? And perhaps other places as well?

Good point, I added the bound to some other places.


cc @withoutboats

Do you want me reassign this PR to @withoutboats?

@Mark-Simulacrum
Copy link
Member

No, I can review, just wanted to ping.

@Mark-Simulacrum
Copy link
Member

r=me with commits squashed.

I tried to do a cursory check for other cases where this would be needed and didn't find anything, though it's no guarantee. It's a bit concerning that these bounds are necessary, in some sense, but I think we just need to be careful -- ultimately no old code should break. Maybe something to note on API guidelines or similar?

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2020
@TimDiekmann TimDiekmann force-pushed the static-alloc-pin-in-box branch from 785df47 to 7387f48 Compare November 28, 2020 14:25
@TimDiekmann
Copy link
Member Author

@Mark-Simulacrum

r=me with commits squashed.

Done, but I don't have the permissions to r+ unless you delegate+.


It's a bit concerning that these bounds are necessary, in some sense, but I think we just need to be careful

I'm not 100% sure neither, if all of them are necessary, but I agree, that staying conservative is the way to go here. Dropping the bound later is no problem.


ultimately no old code should break

This won't be the case as it only affects the allocator-parameter and that is unstable.


Maybe something to note on API guidelines or similar?

Hmm, what do you mean? Regarding the pin-API in general?

@Mark-Simulacrum
Copy link
Member

Yes, as we add allocator parameters, it can be easy to think that it'll always be 'static. Noting somewhere that this is not true explicitly I think would be good. Not directly related to Pin, though.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 28, 2020

📌 Commit 7387f48 has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2020
@TimDiekmann
Copy link
Member Author

I'll add a note to the AllocRef documentation. Thanks for the review!

@withoutboats
Copy link
Contributor

I feel a bit concerned about this, as Pin was certainly not designed with custom allocators in mind. We should be very careful about how we move here and make sure we have more formal consideration of the safety requirements of allocators before stabilization.

The most conservative option would be to get rid of pin_in and have all the other Pin APIs only implemented for Box<T, Global>. Not calling for that, but we should keep pinning as an unresolved question for stabilization and possibly fall back to that.

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 29, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#79327 (Require allocator to be static for boxed `Pin`-API)
 - rust-lang#79340 (Rename "stability" CSS class to "item-info" and combine `document_stability` with `document_short`)
 - rust-lang#79363 (BTreeMap: try to enhance various comments)
 - rust-lang#79395 (Move ui if tests from top-level into `expr/if`)
 - rust-lang#79443 (Improve rustdoc JS tests error output)
 - rust-lang#79464 (Extend doc keyword feature by allowing any ident)
 - rust-lang#79484 (add enable-full-tools to freebsd builds to prevent occasional link er…)
 - rust-lang#79505 (Cleanup: shorter and faster code)
 - rust-lang#79514 (Add test for issue rust-lang#54121: order dependent trait bounds)
 - rust-lang#79516 (Remove unnecessary `mut` binding)
 - rust-lang#79528 (Fix a bootstrap comment)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a2b4d97 into rust-lang:master Nov 29, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 29, 2020
Jules-Bertholet added a commit to Jules-Bertholet/rust that referenced this pull request Dec 5, 2023
rust-lang#79327 added `'static` bounds to the allocator parameter
for various `Box` + `Pin` APIs to ensure soundness.
But it was a bit overzealous, some of the bounds aren't
actually needed.
oli-obk added a commit to oli-obk/rust that referenced this pull request Feb 13, 2024
…r, r=Amanieu

Add `A: 'static` bound for `Arc/Rc::pin_in`

Analogous to rust-lang#79327
Needed to preserve pin's [drop guarantee](https://doc.rust-lang.org/std/pin/index.html#drop-guarantee)
geektype added a commit to geektype/rust that referenced this pull request Feb 13, 2024
…r, r=Amanieu

Add `A: 'static` bound for `Arc/Rc::pin_in`

Analogous to rust-lang#79327
Needed to preserve pin's [drop guarantee](https://doc.rust-lang.org/std/pin/index.html#drop-guarantee)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2024
…, r=Amanieu

Remove useless `'static` bounds on `Box` allocator

rust-lang#79327 added `'static` bounds to the allocator parameter for various `Box` + `Pin` APIs to ensure soundness. But it was a bit overzealous, some of the bounds aren't actually needed.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2024
… r=Amanieu

Add `A: 'static` bound for `Arc/Rc::pin_in`

Analogous to rust-lang#79327
Needed to preserve pin's [drop guarantee](https://doc.rust-lang.org/std/pin/index.html#drop-guarantee)
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Apr 15, 2024
Add `A: 'static` bound for `Arc/Rc::pin_in`

Analogous to rust-lang/rust#79327
Needed to preserve pin's [drop guarantee](https://doc.rust-lang.org/std/pin/index.html#drop-guarantee)
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
Add `A: 'static` bound for `Arc/Rc::pin_in`

Analogous to rust-lang/rust#79327
Needed to preserve pin's [drop guarantee](https://doc.rust-lang.org/std/pin/index.html#drop-guarantee)
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Add `A: 'static` bound for `Arc/Rc::pin_in`

Analogous to rust-lang/rust#79327
Needed to preserve pin's [drop guarantee](https://doc.rust-lang.org/std/pin/index.html#drop-guarantee)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants