Allow deriving Zeroable with explicitly provided bounds

[wdanilo]

Hi, I've got a code like this:

#[derive(Debug, Derivative, Zeroable)]
#[derivative(Default(bound = "T: Default"))]
pub struct LinkedCellArray<T, const N: usize = 32> {
    size:          Cell<usize>,
    first_segment: ZeroableOption<Box<Segment<T, N>>>,
}

And deriving Zeroable adds T: Zeroable bound. This is bad, as I've got impl<T> Zeroable for ZeroableOption<T> {} for any T implemented. Doing manual impl<T, N> Zeroable for LinkedCellArray<T, const N> {} is also bad, as it does not check the soundness when the code is changed. Would it be possible to add an option to Zeroable to provide a custom bound, something like the following? :)

#[derive(Debug, Derivative, Zeroable)]
#[derivative(Default(bound = "T: Default"))]
#[zeroable(bound = "")]
pub struct LinkedCellArray<T, const N: usize = 32> {
    size:          Cell<usize>,
    first_segment: ZeroableOption<Box<Segment<T, N>>>,
}

[Lokathor]

uh, possibly? I'm unclear how the derive code accepting a custom bound could be sure it's the correct bound.

Also, what's the ZeroableOption thing?

[wdanilo]

The idea is that whatever is provided by the user will be used as the bound. If it is incorrect, the compiler will throw an error, so you, as an author of such macros, do not have to worry if the bound is correct. For example, consider the Derivative macro, which I can use as:

#[derive(Derivative)]
#[derivative(Clone(bound = ""))]
struct Foo<T> {
    t: T
}

The compiler will tell me that there is missing bound T: Clone.

Regarding your second question, ZeroableOption is my custom option implementation that is zeroable:

#[repr(u8)]
pub enum ZeroableOption<T> {
    #[default]
    None = 0,
    Some(T),
}

[Lokathor]

Well the compiler definitely will let you impl Zeroable when you have non-Zeroable fields. It's a unsafe trait with no methods, it could (in the "it will compile" sense) be implemented on anything. However, that's why it's unsafe, because the bounds of the impl must be correct for the type in question.

Given your specific ZeroableOption type, i think perhaps you should just write down the unsafe impl directly.

[wdanilo]

I think we have a small misunderstanding here :D

1. For ZeroableOption, yes, I've written the implementation manually. I do not want to implement it manually for LinkedCellArray, as it will be very error-prone. If in the future, someone from our team changes it, the compiler would not catch that it does not implement Zeroable anymore. Consider the following code to see a simpler example. How would you derive Zeroable for this struct?

   struct Foo<T> {
       my_beautiful_field: Option<Box<T>>
   }

   Please note, that if you use derive(Zeroable) here, it will require T: Zeroable, which is invalid, as Option<Box<T>> is zeroable for any T!

2. The safe code generation in the macro is simple. For example, given the code:

   #[derive(Zeroable)]
   #[zeroable(bound = "my_bound")]
   struct Name<T1, T2> {
       field1: Type1
       field2: Type2
    }

   The generated code should be:

   unsafe impl<T1, T2, my_bound> Zeroable for Name<T1, T2> where
       Type1: Zeroable,
       Type2: Zeroable {}

   This way, you can allow the user to pass custom bounds and still check if the bounds are correct.

---

EDIT: Additional info here.

In fact, until it is properly implemented in this carte, I've created an ugly small macro rules for it:

