Make `(A)Rc`/`Weak` `allocator_api` APIs more consistent #119761

zachs18 · 2024-01-09T04:44:01Z

Make Arc/sync::Weak/Rc/rc::Weak unstable feature(allocator_api) APIs more consistent with other containers (well, Box), and with each other.

fn into_raw_with_allocator for all.
- Pairs with from_raw_in (which already exists on all 4 types).
- Name matches Box::into_raw_with_allocator.
- Associated fns on Rc/Arc, methods on Weaks.
fn allocator for all.
- For Rc/Arc, the existing associated fns are changed to allow unsized pointees.
- For Weaks, methods are added.
Remove where A: Clone bound on (stable) Rc::downcast and downcast_unchecked, (unstable) assume_inits, and some (stable) trait impls
- (Arc::{downcast{,_unchecked}, assume_init} were changed by Fix some Arc allocator leaks #120445)
- downcast(_unchecked) and assume_init semantically should not require cloning the allocator; they are essentially just ((un)checked) pointer casts.
- Remove A: Clone bound from From<Vec<T, A>> for Arc<[T], A> (Rc's already didn't have a A: Clone bound) ~~and TryFrom<Arc<[T], A>> for Arc<[T; N], A>~~ (done by Fix some Arc allocator leaks #120445) (see below for Rc)
Make TryFrom<Rc<[T]>> for Rc<[T; N]> allocator-aware.

This is adding 6¹ new unstable functions, changing the bounds on 6 3 unstable functions, and changing the (unstable) bounds on 7 5 stable functions, so I think it needs at least a libs-api FCP? If it also needs an ACP I can make one. (If the parts of this PR should be split I can do that too.)

@rustbot label +A-allocators +T-libs-api -T-libs

Future PR

As a follow-on to this PR, I plan to make a PR/ACP later to move into_raw(_parts) from Container<_, A: Allocator> to only Container<_, Global> (where Container = Vec/Box/Rc/rc::Weak/Arc/sync::Weak) so that users of non-Global allocators have to explicitly handle the allocator when using into_raw-like APIs.

The current behaviors of stdlib containers are inconsistent with respect to what happens to the allocator when into_raw is called (which does not return the allocator)

Type	`into_raw` currently callable with	behavior of `into_raw`
`Box`	any allocator	allocator is dropped
`Vec`	any allocator	allocator is forgotten
`Arc`/`Rc`/`Weak`	any allocator	allocator is forgotten(Arc) (sync::Weak) (Rc) (rc::Weak)

In my opinion, neither implicitly dropping nor implicitly forgetting the allocator is ideal; dropping it could immediately invalidate the returned pointer, and forgetting it could unintentionally leak memory. My (to-be) proposed solution is to just forbid calling into_raw(_parts) on containers with non-Global allocators, and require calling into_raw_with_allocator(/Vec::into_raw_parts_with_alloc)

Technically 5 new and 1 renamed, since rc::Weak::into_raw_with_allocator is not newly added, as it was modified and renamed from rc::Weak::into_raw_and_alloc. ↩

rustbot · 2024-01-09T04:44:09Z

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

zachs18 · 2024-01-09T21:49:14Z

@rustbot label -T-libs

r? rust-lang/libs-api

bors · 2024-01-30T19:28:20Z

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

zachs18 · 2024-02-19T06:50:02Z

@rustbot ready

bors · 2024-04-27T16:07:46Z

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

* Add `into_raw_with_allocator` on `Rc`/`Weak` * Remove `where A: Clone` from `Rc::assume_init`s, `Rc::unwrap_or_clone`, and `Rc::downcast(_unchecked)` * Add `fn allocator(&self)` to `Rc`/`Weak` for all `T: ?Sized` * Make `TryFrom<Rc<[T]>> for Rc<[T; N]>` allocator-aware

…ame collisions

…_clone and From<Vec<T, A>> for Arc<[T], A>

zachs18 · 2024-05-01T04:36:41Z

@BurntSushi Friendly ping (I just realized that the r? libs-api probably didn't send a notification)

BurntSushi · 2024-05-01T11:59:27Z

I'm not really dialed into the allocator work. Can you find someone who is to review this? Maybe @matthieu-m?

BurntSushi · 2024-05-01T12:00:07Z

Alternatively, if this PR can be split up into more digestible chunks, that might help get it reviewed more quickly.

matthieu-m · 2024-05-01T16:18:50Z

@BurntSushi From what I can tell, this PR does not require any Allocator specific knowledge, it doesn't actually even introduce any call Allocator methods (it eliminates calls to clone).

I think any libs member comfortable with API requests can tackle this review without issue.

(I'm neither a libs member, nor am I well-versed in API guidelines)

zachs18 · 2024-05-02T17:34:36Z

I'll probably go ahead and split this into multiple PRs then. I probably won't have time to work on that for a week or so, so I'll leave this open if someone wants to reveiw it until then.

zachs18 · 2024-05-10T19:28:23Z

Split into #124980 and #124981. Later PR will add fn into_raw_with_allocator.

… r=Mark-Simulacrum Relax allocator requirements on some Rc/Arc APIs. Split out from rust-lang#119761 * Remove `A: Clone` bound from `Rc::assume_init`(s), `Rc::downcast`, and `Rc::downcast_unchecked` (`Arc` methods were already relaxed by rust-lang#120445) * Make `From<Rc<[T; N]>> for Rc<[T]>` allocator-aware (`Arc`'s already is). * Remove `A: Clone` from `Rc/Arc::unwrap_or_clone` Internal changes: * Made `Arc::internal_into_inner_with_allocator` method into `Arc::into_inner_with_allocator` associated fn. * Add private `Rc::into_inner_with_allocator` (to match Arc), so other fns don't have to juggle `ManuallyDrop`.

Rollup merge of rust-lang#124981 - zachs18:rc-allocator-generalize-1, r=Mark-Simulacrum Relax allocator requirements on some Rc/Arc APIs. Split out from rust-lang#119761 * Remove `A: Clone` bound from `Rc::assume_init`(s), `Rc::downcast`, and `Rc::downcast_unchecked` (`Arc` methods were already relaxed by rust-lang#120445) * Make `From<Rc<[T; N]>> for Rc<[T]>` allocator-aware (`Arc`'s already is). * Remove `A: Clone` from `Rc/Arc::unwrap_or_clone` Internal changes: * Made `Arc::internal_into_inner_with_allocator` method into `Arc::into_inner_with_allocator` associated fn. * Add private `Rc::into_inner_with_allocator` (to match Arc), so other fns don't have to juggle `ManuallyDrop`.

…-only, r=Mark-Simulacrum Add `fn into_raw_with_allocator` to Rc/Arc/Weak. Split out from rust-lang#119761 Add `fn into_raw_with_allocator` for `Rc`/`rc::Weak`[^1]/`Arc`/`sync::Weak`. * Pairs with `from_raw_in` (which already exists on all 4 types). * Name matches `Box::into_raw_with_allocator`. * Associated fns on `Rc`/`Arc`, methods on `Weak`s. <details> <summary>Future PR/ACP</summary> As a follow-on to this PR, I plan to make a PR/ACP later to move `into_raw(_parts)` from `Container<_, A: Allocator>` to only `Container<_, Global>` (where `Container` = `Vec`/`Box`/`Rc`/`rc::Weak`/`Arc`/`sync::Weak`) so that users of non-`Global` allocators have to explicitly handle the allocator when using `into_raw`-like APIs. The current behaviors of stdlib containers are inconsistent with respect to what happens to the allocator when `into_raw` is called (which does not return the allocator) | Type | `into_raw` currently callable with | behavior of `into_raw`| | --- | --- | --- | | `Box` | any allocator | allocator is [dropped](https://doc.rust-lang.org/nightly/src/alloc/boxed.rs.html#1060) | | `Vec` | any allocator | allocator is [forgotten](https://doc.rust-lang.org/nightly/src/alloc/vec/mod.rs.html#884) | | `Arc`/`Rc`/`Weak` | any allocator | allocator is [forgotten](https://doc.rust-lang.org/src/alloc/sync.rs.html#1487)(Arc) [(sync::Weak)](https://doc.rust-lang.org/src/alloc/sync.rs.html#2726) [(Rc)](https://doc.rust-lang.org/src/alloc/rc.rs.html#1352) [(rc::Weak)](https://doc.rust-lang.org/src/alloc/rc.rs.html#2993) | In my opinion, neither implicitly dropping nor implicitly forgetting the allocator is ideal; dropping it could immediately invalidate the returned pointer, and forgetting it could unintentionally leak memory. My (to-be) proposed solution is to just forbid calling `into_raw(_parts)` on containers with non-`Global` allocators, and require calling `into_raw_with_allocator`(/`Vec::into_raw_parts_with_alloc`) </details> [^1]: Technically, `rc::Weak::into_raw_with_allocator` is not newly added, as it was modified and renamed from `rc::Weak::into_raw_and_alloc`.

Rollup merge of rust-lang#125093 - zachs18:rc-into-raw-with-allocator-only, r=Mark-Simulacrum Add `fn into_raw_with_allocator` to Rc/Arc/Weak. Split out from rust-lang#119761 Add `fn into_raw_with_allocator` for `Rc`/`rc::Weak`[^1]/`Arc`/`sync::Weak`. * Pairs with `from_raw_in` (which already exists on all 4 types). * Name matches `Box::into_raw_with_allocator`. * Associated fns on `Rc`/`Arc`, methods on `Weak`s. <details> <summary>Future PR/ACP</summary> As a follow-on to this PR, I plan to make a PR/ACP later to move `into_raw(_parts)` from `Container<_, A: Allocator>` to only `Container<_, Global>` (where `Container` = `Vec`/`Box`/`Rc`/`rc::Weak`/`Arc`/`sync::Weak`) so that users of non-`Global` allocators have to explicitly handle the allocator when using `into_raw`-like APIs. The current behaviors of stdlib containers are inconsistent with respect to what happens to the allocator when `into_raw` is called (which does not return the allocator) | Type | `into_raw` currently callable with | behavior of `into_raw`| | --- | --- | --- | | `Box` | any allocator | allocator is [dropped](https://doc.rust-lang.org/nightly/src/alloc/boxed.rs.html#1060) | | `Vec` | any allocator | allocator is [forgotten](https://doc.rust-lang.org/nightly/src/alloc/vec/mod.rs.html#884) | | `Arc`/`Rc`/`Weak` | any allocator | allocator is [forgotten](https://doc.rust-lang.org/src/alloc/sync.rs.html#1487)(Arc) [(sync::Weak)](https://doc.rust-lang.org/src/alloc/sync.rs.html#2726) [(Rc)](https://doc.rust-lang.org/src/alloc/rc.rs.html#1352) [(rc::Weak)](https://doc.rust-lang.org/src/alloc/rc.rs.html#2993) | In my opinion, neither implicitly dropping nor implicitly forgetting the allocator is ideal; dropping it could immediately invalidate the returned pointer, and forgetting it could unintentionally leak memory. My (to-be) proposed solution is to just forbid calling `into_raw(_parts)` on containers with non-`Global` allocators, and require calling `into_raw_with_allocator`(/`Vec::into_raw_parts_with_alloc`) </details> [^1]: Technically, `rc::Weak::into_raw_with_allocator` is not newly added, as it was modified and renamed from `rc::Weak::into_raw_and_alloc`.

Generalize `fn allocator` for Rc/Arc. Split out from rust-lang#119761 - For `Rc`/`Arc`, the existing associated `fn`s are changed to allow unsized pointees. - For `Weak`s, new methods are added. `@rustbot` label +A-allocators

Generalize `fn allocator` for Rc/Arc. Split out from rust-lang#119761 - For `Rc`/`Arc`, the existing associated `fn`s are changed to allow unsized pointees. - For `Weak`s, new methods are added. ``@rustbot`` label +A-allocators

Generalize `fn allocator` for Rc/Arc. Split out from rust-lang#119761 - For `Rc`/`Arc`, the existing associated `fn`s are changed to allow unsized pointees. - For `Weak`s, new methods are added. ```@rustbot``` label +A-allocators

Generalize `fn allocator` for Rc/Arc. Split out from rust-lang#119761 - For `Rc`/`Arc`, the existing associated `fn`s are changed to allow unsized pointees. - For `Weak`s, new methods are added. ````@rustbot```` label +A-allocators

Generalize `fn allocator` for Rc/Arc. Split out from rust-lang#119761 - For `Rc`/`Arc`, the existing associated `fn`s are changed to allow unsized pointees. - For `Weak`s, new methods are added. `````@rustbot````` label +A-allocators

Rollup merge of rust-lang#124980 - zachs18:rc-allocator, r=Amanieu Generalize `fn allocator` for Rc/Arc. Split out from rust-lang#119761 - For `Rc`/`Arc`, the existing associated `fn`s are changed to allow unsized pointees. - For `Weak`s, new methods are added. `````@rustbot````` label +A-allocators

rustbot assigned thomcc Jan 9, 2024

This comment has been minimized.

Sign in to view

rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jan 9, 2024

rustbot assigned BurntSushi and unassigned thomcc Jan 9, 2024

zachs18 mentioned this pull request Jan 10, 2024

Fix conversions of Box<T, A> to Rc<T, A> or Arc<T, A> to use allocator correctly for dropping the Box #119812

Closed

This comment was marked as resolved.

Sign in to view

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 Feb 10, 2024

zachs18 force-pushed the rc-into-raw-with-allocator branch from 2134c8e to c412131 Compare February 10, 2024 05:30

zachs18 force-pushed the rc-into-raw-with-allocator branch from 888f0cf to 5a23ea9 Compare February 19, 2024 05:42

This comment has been minimized.

Sign in to view

zachs18 force-pushed the rc-into-raw-with-allocator branch from 5a23ea9 to c3136d9 Compare February 19, 2024 05:53

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2024

zachs18 force-pushed the rc-into-raw-with-allocator branch from c3136d9 to d86ce86 Compare April 12, 2024 17:41

zachs18 added 5 commits April 29, 2024 18:08

make internal_into_inner_with_allocator an associated fn to prevent n…

f9d7307

…ame collisions

Move Arc::allocator to T: ?Sized

a2002ec

Add {Arc,Weak}::into_raw_with_allocator

052a140

Add fn sync::Weak::allocator, remove A: Clone bound on Arc::unwrap_or…

f23292a

…_clone and From<Vec<T, A>> for Arc<[T], A>

add (private) Rc::into_inner_with_allocator, and use it where Arc does

9f3dc91

zachs18 force-pushed the rc-into-raw-with-allocator branch from d86ce86 to 9f3dc91 Compare April 29, 2024 23:10

This was referenced May 10, 2024

Generalize fn allocator for Rc/Arc. #124980

Merged

Relax allocator requirements on some Rc/Arc APIs. #124981

Merged

zachs18 closed this May 10, 2024

zachs18 mentioned this pull request May 13, 2024

Add fn into_raw_with_allocator to Rc/Arc/Weak. #125093

Merged

Make (A)Rc/Weak allocator_api APIs more consistent #119761

Make (A)Rc/Weak allocator_api APIs more consistent #119761

Uh oh!

Conversation

zachs18 commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

rustbot commented Jan 9, 2024

Uh oh!

This comment has been minimized.

zachs18 commented Jan 9, 2024

Uh oh!

bors commented Jan 30, 2024

Uh oh!

This comment was marked as resolved.

This comment has been minimized.

zachs18 commented Feb 19, 2024

Uh oh!

bors commented Apr 27, 2024

Uh oh!

zachs18 commented May 1, 2024

Uh oh!

BurntSushi commented May 1, 2024

Uh oh!

BurntSushi commented May 1, 2024

Uh oh!

matthieu-m commented May 1, 2024

Uh oh!

zachs18 commented May 2, 2024

Uh oh!

zachs18 commented May 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Make `(A)Rc`/`Weak` `allocator_api` APIs more consistent #119761

Make `(A)Rc`/`Weak` `allocator_api` APIs more consistent #119761

zachs18 commented Jan 9, 2024 •

edited

Loading