ReErased regions are local

[lukas-code]

fix #107678
fix #107684
fix #107686
fix #107691
fix #107730

[rustbot]

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

[compiler-errors]

Is there any explanation for why this fixes the issue? The description is a bit light

[lukas-code]

Is there any explanation for why this fixes the issue? The description is a bit light

Ok so basically there's this function ty.is_global(), which is supposed to check if a type is independent of the "caller bounds" of the surrounding context (ParamEnv):

rust/compiler/rustc_middle/src/ty/visit.rs

Lines 160 to 165 in 0c13c17

	/// Indicates whether this value references only 'global'
	/// generic parameters that are the same regardless of what fn we are
	/// in. This is used for caching.
	fn is_global(&self) -> bool {
	!self.has_type_flags(TypeFlags::HAS_FREE_LOCAL_NAMES)
	}

For example:

fn generic<T>() {
    let a = Vec::<i32>::new(); // The type `Vec<i32>` is "global".
    let a = Vec::<T>::new(); // The type `Vec<T>` is "local", because it depends on `T`.
}

This information is then used to "simplify" trait resolution in several places, for example:

(1):

rust/compiler/rustc_middle/src/ty/mod.rs

Lines 1722 to 1728 in 0c13c17

	Reveal::All => {
	if value.is_global() {
	ParamEnvAnd { param_env: self.without_caller_bounds(), value }
	} else {
	ParamEnvAnd { param_env: self, value }
	}
	}

(2):

rust/compiler/rustc_trait_selection/src/traits/select/mod.rs

Lines 1005 to 1014 in 0c13c17

	if !self.is_intercrate()
	&& obligation.is_global()
	&& obligation.param_env.caller_bounds().iter().all(|bound| bound.needs_subst())
	{
	// If a param env has no global bounds, global obligations do not
	// depend on its particular value in order to work, so we can clear
	// out the param env and get better caching.
	debug!("in global");
	obligation.param_env = obligation.param_env.without_caller_bounds();
	}

(3):

rust/compiler/rustc_trait_selection/src/traits/fulfill.rs

Lines 629 to 639 in 0c13c17

	if obligation.predicate.is_global() {
	// no type variables present, can use evaluation for better caching.
	// FIXME: consider caching errors too.
	if infcx.predicate_must_hold_considering_regions(obligation) {
	debug!(
	"selecting trait at depth {} evaluated to holds",
	obligation.recursion_depth
	);
	return ProcessResult::Changed(vec![]);
	}
	}

In particular, for the following code, (1) will delete the caller bounds from this code if the lifetime 'a is erased. This will will lead to bogus results when resolving the projection <&'a str as Trait2>::Assoc in the MIR. This is the root cause of the linked issues.

impl<'a> Trait1 for &'a str
where
    &'a str: Trait2 // <- the caller bounds
{
    fn foo(_: <&'a str as Trait2>::Assoc) {}
}

As far as I can tell this has been wrong forever, but in most real-world cases happened to "work fine" until recently, presumably due to caching. In particular this code happens compile (mostly) fine if Trait2 is unconditionally implemented for &'a str, but will ICE in various ways if it isn't implemented.

Additionally, (3) will lead to worse diagnostics for https://github.com/rust-lang/rust/blob/0c13c172507f01d921808107d2c4ec37b43b982d/tests/ui/recursion/issue-83150.rs at best and cause catastrophic unsoundness at worst, but I haven't been able to produce a concrete example for this and it might not be an issue at all.

This patch fixes all of the above by considering Type<'erased> as "local", i.e. dependent on the "caller bounds" at the cost of slightly worse caching.

---

Update: After looking at this again, it seems like (2) is actually a no-op since obligation.param_env.caller_bounds() is always empty if the condition is true. It least this is the case for all tests in the UI test suite.

[winterqt]

Would a bisect help, as this has started happening relatively recently?

[lukas-code]

Bisect for the vector issue is in #107440 (comment), which points at the rollup #104600.

This regressed in #104411. (I manually checked the try build from #104600 (comment))

Both #107267 and #104411 only reveal this issue. This has been broken since these kind of where clauses have been allowed in 1.7.0: godbolt link.

[compiler-errors]

Curious to see what happens to perf with this. Other than that, I guess this is technically ok -- ReErased isn't really a local region, but it could be.

@bors try @rust-timer queue

cc @rust-lang/types

[bors]

⌛ Trying commit e2a1a2a with merge 1c249e27a88dc284475b988d5fea5ec8f06f8ae8...

[bors]

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

[jackh726]

r=me with good perf

[rust-timer]

Finished benchmarking commit (1c249e27a88dc284475b988d5fea5ec8f06f8ae8): 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.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.5%, -0.4%]	9
Improvements ✅ (secondary)	-3.4%	[-4.2%, -2.5%]	8
All ❌✅ (primary)	-0.8%	[-1.5%, 0.6%]	10

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)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-2.6%	[-3.0%, -1.8%]	3
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-3.2%, -1.9%]	2
All ❌✅ (primary)	-	-	0

[compiler-errors]

very nice

@bors r=jackh726

[bors]

📌 Commit e2a1a2a has been approved by jackh726

It is now in the queue for this repository.

[ehuss]

@bors p=20

This is blocking Cargo's CI (and I assume others).

[bors]

⌛ Testing commit e2a1a2a with merge b082e80...

[bors]

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing b082e80 to master...

[rust-timer]

Finished benchmarking commit (b082e80): 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.8%	[-1.2%, -0.3%]	8
Improvements ✅ (secondary)	-1.5%	[-2.6%, -0.4%]	6
All ❌✅ (primary)	-0.8%	[-1.2%, -0.3%]	8

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)	4.5%	[4.5%, 4.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
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.8%	[-3.3%, -2.3%]	2
All ❌✅ (primary)	-	-	0