What should we do about #[derive(...)] on #[repr(packed)] structs?

[solson]

The derived trait instances for packed structs tend to take references to the packed struct fields, which can cause undefined behavior (unaligned loads) and will be detected as unsafe once #27060 is fixed. So, code using these derives on packed structs will stop compiling.

See also #39682 where I fixed impls in a Rust test and #39683 where I fixed an impl in rustc.

Should we:

1. Make derive detect #[repr(packed)] (can it?) and change the generated code.
2. Generate the code in a way that always works for packed structs (I somewhat doubt this is possible).
3. Do nothing. Let the users deal with manually implementing derivable traits for packed structs.
4. Do something I didn't think of.

cc @Aatch @bluss @retep998 @rust-lang/compiler

[retep998]

Option 1 would probably be ideal. Option 3 would require a crater run and I think a lot of stuff would fail that. Option 2 would be kinda inefficient for all non-packed structs. I don't know what option 4 even is.

[durka]

Can you give an example of the code that would be generated for a #[repr(packed)] struct under option (1)? I don't see why we couldn't do (2) by pretending every struct is packed, except that would probably be slower than necessary, confusing to the optimizer etc.

[solson]

The way to do (1) or (2) is to move every field into an aligned local and pass a reference to that local instead of a reference to the field. Unfortunately, this is only safe if all the fields are Copy.

It's maybe possible that it could work for non-Copy types by unsafely copying them into a local and mem::forgetting them before they drop...

[solson]

Those copies are potentially pretty bad implicit behavior if there is a really large field in the packed struct (say, a long array), which is an argument in favor of (3).

Another option is (3) plus a crates.io crate using custom derive for Clone/Debug/etc on packed structs.

[retep998]

Every packed struct in winapi manually implements Copy/Clone, so this won't break winapi 0.2.8 nor 0.3, so I'm okay with option 3. Just be sure to do crater runs and have a long warning period.

[nikomatsakis]

I think we should make it very easy to do derives on packed, copy structs. There is a bit of a performance footgun to copying into a local variable, but that won't apply to 99% of the structs -- it's a shame to make people write out the impls by hand just because of that.

I could imagine having some "flag" that we pass to derive (e.g., #[derive(PartialEq(copy)]) to explicitly request the behavior, but I think my real preference would be to pursue the idea of a lint that warns against undesirable implicit copies, described in #45683, which could then trigger and warn you when some big field is getting copied just to compare equality.

(Also, one great thing about the copy solution is that I suspect it greatly mitigates the backwards compat risk here.)

[MartinHusemann]

Option 1a: detect #[repr(packed)] and error out on #[derive(...)] from them not implementing manual Copy/Clone?
Is there actually a good stylistic reason to ever use that? Hiding the potential cost and making compiler magic just deal sounds dangerous to me.

[solson]

I'm going to close this in favor of #46043. There is already now some handling of this problem in rustc.