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] Improve IntoBytes error messages #1712

Merged
merged 1 commit into from
Sep 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3837,25 +3837,26 @@ fn mut_from_prefix_suffix<T: FromBytes + KnownLayout + ?Sized>(
///
/// # Error Messages
///
/// Due to the way that the custom derive for `IntoBytes` is implemented, you
/// may get an error like this:
/// On Rust toolchains prior to 1.78.0, due to the way that the custom derive
/// for `IntoBytes` is implemented, you may get an error like this:
///
/// ```text
/// error[E0277]: the trait bound `HasPadding<Foo, true>: ShouldBe<false>` is not satisfied
/// error[E0277]: the trait bound `(): PaddingFree<Foo, true>` is not satisfied
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fallback is arguably an improvement over the previous error!

/// --> lib.rs:23:10
/// |
/// 1 | #[derive(IntoBytes)]
/// | ^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<Foo, true>`
/// | ^^^^^^^^^ the trait `PaddingFree<Foo, true>` is not implemented for `()`
/// |
/// = help: the trait `ShouldBe<VALUE>` is implemented for `HasPadding<T, VALUE>`
/// = help: the following implementations were found:
/// <() as PaddingFree<T, false>>
/// ```
///
/// This error indicates that the type being annotated has padding bytes, which
/// is illegal for `IntoBytes` types. Consider reducing the alignment of some
/// fields by using types in the [`byteorder`] module, adding explicit struct
/// fields where those padding bytes would be, or using `#[repr(packed)]`. See
/// the Rust Reference's page on [type layout] for more information about type
/// layout and padding.
/// fields by using types in the [`byteorder`] module, wrapping field types in
/// [`Unalign`], adding explicit struct fields where those padding bytes would
/// be, or using `#[repr(packed)]`. See the Rust Reference's page on [type
/// layout] for more information about type layout and padding.
///
/// [type layout]: https://doc.rust-lang.org/reference/type-layout.html
///
Expand Down
24 changes: 13 additions & 11 deletions src/util/macro_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@

#![allow(missing_debug_implementations)]

use core::{
marker::PhantomData,
mem::{self, ManuallyDrop},
};
use core::mem::{self, ManuallyDrop};

// TODO(#29), TODO(https://github.com/rust-lang/rust/issues/69835): Remove this
// `cfg` when `size_of_val_raw` is stabilized.
Expand All @@ -35,13 +32,18 @@ use crate::{
Immutable, IntoBytes, Ptr, TryFromBytes, Unalign, ValidityError,
};

/// A compile-time check that should be one particular value.
pub trait ShouldBe<const VALUE: bool> {}

/// A struct for checking whether `T` contains padding.
pub struct HasPadding<T: ?Sized, const VALUE: bool>(PhantomData<T>);

impl<T: ?Sized, const VALUE: bool> ShouldBe<VALUE> for HasPadding<T, VALUE> {}
#[cfg_attr(
zerocopy_diagnostic_on_unimplemented,
diagnostic::on_unimplemented(
message = "`{T}` has inter-field padding",
label = "types with padding cannot implement `IntoBytes`",
note = "consider using `zerocopy::Unalign` to lower the alignment of individual fields",
note = "consider adding explicit fields where padding would be",
note = "consider using `#[repr(packed)]` to remove inter-field padding"
)
)]
pub trait PaddingFree<T: ?Sized, const HAS_PADDING: bool> {}
impl<T: ?Sized> PaddingFree<T, false> for () {}

/// A type whose size is equal to `align_of::<T>()`.
#[repr(C)]
Expand Down
2 changes: 1 addition & 1 deletion zerocopy-derive/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ proc-macro = true
[dependencies]
proc-macro2 = "1.0.1"
quote = "1.0.10"
syn = "2.0.46"
syn = { version = "2.0.46", features = ["full"] }
Copy link
Member Author

Choose a reason for hiding this comment

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

@djkoloski Do you have a sense of how we could implement using the same trick that you pulled to avoid the full feature?

I tested locally on a MacBook Pro on low battery mode, and found that the full feature increased zerocopy-derive cargo build time from 8.9s to 13.3s. That's a large percentage, but a small absolute difference. So I'm not opposed to just sticking with this impl, but it'd be cool if we could avoid it.


[dev-dependencies]
dissimilar = "1.0.9"
Expand Down
19 changes: 3 additions & 16 deletions zerocopy-derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ mod repr;

use proc_macro2::{TokenStream, TokenTree};
use quote::ToTokens;
use syn::{punctuated::Punctuated, token::Colon, PredicateType};

use {
proc_macro2::Span,
Expand Down Expand Up @@ -1222,27 +1221,15 @@ fn impl_block<D: DataExt>(
let validator_context = check.validator_macro_context();
let validator_macro = check.validator_macro_ident();
let t = tag.iter();
// We have to manually construct a bound here because the validator
// context may include type definitions and syn cannot parse type
// definitions in const exprs without the `full` feature enabled.
// Constructing these bounds "verbatim" gets around this issue.
let bounded_ty = Type::Verbatim(quote! {
::zerocopy::util::macro_util::HasPadding<
parse_quote! {
(): ::zerocopy::util::macro_util::PaddingFree<
#type_ident,
{
#validator_context
::zerocopy::#validator_macro!(#type_ident, #(#t,)* #(#variant_types),*)
}
>
});
let mut bounds = Punctuated::new();
bounds.push(parse_quote! { ::zerocopy::util::macro_util::ShouldBe<false> });
WherePredicate::Type(PredicateType {
lifetimes: None,
bounded_ty,
colon_token: Colon::default(),
bounds,
})
}
});

let self_bounds: Option<WherePredicate> = match self_type_trait_bounds {
Expand Down
23 changes: 23 additions & 0 deletions zerocopy-derive/src/output_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,29 @@ fn test_into_bytes() {
}
} no_build
}

test! {
IntoBytes {
#[repr(C)]
struct Foo {
a: u8,
b: u8,
}
} expands to {
#[allow(deprecated)]
unsafe impl ::zerocopy::IntoBytes for Foo
where
u8: ::zerocopy::IntoBytes,
u8: ::zerocopy::IntoBytes,
(): ::zerocopy::util::macro_util::PaddingFree<
Foo,
{ ::zerocopy::struct_has_padding!(Foo, [u8, u8]) },
>,
{
fn only_derive_is_allowed_to_implement_this_trait() {}
}
} no_build
}
}

#[test]
Expand Down
18 changes: 9 additions & 9 deletions zerocopy-derive/tests/ui-msrv/enum.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -343,35 +343,35 @@ error[E0277]: the trait bound `bool: FromBytes` is not satisfied
= help: see issue #48214
= note: this error originates in the derive macro `FromBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `HasPadding<IntoBytes1, true>: ShouldBe<false>` is not satisfied
error[E0277]: the trait bound `(): PaddingFree<IntoBytes1, true>` is not satisfied
--> tests/ui-msrv/enum.rs:533:10
|
533 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes1, true>`
| ^^^^^^^^^ the trait `PaddingFree<IntoBytes1, true>` is not implemented for `()`
|
= help: the following implementations were found:
<HasPadding<T, VALUE> as ShouldBe<VALUE>>
<() as PaddingFree<T, false>>
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `HasPadding<IntoBytes2, true>: ShouldBe<false>` is not satisfied
error[E0277]: the trait bound `(): PaddingFree<IntoBytes2, true>` is not satisfied
--> tests/ui-msrv/enum.rs:544:10
|
544 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes2, true>`
| ^^^^^^^^^ the trait `PaddingFree<IntoBytes2, true>` is not implemented for `()`
|
= help: the following implementations were found:
<HasPadding<T, VALUE> as ShouldBe<VALUE>>
<() as PaddingFree<T, false>>
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `HasPadding<IntoBytes3, true>: ShouldBe<false>` is not satisfied
error[E0277]: the trait bound `(): PaddingFree<IntoBytes3, true>` is not satisfied
--> tests/ui-msrv/enum.rs:550:10
|
550 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes3, true>`
| ^^^^^^^^^ the trait `PaddingFree<IntoBytes3, true>` is not implemented for `()`
|
= help: the following implementations were found:
<HasPadding<T, VALUE> as ShouldBe<VALUE>>
<() as PaddingFree<T, false>>
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
12 changes: 6 additions & 6 deletions zerocopy-derive/tests/ui-msrv/struct.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -110,25 +110,25 @@ error[E0277]: the trait bound `AU16: Unaligned` is not satisfied
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `HasPadding<IntoBytes2, true>: ShouldBe<false>` is not satisfied
error[E0277]: the trait bound `(): PaddingFree<IntoBytes2, true>` is not satisfied
--> tests/ui-msrv/struct.rs:107:10
|
107 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes2, true>`
| ^^^^^^^^^ the trait `PaddingFree<IntoBytes2, true>` is not implemented for `()`
|
= help: the following implementations were found:
<HasPadding<T, VALUE> as ShouldBe<VALUE>>
<() as PaddingFree<T, false>>
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `HasPadding<IntoBytes3, true>: ShouldBe<false>` is not satisfied
error[E0277]: the trait bound `(): PaddingFree<IntoBytes3, true>` is not satisfied
--> tests/ui-msrv/struct.rs:114:10
|
114 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes3, true>`
| ^^^^^^^^^ the trait `PaddingFree<IntoBytes3, true>` is not implemented for `()`
|
= help: the following implementations were found:
<HasPadding<T, VALUE> as ShouldBe<VALUE>>
<() as PaddingFree<T, false>>
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

