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

Add initial support for unsized MaybeUninit wrapper type #2055

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Nov 13, 2024

This is achieved by adding a MaybeUninit associated type to KnownLayout, whose layout is identical to Self except that it admits uninitialized bytes in all positions.

For sized types, this is bound to mem::MaybeUninit<Self>. For potentially unsized structs, we synthesize a doppelganger with the same repr, whose leading fields are wrapped in mem::MaybeUninit and whose trailing field is the MaybeUninit associated type of struct's original trailing field type. This type-level recursion bottoms out at [T], whose MaybeUninit associated type is bound to [mem::MaybeUninit<T>].

Next Steps and Future Possibilities

MaybeUninit<T: ?Sized>

In the near term, we may remove doc(hidden) from our MaybeUninit wrapper. In doing so, we'd be quick-to-ship a gadget that extends the present capabilities of Rust. We might, then, be able to use this feature as a demonstration of our approach, potentially suitable for upstreaming into rustc.

UnalignUnsized<T: ?Sized>

Presently, our Unalign wrapper requires T: Sized, because we could not figure out how to drop unsized values. For sized values, we copy them onto the stack, then run their destructor. With MaybeUninit<T: ?Sized>, we can extend unalign support to unsized values, by first copying them into a Box<MaybeUninit<T>>, casting the box to Box<T>, and dropping it. This process would be skipped for types with trivial drops, keeping the common case simple and efficient.

Value<T, I> where I: Invariants<Validity = Any>

Combining the above two approaches, we could create an invariant-parameterized Ptr analogue for values that generalizes over initialization and alignment.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Haven't looked at most of zerocopy-derive yet, but looked at all of zerocopy.

src/lib.rs Show resolved Hide resolved
src/util/mod.rs Show resolved Hide resolved
src/util/mod.rs Outdated Show resolved Hide resolved
src/util/mod.rs Outdated Show resolved Hide resolved
src/util/mod.rs Show resolved Hide resolved
src/wrappers.rs Outdated Show resolved Hide resolved
src/wrappers.rs Outdated Show resolved Hide resolved
src/wrappers.rs Outdated Show resolved Hide resolved
src/wrappers.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
@jswrenn jswrenn force-pushed the maybe-uninit branch 2 times, most recently from 9844c1f to 9751180 Compare November 14, 2024 14:42
@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 79.54545% with 36 lines in your changes missing coverage. Please review.

Project coverage is 87.42%. Comparing base (664b976) to head (60f0a43).

Files with missing lines Patch % Lines
src/wrappers.rs 61.95% 35 Missing ⚠️
src/util/mod.rs 98.48% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           v0.8.x    #2055      +/-   ##
==========================================
- Coverage   88.04%   87.42%   -0.63%     
==========================================
  Files          16       16              
  Lines        5983     6115     +132     
==========================================
+ Hits         5268     5346      +78     
- Misses        715      769      +54     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@jswrenn jswrenn force-pushed the maybe-uninit branch 4 times, most recently from 235989d to bb140d0 Compare November 14, 2024 20:17
@jswrenn
Copy link
Collaborator Author

jswrenn commented Nov 14, 2024

The remaining CI failures are from cargo-semver-checks; it complains that we've added a new item to KnownLayout:

--- failure trait_associated_type_added: non-sealed public trait added associated type without default value ---

Description:
A non-sealed trait has gained an associated type without a default value, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_associated_type_added.ron

Failed in:
  trait associated type zerocopy::KnownLayout::MaybeUninit in file /home/runner/work/zerocopy/zerocopy/src/lib.rs:742

@jswrenn jswrenn requested a review from joshlf November 19, 2024 15:24
@joshlf
Copy link
Member

joshlf commented Nov 19, 2024

The remaining CI failures are from cargo-semver-checks; it complains that we've added a new item to KnownLayout:

--- failure trait_associated_type_added: non-sealed public trait added associated type without default value ---

Description:
A non-sealed trait has gained an associated type without a default value, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_associated_type_added.ron

Failed in:
  trait associated type zerocopy::KnownLayout::MaybeUninit in file /home/runner/work/zerocopy/zerocopy/src/lib.rs:742

We should report this upstream. In particular, given that KnownLayout already has #[doc(hidden)] items, we know that there can be no downstream implementations of KnownLayout which have not already opted out of semver stability.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

A few more nits, but otherwise LGTM!

src/util/mod.rs Outdated Show resolved Hide resolved
src/util/mod.rs Outdated
// fields both start at the safe offset and the types of those fields are
// transparent wrappers around `Src` and `Dst` [1]. Consequently,
// initializng `Transmute` with with `src` and then reading out `dst` is
// equivalent to transmuting from `Src` to `Dst` [2].
Copy link
Member

Choose a reason for hiding this comment

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

Also need to prove that such a transmute is sound (guaranteed by the caller).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Also note this change.

Copy link
Member

Choose a reason for hiding this comment

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

Is that latter change semantically meaningful? Or do you just prefer that wording?

zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
@jswrenn
Copy link
Collaborator Author

jswrenn commented Nov 20, 2024

We should report this upstream. In particular, given that KnownLayout already has #[doc(hidden)] items, we know that there can be no downstream implementations of KnownLayout which have not already opted out of semver stability.

Filed: obi1kenobi/cargo-semver-checks#997

In the meantime, I think we should permit cargo-semver-checks to fail in CI. Otherwise, our only recourse is to bypass branch protections altogether, which would allow this PR to skip some of our more intensive CI steps that are run only in the merge queue.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
This is achieved by adding a `MaybeUninit` associated type to
`KnownLayout`, whose layout is identical to `Self` except that it admits
uninitialized bytes in all positions.

For sized types, this is bound to `mem::MaybeUninit<Self>`. For
potentially unsized structs, we synthesize a doppelganger with the same
`repr`, whose leading fields are wrapped in `mem::MaybeUninit` and whose
trailing field is the `MaybeUninit` associated type of struct's original
trailing field type. This type-level recursion bottoms out at `[T]`,
whose `MaybeUninit` associated type is bound to `[mem::MaybeUninit<T>]`.

Makes progress towards #1797

SKIP_CARGO_SEMVER_CHECKS=1
@jswrenn jswrenn added this pull request to the merge queue Nov 21, 2024
Merged via the queue into v0.8.x with commit d159e36 Nov 21, 2024
87 checks passed
@jswrenn jswrenn deleted the maybe-uninit branch November 21, 2024 21:58
@jswrenn jswrenn mentioned this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants