rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.24k stars 12.71k forks source link

Poor diagnostic combining #[derive(Debug)] with #[repr(C, packed)] struct #110777

Open jrose-signal opened 1 year ago

jrose-signal commented 1 year ago

Code

#[derive(Debug)]
pub(crate) struct UInt16LE {
    bytes: [u8; 2],
}

#[derive(Debug)]
#[repr(C, packed)]
pub(crate) struct Header {
    version: u8,
    kind: UInt16LE,
}

Current output

error[E0507]: cannot move out of `self.kind` which is behind a shared reference
  --> src/lib.rs:11:5
   |
7  | #[derive(Debug)]
   |          ----- in this derive macro expansion
...
11 |     kind: UInt16LE,
   |     ^^^^^^^^^^^^^^ move occurs because `self.kind` has type `UInt16LE`, which does not implement the `Copy` trait
   |
   = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

Desired output

"cannot form reference to `self.kind` because struct `Header` is `repr(packed)`"
"note: `self.kind` has type `UInt16LE`, which does not implement the `Copy` trait"

Rationale and extra context

I thought this was a regression in 1.69, and it sort of was, but presumably the old code was incorrect, since you can't form a reference to a field in a repr(packed) struct. Nothing in the diagnostic pointed me to that, though, so I had to figure out why this struct was different.

Other cases

No response

Anything else?

No response

Lokathor commented 1 year ago

This diagnostic is poor, but I'd like to point out that packed is pointless in the Header struct and you'd have a better time without it.

jrose-signal commented 1 year ago

This is stripped down from a full example, but you're probably right…it would be enough to use repr(C) and static-assert that the alignment is 1.

RalfJung commented 1 year ago

Yeah our diagnostics regressed when we fixed https://github.com/rust-lang/rust/issues/27060, good point. Not sure how we usually handle special diagnostics for derive -- do we really want code littered all over the compiler that special-cases derive? I don't think we can emit this diagnostic in the derive code itself, but it would still be valuable for compiler maintenance to keep this clean, somehow.

RalfJung commented 1 year ago

Also FWIW, the intended fix is to declare UInt16LE as Copy.

The derive macro has to decide whether to take a reference or a copy without having any type information. And for a packed struct, taking a copy seems like the more promising strategy in general, so that's what we do.

Noratrieb commented 1 year ago

Maybe we could add a PackedDeriveField helper trait (with a T: Copy blanket impl) and add Field: PackedDeriveField trait bounds on the impls. Then, throw a rustc_on_unimplemented on that trait and hope the diagnostics machinery picks it all up.