Erroneous warning about packed struct not implementing Copy

[ghost]

I tried this code:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7d186291bfe30222d01ed0af2fa28c4c

use serde::{Serialize, Deserialize};



#[repr(C)]
#[repr(packed)]
#[derive(Copy, Clone, Serialize, Deserialize, PartialEq, Debug)]
struct Packed {
    a: u8,
    b: u16,
    c: u8
}

fn main() {}

I expected to see this happen: Compile without any warning or error.

Instead, this happened: The following warning was emitted:

   Compiling playground v0.0.1 (/playground)
warning: `#[derive]` can't be used on a `#[repr(packed)]` struct that does not derive Copy (error E0133)
 --> src/main.rs:8:23
  |
8 | #[derive(Copy, Clone, Serialize, Deserialize, PartialEq, Debug)]
  |                       ^^^^^^^^^
  |
  = note: `#[warn(safe_packed_borrows)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>
  = note: this warning originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

    Finished dev [unoptimized + debuginfo] target(s) in 0.65s
     Running `target/debug/playground`

Meta

rustc --version --verbose:

<version>

Backtrace

<backtrace>

[ghost]

@RalfJung From your comment I thought this warning shouldn't happen when derive(Copy) is used.

Is this behaviour incorrect?

[RalfJung]

Hm, the error is certainly not good.

I suspect derive(Serialize) still creates unaligned references to packed fields internally even if the type is Copy. In other words, the macro just currently cannot support packed structs -- that would have to be fixed on the serde side, if it is possible at all.

Fixing the error is a rustc bug, though.

[ghost]

Yeah there exists this serde bug: #1747

[ghost]

But wait, in this issue #46043 in the issue description there is this part:

If your struct already derives Copy and has no generics, the compiler will generate code that copies the fields to locals and will therefore work. Otherwise, you'll have to write the derive implementations manually.

So is it supposed to be done for custom, non fundamental derives too (like serde's Serialize)? Because currently accourding to miri this doesn't happen for serde's Serialize.

Miri says:

error: Undefined Behavior: type validation failed: encountered an unaligned reference (required 2 byte alignment but found 1)

@RalfJung, @arielb1

[RalfJung]

So is it supposed to be done for custom, non fundamental derives too (like serde's Serialize)?

I think each derive needs to be adjusted to handle this... but I don't actually know and did not look at the code.

Because currently accourding to miri this doesn't happen for serde's Serialize.

Yeah, this shows that serde does not properly make a copy.

[ghost]

@RalfJung This is now fixed in serde. It is not released yet (published as 1.0.110.).

However I still think the error message is confusing. If the warning is generated because of incorrect use of references into packed fields in the derived code, then that is what the warning message should say instead of saying that the type doesn't derive Copy (when in fact it does).

[RalfJung]

Agreed, the rustc diagnostic should be improved. What it says is correct for the built-in derive but does not make sense for user-defined derive.

[estebank]

The original code now compiles, but we need a repro to evaluate the diagnostic.

[RalfJung]

@nnethercote did a bunch of derive work recently, not sure if it would have fixed this.

[nnethercote]

I did make changes to packed structs in #104429 but I don't think that will affect this example, which has derive(Copy).

The original error no longer occurs because of the serde fix. One way to get a reproducing test case would be to undo that change in serde. Or it might be reasonable just to close this issue given that the error message is no longer occurring in practice.

[nnethercote]

Actually, looks like #104429 did remove that error. So I think this can be closed.