Rust: Tracking issue for `vec_into_raw_parts`

Created on 25 Oct 2019  路  19Comments  路  Source: rust-lang/rust

https://github.com/rust-lang/rust/pull/65705 adds:

impl String {
    pub fn into_raw_parts(self) -> (*mut u8, usize, usize) {鈥
}
impl<T> Vec<T> {
    pub fn into_raw_parts(self) -> (*mut T, usize, usize) {鈥
}

Things to evaluate before stabilization

  • [ ] [Should they return NonNull<* mut T>](https://github.com/rust-lang/rust/issues/65816#issuecomment-546405936)?
  • [ ] [Should they be associated functions](https://github.com/rust-lang/rust/issues/65816#issuecomment-546448429)?
A-collections A-raw-pointers B-unstable C-tracking-issue Libs-Tracked T-libs

Most helpful comment

Should the returned pointer be a NonNull instead? Box::raw doesn鈥檛 do that because it was stabilized before NonNull was.

All 19 comments

Should the returned pointer be a NonNull instead? Box::raw doesn鈥檛 do that because it was stabilized before NonNull was.

Should these functions be associated functions like Box::into_raw? The Box case is required because we want to avoid colliding with methods exposed by Deref / DerefMut. This isn't a concern for String or Vec<T>, but it may be nice to be consistent across this family of similar functions.

Here's an example where this might have helped with avoiding UB: https://github.com/confio/go-rust-demo/pull/1#discussion_r335152634. There, the vector was destructed manually and mem::forgetted, however, len was used in place of both length and capacity. If Vec::from_raw_parts was a thing, it might have prevented this issue, as, with cap on your hands, you are more likely to do the right thing with it.

If we view this as being the opposite of Vec::from_raw_parts the answers become fairly obvious:

  • The pointer shoud be a plain raw pointer, because that's the type that from_raw_parts takes.
  • The method should be associated so that both visually it matches the style used for from_raw_parts (in a sense), and also because this is a "weird" method that editors don't need to be suggesting with auto-complete.

I think it鈥檚 not at all obvious that deliberately making this method "weird" is a good thing.

The method is weird either way. "This uses the vec by self, but _won't_ free the memory" is sufficiently out there.

I don鈥檛 see how that makes it weird. Vec::into_boxed_slice is the same. It鈥檚 pretty normal for conversions to take self by value.

But into_boxed_slice doesn't leak the memory

Well,the documentation does a great job telling you how to avoid a leak and also leaking it is not unsafe.

I don't disagree with any of that, but also my opinion is unchanged.

It's not a big deal to have this as associated function vs method, but shepmaster is right that the associated version function makes this meta-family of functions feel consistent across the alloc crate.

But again it's a totally unimportant difference either way, and we should just stabilize either form and get on with the day.

In my opinion functions that start with into_ should be methods to match the definition of other methods and the Into trait unless can collide with methods on the Deref::Target while the rest can be left as associated functions if it is conventional.

What's the status of this stabilization? (This looks like the only way to take the buffer of the Vec in an FFI-traversable form without causing an implicit shrink_to_fit potential extra allocation first.)

(resolved 鈥斅燖shepmaster)

@hsivonen it's alreay possible to get the pointer, length, and capacity using the relavant methods on Vec

let vec: Vec<T> = ...;
// so that dropping the vec doesn't deallocate 
let mut vec = ManuallyDrop::new(vec);
// get each part
let len = vec.len();
let cap = vec.cap();
let ptr = vec.as_mut_ptr();
// make the wrapped vec unusable
drop(vec);

You can use Vec::from_raw_parts to reconstruct the Vec. All this without shrink_to_fit ever entering the picture.

Vec::into_raw_parts is some very welcome sugar for the code block above.

(resolved 鈥斅燖shepmaster)

What's the status of this stabilization?

There is no update. If there was an update, it would have been posted as a comment you would have seen or the original comment would have been edited.

I'm going to mark these comments as resolved so that it's possible to read the comments to see what the updates are.

@rust-lang/libs Any thoughts on the unresolved questions in the issue description? I鈥檇 be inclined to say:

  • This method should return a NonNull pointer: the conversion to a raw pointer is easy and safe, the reverse in either unsafe or has an unnecessary branch
  • It should not be changed to an associated function: Deref::Target is not to an arbitrary type, so unlike Box there is no risk of shadowing another method.

@SimonSapin SGTM. The missing parallelism between definitions of into_raw_parts and from_raw_parts is a little unfortunate, but I think NonNull is the right choice for the reasons you stated.

I never stated my preference: I do think that it should be an associated function. My rationale is purely based on consistency. I will probably always use this function as Vec::into_raw_parts(some_vec) so that I don't have to think "oh, is this something that implements Deref so I should use it like this or that".

Arguing against myself, if the team goes with a method, the associated function syntax will still work, so I'll be able to do what I want.

In that case the methods won't be consistent across types, and to me will be a wart.

The missing parallelism [...] is a little unfortunate

To be clear, there's no technical reason that prevents the team from choosing to make this an associated function, AFAIK. There's just no technical reason that forces it to be an associated function.

To clarify, by "missing parallelism," I was referring to the fact that from_raw_parts takes a raw pointer and we probably want into_raw_parts to return NonNull.

NonNull seems like the right type to me too. It exists now and we should use it. But I do think it would be good to look at how we might be able to offer some consistency across these container types before stabilizing this one either way. Since we're stuck with stable APIs effectively forever I think we need to be mindful of keeping things consistent over slowly shifting idioms.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

lambda-fairy picture lambda-fairy  路  3Comments

mcarton picture mcarton  路  3Comments

SharplEr picture SharplEr  路  3Comments

drewcrawford picture drewcrawford  路  3Comments

zhendongsu picture zhendongsu  路  3Comments