Document & implement the transmutation modeled by `BikeshedIntrinsicFrom` #129032

jswrenn · 2024-08-12T23:28:39Z

Documents that BikeshedIntrinsicFrom models transmute-via-union, which is slightly more expressive than the transmute-via-cast implemented by transmute_copy. Additionally, we provide an implementation of transmute-via-union as a method on the BikeshedIntrinsicFrom trait with additional documentation on the boundary between trait invariants and caller obligations.

Whether or not transmute-via-union is the right kind of transmute to model remains up for discussion [1]. Regardless, it seems wise to document the present behavior.

[1] https://rust-lang.zulipchat.com/#narrow/stream/216762-project-safe-transmute/topic/What.20'kind'.20of.20transmute.20to.20model.3F/near/426331967

Tracking Issue: #99571

r? @compiler-errors

cc @scottmcm, @Lokathor

library/core/src/mem/transmutability.rs

joshlf · 2024-08-12T23:49:09Z

library/core/src/mem/transmutability.rs

+/// This trait cannot be implemented explicitly. It is implemented on-the-fly by
+/// the compiler for all types `Src` and `Self` such that, given a set of safety
+/// obligations on the programmer (see [`Assume`]), the compiler has proved that
+/// the bits of a value of type `Src` can be soundly reinterpreted as `Self`.


Suggested change

/// the bits of a value of type `Src` can be soundly reinterpreted as `Self`.

/// the bits of a value of type `Src` can be soundly reinterpreted as a `Self`.

I don't actually agree with this change. "as Self" sounds more correct in my idiolect.

My thinking is that the left-hand side refers to a runtime object - "a value of type Src" - and so the right-hand side should too. Conservatively, we could say "a value of type Self", but IIUC "a Self" is a common shorthand for that.

If we take the Self part out for a moment, it would be common to say "as an i32", or "as an array", so "a Self" probably fits.

I'm inclined to agree with @Lokathor's argument, so I've opted for "a Self".

joshlf · 2024-08-12T23:50:47Z

library/core/src/mem/transmutability.rs

+///     union Transmute<T, U> {
+///         src: ManuallyDrop<T>,
+///         dst: ManuallyDrop<U>,
+///     }


Suggested change

/// union Transmute<T, U> {

/// src: ManuallyDrop<T>,

/// dst: ManuallyDrop<U>,

/// }

/// union Transmute<S, D> {

/// src: ManuallyDrop<S>,

/// dst: ManuallyDrop<D>,

/// }

I assume your intention is to not confuse the reader by re-using Src and Dst? In that case, IMO using S and D makes the correspondence with Src and Dst clearer.

I hadn't put that much thought into this, actually. I don't think there's any risk of confusion from shadowing Src and Dst (even if you didn't realize they were shadowed, the meaning of the code wouldn't change), so I've opted to just use Src and Dst:

https://github.com/rust-lang/rust/compare/96cfefbb125916871760c1df4c2ee29944635e90..1c0e7fac16490ea2e83ab0127c364231a42742ad#diff-85698661a7eb53801df364c9242c8613f7d2e4bca026bc5c88d542c22fa01ddfR21-R23

https://github.com/rust-lang/rust/compare/96cfefbb125916871760c1df4c2ee29944635e90..1c0e7fac16490ea2e83ab0127c364231a42742ad#diff-85698661a7eb53801df364c9242c8613f7d2e4bca026bc5c88d542c22fa01ddfR114-R116

joshlf · 2024-08-12T23:54:58Z

library/core/src/mem/transmutability.rs

+/// Implementations of this trait do not provide any guarantee of portability
+/// across toolchains or compilations. This trait may be implemented for certain
+/// combinations of `Src`, `Self` and `ASSUME` on some toolchains, but not
+/// others. For example, if the layouts of `Src` or `Self` are
+/// non-deterministic, the presence or absence of an implementation of this
+/// trait may also be non-deterministic. Even if `Src` and `Self` have
+/// deterministic layouts (e.g., they are `repr(C)` structs), Rust does not
+/// specify the alignments of its primitive integer types, and layouts that
+/// involve these types may vary across toolchains.


Also mention varying across target architecture and target OS? Both can affect alignment, and architecture can affect size (maybe OS can too? not sure).

Good point! This now says "toolchains, targets or compilations".

https://github.com/rust-lang/rust/compare/96cfefbb125916871760c1df4c2ee29944635e90..1c0e7fac16490ea2e83ab0127c364231a42742ad#diff-85698661a7eb53801df364c9242c8613f7d2e4bca026bc5c88d542c22fa01ddfR59

One change left:

Rust does not specify the alignments of its primitive integer types, and layouts that involve these types may vary across toolchains.

This should say "across toolchains or targets" or something similar.

joshlf · 2024-08-12T23:56:04Z

library/core/src/mem/transmutability.rs

+/// Note that this construction supports some transmutations forbidden by
+/// [`mem::transmute_copy`](super::transmute_copy), namely transmutations that
+/// extend the trailing padding of `Src` to fill `Dst`.


This strikes me as a bit ambiguous. I'd clarify in particular that you're referring to a situation where:

size_of::<Src>() < size_of::<Dst>()

The trailing size_of::<Dst>() - size_of::<Src>() bytes of Dst are permitted to be uninitialized (e.g., are padding)

I've reworded this slightly, and included an example provided by @scottmcm on Zulip: https://github.com/rust-lang/rust/compare/96cfefbb125916871760c1df4c2ee29944635e90..1c0e7fac16490ea2e83ab0127c364231a42742ad#diff-85698661a7eb53801df364c9242c8613f7d2e4bca026bc5c88d542c22fa01ddfR59

joshlf · 2024-08-12T23:56:57Z

library/core/src/mem/transmutability.rs

+/// trait may also be non-deterministic. Even if `Src` and `Self` have
+/// deterministic layouts (e.g., they are `repr(C)` structs), Rust does not
+/// specify the alignments of its primitive integer types, and layouts that
+/// involve these types may vary across toolchains.


I would maybe phrase this as "stability hazards may exist" and mention the alignments of primitive types as one example of such hazards. Don't want to imply by omission that they're the only hazard.

library/core/src/mem/transmutability.rs

joshlf

A few optional changes requested, but LGTM!

library/core/src/mem/transmutability.rs

bors · 2024-08-20T02:26:27Z

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

jswrenn · 2024-08-22T20:06:43Z

@joshlf, could you take another look at this?

I've added a # Safety section BikeshedIntrinsicFrom
I've added doctests to most of the Assume flags.
- No examples yet for Assume::lifetimes, which is both hard to give a succinct example of, and also so subtly dangerous I hesitate to give an example of it.

library/core/src/mem/transmutability.rs

Documents that `BikeshedIntrinsicFrom` models transmute-via-union, which is slightly more expressive than the transmute-via-cast implemented by `transmute_copy`. Additionally, we provide an implementation of transmute-via-union as a method on the `BikeshedIntrinsicFrom` trait with additional documentation on the boundary between trait invariants and caller obligations. Whether or not transmute-via-union is the right kind of transmute to model remains up for discussion [1]. Regardless, it seems wise to document the present behavior. [1] https://rust-lang.zulipchat.com/#narrow/stream/216762-project-safe-transmute/topic/What.20'kind'.20of.20transmute.20to.20model.3F/near/426331967

compiler-errors · 2024-08-26T13:04:25Z

@bors r+

bors · 2024-08-26T13:04:28Z

📌 Commit 2540070 has been approved by compiler-errors

It is now in the queue for this repository.

rustbot assigned compiler-errors Aug 12, 2024

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 Aug 12, 2024

the8472 reviewed Aug 12, 2024

View reviewed changes

library/core/src/mem/transmutability.rs Outdated Show resolved Hide resolved

jswrenn force-pushed the transmute-method branch from b806b5e to 96cfefb Compare August 12, 2024 23:40

This comment has been minimized.

Sign in to view

joshlf suggested changes Aug 13, 2024

View reviewed changes

jswrenn commented Aug 13, 2024

View reviewed changes

library/core/src/mem/transmutability.rs Show resolved Hide resolved

jswrenn force-pushed the transmute-method branch from 96cfefb to 1c0e7fa Compare August 14, 2024 14:13

This comment has been minimized.

Sign in to view

jswrenn force-pushed the transmute-method branch 2 times, most recently from 43dfc04 to b17b899 Compare August 14, 2024 15:16

This comment has been minimized.

Sign in to view

jswrenn force-pushed the transmute-method branch from b17b899 to ac7b552 Compare August 14, 2024 15:55

This comment has been minimized.

Sign in to view

jswrenn force-pushed the transmute-method branch from ac7b552 to 8bf3df9 Compare August 15, 2024 13:09

jswrenn commented Aug 15, 2024

View reviewed changes

library/core/src/mem/transmutability.rs Outdated Show resolved Hide resolved

jswrenn requested review from compiler-errors and joshlf August 15, 2024 13:53

joshlf approved these changes Aug 16, 2024

View reviewed changes

jswrenn force-pushed the transmute-method branch from 8bf3df9 to 7254d9d Compare August 17, 2024 22:32

compiler-errors reviewed Aug 19, 2024

View reviewed changes

library/core/src/mem/transmutability.rs Outdated Show resolved Hide resolved

jswrenn force-pushed the transmute-method branch from 7254d9d to 60cfa88 Compare August 19, 2024 16:25

jswrenn force-pushed the transmute-method branch from 60cfa88 to c3ebec9 Compare August 20, 2024 18:19

jswrenn mentioned this pull request Aug 21, 2024

safe transmute: gracefully bubble-up layout errors #129364

Merged

jswrenn force-pushed the transmute-method branch 2 times, most recently from f31c502 to 5ab5080 Compare August 22, 2024 20:03

This comment has been minimized.

Sign in to view

joshlf suggested changes Aug 22, 2024

View reviewed changes

library/core/src/mem/transmutability.rs Show resolved Hide resolved

jswrenn force-pushed the transmute-method branch from 5ab5080 to aa01623 Compare August 22, 2024 23:17

This comment has been minimized.

Sign in to view

jswrenn force-pushed the transmute-method branch from aa01623 to 2540070 Compare August 23, 2024 14:38

jswrenn mentioned this pull request Aug 23, 2024

Tracking Issue for Transmutability Trait: #[transmutability] #99571

Open

7 tasks

jswrenn requested a review from compiler-errors August 26, 2024 10:42

compiler-errors approved these changes Aug 26, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2024

This was referenced Aug 26, 2024

Rollup of 9 pull requests #129628

Closed

Rollup of 9 pull requests #129632

Merged

bors merged commit 29923b6 into rust-lang:master Aug 27, 2024

rustbot added this to the 1.82.0 milestone Aug 27, 2024

jswrenn mentioned this pull request Aug 27, 2024

Rename BikeshedIntrinsicFrom to TransmuteFrom #129657

Merged

	/// the bits of a value of type `Src` can be soundly reinterpreted as `Self`.
	/// the bits of a value of type `Src` can be soundly reinterpreted as a `Self`.

Uh oh!

Document & implement the transmutation modeled by BikeshedIntrinsicFrom #129032

Document & implement the transmutation modeled by BikeshedIntrinsicFrom #129032

Uh oh!

Conversation

jswrenn commented Aug 12, 2024

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jswrenn Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

joshlf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Aug 20, 2024

Uh oh!

jswrenn commented Aug 22, 2024

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

compiler-errors commented Aug 26, 2024

Uh oh!

bors commented Aug 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Document & implement the transmutation modeled by `BikeshedIntrinsicFrom` #129032

Document & implement the transmutation modeled by `BikeshedIntrinsicFrom` #129032

jswrenn Aug 14, 2024 •

edited

Loading