Use covariance on type relations of field projection types if possible

[b-naber]

It's fine to use covariance here unless we're in a mutating context.

Fixes #96514

Supersedes #105958

r? @lcnr

[lcnr]

that looks really nice 👍 🔥

r=me after nit

[lcnr]

there's NonUseContext::AscribeUserTy which I feel should maybe be invariant 🤔

I can also imagine us adding NonMutatingUse in the future which should be invariant. Can you also exhaustively match on the NonMutatingUseContext and probably make all NonUse invariant. Don't know how to write an example where Covariance is needed here.

Considering that covariance can be unsound if used incorrectly, we should be as defensive as possible here.

[b-naber]

Sorry for the late answer.

We would need NonUseContext::AscribeUserTy for field projections on enums in match statements where we always insert user type projections I believe, e.g.

struct Inv<'a>(&'a mut &'a ());
enum Foo<T> {
    Bar,
    Var(T),
}
type Supertype = Foo<for<'a> fn(Inv<'a>, Inv<'a>)>;

fn foo_nested(x: Foo<Foo<for<'a, 'b> fn(Inv<'a>, Inv<'b>)>>) {
    match x {
        Foo::Bar => {}
        Foo::Var(Supertype::Bar) => {}
        Foo::Var(Supertype::Var(x)) => {}
    }
}

would not compile if we would use ty::Invariant on NonUse(AscribeUserTy).

I agree that we should be very conservative here, but I also can't see how using covariance here might be unsound. Can you show that it is? I've kept this as covariant for now.

[lcnr]

my worry is that we use AscribeUserTy in a position which isn't covariant. E.g. only for arguments of a function we're calling. Thinking more about it I actually can't imagine that happening 🤔

👍

[cjgillot]

The AscribeUserType statement contains a variance. Should it be carried by NonUseContext::AscribeUserTy for use here?

[lcnr]

yes, I think so 👍 while I don't think is exploitable as we only use contravariance in AscribeUserType for places without any projections. Using the following in super_ascribe_user_ty does not cause any failures

                if variance != $(& $mutability)? ty::Variance::Covariant && !place.projection.is_empty() {
                    bug!()
                }

we should still add the variance to the use context though as this isn't something I want to implicitly rely on.

[lcnr]

@bors try @rust-timer queue

probably neutral

[bors]

⌛ Trying commit 6e1d228 with merge 746726458a663cc90ceb903387bfb3974d85ef19...

[bors]

☀️ Try build successful - checks-actions
Build commit: 746726458a663cc90ceb903387bfb3974d85ef19 (746726458a663cc90ceb903387bfb3974d85ef19)

[rust-timer]

Finished benchmarking commit (746726458a663cc90ceb903387bfb3974d85ef19): comparison URL.

Overall result: ✅ 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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.2%]	4
All ❌✅ (primary)	-	-	0

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

Cycles

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

[lcnr]

@rustbot author

[lcnr]

@bors r+

[bors]

📌 Commit 758cc95 has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit 758cc95 with merge e7eaed2...

[bors]

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

[rust-timer]

Finished benchmarking commit (e7eaed2): 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
Improvements ✅ (secondary)	-1.6%	[-1.6%, -1.6%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

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