Improve error messages involving `derive` and `packed`. #99581

nnethercote · 2022-07-22T00:47:15Z

There are two errors involving derive and packed.

`#[derive]` can't be derived on a `#[repr(packed)]` struct with type or const parameters
`#[derive]` can't be derived on a `#[repr(packed)]` struct that does not derive Copy

The second one overstates things. It is possible to use derive on a
repr(packed) struct that doesn't derive Copy in two cases.

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.

This commit improves things in a few ways.

Changes the errors to say this trait can't be derived on this ....
This is more accurate, because it's just this trait and this
packed struct that are a problem, not all derived traits on all
packed structs.
Adds more details to the "ERROR" lines in the test case, enough to
distinguish between the two error messages.
Adds more cases to the test case that don't cause errors, e.g. Default
derives.
Uses a wider variety of builtin traits in the test case, for better coverage.

r? @estebank

rustbot · 2022-07-22T00:47:18Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

nnethercote · 2022-07-22T00:48:19Z

It would be better to replace "this trait can't be derived..." with "Debug can't be derived..." (or whatever trait name is appropriate), but I couldn't work out how to get the trait name. Suggestions are welcome!

cc @RalfJung

RalfJung · 2022-07-22T01:46:18Z

Hm, not sure what the best way is to get there. I'm not usually dabbling much with things outside the interpret folders. ;)

trait_id_of_impl seems useful to get the trait defid. And then maybe try item_name and see what that returns?

wesleywiser · 2022-07-22T14:04:55Z

trait_id_of_impl seems useful to get the trait defid. And then maybe try item_name and see what that returns?

~~This sounds like the right track to me but I think you want to compare the trait DefId with diagnostic item DefId for Debug sort of like what's done here~~

rust/compiler/rustc_lint/src/builtin.rs

Line 830 in 748cb1f

let Some(debug) = cx.tcx.get_diagnostic_item(sym::Debug) else {

Edit: Nevermind! I thought you wanted to see if the trait is specifically the Debug trait, not just get what the name is. Ralf is correct I believe.

estebank · 2022-07-22T17:02:05Z

compiler/rustc_mir_transform/src/check_packed_ref.rs

@@ -37,10 +37,10 @@ fn unsafe_derive_on_repr_packed(tcx: TyCtxt<'_>, def_id: LocalDefId) {
        // 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 \
+            "this trait can't be derived on this `#[repr(packed)]` struct with \


It would be better to replace "this trait can't be derived..." with "Debug can't be derived..." (or whatever trait name is appropriate), but I couldn't work out how to get the trait name. Suggestions are welcome!

I'm pretty sure the def_id here corresponds to Debug, so you can use tcx.item_name(def_id) to modify the message.

This wasn't enough, returning None. But this worked:

tcx.item_name(tcx.trait_id_of_impl(def_id.to_def_id()).expect("derived trait name"))

estebank · 2022-07-22T17:10:22Z

src/test/ui/derives/deriving-with-repr-packed.rs

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


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.

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.

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.)

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).

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.

Then keep it at the same lint level.

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.

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

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

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

nnethercote · 2022-07-25T00:50:22Z

New code is up. It names the traits instead of saying "this trait". Nothing else has changed, for reasons mentioned above.

estebank · 2022-07-25T09:18:27Z

@bors r+

bors · 2022-07-25T09:18:29Z

📌 Commit 168c5b1 has been approved by estebank

It is now in the queue for this repository.

…rrors, r=estebank Improve error messages involving `derive` and `packed`. There are two errors involving `derive` and `packed`. ``` `#[derive]` can't be derived on a `#[repr(packed)]` struct with type or const parameters `#[derive]` can't be derived on a `#[repr(packed)]` struct that does not derive Copy ``` The second one overstates things. It is possible to use derive on a repr(packed) struct that doesn't derive Copy in two cases. - 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. This commit improves things in a few ways. - Changes the errors to say `this trait can't be derived on this ...`. This is more accurate, because it's just *this* trait and *this* packed struct that are a problem, not *all* derived traits on *all* packed structs. - Adds more details to the "ERROR" lines in the test case, enough to distinguish between the two error messages. - Adds more cases to the test case that don't cause errors, e.g. `Default` derives. - Uses a wider variety of builtin traits in the test case, for better coverage. r? `@estebank`

There are two errors involving `derive` and `packed`. ``` `#[derive]` can't be derived on a `#[repr(packed)]` struct with type or const parameters `#[derive]` can't be derived on a `#[repr(packed)]` struct that does not derive Copy ``` The second one overstates things. It is possible to use derive on a repr(packed) struct that doesn't derive Copy in two cases. - 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. This commit improves things in a few ways. - Changes the errors to say `$TRAIT can't be derived on this ...`. This is more accurate, because it's just $TRAIT and *this* packed struct that are a problem, not *all* derived traits on *all* packed structs. - Removed the E0133 error code, because it's incorrect, being a code relating to `unsafe`. - Adds more details to the "ERROR" lines in the test case, enough to distinguish between the two error messages. - Adds more cases to the test case that don't cause errors, e.g. `Default` derives. - Uses a wider variety of builtin traits in the test case, for better coverage.

nnethercote · 2022-07-25T10:20:00Z

I updated to remove the E0133 code.

@bors r=estebank

bors · 2022-07-25T10:20:02Z

📌 Commit f6305dd has been approved by estebank

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#95040 (protect `std::io::Take::limit` from overflow in `read`) - rust-lang#95916 (kmc-solid: Use `libc::abort` to abort a program) - rust-lang#99494 (Use non-relocatable code in nofile-limit.rs test) - rust-lang#99581 (Improve error messages involving `derive` and `packed`.) - rust-lang#99643 (Add `sign-ext` target feature to the WASM target) - rust-lang#99659 (Use `VecMap::get` in `ConstraintLocator::check`) - rust-lang#99690 (add miri-track-caller to more intrinsic-exposing methods) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

JohnTitor · 2022-07-25T14:08:33Z

@nnethercote Make sure a PR isn't rolled up if you push a new change, otherwise it will result in failing to merge.
@bors r-

RalfJung · 2022-07-25T14:26:39Z

Well actually the earlier version of this PR has been merged. So it should probably be closed, and a new PR opened for the changes that were pushed since the r+.

JohnTitor · 2022-07-25T14:40:33Z

Well actually the earlier version of this PR has been merged. So it should probably be closed, and a new PR opened for the changes that were pushed since the r+.

Agree, closing.

@nnethercote Please open a new PR.

bors · 2022-07-25T14:48:04Z

☔ The latest upstream changes (presumably #99707) made this pull request unmergeable. Please resolve the merge conflicts.

nnethercote · 2022-07-25T22:47:28Z

Sorry for any confusion.

Esteban requested changes and then two minutes later gave r+. Half an hour later this PR was included in the rollup. Half an hour after that I saw Esteban's comment and approval via email notifications. Given that only an hour had passed and this PR hadn't been marked for rollup I (incorrectly) assumed I had time to address the comment. I didn't know about the rollup because you don't get email notifications when a PR is included in a rollup and I didn't look directly at the PR.

I'll be more careful about updating PRs that have received r+ in the future, now that I know about this failure mode. I'd also recommend that reviewers use delegate=<username> if they request minor changes but don't need to re-review them. It would be useful if rollup inclusion triggered an email notification, but I don't know if that's possible.

JohnTitor · 2022-07-25T22:55:38Z

I'll be more careful about updating PRs that have received r+ in the future, now that I know about this failure mode. I'd also recommend that reviewers use delegate=<username> if they request minor changes but don't need to re-review them.

Sounds good, it'd be also great if a reviewer r- a PR in such a case (cc @estebank).

It would be useful if rollup inclusion triggered an email notification, but I don't know if that's possible.

Rollup often fails to merge and many PRs are rolled up several times, so it'd be noisy for many contributors I guess 🤔

Mark-Simulacrum · 2022-07-25T23:05:08Z

Arguably, we probably want to cancel the rollup in such a case (since it can't have merged if the r+ is to be processed by bors, since that only happens on open PRs).

In general bors is not very good about actually knowing about rollups including other PRs; to my knowledge bors has no knowledge of that today, even though they're created through the bors UI.

rust-highfive assigned estebank Jul 22, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 22, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2022

nnethercote mentioned this pull request Jul 22, 2022

Be stricter about combining repr(packed) with derive. #99197

Closed

estebank reviewed Jul 22, 2022

View reviewed changes

nnethercote force-pushed the improve-derive-packed-errors branch from ee502cf to 168c5b1 Compare July 25, 2022 00:49

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2022

JohnTitor mentioned this pull request Jul 25, 2022

Rollup of 7 pull requests #99707

Merged

nnethercote force-pushed the improve-derive-packed-errors branch from 168c5b1 to f6305dd Compare July 25, 2022 10:19

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 25, 2022

JohnTitor closed this Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error messages involving `derive` and `packed`. #99581

Improve error messages involving `derive` and `packed`. #99581

nnethercote commented Jul 22, 2022

rustbot commented Jul 22, 2022

nnethercote commented Jul 22, 2022

RalfJung commented Jul 22, 2022

wesleywiser commented Jul 22, 2022 •

edited

Loading

estebank Jul 22, 2022

nnethercote Jul 25, 2022

estebank Jul 22, 2022

estebank Jul 22, 2022

RalfJung Jul 22, 2022

estebank Jul 22, 2022

nnethercote Jul 22, 2022

RalfJung Jul 23, 2022

nnethercote Jul 25, 2022

RalfJung Jul 25, 2022

estebank Jul 25, 2022

RalfJung Jul 25, 2022

nnethercote commented Jul 25, 2022

estebank commented Jul 25, 2022

bors commented Jul 25, 2022

nnethercote commented Jul 25, 2022

bors commented Jul 25, 2022

JohnTitor commented Jul 25, 2022

RalfJung commented Jul 25, 2022

JohnTitor commented Jul 25, 2022

bors commented Jul 25, 2022

nnethercote commented Jul 25, 2022

JohnTitor commented Jul 25, 2022

Mark-Simulacrum commented Jul 25, 2022

Improve error messages involving derive and packed. #99581

Improve error messages involving derive and packed. #99581

Conversation

nnethercote commented Jul 22, 2022

rustbot commented Jul 22, 2022

nnethercote commented Jul 22, 2022

RalfJung commented Jul 22, 2022

wesleywiser commented Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnethercote commented Jul 25, 2022

estebank commented Jul 25, 2022

bors commented Jul 25, 2022

nnethercote commented Jul 25, 2022

bors commented Jul 25, 2022

JohnTitor commented Jul 25, 2022

RalfJung commented Jul 25, 2022

JohnTitor commented Jul 25, 2022

bors commented Jul 25, 2022

nnethercote commented Jul 25, 2022

JohnTitor commented Jul 25, 2022

Mark-Simulacrum commented Jul 25, 2022

Improve error messages involving `derive` and `packed`. #99581

Improve error messages involving `derive` and `packed`. #99581

wesleywiser commented Jul 22, 2022 •

edited

Loading