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

Support derive(KnownLayout) on DSTs #643

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Nov 27, 2023

Makes progress towards #29. Supersedes #541.

For types marked #[repr(C)], this derive requires that the trailing field is KnownLayout:

// For illustrative purposes; `align` and `packed`
// can't actually be combined like this.
#[repr(C, align(2), packed(4))]
struct ReprCStruct<T, U, V> {
    foo: Foo<T>,
    bar: Bar<U>,
    baz: Baz<V>,
}

impl<T, U, V> KnownLayout for ReprCStruct<T, U, V> {
    const LAYOUT: DstLayout = {
        use ::zerocopy::macro_utils::core_reexport::num::NonZeroUsize;
        use ::zerocopy::{DstLayout, KnownLayout};

        let repr_align = NonZeroUsize::new(2); // or `None`, w/o `align`
        let repr_packed = NonZeroUsize::new(4); // or `None`, w/o `packed`

        DstLayout::new(repr_align)
            .extend(DstLayout::for_type::<Foo<T>>(), repr_packed)
            .extend(DstLayout::for_type::<Bar<U>>(), repr_packed)
            .extend(<Baz<V> as KnownLayout>::LAYOUT, repr_packed)
            .pad_to_align()
    };
}

For non-repr(C) layouts, it's only required that Self: Sized.

@jswrenn jswrenn force-pushed the derive-unsized-knownlayout branch from 2557457 to dcc339b Compare November 27, 2023 17:25
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/ext.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
@jswrenn jswrenn force-pushed the derive-unsized-knownlayout branch 2 times, most recently from b500286 to 8d0ddf7 Compare November 27, 2023 18:22
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.

Need to add tests, notably:

  • In src/lib.rs, the impl tests for types like Unalign should test with both sized and unsized types
  • UI failure tests for the following cases:
    • Type is repr(C), trailing field is a concrete type (not a type parameter), and trailing field doesn't implement KnownLayout
    • Type is not repr(C), trailing field is a concrete type (not a type parameter), and trailing field is unsized
  • Non-UI tests for all combinations of the following axes:
    • repr(C)/not repr(C)
    • Trailing field is concrete/generic
    • Trailing field does/does not implement KnownLayout
    • Trailing field is sized/unsized

@jswrenn jswrenn force-pushed the derive-unsized-knownlayout branch 2 times, most recently from 5345efa to af44c49 Compare November 27, 2023 20:49
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.

For success cases, can we also test that we get the expected value for KnownLayout::LAYOUT?

It's probably also worth adding a test along these lines:

#[repr(C)]
struct Foo<A, B, C>(A, B, C);

macro_rules! test_foo {
    ($a:ty, $b:ty, $c:ty => $layout:expr) => {
        assert_eq!(<Foo<$a, $b, $c> as KnownLayout>::LAYOUT, $layout);
    };
}

// TODO: A bunch of different combinations of types with different sizes and alignments

zerocopy-derive/tests/ui-nightly/mid_compile_pass.rs Outdated Show resolved Hide resolved
zerocopy-derive/tests/ui-nightly/struct.rs Show resolved Hide resolved
zerocopy-derive/tests/ui-nightly/struct.rs Show resolved Hide resolved
zerocopy-derive/tests/ui-nightly/late_compile_pass.rs Outdated Show resolved Hide resolved
@jswrenn jswrenn force-pushed the derive-unsized-knownlayout branch 5 times, most recently from 233aa78 to 786a279 Compare November 28, 2023 17:34
Comment on lines +5165 to +5409
struct NotKnownLayout<T = ()> {
_t: T,
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a repr?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed offline: It could use a repr(transparent), but it would be sufficiently interesting if this test started failing on a future Rust version, that it'd be nice to get the heads up.

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@jswrenn jswrenn force-pushed the derive-unsized-knownlayout branch 2 times, most recently from ab87b85 to 8b28748 Compare November 28, 2023 21:17
DSTs must be marked with `repr(C)`. The expansion requires the final
field implement `KnownLayout`.

Makes progress towards #29.
@jswrenn jswrenn force-pushed the derive-unsized-knownlayout branch from 8b28748 to fb70716 Compare November 28, 2023 21:26
@jswrenn jswrenn marked this pull request as ready for review November 28, 2023 21:33
@jswrenn jswrenn requested a review from joshlf November 28, 2023 21:33
@joshlf joshlf added this pull request to the merge queue Nov 29, 2023
Merged via the queue into main with commit 40202e4 Nov 29, 2023
@joshlf joshlf deleted the derive-unsized-knownlayout branch November 29, 2023 04:24
@joshlf joshlf mentioned this pull request Dec 5, 2023
34 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants