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

Implement From<Box<T>> for NonNull<T>. #80611

Closed
wants to merge 3 commits into from

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Jan 2, 2021

This provides an implementation of From<Box<T>> for NonNull<T>.

It's a pretty small conceptual addition, but is good because but avoids a case where the operation is obviously sound, but you'd otherwise need unsafe to do it directly (at least, without an unwrap).

It also can be a bit subtle to implement this correctly (for example, my understanding is that using NonNull::from on a reference derived from the Box would have the wrong provenance), so it's good to provide a guaranteed-correct implementation.

The biggest downside is that this operation leaks (err, it transfers responsibility for cleanup to the caller), but that seems like the point of this operation. This has been noted in the documentation, however, and is in a must_use message.

I didn't do this for the other smart pointer types (Rc<T> and Arc<T>) since it seemed less useful, and is semantically a bit hairier, I think. I'm happy to change that though, if desirable.

I didn't make this generic over the Allocator parameter, mostly because the other From impls largely aren't (only a couple are). It's also not clear to me how that interacts with stability, although it's probably fine. Anyway, happy to change that as well. Actually I was wrong, most of the similar impls are generic over the allocator, so this is too.

This is a trait implementation, so IIUC it's automatically stable (and thus needs to go through an FCP, I believe), and thus is marked as stable. I'm happy to change it to unstable if this wasn't correct information.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2021
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/library/alloc/src/boxed.rs at line 148:
     CoerceUnsized, Deref, DerefMut, DispatchFromDyn, Generator, GeneratorState, Receiver,
 use core::pin::Pin;
 use core::pin::Pin;
-use core::ptr::{self, Unique, NonNull};
+use core::ptr::{self, NonNull, Unique};
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/library/alloc/src/boxed.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
 use core::task::{Context, Poll};
 
 use crate::alloc::{handle_alloc_error, AllocError, Allocator, Global, Layout};
Build completed unsuccessfully in 0:00:18

@m-ou-se
Copy link
Member

m-ou-se commented Jan 2, 2021

Thanks for your PR!

RFC 2969 proposes explicit functions to leak containers like Box into a NonNull. With that in mind, we probably shouldn't have From implementations that leak but instead have explicit functions for these, just like the existing leak functions.

What do think? Since you've already explored this problem by making this PR, it'd be useful to hear your thoughts about that proposal. Feel free to leave some comments on that RFC with your thoughts, and whether it'd satisfy your needs. Thanks!

@thomcc
Copy link
Member Author

thomcc commented Jan 2, 2021

Fair enough, thanks for the pointer! I'll try and find some time to read over it and comment if anything comes to mind.

@thomcc thomcc closed this Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants