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

derive(FromBytes) should implicitly derive FromZeros and TryFromBytes #925

Closed
Tracked by #671
jswrenn opened this issue Feb 22, 2024 · 0 comments · Fixed by #926
Closed
Tracked by #671

derive(FromBytes) should implicitly derive FromZeros and TryFromBytes #925

jswrenn opened this issue Feb 22, 2024 · 0 comments · Fixed by #926

Comments

@jswrenn
Copy link
Collaborator

jswrenn commented Feb 22, 2024

Should deriving FromBytes imply deriving FromZeros and TryFromBytes? Either possible decision on this question has distinct advantages, disadvantages, and precedence. After reviewing the below design document and soliciting feedback from key customers, we've tentatively concluded yes.

Background

Zerocopy defines the following trait hierarchy:

unsafe trait TryFromBytes { ... }
unsafe trait FromZeros: TryFromBytes { ... }
unsafe trait FromBytes: FromZeros { ... }

Implementing these traits manually is an explicitly unsupported workflow; they must be derived.

Question

To derive FromBytes, should a user write:

  • ...this:
    #[derive(TryFromBytes, FromZeros, FromBytes)]
    struct Foo {
        ...
    }
  • ...or this:
    #[derive(FromBytes)]
    struct Foo {
        ...
    }

That is, should deriving a zerocopy trait implicitly derive its super traits?

Evaluation

Pedagogy

How does this decision impact our documentation? The zerocopy documentation makes heavy use of examples to demonstrate its constructs.

Option 1

Under Option 1, our examples must include derives for the full set of super traits, even when those traits are not germane to the example.

This has the unfortunate effect of making simple examples appear more complicated than they otherwise would be. However, the repeated exposure to the trait hierarchy this provides may reinforce the hierarchy in users' minds.

Option 2

Under Option 2, our examples need only include derives for traits germane to the example. This simplifies examples, at the expense of reducing the visibility of zerocopy's trait hierarchy.

Pathological User Experience

How does this decision impact our users? We'll consider the stories of Asher and Willow. Asher is using a version of zerocopy that does not imply the derives, but their user experience would likely be smoother if it did. Willow is using a version of zerocopy that does imply the derives, but their user experience would likely be smoother if it did not.

Option 1

Asher needs to call an API that requires its parameter implement FromBytes. Dutifully, they derive the trait:

#[derive(FromBytes)]
struct Foo {
    ...
}

...and compile. They receive the error message:

error[E0277]: the trait bound `Foo: FromZeroes` is not satisfied
    --> src/lib.rs:3:10
     |
3    | #[derive(FromBytes)]
     |          ^^^^^^^^^ the trait `FromZeroes` is not implemented for `Foo`
     |
     = help: the following other types implement trait `FromZeroes`:
               bool
               char
               isize
               i8
               i16
               i32
               i64
               i128
             and 67 others
note: required by a bound in `zerocopy::FromBytes`
    --> /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zerocopy-0.7.32/src/lib.rs:1803:29
     |
1803 | pub unsafe trait FromBytes: FromZeroes {
     |                             ^^^^^^^^^^ required by this bound in `FromBytes`
     = note: this error originates in the derive macro `FromBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.

...and infer that they need to also implement FromZeros. So they add that to the derive(...):

#[derive(FromZeros, FromBytes)]
struct Foo {
    ...
}

...and compile. They receive the error message:

error[E0277]: the trait bound `Foo: TryFromBytes` is not satisfied
    --> src/lib.rs:3:10
     |
3    | #[derive(FromZeros, FromBytes)]
     |          ^^^^^^^^^ the trait `TryFromBytes` is not implemented for `Foo`
     |
     = help: the following other types implement trait `TryFromBytes`:
               bool
               char
               isize
               i8
               i16
               i32
               i64
               i128
             and 67 others
note: required by a bound in `zerocopy::FromZeros`
    --> /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zerocopy-0.7.32/src/lib.rs:1803:29
     |
1803 | pub unsafe trait FromZeros: TryFromBytes {
     |                             ^^^^^^^^^^^^ required by this bound in `FromZeros`
     = note: this error originates in the derive macro `FromBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.

...and infer that they need to also implement TryFromBytes. So they add that to the derive(...):

#[derive(TryFromBytes, FromZeros, FromBytes)]
struct Foo {
    ...
}

...and compile. This compilation succeeds without errors.

Option 2

Willow has already derived FromBytes on their type:

#[derive(FromBytes)]
struct Foo {
    ...
}

They want to pass their Foo to a function that requires a FromZeros bound. They add the function call, but before trying to compile (which would succeed), they infer that they also need to derive FromZeros:

#[derive(FromZeros, FromBytes)]
struct Foo {
    ...
}

When they compile, they encounter an error message:

error[E0119]: conflicting implementations of trait `zerocopy::FromZeros` for type `Foo`
 --> src/lib.rs:3:22
  |
3 | #[derive(FromZeros, FromBytes)]
  |          ---------  ^^^^^^^^^ conflicting implementation for `Foo`
  |          |
  |          first implementation here
  |
  = note: this error originates in the derive macro `FromZeros` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0119`.

They delete their added derive of FromZeros and then compile without errors.

Precedent

What precedent does either option have that might impact the expectations of our users?

Option 1

In the Rust standard libary, there is no direct analogy for the situation we are considering. At first glance, the Copy: Clone hierarchy in the Rust seems analogous. If Rust could have #[derive(Copy)] imply #[derive(Copy, Clone)], then why doesn't it?

The premise, unfortunately, does not hold. If deriving Copy implied deriving Clone, then Clone could not be manually implementable:

#[derive(Clone)]
struct Foo {
    ...
}

impl Clone for Foo {
    fn clone(&self) -> Self { 
        todo!("a bespoke clone implementation")
    }
}

Compiling this would (in our hypothetical alternate derive(Copy)) produce an error:

error[E0119]: conflicting implementations of trait `Clone` for type `Foo`
 --> src/lib.rs:1:10
  |
1 | #[derive(Copy)]
  |          ^^^^ conflicting implementation for `Foo`
...
6 | impl Clone for Foo {
  | ------------------ first implementation here
  |
  = note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0119`.

An RFC proposing exactly this sort of mechanism for standard library traits was postponed because of exactly these sorts of backwards compatibility challenges.

A difference between this and our scenario is that our traits are not manually implementable; the backwards compatibility issues do not exist.

Nonetheless, it is reasonable to consider that end-users potentially have an expectation that traits must always be explicitly derived (which could lead to confusion).

Option 2

Unstable Rust

The StructuralEq marker indicates that constants of a type are suitable for use in patterns. It cannot be manually implemented; rather it is implied by #[derive(Eq)].

There are a few differences between this example and ours:

  • Eq is not a formal supertrait of StructuralEq
  • StructuralEq trait is not intended to ever be stabilized.
Third-Party Crates
Diesel

The Diesel ORM makes frequent use of this pattern. Nearly every derive it provides also derives other related traits. In some cases, these are super-traits. For instance, derive(Identifiable) implies a derivation of HasTable.

Bytemuck

Bytemuck's derives imply their supertraits. For example, derive(AnyBitPattern) implies derive(Zeroable):

use bytemuck::{AnyBitPattern, Zeroable};

#[derive(AnyBitPattern, Copy, Clone)]
#[repr(C)]
struct Foo;

fn is_also_zeroable() -> impl Zeroable {
    Foo
}
Implied Traits at Use-Sites

Rust provides a similar ergonomic device at trait-use sites: Bringing a trait into scope implicitly brings the methods of its supertraits into scope. For example:

mod upstream {
    pub trait Super {
        fn sup(&self) {}
    }
    
    pub trait Sub: Super {}
    
    impl<T> Super for T {}
    impl<T> Sub for T {}
}

fn test<T>(t: T)
where
    T: upstream::Sub
{
    t.sup();
}

This compiles successfully.

Conclusion

Option 2 offers a tighter development cycle in its pathological case than Option 1, and has precedent in the crates ecosystem.

@joshlf joshlf mentioned this issue Feb 22, 2024
87 tasks
joshlf added a commit that referenced this issue Feb 23, 2024
`#[derive(FromBytes)]` implies `#[derive(FromZeros)]`, which implies
`#[derive(TryFromBytes)]`.

Closes #925
joshlf added a commit that referenced this issue Feb 23, 2024
`#[derive(FromBytes)]` implies `#[derive(FromZeros)]`, which implies
`#[derive(TryFromBytes)]`.

Closes #925
github-merge-queue bot pushed a commit that referenced this issue Feb 23, 2024
`#[derive(FromBytes)]` implies `#[derive(FromZeros)]`, which implies
`#[derive(TryFromBytes)]`.

Closes #925
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 a pull request may close this issue.

1 participant