Do not add leading asterisk in the PartialEq
#124157
Conversation
rustbot
added
S-waiting-on-review
|
Changes to the code generated for builtin derived traits. cc @nnethercote |
This comment has been minimized.
This comment has been minimized.
| @@ -0,0 +1,18 @@ | |||
| //@ check-pass | |||
There was a problem hiding this comment.
It would be better to add this case to the existing tests/ui/deriving/deriving-all-codegen.rs file.
There was a problem hiding this comment.
Moved, thank you for pointing it out.
This comment has been minimized.
This comment has been minimized.
Adding leading asterisk can cause compilation failure for the _types_ that don't implement the `Copy`.
|
@nnethercote the original change for this was here Do you remember why we were dereferencing these values? Looking at the expanded code, it doesn't seem like it was ever needed, right? It also claimed that it improved errors, but there was no new test to show the new output, so I'm not sure if it refers to the " |
|
The changes look good to me, but out of an over abundance of caution I'll do a crater run. @bors try |
Do not add leading asterisk in the `PartialEq` I think we should address this issue, however I am not exactly sure, if this is the right way to do it. It is related to the rust-lang#123056. Imagine the simplified code: ```rust trait MyTrait {} impl PartialEq for dyn MyTrait { fn eq(&self, _other: &Self) -> bool { true } } #[derive(PartialEq)] enum Bar { Foo(Box<dyn MyTrait>), } ``` On the nightly compiler, the `derive` produces invalid code with the weird error message: ``` error[E0507]: cannot move out of `*__arg1_0` which is behind a shared reference --> src/main.rs:11:9 | 9 | #[derive(PartialEq)] | --------- in this derive macro expansion 10 | enum Things { 11 | Foo(Box<dyn MyTrait>), | ^^^^^^^^^^^^^^^^ move occurs because `*__arg1_0` has type `Box<dyn MyTrait>`, which does not implement the `Copy` trait | = note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info) ``` It may be related to the perfect derive problem, although requiring the _type_ to be `Copy` seems unfortunate because it is not necessary. Besides, we are adding the extra dereference only for the diagnostics?
|
@rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
This was the commit. The message said:
I have a memory of some error messages getting worse when using Also, I think "binary operation |
|
It would be good to squash the two commits before merging. But wait for the crater run to finish first. |
|
Finished benchmarking commit (9c7b5f5): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 675.427s -> 673.89s (-0.23%) |
rustbot
added
perf-regression
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
removed
the
S-waiting-on-review
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
|
and I'd like to dig deeper into those two, but it seems like this change is ok to land as-is. @bors r+ cc @rust-lang/compiler for visibility |
bors
added
S-waiting-on-bors
estebank
added
relnotes
|
@estebank Could I help you in any way by further analyzing these two occurrences you mentioned? |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (cb93c24): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 676.422s -> 673.974s (-0.36%) |
|
Looks like a bit more work has to be done to perhaps auto deref the types when we don't manually insert the asterisk? In any case, the regressions are relatively small, and this seems to be a fix that allows more code to compile. We probably can't do much about this, since we just change the derive codegen, and for known reasons we can't do this e.g. selectively based on the type of the field. @rustbot label: +perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
|
@estebank why does this have the relnotes label, it's unclear to me what exactly the user facing change was here |
|
In general I am a bit confused by this PR:
|
|
I don't think this needs relnotes. The failure to fix the original issue is unfortunate, I didn't realize that. |
|
I think I added relnotes because of change of desugaring might be useful to learn about for people doing lower level shenanigans, but you're right that it isn't needed.
My read of #123056 is that the ticket was for the cryptic output: which with this PR it no longer happens, but there's the more general case of the move error itself which could do with better output. Edit: closed the ticket and opened #127215 for more granular tracking. |
I think we should address this issue, however I am not exactly sure, if this is the right way to do it. It is related to the #123056.
Imagine the simplified code:
On the nightly compiler, the
deriveproduces invalid code with the weird error message:It may be related to the perfect derive problem, although requiring the type to be
Copyseems unfortunate because it is not necessary. Besides, we are adding the extra dereference only for the diagnostics?