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

[nia-e]

yeah but i had an idea for a funny commit name if i do change it so. i had to, my hands were tied

[RalfJung]

Additionally, I will remark that the bit in the safety docs of Allocator about allocations being compiler magic (i.e. one cannot rely on an allocation "actually happening" & that the compiler may insert new matching allocate/deallocate calls) are only ever relevant for StaticAllocator (well, specifically when wrapped in NativeAllocator). It's a safety point that is impossible for users to observe so I'm not convinced it'd be breaking to move it to StaticAllocator in the future but I also see no reason to not move it now & make it slightly easier to implement Allocator. I'd lean slightly in favour of keeping the safety req in place since i'm not sure if custom allocators get weird magic opsem treatment today, but figured it's worth bringing up

If calls to the regular Allocator trait are not actually magic and we don't plan to make them magic then I don't think we should document them as magic.

[nia-e]

so, cc @maxdexh @theemathas - do you suspect there will be significantly more soundness bugs to iron out on the core traits? i'd lightly push towards landing this on nightly soonish and adjusting stuff in followup PRs.

in the same spirit, i ditched AllocatorEq since it's more in the realm of "potential future extensions" & isn't necessary right now.

[maxdexh]

I think the feature is in a great state now, thanks for the awesome work :D

I'll be giving the safety requirements another read and will try to break it again when I get time at the end of next week, but I'm pretty happy with how the API looks right now (and glad that it wasn't stabilized in the old state 😅)

Also if this hits nightly it'll be much easier to work with for me as well, so I'm strongly in favor of that.

[maxdexh]

Did you plan to add the A: Send + Sync bounds for Arc in this PR or separately?

[theemathas]

Other than the opsem stuff (which I haven't been following at all and therefore don't know the state of), and btree stuff (which is a later problem), I don't think there are other soundness issues. I'll probably have a few more looks at it before stabilization though.

[nia-e]

Did you plan to add the A: Send + Sync bounds for Arc in this PR or separately?

i'd leave that for the PR that actually moves the allocator out of the arc, which would be a blocker for stabilising anything like Arc::new_in

[maxdexh]

i'd leave that for the PR that actually moves the allocator out of the arc, which would be a blocker for stabilising anything like Arc::new_in

Such a PR will probably be blocked by all the other changes being made to Arc and Rc right now (#132553, which is itself blocked by #141348).

But it's fine if the allocator generic on Arc/Rc stays unstable (I don't like the idea of stabilizing it, but keeping new_in unstable, since people might already start generalizing their APIs)

[nia-e]

the plan is for everything allocator-y about Rc and Arc to stay unstable for now, no worries ^^

[maxdexh]

Institutional knowledge yay

[nia-e]

see the stabilisation doc on the main stabilisation PR (deets somewhat out of date now, but the plan for what to stabilise hasn't been changed) - i've been trying to avoid making too much of this institutional knowledge :D

[maxdexh]

For the record, LinkedList is also sensitive to alloc.clone() unwinding. Not filing an issue because I assume this is getting merged soon (unless you want me to file an issue 👀)

[nia-e]

please do file issues for everything distinct! we need to keep track of what's outstanding as we go and stabilise things