Relax Allocator bounds into pin-safe trait

[djkoloski]

Because Allocator requires that returned memory blocks remain valid
until the instance and all clones are dropped, some types of allocators
cannot fulfill the safety requirements for the trait. This change
relaxes the safety requirements of Allocator to blocks remaining valid
until the allocator becomes unreachable and moves the bound into a
separate PinSafeAllocator trait.

It also relaxes some overly cautious bounds on the Unpin impls for
Box since unpinning a box doesn't require any special consideration.

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[djkoloski]

r? @yaahc

Relevant discussion:

• Original issue: Relax A: 'static bound for boxed Pin APIs #90822
• First pass at a soundness fix: Restrict Allocator impl to &'static A #94069
• Zulip chat regarding allocator traits

@TimDiekmann and @CAD97 for wg-allocators visibility.

[CAD97]

I believe Box::leak also needs to be bound by A: PinSafeAllocator. Otherwise, this matches what I was looking for with extracting the pin safe guarantee into its own extension trait.

[CAD97]

This is, of course, the hard part of this PR.

I'm thinking of the guarantee in a similar way to that of references, in that it's a "while the handle exists." Of course, under the Stacked Borrows model (and, to an extent, NLL), it's not when the borrow exists, but until when the borrow is last used.

So there're two ways I currently see to explain the weaker (not pin safe) guarantee, borrowing language from references:

• The memory regions are valid for the lifetime of the allocator. This is complicated by the written 'static lifetime not actually being "the lifetime of the allocator" in this fashion, but an upper bound, so probably not a good way to explain it.
• The memory regions are valid until the point when the last clone is last used.

Effectively, what we're looking for is the dual of the pinning guarantee.

So I think how I'd draft this is

Memory blocks must retain their validity until the allocator instance and all of its clones are dropped, forgotten, leaked, or otherwise never accessed again. (In other words, when a lifetime held by the allocator instance is allowed to expire.)

(And now writing that I'm interested in how the #[may_dangle] borrowck eyepatch interacts with the guarantee, since that (very roughly) allows a lifetime to expire before the container. IIUC it doesn't, since the eyepatch refers to the lifetime within a generic argument, but I'm never certain w.r.t. may_dangle.)

[djkoloski]

I updated the wording to be more explicit about these conditions. Does that work?

[djkoloski]

Looks like changing leak comes with some baggage. I'll have to take a closer look later on.

[djkoloski]

I pushed through some changes to Box::leak. There were some comments about MIRI in there, so it might be a good idea to get these tested properly under it.

[bors]

☔ The latest upstream changes (presumably #94901) made this pull request unmergeable. Please resolve the merge conflicts.

[djkoloski]

@rustbot label +T-libs-api -T-compiler
@rustbot ping @rust-lang/wg-allocators

[tmandry]

cc @rust-lang/wg-allocators

[Amanieu]

Another possibility would be to change the safety condition for Allocator to :

Memory blocks returned from an allocator must point to valid memory and retain their validity until the instance and all of its clones are dropped, or for the lifetime of the allocator type, whichever is shorter.

(This could probably be worded better...)

This moves the ball back into Box::pin_in's camp to required A: 'static, but it removes a footgun for implementers of non-'static allocators.

I'm hesitant to add more complexity to the Allocator API just to support pinning, which is a fairly niche operation.

[djkoloski]

I think it would be sufficient to say:

Memory blocks returned from an allocator must point to valid memory and retain their validity for the lifetime of the allocator type.

As a few examples:

• 'static allocators must leak memory indefinitely (for a 'static lifetime) if forgotten.
• Any allocator that manages backing memory with a non-static lifetime 'a has its lifetime bounded by 'a. Therefore, it's safe for the allocator to guarantee that the memory will not be repurposed during 'a.

I've been prototyping out what this would look like in a crate and it seems like 'static allocators would be compatible with Pin.

As an aside, I've also been trying out changing Pin<P> to only pin for the lifetime of P and that seems promising as well. I think there's a parallel here between generalizing the validity guarantees of Allocator and generalizing the pinning guarantees of Pin.

[CAD97]

Personally I think that extracting the pinning guarantees to a separate trait simplifies the main API w.r.t. pinning rather than complicates it for pinning, but if a simple single-trait solution works, then that's better.

Personally, I think I'd word it something along the lines of

Memory blocks allocated by this allocator must remain valid for the lifetime of the allocator or until all clones of the allocator are dropped, whichever is sooner.

In an equivalent formulation, given T: ?Unpin, A: 'a + Allocator, A must allocate blocks compatible with the pinning guarantees of Pin<Box<T, A>>, which can be leaked into Pin<&'a mut T>.

(This is sort of circular, as the purpose of this clarification is to make Pin<Box<_, A>> work, but Box is "how safe allocations work" and clearly communicates the 'a-or-drop-whichever-is-first.)

---

As an aside, I've also been trying out changing Pin<P> to only pin for the lifetime of P and that seems promising as well.

This sounds sketchy, though, because how do you keep the moral equivalent of the following from blowing up? (Off topic for this thread, feel free to ping me on Zulip)

let mut future: impl Future;
'a: {
    Pin::new(&'a mut future).poll();
}
// 'a is over, future "isn't" pinned
// caveat emptor: yes it is, per current pin documentation
forget(future);
// reuse future's backing memory
// if the async runtime stored a pointer to the future, 💣

[bors]

☔ The latest upstream changes (presumably #95678) made this pull request unmergeable. Please resolve the merge conflicts.

[yaahc]

r? rust-lang/libs-api

[JohnCSimon]

Ping from triage:
@djkoloski
Can you please address the merge conflict?

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[djkoloski]

@JohnCSimon PR has been fixed up.

[bors]

☔ The latest upstream changes (presumably #100982) made this pull request unmergeable. Please resolve the merge conflicts.

[KittyBorgX]

@rustbot author
@djkoloski Hey! It looks like there are merge conflicts, so can you sort them out? A reviewer will review the code once you're done!

[Dylan-DPC]

Closing this as inactive