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

Standardize whether T: KnownLayout requires T's fields to be KnownLayout #1162

Closed
3 tasks
Tracked by #29
joshlf opened this issue May 2, 2024 · 2 comments · Fixed by #1476
Closed
3 tasks
Tracked by #29

Standardize whether T: KnownLayout requires T's fields to be KnownLayout #1162

joshlf opened this issue May 2, 2024 · 2 comments · Fixed by #1476
Assignees
Labels
blocking-next-release This issue should be resolved before we release on crates.io

Comments

@joshlf
Copy link
Member

joshlf commented May 2, 2024

Progress

  • Restrict KnownLayout for Unalign<T> to require T: KnownLayout (add a test to confirm this behavior doesn't regress)
    • Record the fact that the reason for doing it this way (rather than always implementing it manually - even when feature = "derive" - without a T: KnownLayout bound) is to be forwards-compatible with supporting unsized Unalign (#209)
  • Teach impl_or_verify! to support KnownLayout - the Unalign bounds issue wasn't caught because, for KnownLayout, we use impl_known_layout! rather than impl_or_verify!
  • [Possibly after 0.8] Relax so that T: KnownLayout never requires its fields to be KnownLayout

Details

Relates to #29.

Consider the following code:

#[derive(KnownLayout]
#[repr(C)]
pub struct Foo<T>(T);

Since T has an implicit T: Sized bound, our emitted derive should not require that T: KnownLayout, but it does. This is currently affecting Unalign, resulting in it having a different bound when compiling with feature = "derive" (namely, T: KnownLayout) than when compiling without feature = "derive".

@jswrenn
Copy link
Collaborator

jswrenn commented May 9, 2024

In discussion with @joshlf, we decided to make the KnownLayout requirement recursive. Our reasoning for this is that all of our other traits have recursive requirements, so users are unlikely to find themselves in a situation where the recursive requirement of, say, FromBytes can be satisfied but KnownLayout cannot.

@jswrenn jswrenn self-assigned this May 9, 2024
@joshlf joshlf changed the title KnownLayout requires T: KnownLayout unnecessarily Standardize whether T: KnownLayout requires T's fields to be KnownLayout May 30, 2024
@joshlf joshlf added the blocking-next-release This issue should be resolved before we release on crates.io label May 30, 2024
@joshlf
Copy link
Member Author

joshlf commented Jul 1, 2024

In a discussion with @jswrenn and @djkoloski, we decided:

  • Based on this comment, we won't have any use for T: KnownLayout to require T's fields to be KnownLayout
  • It is unlikely that we will want to support unsized Unalign
  • So long as Unalign is always Sized, we can unconditionally manually implement KnownLayout for Unalign<T> even when T: ?KnownLayout, and not conditional on feature = "derive"

jswrenn added a commit that referenced this issue Jul 1, 2024
We should not use `derive(KnownLayout)` on `Unalign`, because the
derive is not smart enough to realize that `Unalign<T>` is always
sized and thus emits a `KnownLayout` impl bounded on
`T: KnownLayout.` This is overly restrictive.

Fixes #1162
jswrenn added a commit that referenced this issue Jul 1, 2024
We should not use `derive(KnownLayout)` on `Unalign`, because the
derive is not smart enough to realize that `Unalign<T>` is always
sized and thus emits a `KnownLayout` impl bounded on
`T: KnownLayout.` This is overly restrictive.

Fixes #1162
github-merge-queue bot pushed a commit that referenced this issue Jul 1, 2024
We should not use `derive(KnownLayout)` on `Unalign`, because the
derive is not smart enough to realize that `Unalign<T>` is always
sized and thus emits a `KnownLayout` impl bounded on
`T: KnownLayout.` This is overly restrictive.

Fixes #1162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking-next-release This issue should be resolved before we release on crates.io
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants