Document & implement the transmutation modeled by `BikeshedIntrinsicFrom` by jswrenn · Pull Request #129032 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation58 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

Relevant to the library team, which will review and decide on the PR/issue.

labels

This comment has been minimized.

/// 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`.

Choose a reason for hiding this comment

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

/// 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`.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Comment on lines 21 to 29

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

Choose a reason for hiding this comment

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

/// 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.

Choose a reason for hiding this comment

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

Comment on lines 42 to 50

/// 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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Comment on lines 36 to 38

/// 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`.

Choose a reason for hiding this comment

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

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)

Choose a reason for hiding this comment

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

Comment on lines 47 to 50

/// 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.

Choose a reason for hiding this comment

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

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

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

A few optional changes requested, but LGTM!

@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.

This comment has been minimized.

This comment has been minimized.

📌 Commit 2540070 has been approved by compiler-errors

It is now in the queue for this repository.

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

…er-errors

Document & implement the transmutation modeled by BikeshedIntrinsicFrom

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: rust-lang#99571

r? @compiler-errors

cc @scottmcm, @Lokathor

This was referenced

Aug 26, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

…iaskrgr

Rollup of 9 pull requests

Successful merges:

rust-lang#126013 (Add #[warn(unreachable_pub)] to a bunch of compiler crates)
rust-lang#128157 (deduplicate and clarify rules for converting pointers to references)
rust-lang#129032 (Document & implement the transmutation modeled by BikeshedIntrinsicFrom)
rust-lang#129250 (Do not ICE on non-ADT rcvr type when looking for crate version collision)
rust-lang#129340 (Remove Duplicate E0381 Label)
rust-lang#129560 ([rustdoc] Generate source link on impl associated types)
rust-lang#129622 (Remove a couple of unused feature enables)
rust-lang#129625 (Rename ParenthesizedGenericArgs to GenericArgsMode)
rust-lang#129626 (Remove ParamMode::ExplicitNamed)

Failed merges:

rust-lang#128166 (Improved checked_isqrt and isqrt methods)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Rollup merge of rust-lang#129032 - jswrenn:transmute-method, r=compiler-errors

Document & implement the transmutation modeled by BikeshedIntrinsicFrom

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: rust-lang#99571

r? @compiler-errors

cc @scottmcm, @Lokathor

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Relevant to the library team, which will review and decide on the PR/issue.