Add #[repr(transparent)] for several types #61969

MikailBag · 2019-06-19T20:22:16Z

In some functions, types mentioned in this PR are transmuted into their inner value.
Example for PathBuf: https://github.com/rust-lang/rust/blob/master/src/libstd/path.rs#L1132.
This PR adds #[repr(transparent)] to those types, so their correct behavior doesn't depend on compiler details. (As far as I understand, currently that line, converting PathBuf to Vec<u8>, is UB).

rust-highfive · 2019-06-19T20:22:27Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/libstd/ffi/c_str.rs

hanna-kruppe · 2019-06-20T11:43:59Z

Also note that we don't really need repr(transparent) to justify the transmutes. They're all in libstd, which is tied to a specific compiler version (and updated in tandem with rustc) and privileged (in fact, required) to exploit additional knowledge about the compiler that other code doesn't get to rely on.

(Not that this is something we're ever expected to change. Indeed I've never seen anyone seriously argue that struct Foo(T); should have different layout from T, and I've enough bikeshedding about struct layouts to last a lifetime.)

MikailBag · 2019-06-23T20:14:06Z

Also note that we don't really need repr(transparent) to justify the transmutes. They're all in libstd, which is tied to a specific compiler version (and updated in tandem with rustc) and privileged (in fact, required) to exploit additional knowledge about the compiler that other code doesn't get to rely on.

I agree that some code in libstd has to rely on compiler internals.
But, as far as I understand (this is my first PR to Rust), even libstd shoudn't use undocumented behaviour unless it is useful. I don't see, how missing #[repr] can be helpful for libstd.

Also, many other std types which are transmuted do have #[repr(transparent)], e.g. UnsafeCell. So, situation where CStr is not #[repr(transparent)], but UnsafeCell is, doesn't seem consistent to my mind.

hanna-kruppe · 2019-06-23T20:26:14Z

As @Centril noted, marking (public) types repr(transparent) is a commitment that we may not (in the case of CStr, definitely do not) want to make. That's why some types don't have it.

Centril · 2019-06-24T01:12:33Z

Instead of a semver commitment, I would leave comments on type definitions as well as where the transmutes happen with explicit disclaimers for users reading those about not being semver guarantees.

Mark-Simulacrum · 2019-07-09T21:15:59Z

Reassigning this to @Centril and marking as waiting-on-author. This PR will want to be updated to remove the repr(transparent) annotations and instead leave commentary on the unsafe code "relying" on those annotations indicating that libstd is special in that it can rely on the compiler's current behavior.

Ideally, though, I think we should discuss a way -- if it doesn't exist -- for us to make repr(transparent) annotations on types for internal use only, perhaps by cfg'ing them out of the docs somehow.

Centril · 2019-07-26T16:25:47Z

src/libstd/ffi/c_str.rs

@@ -195,6 +195,7 @@ pub struct CString {
 /// [`from_ptr`]: #method.from_ptr
 #[derive(Hash)]
 #[stable(feature = "rust1", since = "1.0.0")]
+// #[repr(transparent)] FIXME: should be enabled, when attribute privacy is implemented


Can you add something like:

// We rely on this internally, but this is *not* a stability guarantee.

as well?

Centril · 2019-07-26T16:32:54Z

r=me rollup when green with comment above ^-- addressed in some fashion.

JohnTitor · 2019-08-04T07:36:05Z

Ping from triage: @MikailBag, waiting on your response, all you have to do is add a comment.

MikailBag · 2019-08-04T15:24:39Z

Currently I'm vacation, and don't have access to my PC)
I believe I will add proposed comments tomorrow.

MikailBag · 2019-08-08T19:42:45Z

Sorry for delay.
I hope new comments are OK)

