fix normalizing in different ParamEnvs with the same InferCtxt

[lukas-code]

This PR changes the key of the projection cache from just AliasTy to (AliasTy, ParamEnv) to allow normalizing in different ParamEnvs without resetting caches. Previously, normalizing the same alias in different param envs would always reuse the cached result from the first normalization, which is incorrect if the projection clauses in the param env have changed.

Fixing this bug allows us to get rid of InferCtxt::clear_caches, which was only used by the AutoTraitFinder, because it requires normalizing in different param envs.

r? @fmease

[fmease]

won't that lead to tons of cache misses?

[lukas-code]

Almost all code that isn't the AutoTraitFinder will always use the same ParamEnv for a single InferCtxt, so I don't expect there to be many cache misses if any at all. But maybe start a perf run just to confirm? (I don't have permission)

[fmease]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 31a05a2 with merge ceb60ad...

[bors]

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

[compiler-errors]

I thought I tried this before and the perf regression was really bad 🤔

[rust-timer]

Finished benchmarking commit (ceb60ad): 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.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-2.4%, -0.2%]	10
Improvements ✅ (secondary)	-0.9%	[-0.9%, -0.9%]	1
All ❌✅ (primary)	-0.6%	[-2.4%, 0.3%]	11

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.3%	[3.3%, 3.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.3%	[3.3%, 3.3%]	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)	6.3%	[5.3%, 7.3%]	5
Improvements ✅ (primary)	-1.8%	[-1.8%, -1.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.8%	[-1.8%, -1.8%]	1

Binary size

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

Bootstrap: 672.886s -> 671.734s (-0.17%)
Artifact size: 315.23 MiB -> 315.25 MiB (0.01%)

[compiler-errors]

Awesome. Glad to see this leads to a doc perf win, too.

[compiler-errors]

@bors r+

[bors]

📌 Commit 31a05a2 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit 31a05a2 with merge b6bd939...

[bors]

💔 Test failed - checks-actions

[compiler-errors]

Ugh. See #117131 and #117542.

Here's a minimal-ish example:

use std::future::Future;

struct Size<T>(T);

trait Foo {
    fn foo<K, V>() -> impl Future<Output = Size<impl Sized>> {
        async {
            Size(())
        }
    }
}

fn main() {}

We can no longer rely on the RPITIT -> opaque tys having been stashed in the projection cache, so we need to use the normalize_param_env everywhere in check_type_bounds. Not certain if that will regress behavior due to incompleteness of choosing certain predicates over others in the old trait solver.

[lukas-code]

We can no longer rely on the RPITIT -> opaque tys having been stashed in the projection cache

It's actually the opposite happening here: assumed_wf_types is will attempt to normalize the RPITIT in the normal param_env first, so we end up with RPITIT -> RPITIT in the projection cache.

rust/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs

Line 2005 in fecb7b4

let assumed_wf_types = ocx.assumed_wf_types_and_report_errors(param_env, impl_ty_def_id)?;

When normalizing the obligation in the normalize_param_env later, we pick up the RPITIT -> RPITIT from the cache, so we don't normalize RPITIT -> opaque type at all.

rust/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs

Lines 2038 to 2046 in fecb7b4

	let normalize_param_env = param_env_with_gat_bounds(tcx, impl_ty, impl_trait_ref);
	for mut obligation in util::elaborate(tcx, obligations) {
	let normalized_predicate =
	ocx.normalize(&normalize_cause, normalize_param_env, obligation.predicate);
	debug!("compare_projection_bounds: normalized predicate = {:?}", normalized_predicate);
	obligation.predicate = normalized_predicate;
	ocx.register_obligation(obligation);
	}

---

This makes me think that maybe we shouldn't normalize the RPITITs in default method implementations at all, because AFAICT there is not additional type information to be gained. Because it's impossible for a default method to specify a return type that's more concrete than the RPITIT that it's defining.

[compiler-errors]

This makes me think that maybe we shouldn't normalize the RPITITs in default method implementations at all, because AFAICT there is not additional type information to be gained. Because it's impossible for a default method to specify a return type that's more concrete than the RPITIT that it's defining.

I'm not certain what you mean -- RPITITs can specify default method bodies that constrain the opaque:

trait RPITIT {
    fn test() -> impl Sized { 0 }
}

[compiler-errors]

Oh, do you mean just in param_env_with_gat_bounds? I mean, if it works then sure. It's too early in the morning for me to consider the implications of not doing so though.

[lukas-code]

I'm not certain what you mean -- RPITITs can specify default method bodies that constrain the opaque:

Yeah but type checking will only see impl Sized and doesn't know that it's actually i32.

Opposed to this, wich actually knows the concrete type for <i32 as RPITIT>::test:

trait RPITIT {
    fn test() -> impl Sized;
}

impl RPITIT for i32 {
    fn test() -> i32 { 0 }
}

Oh, do you mean just in param_env_with_gat_bounds?

Yes, exactly. RevealAll etc still have to normalize the RPITIT of course.

[lukas-code]

Alright, I found the actual problem. It was a missing super_fold_with.

[compiler-errors]

Oh that's incredibly embarrassing but also i'm unsurprised that normalization usually hides this from being an issue. Thanks for looking into this.

@bors r+

[bors]

📌 Commit 5a2b335 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit 5a2b335 with merge 1b3fba0...

[bors]

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

[rust-timer]

Finished benchmarking commit (1b3fba0): 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)	4.1%	[4.1%, 4.1%]	1
Improvements ✅ (primary)	-0.7%	[-2.4%, -0.2%]	9
Improvements ✅ (secondary)	-0.9%	[-0.9%, -0.9%]	1
All ❌✅ (primary)	-0.7%	[-2.4%, -0.2%]	9

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.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	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)	3.9%	[3.9%, 3.9%]	1
Improvements ✅ (primary)	-1.4%	[-1.9%, -0.9%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.4%	[-1.9%, -0.9%]	2

Binary size

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

Bootstrap: 669.682s -> 674.072s (0.66%)
Artifact size: 315.52 MiB -> 315.57 MiB (0.02%)

[lukas-code]

The regression is likely noise, it went back down in the following merge: #124241 (comment) / #124241 (comment).

@rustbot label perf-regression-triaged