Combine `Sub` and `Equate` #121462

compiler-errors · 2024-02-22T15:53:59Z

Combine Sub and Equate into a new relation called TypeRelating (that name sounds familiar...)

Tracks the difference between Sub and Equate via ambient_variance: ty::Variance much like the NllTypeRelating relation, but implemented slightly jankier because it's a more general purpose relation.

r? lcnr

rustbot · 2024-02-22T15:54:08Z

Type relation code was changed

cc @compiler-errors, @lcnr

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

lcnr

I am not too happy with RelateDirection. Because it doesn't really feel appropriate given that it is used in relate only together with a_is_expected.

I would personally prefer tracking the variance in TypeRelating - supporting Contravariance - and changing relate_with_variance to simply be self.variance = self.variance.xform(variance);. This means we then don't need a_is_expected at all and (after removing (it from) Lub) can change the Generalizer to not compare a_is_expected with target_is_expected.

Apart from that 👍

compiler/rustc_infer/src/infer/relate/type_relating.rs

…rors fix generalizer unsoundness I ended up getting confused while trying to flip the variances when flipping the order. Should be all right now. This is only exploitable when generalizing if the `ambient_variance` of the relation is `Contravariant`. This can currently only be the case in the NLL generalizer which only rarely generalizes, causing us to miss this regression. Very much an issue with rust-lang#121462 however.

Rollup merge of rust-lang#121479 - lcnr:fix-generalize, r=compiler-errors fix generalizer unsoundness I ended up getting confused while trying to flip the variances when flipping the order. Should be all right now. This is only exploitable when generalizing if the `ambient_variance` of the relation is `Contravariant`. This can currently only be the case in the NLL generalizer which only rarely generalizes, causing us to miss this regression. Very much an issue with rust-lang#121462 however.

bors · 2024-02-23T15:14:06Z

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

lcnr · 2024-02-26T06:37:19Z

compiler/rustc_infer/src/infer/relate/combine.rs

+//! There are four type combiners: [TypeRelating], [Lub], and [Glb],
+//! (and `NllTypeRelating` in rustc_borrowck, which is only used for NLL).


Suggested change

//! There are four type combiners: [TypeRelating], [Lub], and [Glb],

//! (and `NllTypeRelating` in rustc_borrowck, which is only used for NLL).

//! There are four type combiners: [TypeRelating], [Lub], and [Glb],

//! and `NllTypeRelating` in rustc_borrowck, which is only used for NLL.

lcnr · 2024-02-26T06:38:32Z

compiler/rustc_infer/src/infer/relate/type_relating.rs

+/// Enforce that `a` is equal to or a subtype of `b`.
+pub struct TypeRelating<'combine, 'a, 'tcx> {
+    fields: &'combine mut CombineFields<'a, 'tcx>,
+    a_is_expected: bool,


a_is_expected should be unnecessary now, as we can instead flip the related types by using xform(ty::Contravariance). Where do we modify a_is_expected right now?

It's not that we modify a_is_expected from within the relation. It's that we construct the relation (and therefore pass in a_is_expected) separately from where we use it. I think this is prohibitively invasive to fix right now.

hmm, we could then simpy flip the args if Trace { a_is_expected } is false. But yeah, am fine with doing that in a separate PR

lcnr · 2024-02-26T06:42:34Z

@bors try @rust-timer queue

Combine `Sub` and `Equate` Combine `Sub` and `Equate` into a new relation called `TypeRelating` (that name sounds familiar...) Tracks the difference between `Sub` and `Equate` via `ambient_variance: ty::Variance` much like the `NllTypeRelating` relation, but implemented slightly jankier because it's a more general purpose relation. r? lcnr

bors · 2024-02-26T06:43:46Z

⌛ Trying commit 3034b2b with merge c30f81e...

bors · 2024-02-26T08:12:06Z

☀️ Try build successful - checks-actions
Build commit: c30f81e (c30f81e2c10de46286b6ad1e53b8e11995ad024e)

bors · 2024-02-29T04:01:08Z

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

lcnr

r=me after final comment cleanup.

Thanks for your work ❤️

compiler/rustc_infer/src/infer/relate/generalize.rs

compiler/rustc_infer/src/infer/relate/type_relating.rs

bors · 2024-02-29T22:31:25Z

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

lcnr · 2024-03-01T10:29:32Z

@bors r+ rollup=never

bors · 2024-03-01T10:29:35Z

📌 Commit 5072b65 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-03-01T10:30:45Z

⌛ Testing commit 5072b65 with merge b0696a5...

bors · 2024-03-01T12:57:12Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing b0696a5 to master...

rust-timer · 2024-03-01T14:11:26Z

Finished benchmarking commit (b0696a5): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.5%	[0.2%, 0.9%]	13
Regressions ❌ (secondary)	0.5%	[0.3%, 0.8%]	17
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.9%, -0.4%]	7
All ❌✅ (primary)	0.5%	[0.2%, 0.9%]	13

Max RSS (memory usage)

Results

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.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.9%	[-3.9%, -3.9%]	1
All ❌✅ (primary)	2.2%	[2.2%, 2.2%]	1

Cycles

Results

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.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.7%, 0.7%]	1

Binary size

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

Bootstrap: 653.924s -> 651.828s (-0.32%)
Artifact size: 311.55 MiB -> 311.10 MiB (-0.15%)

lcnr · 2024-03-01T14:16:02Z

as stated in #121462 (comment):

Some ideas how to reduce the perf impact. Even if they don't completely remove it, this will allow significant improvements and cleanups going forward and generally simplifies the core type system. I would merge this even with these regressions.

rustbot assigned lcnr Feb 22, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Feb 22, 2024