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

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

Open
jrose-signal opened this issue Apr 24, 2023 · 5 comments
Open
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-repr-packed Area: the naughtiest repr T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jrose-signal
Copy link

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

@jrose-signal jrose-signal added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 24, 2023
@Lokathor
Copy link
Contributor

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
Copy link
Author

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
Copy link
Member

RalfJung commented Apr 24, 2023

Yeah our diagnostics regressed when we fixed #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
Copy link
Member

RalfJung commented Apr 24, 2023

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
Copy link
Member

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.

bors added a commit to rust-lang-ci/rust that referenced this issue Nov 7, 2023
…ompiler-errors

Emit explanatory note for move errors in packed struct derives

Derive expansions for packed structs with non-`Copy` fields cause move errors because they prefer copying over borrowing since borrowing the fields of a packed struct can result in unaligned access.

This underlying cause of the errors, however, is not apparent to the user. This PR adds a diagnostic note to make it clear to the user (the new note is on the second last line):

```
tests/ui/derives/deriving-with-repr-packed-move-errors.rs:13:16
   |
12 | #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Default)]
   |          ----- in this derive macro expansion
13 | struct StructA(String);
   |                ^^^^^^ move occurs because `self.0` has type `String`, which does not implement the `Copy` trait
   |
   = note: `#[derive(Debug)]` triggers a move because taking references to the fields of a packed struct is undefined behaviour
   = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)
```

Fixes rust-lang#117406

Partially addresses rust-lang#110777
@workingjubilee workingjubilee added the A-repr-packed Area: the naughtiest repr label Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-repr-packed Area: the naughtiest repr T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants