Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
May 20, 2024

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented May 13, 2024

Split out from #119761

Add fn into_raw_with_allocator for Rc/rc::Weak1/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 Weaks.
Future PR/ACP

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)

Footnotes

  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.

@rustbot
Copy link
Collaborator

rustbot commented May 13, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 13, 2024
@zachs18

This comment was marked as resolved.

@rustbot rustbot added the A-allocators Area: Custom and system allocators label May 13, 2024
@zachs18 zachs18 marked this pull request as draft May 13, 2024 22:59
@rust-log-analyzer

This comment has been minimized.

@zachs18

This comment was marked as outdated.

@zachs18

This comment was marked as outdated.

@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 May 14, 2024
... since fn allocator doesn't exist yet.
@zachs18
Copy link
Contributor Author

zachs18 commented May 17, 2024

Replaced uses of fn allocator (which doesn't exist yet) with directly accessing the alloc field. No longer blocked on #124980.

@rustbot ready

@rustbot 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 May 17, 2024
@zachs18 zachs18 marked this pull request as ready for review May 17, 2024 03:12
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 19, 2024

📌 Commit c895f6e has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#125034 (Weekly `cargo update`)
 - rust-lang#125093 (Add `fn into_raw_with_allocator` to Rc/Arc/Weak.)
 - rust-lang#125282 (Never type unsafe lint improvements)
 - rust-lang#125301 (fix suggestion in E0373 for !Unpin coroutines)
 - rust-lang#125302 (defrost `RUST_MIN_STACK=ice rustc hello.rs`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7389416 into rust-lang:master May 20, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2024
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`.
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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants