internally change regions to be covariant #107339

aliemjay · 2023-01-26T23:27:58Z

Surprisingly, we consider the reference type &'a T to be contravaraint in its lifetime parameter. This is confusing and conflicts with the documentation we have in the reference, rustnomicon, and rustc-dev-guide. This also arguably not the correct use of terminology since we can use &'static u8 in a place where &' a u8 is expected, this implies that &'static u8 <: &' a u8 and consequently 'static <: ' a, hence covariance.

Because of this, when relating two types, we used to switch the argument positions in a confusing way:
Subtype(&'a u8 <: &'b u8) => Subtype('b <: 'a) => Outlives('a: 'b) => RegionSubRegion('b <= 'a)

The reason for the current behavior is probably that we wanted Subtype('b <: 'a) and RegionSubRegion('b <= 'a) to be equivalent, but I don' t think this is a good reason since these relations are sufficiently different in that the first is a relation in the subtyping lattice and is intrinsic to the type-systems, while the the second relation is an implementation detail of regionck.

This PR changes this behavior to use covariance, so..
Subtype(&'a u8 <: &'b u8) => Subtype('a <: 'b) => Outlives('a: 'b) => RegionSubRegion('b <= 'a)

Resolves #103676

r? @lcnr

lcnr

nice 👍

please also update it in the region solver though as we still flip the variance of regions there right now

compiler/rustc_hir_analysis/src/variance/constraints.rs

aliemjay · 2023-01-27T09:42:08Z

I've updated the PR description.

please also update it in the region solver though as we still flip the variance of regions there right now

I think that can be an issue for a separate PR. For now, Subtype('a <: 'b) => Outlives('a : 'b) => RegionSubRegion('b <= 'a) makes sense to land as-is, IMO.

lcnr · 2023-01-27T11:12:31Z

might impact perf

@bors try

Surprisingly, we consider the reference type &'a T to be contravaraint in its lifetime parameter. This is confusing and conflicts with the documentation we have in the reference, rustnomicon, and rustc-dev-guide. This also arguably not the correct use of terminology since we can use &'static u8 in a place where &' a u8 is expected, this implies that &'static u8 <: &' a u8 and consequently 'static <: ' a, hence covariance.

I don't think 'static <: 'a is a consequence of &'static u8 <: &'a u8. You can equally as well say &'static u8 <: &'a u8 holds if 'a <: 'static holds which is what references being contravariant over regions means 🤔

I am not too convinced that there is a meaningful difference between RegionSubRegion and subtyping for regions. By flipping the variance inside of the relation of regions instead of the type relation itself this is still pretty confusing. E.g. seeing lub being used in glb is a pretty big red flag that something wrong here '^^

If you don't intend to work on changing RegionSubRegion I am fine with landing this as is while keeping an issue open to also flip RegionSubRegion (or ideally, remove the whole concept of Sub from regionck and nll and instead always talk about outlives)

r=me after perf

bors · 2023-01-27T11:12:41Z

⌛ Trying commit 43cb610 with merge b5d497c598ccc0e78ce7392fced08b711191993d...

lcnr · 2023-01-27T11:12:52Z

@rust-timer queue

aliemjay · 2023-01-27T12:37:58Z

I don't think 'static <: 'a is a consequence of &'static u8 <: &'a u8. You can equally as well say &'static u8 <: &'a u8 holds if 'a <: 'static holds which is what references being contravariant over regions means thinking

Agree! In retrospect, this was a rather ridiculous argument :sweat smile:.

This however leaves us with only one justification for the change, and that is to bring the implementation in line with the documentation (see the Reference and Rustnomicon, which agree that 'static <: 'a.)

If you don't intend to work on changing RegionSubRegion I am fine with landing this as is while keeping an issue open to also flip RegionSubRegion (or ideally, remove the whole concept of Sub from regionck and nll and instead always talk about outlives)

One complication is that even when removing the concept of Sub from regionck we would still have to use glb in lub and vice-versa, because I don't think one can argue against lub('static, 'a) = 'static.

Since this is more complicated than I initially believed, I think a T-types FCP may be necessary.

lcnr · 2023-01-27T12:51:49Z

One complication is that even when removing the concept of Sub from regionck we would still have to use glb in lub and vice-versa, because I don't think one can argue against lub('static, 'a) = 'static.

One can argue against anything if one is so inclined :3 See this previous discussion: https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/relating.20regions.20variance/near/284076240

For covariant regions to make sense the idea is to think of regions as a set of aliases. This is apparently how polonius models regions ^^ there the lub('static, 'a) = 'a does in fact hold, as 'static means there are no aliasing regions or sth.

Not exactly sure what exactly is represented by the concept of an alias but I don't think we need an MCP for this as we've already had a previous discussion with signoff from niko, but we could open if you prefer ^^

aliemjay · 2023-01-27T13:00:51Z

Not exactly sure what exactly is represented by the concept of an alias but I don't think we need an MCP for this as we've already had a previous discussion with signoff from niko, but we could open if you prefer ^^

mmm, I think we can ignore the FCP.

bors · 2023-01-27T13:14:55Z

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

bors · 2023-01-27T13:14:55Z

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

rust-timer · 2023-01-27T17:06:16Z

Finished benchmarking commit (b5d497c598ccc0e78ce7392fced08b711191993d): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

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

Cycles

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

lcnr · 2023-01-27T17:31:24Z

alright depending on how you want go about this, @aliemjay

@bors delegate+

bors · 2023-01-27T17:31:28Z

✌️ @aliemjay can now approve this pull request

aliemjay · 2023-01-27T19:34:26Z

@bors r+

bors · 2023-01-27T19:34:30Z

📌 Commit 43cb610 has been approved by aliemjay

It is now in the queue for this repository.

lcnr · 2023-01-27T21:54:57Z

@bors rollup

lcnr · 2023-01-27T21:55:41Z

@bors r- shouldn't approve your own PRs ^^ @bors r=lcnr

@bors r+

bors · 2023-01-27T21:55:42Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Resolve documentation links in rustc and store the results in metadata #94857

bors · 2023-01-27T21:55:43Z

📌 Commit 43cb610 has been approved by `lcnr``

It is now in the queue for this repository.

bors · 2023-01-27T21:55:45Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Resolve documentation links in rustc and store the results in metadata #94857

bors · 2023-01-27T21:55:46Z

📌 Commit 43cb610 has been approved by lcnr

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#107022 (Implement `SpecOptionPartialEq` for `cmp::Ordering`) - rust-lang#107100 (Use proper `InferCtxt` when probing for associated types in astconv) - rust-lang#107103 (Use new solver in `evaluate_obligation` query (when new solver is enabled)) - rust-lang#107190 (Recover from more const arguments that are not wrapped in curly braces) - rust-lang#107306 (Correct suggestions for closure arguments that need a borrow) - rust-lang#107339 (internally change regions to be covariant) - rust-lang#107344 (Minor tweaks in the new solver) - rust-lang#107373 (Don't merge vtables when full debuginfo is enabled.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned lcnr Jan 26, 2023

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. labels Jan 26, 2023

internally change regions to be covariant

381187d

aliemjay force-pushed the covariant branch from 7647f93 to 381187d Compare January 27, 2023 01:05

lcnr reviewed Jan 27, 2023

View reviewed changes

compiler/rustc_hir_analysis/src/variance/constraints.rs Outdated Show resolved Hide resolved

update comment on trait objects

43cb610

aliemjay marked this pull request as ready for review January 27, 2023 09:44

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 27, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 27, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2023

matthiaskrgr mentioned this pull request Jan 28, 2023

Rollup of 8 pull requests #107400

Merged

bors merged commit a5caa98 into rust-lang:master Jan 28, 2023

rustbot added this to the 1.69.0 milestone Jan 28, 2023

lcnr mentioned this pull request Jan 30, 2023

Internally change regions to be covariant #103676

Open

aliemjay deleted the covariant branch January 30, 2023 13:12

compiler-errors mentioned this pull request Oct 19, 2023

Document generic parameter variance in rustdoc rust-lang/rfcs#3515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internally change regions to be covariant #107339

internally change regions to be covariant #107339

aliemjay commented Jan 26, 2023 •

edited

Loading

lcnr left a comment •

edited

Loading

aliemjay commented Jan 27, 2023

lcnr commented Jan 27, 2023

bors commented Jan 27, 2023

lcnr commented Jan 27, 2023

This comment has been minimized.

aliemjay commented Jan 27, 2023 •

edited

Loading

lcnr commented Jan 27, 2023

aliemjay commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

This comment has been minimized.

rust-timer commented Jan 27, 2023

lcnr commented Jan 27, 2023 •

edited

Loading

bors commented Jan 27, 2023

aliemjay commented Jan 27, 2023

bors commented Jan 27, 2023

lcnr commented Jan 27, 2023

lcnr commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

internally change regions to be covariant #107339

internally change regions to be covariant #107339

Conversation

aliemjay commented Jan 26, 2023 • edited Loading

lcnr left a comment • edited Loading

Choose a reason for hiding this comment

aliemjay commented Jan 27, 2023

lcnr commented Jan 27, 2023

bors commented Jan 27, 2023

lcnr commented Jan 27, 2023

This comment has been minimized.

aliemjay commented Jan 27, 2023 • edited Loading

lcnr commented Jan 27, 2023

aliemjay commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

This comment has been minimized.

rust-timer commented Jan 27, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

lcnr commented Jan 27, 2023 • edited Loading

bors commented Jan 27, 2023

aliemjay commented Jan 27, 2023

bors commented Jan 27, 2023

lcnr commented Jan 27, 2023

lcnr commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

aliemjay commented Jan 26, 2023 •

edited

Loading

lcnr left a comment •

edited

Loading

aliemjay commented Jan 27, 2023 •

edited

Loading

lcnr commented Jan 27, 2023 •

edited

Loading