Improve some deriving code and add a test by nnethercote · Pull Request #98376 ·...

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

Trying commit 6dbf740 with merge 85aa844...

Try build successful - checks-actions
Build commit: 85aa844 (85aa84416c76589d4348da14583233ee4de50e8d)

Finished benchmarking commit (85aa844): comparison url.

Instruction count

• Primary benchmarks: relevant improvements found
• Secondary benchmarks: relevant improvements found

mean1	max	count2
Regressions (primary)	N/A	N/A	0
Regressions (secondary)	0.7%	1.1%	2
Improvements (primary)	-0.8%	-1.8%	65
Improvements (secondary)	-5.8%	-11.4%	27
All (primary)	-0.8%	-1.8%	65

Max RSS (memory usage)

Results

Cycles

Results

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

1. the arithmetic mean of the percent change

2. number of relevant changes

I wonder how would perf. look like without the last commit? Without the ne change.

I wonder how would perf. look like without the last commit? Without the ne change.

I haven't measured but I'm confident that the last commit is responsible for 99% the perf wins. The first commit is the only other one that affects the generated code, and it just removes one true && per method, which is a tiny change.

I liked @jyn514's comment on Zulip, about the possibility of behavioural changes with this PR if you derive PartialEq on a type that contains a type that has a buggy hand-written PartialEq impl: "we already document that you have to obey this constraint [that eq and ne are inverses], I don't think it's reasonable to expect us to be bug for bug compatible".

@bors r+

Commit fb58eff has been approved by petrochenkov

The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

I think a remnant of PartialEq::ne is still contained within the coverage report in that test, and may require being regenerated (I'm not sure how that's done though).

Do the run-make and run-make-fulldeps tests pass for you locally ?

I think a remnant of PartialEq::ne is still contained within the coverage report in that test, and may require being regenerated (I'm not sure how that's done though).

Do the run-make and run-make-fulldeps tests pass for you locally ?

They do, both via ./x.py test and by running them directly.

The problem appears to be this:

2022-06-28T13:25:51.5003686Z +++ /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make-fulldeps/coverage-reports/coverage-reports/actual_show_coverage.issue-84561.txt	2022-06-28 13:25:27.256476002 +0000
2022-06-28T13:25:51.5003860Z @@ -2,12 +2,6 @@
2022-06-28T13:25:51.5003962Z      2|       |
2022-06-28T13:25:51.5004183Z      3|       |// expect-exit-status-101
2022-06-28T13:25:51.5004324Z      4|     21|#[derive(PartialEq, Eq)]
2022-06-28T13:25:51.5004492Z -  ------------------
2022-06-28T13:25:51.5005071Z -  | <issue_84561::Foo as core::cmp::PartialEq>::eq:
2022-06-28T13:25:51.5005312Z -  |    4|     21|#[derive(PartialEq, Eq)]
2022-06-28T13:25:51.5005479Z -  ------------------
2022-06-28T13:25:51.5005864Z -  | Unexecuted instantiation: <issue_84561::Foo as core::cmp::PartialEq>::ne
2022-06-28T13:25:51.5006026Z -  ------------------
2022-06-28T13:25:51.5006150Z      5|       |struct Foo(u32);
2022-06-28T13:25:51.5006266Z      6|      1|fn test3() {
2022-06-28T13:25:51.5006462Z      7|      1|    let is_true = std::env::args().len() == 1;
2022-06-28T13:25:51.5006686Z ------------------------------------------

I don't understand these coverage tests at all, so I'm not sure if just updating the expected output to remove those lines is reasonable...

Oh, it seems the coverage-reports test is ignored when I run it locally, bleh.

I don't understand these coverage tests at all, so I'm not sure if just updating the expected output to remove those lines is reasonable...

Neither do I, but at least seeing an "unexecuted instantiation of PartialEq::ne" made me think the generated ne wasn't called nor covered before, even if the test calls assert_ne!.

And that seemed to match the idea that it was a safe function to delete indeed.

I think this PR should have a T-libs-api FCP on it, since it does modify stable behavior. It's likely okay to change that behavior - we document that eq and ne should match - but I could see this breaking users if they're writing custom impls and so I think it merits additional consideration.

At minimum, marking relnotes and will edit the description with a short note for why.

I'm going to remove the last commit from this PR, which is the one that removes the ne derive. Reason being that the earlier commits (especially the first one) are blocking my other work on derives (#98446 and other stuff I haven't yet created PRs for).

I will then file a new PR just for the ne removal, and we can scrutinize that separately.

I removed the final commit. I'll file the new PR for the ne removal once this PR is merged.

@bors r=petrochenkov

Commit 02d2cdf has been approved by petrochenkov

Testing commit 02d2cdf with merge 126e3df...

Test successful - checks-actions
Approved by: petrochenkov
Pushing 126e3df to master...

I'll file the new PR for the ne removal once this PR is merged.

That is #98655.

Finished benchmarking commit (126e3df): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

Cycles

Results

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change

2. number of relevant changes