#[macro_export]
macro_rules! derive_zeroable {
    (
        $(#$meta:tt)*
        pub struct $name:ident $([ $($bounds:tt)* ] [ $($bounds_def:tt)* ])? {
            $($field:ident : $ty:ty),* $(,)?
        }
    ) => {
        $(#$meta)*
        pub struct $name $(< $($bounds_def)* >)? {
            $($field : $ty),*
        }
        unsafe impl $(< $($bounds_def)* >)? $crate::Zeroable for $name $(< $($bounds)* >)?
            where $($ty: Zeroable),* {}
    };
}

Which I'm using as:

derive_zeroable! {
    #[derive(Debug, Derivative)]
    #[derivative(Default(bound = "T: Default"))]
    pub struct LinkedArrayRefCell[T, N][T, const N: usize] {
        size:          Cell<usize>,
        first_segment: OptRefCell<ZeroableOption<Segment<T, N>>>,
    }
}

and it generates the following Zeroable impl:

unsafe impl<T, const N: usize> ::enso_frp2::Zeroable for LinkedArrayRefCell<T, N>
    where Cell<usize>: Zeroable, OptRefCell<ZeroableOption<Segment<T, N>>>: Zeroable {}

Please note, that if you generate the impl this way from the macro, there is no possibility for the user-provided bound to make the implementation unsafe, as it will always be checked by the compiler.

[Lokathor]

I guess if the bound can't actually cause unsound impls to be emitted then it's fine to have a user provided bound. I'm not going to adjust the proc-macro myself, but if someone else does the PR we can get this added.

[zachs18]

I have a draft PR #196 that adds user-custom bounds for Zeroable, but considering that "Perfect Derive" is also possible (emit a bound for each field's type exactly), it might be better to just have derive(Zeroable) emit that (perhaps as an opt-in option; see the "semver hazard" section).

code example

If the following derive

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

emitted the following impl

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.

[wdanilo]

@zachs18 amazing to see it! Please note that if you are not emitting bounds automatically for every field, then this can't be considered Safe, as I could provide bounds = "" and it will succeed even if some fields do not implement Zeroable. Deriving implies can't work in such a way, so adding these bounds is required IMO.

If you don't want the checks and want to have unsafe impl, you can always write unsafe impl<...> Zeroable for .... But when using deriving, this impl should always be safe.

[zachs18]

See #196 (comment) for why the PR is indeed safe (it doesn't compile if the given bounds do not guarantee that all fields are Zeroable, same as before with e.g. #[derive(Zeroable)] struct A<T>(Box<T>);).

---

Aside from that, I see a few main ways to resolve this issue:

1. Always use "Perfect Derive" for Zeroable, i.e. just bound every field's type on Zeroable (instead of every generic), without allowing custom bounds: The simplest from a user's perspective, but also the least customizable, and also changes existing derived impls. (Also this might cause semver issues with changing the bounds of existing derived impls? I couldn't come up with a situation where it'd be a problem, but regardless option 2/1b addresses this)
2. (1b) Allow users to opt-in to "Perfect Derive" with an attribute, e.g. #[zeroable(perfect)] (and still use the current "bound-each-generic" by default). Basically same as 1, but doesn't change existing impls.
3. Opt-in customizable bounds without "Perfect Derive": (Current status of my PR): Requires the user to manually add required bounds (otherwise, compiler error).
4. Opt-in customizable bounds with "Perfect Derive": allows the user to further restrict the (implicit) perfect derive bounds,

IMO, given option 2, I don't see much use for the additional customization that 3 or 4 provide (perhaps so users can avoid the "semver hazard" by adding more conservative bounds than strictly necessary? So that a user could change their underlying type in their library without needing to strengthen a bound which might require a version bump).

Note on "Perfect Derive": "Perfect Derive"-by-default has the "semver hazard" that changing private fields can change the public impl's bounds, similar to Send. (Though I don't know how common it is for a library to have a public type with private fields that derives Zeroable anyway.) If using "perfect derive" is explicit and opt-in (like in option 2), then I think it's fine.

(IMO Option 2 is best, followed by 4, then 3, then 1)

@Lokathor Which option do you think is best (or something else)?

@wdanilo Would option 2 work for you, or would you rather have explicitly specified bounds?

[wdanilo]

@zachs18 oh, I apologize for super late reply! Somehow I missed the notification.

You can't just use "perfect derive" without allowing users to provide custom bounds, as this would be extremely limited. In order to keep old code working as is, we can indeed do something like that:

1. #[derive(Zeroable)] would work as it worked in the past (with bounds added to all generic params by default).
2. #[derive(Zeroable)] + #[zeroable(bounds="...")] to use "perfect derive" + custom bounds.

I believe that this will just cover all cases and will be a very consistent way of doing this (e.g. derivative works this way).

How does it sound to you?

[Lokathor]

I also let this topic totally slip my mind.

Reading the options, I agree that Option 2/(1b) seems best. If I understand right: It doesn't change any existing users, it just adds a new opt-in for people to use if the existing "basic derive" doesn't work in their case.

EDIT: I admit that I don't remember enough to understand your concerns offhand wdanilo, but I'd say that my priorities in general are:

• don't break any existing users of course
• let people who need it opt-in to whatever the extra new system will be

[wdanilo]

@Lokathor I think thy my point 2 is superior than 2/(1b) as it is more consistent with how other crates work. It provides the same features and matches the priorities you listed. @zachs18 do you agree with that? Basically, you don't need to write #[zeroable(perfect)] - you don't need that. If there is #[zeroable(bounds="...")] we are opt-ining for perfect derive.