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 trivial is_bit_valid when possible #1303

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joshlf
Copy link
Member

@joshlf joshlf commented May 19, 2024

When deriving FromBytes on a type with no generic parameters, the implied TryFromBytes derive's is_bit_valid impl is generated as always returning true. This is faster to codegen, is faster to compile, and is friendlier on the optimizer.

Makes progress on #5

@joshlf joshlf mentioned this pull request May 19, 2024
35 tasks
@joshlf joshlf force-pushed the try-from-bytes-from-bytes branch from c4af213 to 5e1948a Compare May 19, 2024 02:39
jswrenn
jswrenn previously approved these changes May 19, 2024
@joshlf joshlf force-pushed the try-from-bytes-from-bytes branch from 5e1948a to e80cb64 Compare May 19, 2024 03:11
@joshlf joshlf marked this pull request as ready for review May 19, 2024 03:11
@joshlf joshlf enabled auto-merge May 19, 2024 03:11
@joshlf joshlf requested a review from jswrenn May 19, 2024 03:11
@joshlf joshlf disabled auto-merge May 19, 2024 03:19
zerocopy-derive/src/lib.rs Outdated Show resolved Hide resolved
@joshlf joshlf force-pushed the try-from-bytes-from-bytes branch from e80cb64 to c0d052c Compare May 19, 2024 05:05
@joshlf joshlf enabled auto-merge May 19, 2024 05:06
@joshlf joshlf dismissed jswrenn’s stale review May 19, 2024 05:06

Discussed offline: previous version was unsound; requesting a re-review for the fix

@joshlf joshlf force-pushed the try-from-bytes-from-bytes branch from c0d052c to d14d132 Compare May 19, 2024 05:12
When deriving `FromBytes` on a type with no generic parameters, the
implied `TryFromBytes` derive's `is_bit_valid` impl is generated as
always returning `true`. This is faster to codegen, is faster to
compile, and is friendlier on the optimizer.

Makes progress on #5
@joshlf joshlf force-pushed the try-from-bytes-from-bytes branch from d14d132 to b8eeb75 Compare May 19, 2024 05:23
@joshlf joshlf disabled auto-merge May 19, 2024 05:23
@joshlf
Copy link
Member Author

joshlf commented May 19, 2024

@jswrenn Take a look at the .stderr files. The extra Self: FromBytes check added in is_bit_valid results in significantly more verbose error messages. It's still obvious what's happening, so it's not a huge deal. I could also see replacing this with one of the following:

  • Convince ourselves that there's absolutely no way for compilation to succeed if FromBytes isn't satisfied (risky - what about future language changes a la trivial bounds?)
  • In the #[derive(FromBytes)] case, do impl TryFromBytes for Type where Type: FromBytes; maybe this will generate better error messages? Significantly harder to wire through our existing codebase, though
  • In all cases of #[derive(Subtrait)], do impl Trait for Type where Type: Subtrait (so that we don't have to special-case this); now all code can more soundly rely on the top-level trait actually being satisfied. Probably still more verbose errors than currently, but maybe not as bad as with the current solution in this PR.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.84%. Comparing base (3aef801) to head (b8eeb75).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1303   +/-   ##
=======================================
  Coverage   87.84%   87.84%           
=======================================
  Files          15       15           
  Lines        5191     5191           
=======================================
  Hits         4560     4560           
  Misses        631      631           

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

@joshlf joshlf changed the title Derive trivial is_bit_valid when possible Derive trivial is_bit_valid when possible May 19, 2024
@jswrenn
Copy link
Collaborator

jswrenn commented May 21, 2024

I expected the expansion for this:

#[derive(FromBytes)]
struct Foo {
    a: u8,
    b: u16,
}

...to look like this:

unsafe impl ::zerocopy::TryFromBytes for Foo
where
    u8: ::zerocopy::FromBytes,
    u16: ::zerocopy::FromBytes,
{
    fn only_derive_is_allowed_to_implement_this_trait() {}
    fn is_bit_valid<A>(candidate: ::zerocopy::Maybe<Self, A>) -> bool
    where
        A: ::zerocopy::pointer::invariant::Aliasing
            + ::zerocopy::pointer::invariant::AtLeast<
                ::zerocopy::pointer::invariant::Shared,
            >,
    {
        true
    }
}

Why doesn't it? Am I missing some subtlety here?

@joshlf
Copy link
Member Author

joshlf commented May 21, 2024

It's unnecessary. If the top-level trait is FromBytes, then we already know that a trivial is_bit_valid impl is sound because if it wasn't, the FromBytes derive itself would fail. The bound added inside the body of is_bit_valid is just a hedge in case that reasoning has a hole.

I could also see replacing it with impl TryFromBytes for Foo where Foo: FromBytes. However, that has the downside of being non-local in terms of the control flow inside the derive code. The current approach has the advantage that you can reason about the soundness of try_gen_trivial_is_bit_valid entirely by looking at its body.

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.

None yet

3 participants