Be stricter about combining repr(packed) with derive.
#99197
Conversation
…est. It's an interesting case, requiring the use of `&&` in `Debug::fmt`.
Because the generatedd code is different to a `Copy` packed struct.
Because the generated code is different to fieldless enum with multiple variants.
It's always `ast::Mutability::Not`.
E.g. improving code like this:
```
match &*self {
&Enum1::Single { x: ref __self_0 } => {
::core::hash::Hash::hash(&*__self_0, state)
}
}
```
to this:
```
match self {
Enum1::Single { x: __self_0 } => {
::core::hash::Hash::hash(&*__self_0, state)
}
}
```
by removing the `&*`, the `&`, and the `ref`.
I suspect the current generated code predates deref-coercion.
The commit also gets rid of `use_temporaries`, instead passing around
`always_copy`, which makes things a little clearer. And it fixes up some
comments.
By producing `&T` expressions for fields instead of `T`. This matches what the existing comments (e.g. on `FieldInfo`) claim is happening, and it's also what most of the trait-specific code needs. The exception is `PartialEq`, which needs `T` expressions for lots of special case error messaging to work. So we now convert the `&T` back to a `T` for `PartialEq`.
Use `tag` in names of things referring to tags, instead of the mysterious `vi`. Also change `arg_N` in output to `argN`, which has the same length as `self` and so results in nicer vertical alignments.
To avoid computing a bunch of stuff that it doesn't need.
Currently, for the enums and comparison traits we always check the tag for equality before doing anything else. This is a bit clumsy. This commit changes things so that the tags are handled very much like a zeroth field in the enum. For `eq`/ne` this makes the code slightly cleaner. For `partial_cmp` and `cmp` it's a more notable change: in the case where the tags aren't equal, instead of having a tag equality check followed by a tag comparison, it just does a single tag comparison. The commit also improves how `Hash` works for enums: instead of having duplicated code to hash the tag for every arm within the match, we do it just once before the match. All this required replacing the `EnumNonMatchingCollapsed` value with a new `EnumTag` value. For fieldless enums the new code is particularly improved. All the code now produced is close to optimal, being very similar to what you'd write by hand.
Because it's more concise than the `let` form.
Currently there are two errors you can get when using `derive` on a `repr(packed)` struct: - "`#[derive]` can't be used on a `#[repr(packed)]` struct with type or const parameters (error E0133)" - "`#[derive]` can't be used on a `#[repr(packed)]` struct that does not derive Copy (error E0133)" These were introduced under rust-lang#82523, because they can lead to unaligned references, which is UB. They are in the process of being upgraded to hard errors. This is all fine, but the second error 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))`. - Or, If `Default` is the only trait derived. These exceptions don't seem particularly important or useful, and the language would be simpler if they are removed. So this commit does that, making these cases a hard error. The new checks are done at derive code creation time which means that `unsafe_derive_on_repr_packed` is no longer needed at MIR build time. Test changes: - deriving-with-repr-packed.rs has numerous changes to the code to broaden coverage. Some of the errors are reported twice now, which is hard to avoid but harmless (and users won't see duplicates thanks to the normal error de-duplication). Also, if a packed struct is both non-`Copy` and has type params, both those errors are now reported. - deriving-all-codegen.rs has the `PackedNonCopy` case removed, because it's no longer allowed. - packed.rs needs `Copy` added to some packed struct that currently only derive `Default`.
rustbot
added
the
T-compiler
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
⌛ Trying commit 9213a54 with merge 53184dc07942a1944769a9e15fc2cb08c9f62e9d... |
|
BTW, this kind of falls under #82523. |
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
the
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
Hmm, 99 regressing crates in the crater run, which seems too high to do this as-is. So, possible course of action:
|
|
If we accept the same code when hand-written, I don't quite see the advantage of rejecting it for automatic derive. |
|
Yeah, I agree. The advantage of the blanket ban is simplicity. But that would ban some legitimate things that are currently accepted. In particular, So I think I will improve the error message a little but leave everything else alone. |
|
The follow-up is #99581. |
|
Maybe something like this can be salvaged if we find a way to support some of the cases that started to error here... see #82523 (comment). |
Currently there are two errors you can get when using
deriveon arepr(packed)struct:#[derive]can't be used on a#[repr(packed)]struct with typeor const parameters (error E0133)"
#[derive]can't be used on a#[repr(packed)]struct that does notderive Copy (error E0133)"
These were introduced under #82523, because they can lead to unaligned
references, which is UB. They are in the process of being upgraded to
hard errors.
This is all fine, but the second error overstates things. It is
currently possible to use
deriveon arepr(packed)struct that doesn't deriveCopyin two cases.repr(packed), or N forrepr(packed(N)).Defaultis the only trait derived.These exceptions don't seem particularly important or useful, and the
language would be simpler if they are removed. So this commit does that,
making these cases a hard error. The new checks are done at derive code
creation time which means that
unsafe_derive_on_repr_packedis nolonger needed at MIR build time.
Test changes:
broaden coverage. Some of the errors are reported twice now,
which is hard to avoid but harmless (and users won't see duplicates
thanks to the normal error de-duplication). Also, if a packed struct
is both non-
Copyand has type params, both those errors are nowreported.
PackedNonCopycase removed, becauseit's no longer allowed.
Copyadded to some packed struct that currently onlyderive
Default.r? @ghost