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

Use #[diagnostic::on_unimplemented] to improve padding error messages #1696

Closed
Tracked by #671
joshlf opened this issue Sep 20, 2024 · 1 comment
Closed
Tracked by #671
Labels
blocking-next-release-publicization experience-hard This issue is hard, and requires a lot of experience help wanted Extra attention is needed

Comments

@joshlf
Copy link
Member

joshlf commented Sep 20, 2024

Currently, the error message emitted when a type has padding (but isn't supposed to) is quite bad:

zerocopy/src/lib.rs

Lines 3783 to 3805 in 782c12f

/// # Error Messages
///
/// 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
/// --> lib.rs:23:10
/// |
/// 1 | #[derive(IntoBytes)]
/// | ^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<Foo, true>`
/// |
/// = help: the trait `ShouldBe<VALUE>` is implemented for `HasPadding<T, VALUE>`
/// ```
///
/// 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.
///
/// [type layout]: https://doc.rust-lang.org/reference/type-layout.html

Using #[diagnostic::on_unimplemented], we should be able to significantly improve this error message.

However, it'd be ideal if we changed the way we do enforcement. Currently, the type in question shows up as an argument in the HasPadding type. In order to get the best ergonomics, we need to the type to be in a position where #[diagnostic::on_unimplemented] has access to its name. In particular, that means that it either needs to show up as an argument to the trait that isn't implemented ({T} in the attribute's message, label, or note) or as the type that the trait isn't implemented for ({Self} in the attribute's message, label, or note).

@joshlf joshlf added help wanted Extra attention is needed experience-hard This issue is hard, and requires a lot of experience blocking-next-release-publicization labels Sep 20, 2024
@joshlf joshlf mentioned this issue Sep 20, 2024
87 tasks
@joshlf
Copy link
Member Author

joshlf commented Sep 22, 2024

Closed by #1712

@joshlf joshlf closed this as completed Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking-next-release-publicization experience-hard This issue is hard, and requires a lot of experience help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant