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.

Sign up for GitHub

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

Jump to bottom

Make std::mem::transmute_copy accept ?Sized inputs #112457

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make std::mem::transmute_copy accept ?Sized inputs #112457

wants to merge 3 commits into from
+9 −4

Conversation

nvzqz
Copy link
Contributor

@nvzqz nvzqz commented Jun 9, 2023 •
edited
Loading

This enables conversions from dynamically-sized types like [u8].

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2023

r? @scottmcm

(rustbot has picked a reviewer for you, use r? to override)

@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 Jun 9, 2023
@nvzqz
Copy link
Contributor Author

nvzqz commented Jun 9, 2023

@rustbot label +T-libs-api -T-libs

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 9, 2023
@nvzqz nvzqz force-pushed the feat/unsized-transmute_copy branch from 622eacf to f5bc1e6 Compare June 9, 2023 11:52
nvzqz added 2 commits June 9, 2023 07:53
@nvzqz
Make std::mem::transmute_copy accept ?Sized inputs
f832add 
This enables conversions from dynamically-sized types like `[u8]`.
@nvzqz
Add std::mem::transmute_copy slice test
c9575a6
@nvzqz nvzqz force-pushed the feat/unsized-transmute_copy branch from f5bc1e6 to c9575a6 Compare June 9, 2023 11:54
@lcnr lcnr added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jun 9, 2023
workingjubilee
workingjubilee reviewed Jun 9, 2023
View reviewed changes
library/core/src/mem/mod.rs Outdated
Comment on lines 1056 to 1059
assert!(
size_of::<Src>() >= size_of::<Dst>(),
size_of_val(src) >= size_of::<Dst>(),
"cannot transmute_copy if Dst is larger than Src"
);
Copy link
Member

@workingjubilee workingjubilee Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer guaranteed to be compiled out in --release. I do not believe we want this assert in this case.

Copy link
Contributor Author

@nvzqz nvzqz Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to debug_assert! in 3f1825a

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not guaranteed to be compiled out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbu- because size_of_val for dynamically-sized types requires a runtime check since the type's size is not known at compile-time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to debug_assert! means this check no longer triggers for 99% of users who do not work with a debug build of the stdlib.

@nvzqz
Change std::mem::transmute_copy to use debug_assert!
3f1825a 
Code that uses this function is likely performance-sensitive, so we do
not want to emit an assert for dynamically-sized types in release mode.
@the8472
Copy link
Member

the8472 commented Jun 9, 2023 •
edited
Loading

Transmute only requires the bits to be valid for Dst Src, not necesarily that Dst Src is in a safe state.

Do we have any requirements on custom DSTs that their size must be computable at all times? One might get funny effects if such a type is trying to compute its size in the middle of unsafe code mucking with its bytes.

@nvzqz
Copy link
Contributor Author

nvzqz commented Jun 9, 2023

@the8472 size_of::<Dst>() emits a compile-time constant, so I don't see how one would get funny effects at runtime

@the8472
Copy link
Member

the8472 commented Jun 9, 2023

Hrrm, right, only the source is unsized. Still, the assert would try to compute the dynamic size of the source in debug builds of the standard library.

@scottmcm
Copy link
Member

Flipping this over to a libs-api reviewer since this needs an FCP due to being new stable capabilty
r? @m-ou-se

@rustbot rustbot assigned m-ou-se and unassigned scottmcm Jun 15, 2023
@workingjubilee
Copy link
Member

Tangentially: ptr::read will be const fn stable in 1.71.

@scottmcm
Copy link
Member

Out of curiosity, do you have any uses in mind for anything but [u8]? For example, using this with trait objects feels much weirder than with byte slices.

But coming from [u8], bytes.as_ptr().cast::<Target>().read_unaligned() would also work fine -- from a byte slice using the align-1 version is always fine -- and that makes me a bit unconvinced that changing transmute_copy is the right choice.

@bors
Copy link
Contributor

bors commented Sep 16, 2023

☔ The latest upstream changes (presumably #115520) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

@nvzqz would be nice to have a reply to #112457 (comment) and can resolve the conflicts if interested in moving this forward? thanks

@Dylan-DPC Dylan-DPC 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 Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee left review comments

@tbu- tbu- tbu- left review comments

@thomcc thomcc thomcc left review comments

Assignees

@m-ou-se m-ou-se

Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.