rust-highfive · 2019-08-08T20:06:06Z

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-08-08T19:46:37.0284911Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-08T19:46:37.0503906Z ##[command]git config gc.auto 0
2019-08-08T19:46:37.0594277Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-08T19:46:37.0657429Z ##[command]git config --get-all http.proxy
2019-08-08T19:46:37.0810957Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/61969/merge:refs/remotes/pull/61969/merge
---
2019-08-08T19:47:13.7929108Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-08T19:47:13.7929178Z 
2019-08-08T19:47:13.7929390Z   git checkout -b <new-branch-name>
2019-08-08T19:47:13.7929420Z 
2019-08-08T19:47:13.7929469Z HEAD is now at 8e5eab5b4 Merge c284340450580470fe98ca3718642aa19b3fc035 into 2628f579f6246df385acf9203bf2ffb6aedf5ccc
2019-08-08T19:47:13.8101331Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-08T19:47:13.8104070Z ==============================================================================
2019-08-08T19:47:13.8104784Z Task         : Bash
2019-08-08T19:47:13.8104834Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-08T19:53:21.4534714Z    Compiling serde_json v1.0.40
2019-08-08T19:53:26.0958426Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-08-08T19:53:35.4579587Z     Finished release [optimized] target(s) in 1m 37s
2019-08-08T19:53:35.4651980Z tidy check
2019-08-08T19:53:36.0525140Z tidy error: /checkout/src/libstd/path.rs:1127: line longer than 100 chars
2019-08-08T19:53:36.0525370Z tidy error: /checkout/src/libstd/path.rs:1129: line longer than 100 chars
2019-08-08T19:53:36.0531324Z tidy error: /checkout/src/libstd/path.rs:1755: line longer than 100 chars
2019-08-08T19:53:36.0584488Z tidy error: /checkout/src/libstd/ffi/os_str.rs:103: line longer than 100 chars
2019-08-08T19:53:36.0591445Z tidy error: /checkout/src/libstd/ffi/c_str.rs:199: line longer than 100 chars
2019-08-08T19:53:36.0591526Z tidy error: /checkout/src/libstd/ffi/c_str.rs:201: line longer than 100 chars
2019-08-08T19:53:36.0889025Z tidy error: /checkout/src/libstd/sys_common/os_str_bytes.rs:24: line longer than 100 chars
2019-08-08T19:53:37.5557130Z some tidy checks failed
2019-08-08T19:53:37.5560023Z 
2019-08-08T19:53:37.5560023Z 
2019-08-08T19:53:37.5561494Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-08-08T19:53:37.5561693Z 
2019-08-08T19:53:37.5561725Z 
2019-08-08T19:53:37.5572257Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-08T19:53:37.5572579Z Build completed unsuccessfully in 0:01:40
2019-08-08T19:53:37.5572579Z Build completed unsuccessfully in 0:01:40
2019-08-08T19:53:38.9417095Z ##[error]Bash exited with code '1'.
2019-08-08T19:53:38.9449216Z ##[section]Starting: Checkout
2019-08-08T19:53:38.9451514Z ==============================================================================
2019-08-08T19:53:38.9451574Z Task         : Get sources
2019-08-08T19:53:38.9451622Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Centril · 2019-08-08T21:15:50Z

The textual changes you made look but you'll need to add some line-breaks to pacify the tidy tool.

r=me rollup once that is done.

MikailBag · 2019-08-11T11:31:57Z

Oh, it seems you didn't get any notifications.
I added several line-breaks and squashed all changes in one commit.

Centril · 2019-08-11T11:41:41Z

Thanks! @bors r+ rollup

bors · 2019-08-11T11:41:42Z

📌 Commit 740f8db has been approved by Centril

Add #[repr(transparent)] for several types In some functions, types mentioned in this PR are transmuted into their inner value. Example for `PathBuf`: https://github.com/rust-lang/rust/blob/master/src/libstd/path.rs#L1132. This PR adds `#[repr(transparent)]` to those types, so their correct behavior doesn't depend on compiler details. (As far as I understand, currently that line, converting `PathBuf` to `Vec<u8>`, is UB).

@ghost

Rollup of 8 pull requests Successful merges: - #61969 (Add #[repr(transparent)] for several types) - #62108 (Use sharded maps for queries) - #63149 (resolve: Populate external modules in more automatic and lazy way) - #63346 (Lint on some incorrect uses of mem::zeroed / mem::uninitialized) - #63433 (Miri shouldn't look at types) - #63440 (rename RUST_CTFE_BACKTRACE to RUSTC_CTFE_BACKTRACE) - #63442 (Add an example to show how to insert item to a sorted vec) - #63459 (syntax: account for CVarArgs being in the argument list.) Failed merges: r? @ghost

Add #[repr(transparent)] for several types In some functions, types mentioned in this PR are transmuted into their inner value. Example for `PathBuf`: https://github.com/rust-lang/rust/blob/master/src/libstd/path.rs#L1132. This PR adds `#[repr(transparent)]` to those types, so their correct behavior doesn't depend on compiler details. (As far as I understand, currently that line, converting `PathBuf` to `Vec<u8>`, is UB).

@ghost

Rollup of 8 pull requests Successful merges: - #61969 (Add #[repr(transparent)] for several types) - #63346 (Lint on some incorrect uses of mem::zeroed / mem::uninitialized) - #63433 (Miri shouldn't look at types) - #63440 (rename RUST_CTFE_BACKTRACE to RUSTC_CTFE_BACKTRACE) - #63441 (Derive Debug for CrateInfo) - #63442 (Add an example to show how to insert item to a sorted vec) - #63453 (rustdoc: general cleanup) - #63464 (Copy ty::Instance instead of passing by reference) Failed merges: r? @ghost

Hide repr attribute from doc of types without guaranteed repr Rustdoc has an undesirable behavior of blindly copying `repr` into the documentation of structs and enums, even when there is no particular repr that the type guarantees to its users. This is a source of confusion for standard library users who assume the fact that a repr is documented means it must be something the standard library promises they can rely on (in transmutes, or FFI). Some issues on the topic of rustdoc's incorrect handling of `repr`: - rust-lang#66401 - rust-lang#90435 In places, the standard library currently works around this confusing rustdoc behavior by just omitting `repr(transparent)` altogether even where it should be required if equivalent code were being written outside of the standard library. See rust-lang#61969. IMO that is even more confusing, even for standard library maintainers — see rust-lang#105018 (comment). It's also not something that works for other reprs like `C` or `u8` which cannot just be omitted even in standard library code. This PR tries a different approach for some types that are being currently incorrectly documented with a repr. > **Warning** > This PR does not imply that every type that still has a `repr` attribute in its docs after this PR is now public for users to rely on. This PR only tries to reduce harm from this longstanding rustdoc issue.

rust-highfive assigned joshtriplett Jun 19, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2019

Centril reviewed Jun 20, 2019

View reviewed changes

src/libstd/ffi/c_str.rs Show resolved Hide resolved

Mark-Simulacrum assigned Centril and unassigned joshtriplett Jul 9, 2019

Mark-Simulacrum 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 9, 2019

Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 26, 2019

Centril reviewed Jul 26, 2019

View reviewed changes

Centril 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 26, 2019

Add FIXME-s that some types should be transparent

740f8db

MikailBag force-pushed the master branch from c284340 to 740f8db Compare August 9, 2019 14:42

bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 11, 2019

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 11, 2019

Mark-Simulacrum mentioned this pull request Aug 11, 2019

Rollup of 8 pull requests #63465

Closed

Mark-Simulacrum mentioned this pull request Aug 11, 2019

Rollup of 8 pull requests #63471

Merged

bors merged commit 740f8db into rust-lang:master Aug 11, 2019

dtolnay mentioned this pull request Feb 5, 2023

Hide repr attribute from doc of types without guaranteed repr #107680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add #[repr(transparent)] for several types #61969

Add #[repr(transparent)] for several types #61969

MikailBag commented Jun 19, 2019

rust-highfive commented Jun 19, 2019

hanna-kruppe commented Jun 20, 2019

MikailBag commented Jun 23, 2019

hanna-kruppe commented Jun 23, 2019

Centril commented Jun 24, 2019

Mark-Simulacrum commented Jul 9, 2019

Centril Jul 26, 2019

Centril commented Jul 26, 2019

JohnTitor commented Aug 4, 2019

MikailBag commented Aug 4, 2019

MikailBag commented Aug 8, 2019

rust-highfive commented Aug 8, 2019

Centril commented Aug 8, 2019

MikailBag commented Aug 11, 2019

Centril commented Aug 11, 2019

bors commented Aug 11, 2019

Add #[repr(transparent)] for several types #61969

Add #[repr(transparent)] for several types #61969

Conversation

MikailBag commented Jun 19, 2019

rust-highfive commented Jun 19, 2019

hanna-kruppe commented Jun 20, 2019

MikailBag commented Jun 23, 2019

hanna-kruppe commented Jun 23, 2019

Centril commented Jun 24, 2019

Mark-Simulacrum commented Jul 9, 2019

Centril Jul 26, 2019

Choose a reason for hiding this comment

Centril commented Jul 26, 2019

JohnTitor commented Aug 4, 2019

MikailBag commented Aug 4, 2019

MikailBag commented Aug 8, 2019

rust-highfive commented Aug 8, 2019

Centril commented Aug 8, 2019

MikailBag commented Aug 11, 2019

Centril commented Aug 11, 2019

bors commented Aug 11, 2019