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 documentation on mem::transmute #64073

Closed
Shnatsel opened this issue Sep 1, 2019 · 3 comments · Fixed by #69582
Closed

Confusing documentation on mem::transmute #64073

Shnatsel opened this issue Sep 1, 2019 · 3 comments · Fixed by #69582
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools

Comments

@Shnatsel
Copy link
Member

Shnatsel commented Sep 1, 2019

Documentation on mem::transmute presents the following piece of code:

let store = [0, 1, 2, 3];
let mut v_orig = store.iter().collect::<Vec<&i32>>();

// Using transmute: this is Undefined Behavior, and a bad idea.
// However, it is no-copy.
let v_transmuted = unsafe {
    std::mem::transmute::<Vec<&i32>, Vec<Option<&i32>>>(
        v_orig.clone())
};

// This is the suggested, safe way.
// It does copy the entire vector, though, into a new array.
let v_collected = v_orig.clone()
                        .into_iter()
                        .map(|r| Some(r))
                        .collect::<Vec<Option<&i32>>>();

// The no-copy, unsafe way, still using transmute, but not UB.
// This is equivalent to the original, but safer, and reuses the
// same `Vec` internals. Therefore, the new inner type must have the
// exact same size, and the same alignment, as the old type.
// The same caveats exist for this method as transmute, for
// the original inner type (`&i32`) to the converted inner type
// (`Option<&i32>`), so read the nomicon pages linked above.
let v_from_raw = unsafe {
    Vec::from_raw_parts(v_orig.as_mut_ptr() as *mut Option<&i32>,
                        v_orig.len(),
                        v_orig.capacity())
};
std::mem::forget(v_orig);

The problem is it's never explained why the first described solution is Undefined Behavior. This is confusing.

Second, the use of transmute from &i32 to Option<&i32> which is inherently unsafe is not very illustrative. This is actually guaranteed to be safe, but that's not explicitly called out in the snippet. Perhaps transmuting NonZeroU8 to u8 or possibly char to u32 would be a better example.

Finally, the proposed "correct" way of doing this seems to actually contain undefined behavior: std::mem::forget(v_orig); is called only after the new vector is created, and until it's called there are two mutable references to the backing storage of v_orig.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Sep 2, 2019

Second, the use of transmute from &i32 to Option<&i32> which is inherently unsafe is not very illustrative. Perhaps transmuting NonZeroU8 to u8 or possibly char to u32 would be a better example.

I don't see the difference. We do guarantee that &T -> Option<&T> works, and that this type punning will specifically map r: &T to Some(r). (And the other direction also works if you restrict yourself to Some values.)

@Shnatsel
Copy link
Member Author

Shnatsel commented Sep 2, 2019

I was not aware this is guaranteed to work, and it's not explicitly called out in the snippet. So it's not UB, but the clarity can probably be improved.

@GuillaumeGomez GuillaumeGomez added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Sep 6, 2019
@RalfJung
Copy link
Member

I think this is less "UB" and more "unspecified behavior": Vec is repr(Rust) so you cannot transmute it because you cannot rely on how it is laid out.

So it's not UB, but the clarity can probably be improved.

Exactly. PRs welcome. :D

The docs should probably also mention what came up in rust-lang/rust-clippy#4484: if you do this at home, make sure the size and alignment of both types is the same, because otherwise the allocator will be unhappy with you.

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants