Skip to content

allocator: refactor for stabilisation - #157428

Open
nia-e wants to merge 7 commits into
rust-lang:mainfrom
nia-e:allocator-refactor
Open

allocator: refactor for stabilisation#157428
nia-e wants to merge 7 commits into
rust-lang:mainfrom
nia-e:allocator-refactor

Conversation

@nia-e

@nia-e nia-e commented Jun 4, 2026

Copy link
Copy Markdown
Member

View all comments

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

r? libs

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 4, 2026
@nia-e nia-e added the A-allocators Area: Custom and system allocators label Jun 4, 2026
Comment thread library/core/src/alloc/mod.rs Outdated
@rust-log-analyzer

This comment has been minimized.

qaijuang

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@nia-e

nia-e commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

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.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Comment thread library/alloc/src/str.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs Outdated
@clarfonthey

Copy link
Copy Markdown
Contributor

@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)

@rustbot rustbot 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 Jun 4, 2026
Comment thread library/core/src/alloc/mod.rs
@rust-log-analyzer

This comment has been minimized.

Comment thread library/core/src/alloc/mod.rs Outdated
Comment thread library/core/src/alloc/mod.rs
@rust-bors

This comment has been minimized.

yk what we can do btree later

raaaaagh

pain and suffering

*unleaks your box* whats thiSegmentation fault (core dumped)

straight up testing it. and by it,

well, let's just say. "my allocator"

linked and listed

the unnecesary bits? gone. reduced to atoms
@nia-e
nia-e force-pushed the allocator-refactor branch from a41958f to acb148f Compare August 4, 2026 18:10
@rustbot

rustbot commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

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

nia-e commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

(apologies for the squash-and-rebase, nothing meaningful to review should have changed between force-pushes)

@rust-log-analyzer

This comment has been minimized.

Comment on lines -2170 to +2179
if self.len() > source.len() {
self.split_off(source.len());
}
for (elem, source_elem) in self.iter_mut().zip(&mut source_iter) {
for elem in self.iter_mut() {
let Some(source_elem) = source_iter.next() else {
break;
};
elem.clone_from(source_elem);
}
while self.len() > source.len() {
self.pop_back();
}

@clarfonthey clarfonthey Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreasonably upset at this code just because of how limited the LinkedList API is. Should be doable, but isn't… :(

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

samesies qwq

Comment on lines 1899 to +1908
/// This function is mainly useful for data that lives for the remainder of the program's life,
/// i.e., memory that is meant to leak. Reconstructing ("unleaking") a `Box` from the mutable
/// reference returned here (e.g. via [`Box::from_raw`]) is a grey area (meaning it is possible
/// under specific circumstances but many seemingly harmless ways of doing it are undefined
/// behavior) and should be avoided. If the memory should eventually be freed, prefer to use
/// [`Box::into_raw`] or [`Box::into_non_null`] instead.
///
/// However, "unleaking" as per the above via [`Box::from_raw_in`] is only sound
/// for the global allocator.
///

@clarfonthey clarfonthey Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two paragraphs combined read quite weird. I think you probably want to just separate the idea of "unleaking" into a second paragraph, starting with the no-unleak case and then clarifying unleaking only works for Global.

View changes since the review

@RalfJung RalfJung Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this is an attempt to partially spell out what "specific circumstances" make unleaking sound. If that's what we want to do we should actually spell it out IMO:

Unleaking is allowed if

  • A is Global, and
  • the pointer passed to from_raw was derived from the reference returned by this method in a way that all intermediate references on the derivation path are &mut T (no shared references, no references to other types).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's better wording, yes. i'll adapt that, ty <3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and it may be worth reminding the reader that if a function takes an &mut argument, freeing that while the function runs is UB. So it'd be okay to to Box::from_raw inside such a function but not to drop that box.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't find a nice way to express this concisely so i'll just mention it's only sound for Global. that should at least be no more vague than it was before ^^

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, but why is it a separate paragraph? IMO the new sentence should go just after the "should be avoided".

#[cfg(not(no_global_oom_handling))]
#[stable(feature = "box_slice_clone", since = "1.3.0")]
impl Clone for Box<str> {
impl<A: Allocator + Clone> Clone for Box<str, A> {

@clarfonthey clarfonthey Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would just comment here: not AllocatorClone since the new allocation doesn't have to be related to the original.

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to add that, sure thing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly mention because, my initial gut reaction was "wait, why not AllocatorClone?" and even though I figured it out, I figure just adding the extra comment helps.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh oops i thought i alr pushed the change i'd made here. yes hi i did it now and it's pushed

Comment thread library/alloc/src/str.rs Outdated
#[unstable(feature = "allocator_api", issue = "32838")]
#[must_use]
#[inline]
pub unsafe fn from_boxed_utf8_unchecked_in<A: crate::alloc::Allocator>(

@clarfonthey clarfonthey Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like this method, because I don't think we should use the _in naming when the allocator is part of a box, rather than as an extra parameter.

I assume there are problems with parameterising from_boxed_utf8_unchecked normally?

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like this method, because I don't think we should use the _in naming when the allocator is part of a box, rather than as an extra parameter.

Fair, happy to rename it and/or make it private, we just need the functionality for this clone impl.

I assume there are problems with parameterising from_boxed_utf8_unchecked normally?

Would be a breaking change to a stable api :c

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess i should explicitly ask - what would be preferred here? i'm fine with any option as long as I can get the semantics of this function

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I would prefer just this being either private or a method on Box for now. I just don't want it public to imply this is the method we want; maybe with an edition redirect we could update the old method.

Comment thread library/core/src/alloc/mod.rs
Comment thread library/core/src/alloc/mod.rs
Comment thread library/core/src/alloc/mod.rs
@Darksonn

Darksonn commented Aug 6, 2026

Copy link
Copy Markdown
Member

This PR seems to make multiple unrelated changes. Is it possible to get an overview of all the changes it's making? Or even better, perhaps we could split out this into multiple PRs so that we can merge the things we agree on now, without blocking them on the things still being discussed?

@nia-e

nia-e commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

I'm unsure how i'd go about splitting this up, since many of the miscellaneous library changes are necessary for the API to be sound. (ofc, if you have an idea that'd lighten the review burden, i'm happy to do it and i think @clarfonthey would be relieved too)

In short, the changes made are:

  • split out requirements from Allocator into the 2 new traits StaticAllocator and AllocatorClone, as not all allocators we want to support can obey those semantics
    • StaticAllocator is required for pinning and acting as a global allocator, effectively being a promise that this allocator will never free memory without an explicit deallocate call (and not from e.g. dropping)
    • AllocatorClone promises semantics about what cloning an allocator does, namely that it requires the clone not to unwind and that the new allocator is interchangeable with the old one. this is
  • update std APIs to use these new trait bounds where necessary for soundness
  • implement said traits for std pointer types where doing so is sound, to facilitate using methods bound by them even if the new traits take a longer while to be proposed for stabilisation than the main one (i guess this could be split out?)

i'm quite happy with the fact that (beyond adding the ban on unwinding out of an allocating method or drop) none of this tightens requirements on Allocator implementors at all.

@maxdexh

maxdexh commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Also, unlike the old allocator api, we think this is sound (modulo some details in the docs we haven't discussed fully) :D

@rust-log-analyzer

This comment has been minimized.

Comment thread library/alloc/src/boxed.rs Outdated
Co-authored-by: Ralf Jung <post@ralfj.de>
@rust-log-analyzer

This comment has been minimized.

@nia-e nia-e mentioned this pull request Aug 8, 2026
17 tasks
@rust-bors

rust-bors Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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

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

Labels

A-allocators Area: Custom and system allocators S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet