region unification: update universe of region vars #121442

lcnr · 2024-02-22T11:08:02Z

necessary for #119106. see inline comment for why this is necessary

lcnr · 2024-02-22T11:08:11Z

region unification: update universe of region vars necessary for rust-lang#119106. see inline comment for why this is necessary r? `@compiler-errors` `@BoxyUwU`

bors · 2024-02-22T11:09:20Z

⌛ Trying commit a7ea3de with merge 9d779e1...

lcnr · 2024-02-22T11:17:20Z

compiler/rustc_infer/src/infer/region_constraints/mod.rs

-            .unwrap_or_else(|| ty::Region::new_var(tcx, root_vid));
-
-        // Don't resolve a variable to a region that it cannot name.
-        if self.var_universe(vid).can_name(self.universe(resolved)) {


if we equate an infer var with a root which is in a higher universe, we previously did not return that root vid here. This caused the PlaceholderReplacer to not map a placeholder region back to a bound var in deeply_normalize (with #119106) due to the following setup:

for<'a> fn(RigidAlias<'a>) gets instantiated as fn(RigidAlias<'!a>)

normalizing RigidAlias<'!a> emits AliasRelate(RigidAlias<'!a>, ?u)

uniquifying regions in the input results in the canonical goal exists<U> exists<'a> AliasRelate(RigidAlias<'a>, U)

inside of the query we instantiate this to get RigidAlias<'?a.1> and ?u.0

NormalizesTo fails, so we first instantiate ?u.0 with RigidAlias<'new.whatever>, the universe of that gets pulled down into universe 0

we then equate the args of RigidAlias<'new.0> with RigidAlias<'a.1>, equating 'new and 'a, using 'a as the root in the unification table

when canonicalizing the response, we are unable to canonicalize 'new as 'a because the universe of 'a is higher than the universe of 'new

because of this RigidAlias<'!a> normalizes to RigidAlias<'new> where 'new is constrained to be equal to '!a

we fail to map 'new back to 'a in the PlaceholderReplacer

inside of the query we instantiate this to get RigidAlias<'?a.1> and ?u.0

This looks like an issue with the new solver canonicalization! the universe of ?u should certainly be able to name '?a. How does ?a end up in U1 and ?u as U0?

In the new solver all regions are canonicalized to new inference variables in a single universe that is higher than everything else in the input. ?u can name '?a even with this setup it just requires lowering '?a's universe when equating it which is something we already do for cases like ?x.0 eq ?y.1. Even if the new solver did not have this canonicalization scheme I would still expect this PR to be required somehow as it is just wrong for '?x.0 eq '?y.1 to not allow resolving '?x to '?y later on and there should be other ways to wind up with a region variable in a higher universe that is equated with something in a lower one

compiler/rustc_middle/src/infer/unify_key.rs

lcnr · 2024-02-22T12:19:25Z

cc @aliemjay #108121

compiler-errors · 2024-02-22T15:21:34Z

compiler/rustc_middle/src/infer/unify_key.rs

+    fn unify_values(value1: &Self, value2: &Self) -> Result<Self, Self::Error> {
+        match (*value1, *value2) {
+            (RegionVariableValue::Known { .. }, RegionVariableValue::Known { .. }) => {
+                // FIXME: We could alternatively use the value with a lower universe,


Why didn't you implement this? Do we expect this branch to be hit?

I expect it to be hit, I can't completely tell whether it's desirable though 🤷

I personally don't think it is really 🤔 so yeah, should maybe just remove the FIXME 😁

yeah, I think that after reworking lexical region resolution, we may want to look into not emitting subtype constraints if we're able to unify the region vids. At that point unifying known regions won't work anymore anyways.

lcnr · 2024-02-22T16:00:36Z

@rust-timer build 9d779e1

rust-timer · 2024-02-22T17:18:11Z

Finished benchmarking commit (9d779e1): 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)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-2.0%, -0.4%]	8
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)	2.3%	[1.2%, 3.4%]	2
Regressions ❌ (secondary)	3.4%	[1.9%, 4.4%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-3.4%, -0.6%]	4
All ❌✅ (primary)	2.3%	[1.2%, 3.4%]	2

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
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.9%, -2.3%]	6
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 649.949s -> 649.436s (-0.08%)
Artifact size: 310.94 MiB -> 310.96 MiB (0.01%)

compiler-errors · 2024-02-22T17:26:47Z

thank u 👍

@bors r+

bors · 2024-02-22T17:26:50Z

📌 Commit 12b9040 has been approved by compiler-errors

It is now in the queue for this repository.

lcnr · 2024-02-22T17:55:17Z

removed the FIXME

@bors r=compiler-errors

bors · 2024-02-22T17:55:21Z

📌 Commit 16350d7 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-02-22T23:08:12Z

⌛ Testing commit 16350d7 with merge 78a224a...

…iler-errors region unification: update universe of region vars necessary for rust-lang#119106. see inline comment for why this is necessary r? `@compiler-errors` `@BoxyUwU`

rust-log-analyzer · 2024-02-23T00:02:08Z

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] text_edit test:false 0.724
[RUSTC-TIMING] expect_test test:false 2.015
[RUSTC-TIMING] countme test:false 1.405
[RUSTC-TIMING] rowan test:false 2.648
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
##[group]Clock drift check
  local time: Fri Feb 23 00:01:52 UTC 2024
  local time: Fri Feb 23 00:01:52 UTC 2024
Session terminated, killing shell... ...killed.
##[error]The operation was canceled.
Cleaning up orphan processes

bors · 2024-02-23T00:02:32Z

💔 Test failed - checks-actions

lcnr · 2024-02-23T07:41:44Z

@bors retry

bors · 2024-02-23T10:13:37Z

⌛ Testing commit 16350d7 with merge ea2cc43...

bors · 2024-02-23T12:11:43Z

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

rust-timer · 2024-02-23T13:27:06Z

Finished benchmarking commit (ea2cc43): 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.4%	[-1.9%, -0.4%]	8
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)	1.6%	[1.5%, 1.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-4.4%, -2.4%]	3
All ❌✅ (primary)	-	-	0

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.6%, -2.2%]	6
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 650.894s -> 649.932s (-0.15%)
Artifact size: 310.95 MiB -> 310.98 MiB (0.01%)

rustbot assigned compiler-errors 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. labels Feb 22, 2024

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 Feb 22, 2024

This comment has been minimized.

Sign in to view

lcnr commented Feb 22, 2024

View reviewed changes

lcnr force-pushed the region-var-universe-uwu branch from a7ea3de to 2bf4c79 Compare February 22, 2024 11:19

lcnr commented Feb 22, 2024

View reviewed changes

compiler/rustc_middle/src/infer/unify_key.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Feb 22, 2024

View reviewed changes

compiler-errors approved these changes Feb 22, 2024

View reviewed changes

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 Feb 22, 2024

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 Feb 22, 2024

lcnr added 2 commits February 22, 2024 18:54

region unification update universe of region vars

db950ef

add comment

16350d7

lcnr force-pushed the region-var-universe-uwu branch from 12b9040 to 16350d7 Compare February 22, 2024 17:54

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

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 Feb 23, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 23, 2024

bors merged commit ea2cc43 into rust-lang:master Feb 23, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 23, 2024

bors mentioned this pull request Feb 23, 2024

avoid generalization inside of aliases #119106

Merged

lcnr deleted the region-var-universe-uwu branch February 26, 2024 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

region unification: update universe of region vars #121442

region unification: update universe of region vars #121442

lcnr commented Feb 22, 2024

lcnr commented Feb 22, 2024

This comment has been minimized.

bors commented Feb 22, 2024

This comment has been minimized.

lcnr Feb 22, 2024 •

edited

Loading

aliemjay Apr 2, 2024 •

edited

Loading

BoxyUwU Apr 2, 2024

lcnr commented Feb 22, 2024

compiler-errors Feb 22, 2024

lcnr Feb 22, 2024

lcnr Feb 22, 2024

lcnr commented Feb 22, 2024

This comment has been minimized.

rust-timer commented Feb 22, 2024

compiler-errors commented Feb 22, 2024

bors commented Feb 22, 2024

lcnr commented Feb 22, 2024

bors commented Feb 22, 2024

bors commented Feb 22, 2024

rust-log-analyzer commented Feb 23, 2024

bors commented Feb 23, 2024

lcnr commented Feb 23, 2024

bors commented Feb 23, 2024

bors commented Feb 23, 2024

rust-timer commented Feb 23, 2024

region unification: update universe of region vars #121442

region unification: update universe of region vars #121442

Conversation

lcnr commented Feb 22, 2024

lcnr commented Feb 22, 2024

This comment has been minimized.

bors commented Feb 22, 2024

This comment has been minimized.

lcnr Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

aliemjay Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr commented Feb 22, 2024

This comment has been minimized.

rust-timer commented Feb 22, 2024

Overall result: ✅ improvements - no action needed

compiler-errors commented Feb 22, 2024

bors commented Feb 22, 2024

lcnr commented Feb 22, 2024

bors commented Feb 22, 2024

bors commented Feb 22, 2024

rust-log-analyzer commented Feb 23, 2024

bors commented Feb 23, 2024

lcnr commented Feb 23, 2024

bors commented Feb 23, 2024

bors commented Feb 23, 2024

rust-timer commented Feb 23, 2024

Overall result: ✅ improvements - no action needed

lcnr Feb 22, 2024 •

edited

Loading

aliemjay Apr 2, 2024 •

edited

Loading