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

Confusing wording of Vec::from_raw_parts pointer allocation requirement #98780

Closed
Hawk777 opened this issue Jul 1, 2022 · 4 comments · Fixed by #99216
Closed

Confusing wording of Vec::from_raw_parts pointer allocation requirement #98780

Hawk777 opened this issue Jul 1, 2022 · 4 comments · Fixed by #99216
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug.

Comments

@Hawk777
Copy link

Hawk777 commented Jul 1, 2022

The Alternatives section of the documentation for core::mem::transmute has this to say about a better way to turn a Vec<&X> into a Vec<Option<&X>>:

// This is the proper no-copy, unsafe way of "transmuting" a `Vec`, without relying on the
// data layout. Instead of literally calling `transmute`, we perform a pointer cast, but
// in terms of converting the original inner type (`&i32`) to the new one (`Option<&i32>`),
// this has all the same caveats. Besides the information provided above, also consult the
// [`from_raw_parts`] documentation.
let v_from_raw = unsafe {
    // Ensure the original vector is not dropped.
    let mut v_clone = std::mem::ManuallyDrop::new(v_clone);
    Vec::from_raw_parts(v_clone.as_mut_ptr() as *mut Option<&i32>,
                        v_clone.len(),
                        v_clone.capacity())
};

The documentation for Vec<T>::from_raw_parts has this to say about where you can get the pointer from:

ptr needs to have been previously allocated via String/Vec<T> (at least, it’s highly likely to be incorrect if it wasn’t).

A strict reading, if I understand correctly, says that these two statements are contradictory. The transmute documentation says that it is safe to create a Vec<&i32> and then use from_raw_parts to create a Vec<Option<&i32>> out of its pointer. But the Vec documentation says that, when creating a Vec<T>, ptr must have previously been allocated via precisely and only Vec<T>, not Vec<U> for some other type U—not even if U happens to be transmutable from T, not even if T and U are ABI-compatible, not only if T is a repr(transparent) wrapper around U, but only T itself.

I suspect it was meant to say it must have previously been allocated by Vec, not Vec<T>; after all, if it could only have been allocated by Vec<T> in the first place, the next bullet point would be redundant (because obviously T has the same alignment as T).

@Hawk777 Hawk777 added the C-bug Category: This is a bug. label Jul 1, 2022
@Rageking8
Copy link
Contributor

@rustbot label A-docs

@rustbot rustbot added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Jul 3, 2022
@5225225
Copy link
Contributor

5225225 commented Jul 14, 2022

Relevant: #99216 looks like it will fix this if approved, by explicitly allowing allocation using something other than Vec (which can be a Vec or even manual allocation)

@Hawk777
Copy link
Author

Hawk777 commented Jul 14, 2022

Hm, that PR does remove the text I pointed out, but it also adds this text:

The first length values must be properly initialized values of type T.

Isn’t that equally problematic (albeit a slightly different but related problem, since it’s talking about the contents of the memory rather than its ), since it is actually fine for them to be properly initialized values of some other type U that is suitably bytewise-compatible (for a proper choice of wording for “suitably compatible”) with T but is not actually T itself? Or does “properly initialized values of type T” actually also cover that (i.e. does it mean “bytes containing valid representations of T” and not “memory regions which have been constructed as Ts”?).

@5225225
Copy link
Contributor

5225225 commented Jul 14, 2022

Rust doesn't have typed memory, types only have effect on accesses, so if some memory can be interpreted as some type T and it's valid at that type, it's a properly initialized value of type T. Nothing remembers the "original type" of some data.

Just like how you can cast a *const u32 to a *const i32 and use them interchangeably. This is a safe operation if you use bytemuck, too.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 3, 2022
docs: be less harsh in wording for Vec::from_raw_parts

In particular, be clear that it is sound to specify memory not
originating from a previous `Vec` allocation. That is already suggested
in other parts of the documentation about zero-alloc conversions to Box<[T]>.

Incorporate a constraint from `slice::from_raw_parts` that was missing
but needs to be fulfilled, since a `Vec` can be converted into a slice.

Fixes rust-lang#98780.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 3, 2022
docs: be less harsh in wording for Vec::from_raw_parts

In particular, be clear that it is sound to specify memory not
originating from a previous `Vec` allocation. That is already suggested
in other parts of the documentation about zero-alloc conversions to Box<[T]>.

Incorporate a constraint from `slice::from_raw_parts` that was missing
but needs to be fulfilled, since a `Vec` can be converted into a slice.

Fixes rust-lang#98780.
@bors bors closed this as completed in 2110d2d Oct 3, 2022
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
docs: be less harsh in wording for Vec::from_raw_parts

In particular, be clear that it is sound to specify memory not
originating from a previous `Vec` allocation. That is already suggested
in other parts of the documentation about zero-alloc conversions to Box<[T]>.

Incorporate a constraint from `slice::from_raw_parts` that was missing
but needs to be fulfilled, since a `Vec` can be converted into a slice.

Fixes rust-lang/rust#98780.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants