Skip to content

Improve error messages involving derive and packed. #99581

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Improve error messages involving derive and packed. #99581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions compiler/rustc_mir_transform/src/check_packed_ref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,16 @@ fn unsafe_derive_on_repr_packed(tcx: TyCtxt<'_>, def_id: LocalDefId) {
tcx.struct_span_lint_hir(UNALIGNED_REFERENCES, lint_hir_id, tcx.def_span(def_id), |lint| {
// FIXME: when we make this a hard error, this should have its
// own error code.
let message = if tcx.generics_of(def_id).own_requires_monomorphization() {
"`#[derive]` can't be used on a `#[repr(packed)]` struct with \
type or const parameters (error E0133)"
let extra = if tcx.generics_of(def_id).own_requires_monomorphization() {
"with type or const parameters"
} else {
"`#[derive]` can't be used on a `#[repr(packed)]` struct that \
does not derive Copy (error E0133)"
"that does not derive `Copy`"
};
let message = format!(
"`{}` can't be derived on this `#[repr(packed)]` struct {}",
tcx.item_name(tcx.trait_id_of_impl(def_id.to_def_id()).expect("derived trait name")),
extra
);
lint.build(message).emit();
});
}
Expand Down
38 changes: 26 additions & 12 deletions src/test/ui/derives/deriving-with-repr-packed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,43 @@
#![deny(unaligned_references)]

// check that derive on a packed struct with non-Copy fields
// correctly. This can't be made to work perfectly because
// we can't just use the field from the struct as it might
// not be aligned.
// Check that deriving certain builtin traits on certain packed structs cause
// errors. This happens when the derived trait would need to use a potentially
// misaligned reference. But there are two cases that are allowed:
// - If all the fields within the struct meet the required alignment: 1 for
// `repr(packed)`, or `N` for `repr(packed(N))`.
// - If `Default` is the only trait derived, because it doesn't involve any
// references.

#[derive(Copy, Clone, PartialEq, Eq)]
//~^ ERROR `#[derive]` can't be used
#[derive(Copy, Clone, Default, PartialEq, Eq)]
//~^ ERROR `Clone` can't be derived on this `#[repr(packed)]` struct with type or const parameters
//~| hard error
//~^^^ ERROR `#[derive]` can't be used
//~^^^ ERROR `PartialEq` can't be derived on this `#[repr(packed)]` struct with type or const parameters
//~| hard error
#[repr(packed)]
pub struct Foo<T>(T, T, T);

#[derive(PartialEq, Eq)]
//~^ ERROR `#[derive]` can't be used
#[derive(Default, Hash)]
//~^ ERROR `Hash` can't be derived on this `#[repr(packed)]` struct that does not derive `Copy`
//~| hard error
#[repr(packed)]
pub struct Bar(u32, u32, u32);

#[derive(PartialEq)]
// This one is fine because the field alignment is 1.
#[derive(Default, Hash)]
#[repr(packed)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we would modify unsafe_derive_on_repr_packed to

  • either return the lint for further changes or accept extra metadata to be able to point at the repr attribute as well
  • point at the struct where this happened
  • have a label on the primary span

Ideally, I would want something like the following:

error[E0133]: this trait can't be derived on this `#[repr(packed)]` struct with type or const parameters
  --> $DIR/deriving-with-repr-packed.rs:11:16
   |
LL | #[derive(Copy, Clone, Default, PartialEq, Eq)]
   |                ^^^^^ can't derive this in a `repr(packed)` struct with type parameters
LL | #[repr(packed)]
   | --------------- struct `Foo` can't be `derive(Clone)` because of this `repr`
LL | pub struct Foo<T>(T, T, T);
   |                - struct `Foo` can't be `derive(Clone)` because of this type parameter
   |
note: the lint level is defined here
  --> $DIR/deriving-with-repr-packed.rs:1:9
   |
LL | #![deny(unaligned_references)]
   |         ^^^^^^^^^^^^^^^^^^^^
   = 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 #82523 <https://github.com/rust-lang/rust/issues/82523>
   = note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)