Expand Down
6 changes: 3 additions & 3 deletions zerocopy-derive/tests/ui-msrv/union.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@ error[E0277]: the trait bound `UnsafeCell<()>: zerocopy::Immutable` is not satis
= help: see issue #48214
= note: this error originates in the derive macro `Immutable` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `HasPadding<IntoBytes2, true>: ShouldBe<false>` is not satisfied
error[E0277]: the trait bound `(): PaddingFree<IntoBytes2, true>` is not satisfied
--> tests/ui-msrv/union.rs:39:10
|
39 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes2, true>`
| ^^^^^^^^^ the trait `PaddingFree<IntoBytes2, true>` is not implemented for `()`
|
= help: the following implementations were found:
<HasPadding<T, VALUE> as ShouldBe<VALUE>>
<() as PaddingFree<T, false>>
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
30 changes: 21 additions & 9 deletions zerocopy-derive/tests/ui-nightly/enum.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -413,41 +413,53 @@ help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
9 + #![feature(trivial_bounds)]
|

error[E0277]: the trait bound `HasPadding<IntoBytes1, true>: ShouldBe<false>` is not satisfied
error[E0277]: `IntoBytes1` has inter-field padding
--> tests/ui-nightly/enum.rs:533:10
|
533 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes1, true>`
| ^^^^^^^^^ types with padding cannot implement `IntoBytes`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is ridiculously good. I wonder if we could even say how many bytes of inter-field padding there with const generics, but that's just yak shaving.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #1717 to track

|
= help: the trait `ShouldBe<true>` is implemented for `HasPadding<IntoBytes1, true>`
= help: the trait `PaddingFree<IntoBytes1, true>` is not implemented for `()`
= note: consider using `zerocopy::Unalign` to lower the alignment of individual fields
= note: consider adding explicit fields where padding would be
= note: consider using `#[repr(packed)]` to remove inter-field padding
= help: the trait `PaddingFree<IntoBytes1, false>` is implemented for `()`
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
|
9 + #![feature(trivial_bounds)]
|

error[E0277]: the trait bound `HasPadding<IntoBytes2, true>: ShouldBe<false>` is not satisfied
error[E0277]: `IntoBytes2` has inter-field padding
--> tests/ui-nightly/enum.rs:544:10
|
544 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes2, true>`
| ^^^^^^^^^ types with padding cannot implement `IntoBytes`
|
= help: the trait `ShouldBe<true>` is implemented for `HasPadding<IntoBytes2, true>`
= help: the trait `PaddingFree<IntoBytes2, true>` is not implemented for `()`
= note: consider using `zerocopy::Unalign` to lower the alignment of individual fields
= note: consider adding explicit fields where padding would be
= note: consider using `#[repr(packed)]` to remove inter-field padding
= help: the trait `PaddingFree<IntoBytes2, false>` is implemented for `()`
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
|
9 + #![feature(trivial_bounds)]
|

error[E0277]: the trait bound `HasPadding<IntoBytes3, true>: ShouldBe<false>` is not satisfied
error[E0277]: `IntoBytes3` has inter-field padding
--> tests/ui-nightly/enum.rs:550:10
|
550 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes3, true>`
| ^^^^^^^^^ types with padding cannot implement `IntoBytes`
|
= help: the trait `ShouldBe<true>` is implemented for `HasPadding<IntoBytes3, true>`
= help: the trait `PaddingFree<IntoBytes3, true>` is not implemented for `()`
= note: consider using `zerocopy::Unalign` to lower the alignment of individual fields
= note: consider adding explicit fields where padding would be
= note: consider using `#[repr(packed)]` to remove inter-field padding
= help: the trait `PaddingFree<IntoBytes3, false>` is implemented for `()`
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
Expand Down
20 changes: 14 additions & 6 deletions zerocopy-derive/tests/ui-nightly/struct.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -241,27 +241,35 @@ help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
9 + #![feature(trivial_bounds)]
|

error[E0277]: the trait bound `HasPadding<IntoBytes2, true>: ShouldBe<false>` is not satisfied
error[E0277]: `IntoBytes2` has inter-field padding
--> tests/ui-nightly/struct.rs:107:10
|
107 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes2, true>`
| ^^^^^^^^^ types with padding cannot implement `IntoBytes`
|
= help: the trait `ShouldBe<true>` is implemented for `HasPadding<IntoBytes2, true>`
= help: the trait `PaddingFree<IntoBytes2, true>` is not implemented for `()`
= note: consider using `zerocopy::Unalign` to lower the alignment of individual fields
= note: consider adding explicit fields where padding would be
= note: consider using `#[repr(packed)]` to remove inter-field padding
= help: the trait `PaddingFree<IntoBytes2, false>` is implemented for `()`
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
|
9 + #![feature(trivial_bounds)]
|

error[E0277]: the trait bound `HasPadding<IntoBytes3, true>: ShouldBe<false>` is not satisfied
error[E0277]: `IntoBytes3` has inter-field padding
--> tests/ui-nightly/struct.rs:114:10
|
114 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes3, true>`
| ^^^^^^^^^ types with padding cannot implement `IntoBytes`
|
= help: the trait `ShouldBe<true>` is implemented for `HasPadding<IntoBytes3, true>`
= help: the trait `PaddingFree<IntoBytes3, true>` is not implemented for `()`
= note: consider using `zerocopy::Unalign` to lower the alignment of individual fields
= note: consider adding explicit fields where padding would be
= note: consider using `#[repr(packed)]` to remove inter-field padding
= help: the trait `PaddingFree<IntoBytes3, false>` is implemented for `()`
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
Expand Down
10 changes: 7 additions & 3 deletions zerocopy-derive/tests/ui-nightly/union.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,17 @@ help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
9 + #![feature(trivial_bounds)]
|

error[E0277]: the trait bound `HasPadding<IntoBytes2, true>: ShouldBe<false>` is not satisfied
error[E0277]: `IntoBytes2` has inter-field padding
--> tests/ui-nightly/union.rs:39:10
|
39 | #[derive(IntoBytes)]
| ^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<IntoBytes2, true>`
| ^^^^^^^^^ types with padding cannot implement `IntoBytes`
|
= help: the trait `ShouldBe<true>` is implemented for `HasPadding<IntoBytes2, true>`
= help: the trait `PaddingFree<IntoBytes2, true>` is not implemented for `()`
= note: consider using `zerocopy::Unalign` to lower the alignment of individual fields
= note: consider adding explicit fields where padding would be
= note: consider using `#[repr(packed)]` to remove inter-field padding
= help: the trait `PaddingFree<IntoBytes2, false>` is implemented for `()`
= help: see issue #48214
= note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
help: add `#![feature(trivial_bounds)]` to the crate attributes to enable
Expand Down
Loading
Loading