Switch from derivative to derive-where

[GrigorenkoPV]

This is a part of the effort to get rid of syn 1.* in compiler's dependencies: #109302

Derivative has not been maintained in nearly 3 years¹. It also depends on syn 1.*.

This PR replaces derivative with derive-where², a not dead alternative, which uses syn 2.*.

A couple of Debug formats have changed around the skipped fields³, but I doubt this is an issue.

Footnotes

1. https://github.com/mcarton/rust-derivative/issues/117 ↩

2. https://lib.rs/crates/derive-where ↩

3. See the changes in tests/ui ↩

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[compiler-errors]

@bors try @rust-timer queue

derives on very fundamental types may change their perf characteristics

[bors]

⌛ Trying commit b8bc939 with merge 9fbecc0...

[bors]

☀️ Try build successful - checks-actions
Build commit: 9fbecc0 (9fbecc05053b6812019f0ec0efda491fa8939a6e)

[rust-timer]

Finished benchmarking commit (9fbecc0): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

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-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.3%]	6
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 2.0%)

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

	mean	range	count
Regressions ❌ (primary)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.0%	[2.0%, 2.0%]	1

Cycles

Results (primary -2.5%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Binary size

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

Bootstrap: 697.296s -> 695.903s (-0.20%)
Artifact size: 326.82 MiB -> 326.79 MiB (-0.01%)

[GrigorenkoPV]

The job mingw-check failed! Check out the build log: (web) (plain)

The failures here are quite strange. The only thing that I did change was adding derivation of Eq to CanonicalVarKind, because without it the code refused to compile. My working theory is that derivative was fine with just CanonicalVarKind: PartialEq to derive CanonicalVarInfo: Eq, while it should not have been. derive-where, meanhwile, correctly infers that CanonicalVarKind: Eq is also needed. But hey, that's just a theory.

However, what clippy complains about is actually derivations of Hash with a manual PartialEq implementation. ...which was the case even before my changes. Huh.

Sounds like I need to do some investigation with cargo expand.

[GrigorenkoPV]

The job mingw-check failed! Check out the build log: (web) (plain)

The failures here are quite strange. The only thing that I did change was adding derivation of Eq to CanonicalVarKind, because without it the code refused to compile. My working theory is that derivative was fine with just CanonicalVarKind: PartialEq to derive CanonicalVarInfo: Eq, while it should not have been. derive-where, meanhwile, correctly infers that CanonicalVarKind: Eq is also needed. But hey, that's just a theory.

derivative:

    #[allow(unused_qualifications)]
    impl<I: Interner> ::std::cmp::Eq for CanonicalVarInfo<I> {}

derive-where:

    #[automatically_derived]
    impl<I: Interner> ::core::cmp::Eq for CanonicalVarInfo<I>
    where
        I: Interner,
    {
        #[inline]
        fn assert_receiver_is_total_eq(&self) {
            struct __AssertEq<__T: ::core::cmp::Eq + ?::core::marker::Sized>(
                ::core::marker::PhantomData<__T>,
            );
            let _: __AssertEq<CanonicalVarKind<I>>;
        }
    }

So my theory seems to be correct.

However, what clippy complains about is actually derivations of Hash with a manual PartialEq implementation. ...which was the case even before my changes. Huh.

The culprit for this one can also be seen above. It's the #[automatically_derived] attribute.

So, all in all, I would say derive-where does a better job, at least as far as type checks and attributes go. As for the performance changes, I am not really equipped to judge those.

Should I just silence the clippy warnings?

[petrochenkov]

The warnings are in rustc_type_ir, so I'll leave it to the types team how to deal with them.
Otherwise LGTM, the perf changes are not significant.
r? @compiler-errors

[compiler-errors]

Perf changes are noise. I think it's fine for us to. As a follow-up, we should look at not manually implementing PartialEq for these types but instead going thru a derive -- I believe they're only there b/c the automatic derives from derivative was performing poorly.

Please rebase this PR, and I'll take another look at it just for certainty. I think I may have added some derivative calls recently.

[compiler-errors]

@rustbot author

[compiler-errors]

Sorry, I didn't finish my sentence.

I think it's fine for us to

[...] ignore the clippy warnings regarding manually derived impls.

[GrigorenkoPV]

I think it's fine for us to

[...] ignore the clippy warnings regarding manually derived impls.

Done.

As a follow-up, we should look at not manually implementing PartialEq for these types but instead going thru a derive

I will try to tackle this later in a separate PR

[bors]

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

[compiler-errors]

@bors r+

[bors]

📌 Commit 59f88d3 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit 59f88d3 with merge 2f26b2a...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 2f26b2a to master...

[rust-timer]

Finished benchmarking commit (2f26b2a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	16
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.4%]	8
All ❌✅ (primary)	-0.2%	[-0.3%, -0.2%]	16

Max RSS (memory usage)

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

Cycles

Results (secondary -6.5%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.5%	[-6.5%, -6.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 770.291s -> 771.056s (0.10%)
Artifact size: 328.92 MiB -> 328.92 MiB (-0.00%)