With just the impl def id, you probably don't need to pass anything else (and instead fetch the attrs from the impl item by querying for it, complicating unsafe_derive_on_repr_packed a bit more).

The detail of modifying the lint to have an error code might not be possible today, I'm checking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the error code, try calling lint.code(E0113) before emitting it... it might work or it might completely blow up in fun ways, no middle ground.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this should become a hard error in the not-too-distant future. It is already a future compat warning (showing up in cargo's future compat reports) in current stable.

Not sure if this changes anything, though. But if it's easier to just make this particular instance of the error (the one related to derive) a hard error already now, I think that's fine. (Though we would have to crater it.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is around the non-standard output with the error code being referred in the message body. If calling .code() sets the error code without causing the lint machinery to break, I'd prefer that, otherwise we can ignore that part (until we turn it into an error).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try these suggestions on Monday. But I am only willing to spend a small amount of additional time on improving an obscure warning. E.g. if a crater run becomes necessary to merge this PR, I'll just close the PR :) Or if someone else wants to take over that would be fine by me too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then keep it at the same lint level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried adding .code(rustc_errors::DiagnosticId::Error("E0133".to_string())). It change the errors to look like this:

error[E0133]: `Debug` can't be derived on this `#[repr(packed)]` struct that does not derive `Copy`

which is good. But it also eliminated the Future incompatibility report section, which repeats each error under a Future breakage diagnostic heading. I don't know if this is desired.

Also, is the (existing) error code wrong? If I run rustc --explain E0133 I get an explanation "Unsafe code was used outside of an unsafe function or block" which doesn't seem related.

So I've left this part unchanged for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll definitely want to keep them in the future incompat report.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just skip the .code() call and remove the mention of E0133 in the message then :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah this is the unsafe code error code. It doesn't make much sense here any more anyway.

pub struct Bar2(u8, i8, bool);

// This one is fine because the field alignment is 2, matching `packed(2)`.
#[derive(Default, Hash)]
#[repr(packed(2))]
pub struct Bar3(u16, i16, bool);

// This one is fine because it's not packed.
#[derive(Debug, Default)]
struct Y(usize);

#[derive(PartialEq)]
//~^ ERROR `#[derive]` can't be used
#[derive(Debug, Default)]
//~^ ERROR `Debug` can't be derived on this `#[repr(packed)]` struct that does not derive `Copy`
//~| hard error
#[repr(packed)]
struct X(Y);
Expand Down
68 changes: 34 additions & 34 deletions src/test/ui/derives/deriving-with-repr-packed.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
error: `#[derive]` can't be used on a `#[repr(packed)]` struct with type or const parameters (error E0133)
--> $DIR/deriving-with-repr-packed.rs:8:16
error: `Clone` can't be derived on this `#[repr(packed)]` struct with type or const parameters
--> $DIR/deriving-with-repr-packed.rs:11:16
|
LL | #[derive(Copy, Clone, PartialEq, Eq)]
LL | #[derive(Copy, Clone, Default, PartialEq, Eq)]
| ^^^^^
|
note: the lint level is defined here
Expand All @@ -13,43 +13,43 @@ LL | #![deny(unaligned_references)]
= note: for more information, see issue #82523 <https://github.com/rust-lang/rust/issues/82523>
= note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)

error: `#[derive]` can't be used on a `#[repr(packed)]` struct with type or const parameters (error E0133)
--> $DIR/deriving-with-repr-packed.rs:8:23
error: `PartialEq` can't be derived on this `#[repr(packed)]` struct with type or const parameters
--> $DIR/deriving-with-repr-packed.rs:11:32
|
LL | #[derive(Copy, Clone, PartialEq, Eq)]
| ^^^^^^^^^
LL | #[derive(Copy, Clone, Default, PartialEq, Eq)]
| ^^^^^^^^^
|
= 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 #82523 <https://github.com/rust-lang/rust/issues/82523>
= note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)

error: `#[derive]` can't be used on a `#[repr(packed)]` struct that does not derive Copy (error E0133)
--> $DIR/deriving-with-repr-packed.rs:16:10
error: `Hash` can't be derived on this `#[repr(packed)]` struct that does not derive `Copy`
--> $DIR/deriving-with-repr-packed.rs:19:19
|
LL | #[derive(PartialEq, Eq)]
| ^^^^^^^^^
LL | #[derive(Default, Hash)]
| ^^^^
|
= 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 #82523 <https://github.com/rust-lang/rust/issues/82523>
= note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)
= note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)

error: `#[derive]` can't be used on a `#[repr(packed)]` struct that does not derive Copy (error E0133)
--> $DIR/deriving-with-repr-packed.rs:25:10
error: `Debug` can't be derived on this `#[repr(packed)]` struct that does not derive `Copy`
--> $DIR/deriving-with-repr-packed.rs:39:10
|
LL | #[derive(PartialEq)]
| ^^^^^^^^^
LL | #[derive(Debug, 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 #82523 <https://github.com/rust-lang/rust/issues/82523>
= note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)
= note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 4 previous errors

Future incompatibility report: Future breakage diagnostic:
error: `#[derive]` can't be used on a `#[repr(packed)]` struct with type or const parameters (error E0133)
--> $DIR/deriving-with-repr-packed.rs:8:16
error: `Clone` can't be derived on this `#[repr(packed)]` struct with type or const parameters
--> $DIR/deriving-with-repr-packed.rs:11:16
|
LL | #[derive(Copy, Clone, PartialEq, Eq)]
LL | #[derive(Copy, Clone, Default, PartialEq, Eq)]
| ^^^^^
|
note: the lint level is defined here
Expand All @@ -62,11 +62,11 @@ LL | #![deny(unaligned_references)]
= note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)

Future breakage diagnostic:
error: `#[derive]` can't be used on a `#[repr(packed)]` struct with type or const parameters (error E0133)
--> $DIR/deriving-with-repr-packed.rs:8:23
error: `PartialEq` can't be derived on this `#[repr(packed)]` struct with type or const parameters
--> $DIR/deriving-with-repr-packed.rs:11:32
|
LL | #[derive(Copy, Clone, PartialEq, Eq)]
| ^^^^^^^^^
LL | #[derive(Copy, Clone, Default, PartialEq, Eq)]
| ^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/deriving-with-repr-packed.rs:1:9
Expand All @@ -78,11 +78,11 @@ LL | #![deny(unaligned_references)]
= note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)

Future breakage diagnostic:
error: `#[derive]` can't be used on a `#[repr(packed)]` struct that does not derive Copy (error E0133)
--> $DIR/deriving-with-repr-packed.rs:16:10
error: `Hash` can't be derived on this `#[repr(packed)]` struct that does not derive `Copy`
--> $DIR/deriving-with-repr-packed.rs:19:19
|
LL | #[derive(PartialEq, Eq)]
| ^^^^^^^^^
LL | #[derive(Default, Hash)]
| ^^^^
|
note: the lint level is defined here
--> $DIR/deriving-with-repr-packed.rs:1:9
Expand All @@ -91,14 +91,14 @@ LL | #![deny(unaligned_references)]
| ^^^^^^^^^^^^^^^^^^^^
= 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 #82523 <https://github.com/rust-lang/rust/issues/82523>
= note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)
= note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)

Future breakage diagnostic:
error: `#[derive]` can't be used on a `#[repr(packed)]` struct that does not derive Copy (error E0133)
--> $DIR/deriving-with-repr-packed.rs:25:10
error: `Debug` can't be derived on this `#[repr(packed)]` struct that does not derive `Copy`
--> $DIR/deriving-with-repr-packed.rs:39:10
|
LL | #[derive(PartialEq)]
| ^^^^^^^^^
LL | #[derive(Debug, Default)]
| ^^^^^
|
note: the lint level is defined here
--> $DIR/deriving-with-repr-packed.rs:1:9
Expand All @@ -107,5 +107,5 @@ LL | #![deny(unaligned_references)]
| ^^^^^^^^^^^^^^^^^^^^
= 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 #82523 <https://github.com/rust-lang/rust/issues/82523>
= note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)
= note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)