allocator: refactor for stabilisation

[nia-e]

View all comments

Adds my current proposal per the doc in #156882 and follow-up Zulip conversations (notably for dyn-compat) unstably.

r? libs

[nia-e]

Note that the no-panic bounds introduced close #156490 and #155746. However, if we want to relax them in the future, we may need to adjust our collection types to be more resilient.

[clarfonthey]

This guarantee seems fine on the surface, but I'm trying to wrap my head around what's actually being guaranteed here. Like, clearly, it'd be wildly unsafe to offer an invalidate_everything method on an allocator that just deletes the backing memory without requiring any of the things that are using it to be dropped, but this feels like it's opening the door for that kind of method "as long as you're careful" which, doesn't make a lot of sense.

Like, I'm trying to gauge what value is being gained by this guarantee and it mostly just feels like it's making things more confusing without actually helping.

View changes since the review

[nia-e]

we're saying that such an invalidate_everything method is allowed to exist, and you can't rely on the allocator having not yet been dropped for soundness. in other words, so long as you hold a &alloc (thus preventing a &mut alloc from being created), you can trust the memory you have is fine; but if you lose the &alloc and get a new one back, your memory might have been scribbled over and you must act as such

[clarfonthey]

Okay, but wouldn't that be the same thing as the lifetime expiring as before? Technically, even though both of them are written as &A, you've gotten a new &A lifetime in that case.

[nia-e]

i think the extra guarantee here is that if you do hold a &mut alloc, you can call methods that take alloc by-shared-ref without worry but you can't pass the actual &mut to an untrusted function and expect your allocator to be okay at the end. but i agree that's not obviously guaranteed

[clarfonthey]

Isn't that just totally breaking the aliasing guarantees, though? Since that &mut reference wouldn't be unique.

[nia-e]

...you know, you make a good point. i'll revisit the reasoning for this, i recall adding this in response to something being brought up

[nia-e]

nvm, i'm being stupid. the following is the reason:

let mut alloc = SomeAllocator::new();
let ptr = alloc.allocate(...);
alloc.something_by_shared_ref();
// ptr is still guaranteed to be valid
alloc.trusted_method_by_unique_ref();
// ptr is still valid because we know for sure the method is trusted not to mess w/ allocator state
alloc.untrusted_method_by_unique_ref();
// ptr must be assumed to be maybe-invalid even if the lifetime of alloc is not expired and ptr hasn't yet been deallocated

[clarfonthey]

I guess that I was technically thinking of Box whose lifecycle is intrinsically tied to the lifetime of the allocator parameter, whereas in this case if you just call alloc and dealloc manually the "lifetime" is not really tracked at all. So, yes, mutable borrows can happen on the allocator and it's fine, and you guarantee this doesn't happen by taking a non-mutable borrow.

[clarfonthey]

Seeing this thread from @RalfJung: #157428 (comment)

I think we probably also need to be careful about how we define the relationship between these rules and StaticAllocator, since "lifetime expiration" in those cases refers to the allocator value and not references in that case.

[clarfonthey]

@rustbot author

(mostly so you can more clearly signal when you think things are ready; I've commented here already so I'll see any additional changes for review as they're made)

[nia-e]

I dropped the 'static bound here (and thus implicitly in Box::pin_in, etc.); since this trait is about being able to be lifetime-subtyped safely, it would mean that you need to be able to coerce to a StaticAllocator + 'a so the whole guarantee about "this is Actually Static I Promise" has weight. cc @rust-lang/opsem in case i did a bad here

View changes since the review

[RalfJung]

That's more of a @rust-lang/types question

[clarfonthey]

Perhaps a "sealed" trait might be the most useful way to handle this?

By that I mean, put the trait in a private module but mark it as public, and change the impl restriction to impl(super). You should be able to reference it still but rustdoc won't document it.

View changes since the review

[nia-e]

and the methods go on the sealed trait? hm, maybe. i'll try

[clarfonthey]

Yeah, I don't know what exactly we would call this. It's the same mechanism we use for sealing traits but here it would just be to hide it from rustdoc, essentially, so it stops trying to link to it.

There is probably some rustdoc bug hidden under this but I think we can work around it for now.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[nia-e]

think this addresses the immediate stuff, and gets this into a mergeable state. also cc @rust-lang/libs-api in case there are objections to having this api on nightly

@rustbot ready

[clarfonthey]

Would probably be worth also talking with @theemathas and figuring out whether part of this should be moved into #156544, or if this should supercede that.

[nia-e]

@theemathas I'm a little unsure about the language w.r.t. unique references due to how it interacts with Clone. If an allocator provides a delete_everything(&mut self) method alongside AllocatorClone, doesn't that mean that cloning an allocator and then calling delete_everything on said clone is allowed to invalidate allocations made by the original? That would mean that even giving out shared references is disallowed which... no.

Alternatively, we can just say that AllocatorClone requires that unique references not be sufficient to invalidate state, which means that the above method cannot be implemented on a type that is AllocatorClone (unless marked unsafe ig)

edit: even with this, i would keep the language around it being unsound to offer a mutable reference to the current allocator bc i realise otherwise someone could mem::swap a different allocator into e.g. a box 🫠

[clarfonthey]

Yeah, the real danger with mutable references is the ability to swap out values entirely, not just call a fixed set of methods. So it makes sense to just blanket forbid them out of caution.