always resolve to universal regions if possible

[aliemjay]

RegionConstraintCollector::opportunistic_resolve_var, which is used in canonicalization and projection logic, doesn't resolve the region var to an equal universal region. So if we have equated 'static == '1 == '2, it doesn't resolve '1 or '2 to 'static. Now it does!

Addresses review comment #107376 (comment).

r? @lcnr

[bors]

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

[lcnr]

that merge conflict is unfortunate 😅

r=me after perf

[aliemjay]

The original change introduced a new unsoundness (see last commits). It is caused by resolving a region variable to a placeholder region from a universe that it can't name. I originally thought this is not a problem because region resolution doesn't rely on unification_table and we would eventually have a universe error, but it tuned out that canonicalization uses opportunistic resolution on query region constraints opening a loophole in this logic.

Theoretically, it is a preexisting bug because in the previous setup we could resolve a variable to another variable of higher universe, and thus allowing it to name more regions, but I can't think of an example where we unify two region variables from different universes inside a canonical query in order to exploit this.

[lcnr]

@bors try @rust-timer queue

I am unhappy with bfd3501 as it feels fairly brittle. Can look into this myself at some point, but how difficult is it to 1. eagerly error when unifying a region with a placeholder from a higher universe and 2. when unifying 2 regions 'a and 'b, set their universes to min(uv('a), uv('b)).

[bors]

⌛ Trying commit 095b5fa with merge 6296cbeaecfc306187cacaad62bbb14d77d96ded...

[bors]

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

[bors]

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

[rust-timer]

Finished benchmarking commit (6296cbeaecfc306187cacaad62bbb14d77d96ded): 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.7%	[0.7%, 0.7%]	2
Improvements ✅ (primary)	-0.6%	[-1.1%, -0.2%]	4
Improvements ✅ (secondary)	-1.6%	[-2.6%, -0.6%]	18
All ❌✅ (primary)	-0.6%	[-1.1%, -0.2%]	4

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.5%	[2.5%, 4.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[-2.9%, 4.6%]	3

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.8%	[-3.3%, -1.8%]	7
All ❌✅ (primary)	-	-	0

[aliemjay]

I am unhappy with bfd3501 as it feels fairly brittle. Can look into this myself at some point, but how difficult is it to 1. eagerly error when unifying a region with a placeholder from a higher universe and 2. when unifying 2 regions 'a and 'b, set their universes to min(uv('a), uv('b)).

I don't see a problem with the current approach. unification_table stores what vars/values we unified and only when we use it for anything useful we should concern ourselves with universes. I think the alternative approach would be unnecessarily painful and probably a perf hit.

Anway, I tried to implement alternative approach in https://github.com/aliemjay/rust/tree/resolve-var-region-alt but I don't really like it especially how the universes of region variables change during inference.

[aliemjay]

cc #108867

[lcnr]

looked a bit more into this and also at #108867

I think I agree with you now that this approach is nicer but thought of a way to make this harder to misuse by accident in the future.

r=me after this nit

[lcnr]

this is the only use of probe_value for regions 🤔 so yeah I agree that this is fine.

Can you add a comment to UnifiedRegion that the assigned value may not be nameable by the region var?

Or even better, change unified region to

pub(super?) struct UnifiedRegion {
    resolved: Option<Region<'tcx>>,
}

impl UnifiedRegion {
    /// Something something the resolved region may not be nameable by
    /// this region variable.
    pub fn get_resolved_region_ignoring_universes(self) -> Option<Region<'tcx>> {
        self.resolved
    }
}

[lcnr]

@bors r+ rollup=never

[bors]

📌 Commit 228f408 has been approved by lcnr

It is now in the queue for this repository.

[aliemjay]

@lcnr one more commit to review 😅

[aliemjay]

I'll pull it to another PR. It's not that important.

[lcnr]

@bors r-

@bors r+

[bors]

📌 Commit 0b232d0 has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit 0b232d0 with merge 7c306f6...

[bors]

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

[rust-timer]

Finished benchmarking commit (7c306f6): 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
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	2
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	3
Improvements ✅ (secondary)	-1.4%	[-1.7%, -0.7%]	9
All ❌✅ (primary)	-0.2%	[-0.3%, -0.2%]	3

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.7%	[1.1%, 2.2%]	2
Improvements ✅ (primary)	-3.3%	[-3.3%, -3.2%]	2
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	-3.3%	[-3.3%, -3.2%]	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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-3.1%, -2.6%]	7
All ❌✅ (primary)	-	-	0

[nnethercote]

The improvements greatly outweight the miniscule regressions here. The cycles and wall-time numbers for wg-grammar are especially nice.

@rustbot label: +perf-regression-triaged