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

Zeroable derive custom bounds #196

Merged
merged 7 commits into from
Jul 26, 2023
Merged

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Jun 12, 2023

Adds the ability to use custom bounds when using derive(Zeroable) instead of the normal "add a bound to each of the generics".

The normal soundness checks are still performed, so the custom bound must guarantee that the struct is actually zeroable, or compilation will fail.

Unresolved questions:

Emit bounds for each field If
#[derive(Zeroable)]
struct Struct<T, U, V> {
  a: Cell<usize>,
  b: PhantomData<T>,
  c: Option<Box<U>>,
  d: [V; 3],
}

emitted

unsafe impl<T, U, V> Zeroable for Struct<T, U, V>
  where
    Cell<usize>: Zeroable,
    PhantomData<T>: Zeroable,
    Option<Box<U>>: Zeroable,
    [V; 3]: Zeroable,
 {}

that would work and not require the user to give any bounds.

Possible future work:

@zachs18 zachs18 changed the title [WIP] Zeroable custom bounds Zeroable derive custom bounds Jun 16, 2023
@wdanilo
Copy link

wdanilo commented Jun 16, 2023

As I replied in the second thread, I believe the bounds generated for each field are a must-have, otherwise this is unsafe with wrongly provided bounds. If the user wants such an unsafe impl, they can always write unsafe impl<...> Zeroable for ..., but deriving should be always safe.

@zachs18
Copy link
Contributor Author

zachs18 commented Jun 16, 2023

As stated, the normal soundness checks are still performed, so the derive macro cannot produce unsound code that compiles, as it also emits checks that guarantee that each field is Zeroable. There is a doctest for this in the PR (lines 122-133 of derive/src/lib.rs).

For a concrete example

For a concrete example, this code (under this PR) expands to the following code.

#[derive(bytemuck::Zeroable)]
#[zeroable(bound = "u32: Copy, U: bytemuck::Zeroable")]
struct Asdf<T, U> {
    x: Box<T>,
    y: U,
}

// expands to
struct Asdf<T, U> {
    x: Box<T>,
    y: U,
}
const _: fn() = || {
    #[allow(clippy::missing_const_for_fn)]
    #[doc(hidden)]
    fn check<T, U>()
    where
        u32: Copy,
        U: bytemuck::Zeroable,
    {
        fn assert_impl<T: ::bytemuck::Zeroable>() {}
        assert_impl::<Box<T>>();
    }
};
const _: fn() = || {
    #[allow(clippy::missing_const_for_fn)]
    #[doc(hidden)]
    fn check<T, U>()
    where
        u32: Copy,
        U: bytemuck::Zeroable,
    {
        fn assert_impl<T: ::bytemuck::Zeroable>() {}
        assert_impl::<U>();
    }
};
unsafe impl<T, U> ::bytemuck::Zeroable for Asdf<T, U>
where
    u32: Copy,
    U: bytemuck::Zeroable,
{
}

The fn checks are where the soundness checks are performed, and they will not compile if the fields are not all guaranteed to be Zeroable.

(Also, note to self, unrelated to this PR: the multiple separate const _s could probably be rolled into one.)

@wdanilo
Copy link

wdanilo commented Jun 18, 2023

@zachs18 thanks for the reply and for providing the expansion code, very helpful! What is the reason the expanded code generates the const _: fn() = || { ... } things? I don't think they are needed, unless I'm missing some obvious thing here. The following code is enough and is checking the same things:

unsafe impl<T, U> ::bytemuck::Zeroable for Asdf<T, U>
where
    u32: Copy,
    U: bytemuck::Zeroable {}

@zachs18 zachs18 marked this pull request as ready for review July 17, 2023 02:37
@zachs18
Copy link
Contributor Author

zachs18 commented Jul 17, 2023

(The failing CI on mips/mips64 is due to rust-lang/rust#113274 )
Okay, I added "perfect derive" semantics to #[zeroable(bound = "...")], i.e. when explicit bounds are given, the given bounds are emitted in addition to a bound for each field's type. (If no explicit bounds are given, the current "bound each type generic" behavior is unchanged).

Marking this PR as ready for review @Lokathor @wdanilo .

Copy link
Owner

@Lokathor Lokathor left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'll wait a while for wdanilo to have a chance to look at it.

@Lokathor Lokathor merged commit 39b42b8 into Lokathor:main Jul 26, 2023
@Lokathor Lokathor added semver-minor semver minor change semver-derive We need to update the main crate's use of the derive crate labels Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-derive We need to update the main crate's use of the derive crate semver-minor semver minor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants