more eagerly instantiate binders

[lcnr]

The old solver sometimes incorrectly used sub, change it to explicitly instantiate binders and use eq instead. While doing so I also moved the instantiation before the normalize calls. This caused some observable changes, will explain these inline. This PR therefore requires a crater run and an FCP.

r? types

[lcnr]

@bors try

[lcnr]

By instantiating before we normalize, we allow more projections to be replaced by infer vars if their normalization is ambiguous, e.g. for<'a> <?0 as Trait<'a>>::Assoc stays as is while <?0 as Trait<'!a>>::Assoc gets replaced with an inference variable ?term and result in a nested Projection(<?0 as Trait<'!a>>::Assoc, ?term) goal.

[lcnr]

same here, can allow additional normalization

[lcnr]

and here

[lcnr]

and here

[lcnr]

This previously compiled because of incompleteness when proving for<'a, 'b> cmp_eq<?0, ?1, ?2>: Fn(A::RefType<'a>, B::RefType<'b>) -> O

This constrained ?0 to A even though there could have been another type for which <?x as Scalar>::RefType<'a> eq <A as Scalar>::RefType<'a>> holds, this is related to #102048 and is fixed because we now normalize for<'a> <?x as Scalar>::RefType<'a> after instantiating the binder, causing it to be replaced with a fresh infer var.

[compiler-errors]

I believe the (new) behavior is correct -- #108918 (comment).

I don't believe this test should have ever compiled because it relies on the incompleteness of not normalizing aliases with escaping bound regions.

[lcnr]

Yes, should have stated so explicitly, this breakage is desired imo

[lcnr]

this one is interesting, have to figure out why this test previously compiled

[lcnr]

this previously compiled as in match_poly_trait_ref we used sup which ends up using Sub with lhs and rhs flipped. Not flipping them results in the region infer var of the normalized self type to not be the root var. We then use this self type for the implied bounds, canonicalization of which does opportunistically resolve regions, so the resulting implied bound uses the semantically equal, but not structurally equal root infer var.

Computing the verify bounds for the alias outlives then does a structural equality check which now fails, as we never opportunistically resolve regions in outlives bounds.

The following diff fixes this:

diff --git a/compiler/rustc_infer/src/infer/outlives/obligations.rs b/compiler/rustc_infer/src/infer/outlives/obligations.rs
index c0a99e5cc41..a17b099bf4f 100644
--- a/compiler/rustc_infer/src/infer/outlives/obligations.rs
+++ b/compiler/rustc_infer/src/infer/outlives/obligations.rs
@@ -62,6 +62,7 @@
 use crate::infer::outlives::components::{push_outlives_components, Component};
 use crate::infer::outlives::env::RegionBoundPairs;
 use crate::infer::outlives::verify::VerifyBoundCx;
+use crate::infer::resolve::OpportunisticRegionResolver;
 use crate::infer::{
     self, GenericKind, InferCtxt, RegionObligation, SubregionOrigin, UndoLog, VerifyBound,
 };
@@ -71,6 +72,7 @@
 use rustc_middle::traits::query::NoSolution;
 use rustc_middle::ty::{self, GenericArgsRef, Region, Ty, TyCtxt, TypeVisitableExt};
 use rustc_middle::ty::{GenericArgKind, PolyTypeOutlivesPredicate};
+use rustc_middle::ty::TypeFoldable;
 use rustc_span::DUMMY_SP;
 use smallvec::smallvec;
 
@@ -177,6 +179,7 @@ pub fn process_registered_region_obligations(
                         .no_bound_vars()
                         .expect("started with no bound vars, should end with no bound vars");
 
+                let (sup_type, sub_region) = (sup_type, sub_region).fold_with(&mut OpportunisticRegionResolver::new(self));
                 debug!(?sup_type, ?sub_region, ?origin);
 
                 let outlives = &mut TypeOutlives::new(

[jackh726]

Does that diff result in any other differences?

[compiler-errors]

See the diagnostic changes in #121743

[jackh726]

Test changes overall look fine, other than gluon_salsa that I have to think more about.

Haven't really reviewed the code changes, but I'll do that soonish, since we have to wait for process anyways.

[lcnr]

these changes are currently breaking, changed them to match the new solver for now (but both solvers should probably instantiate eagerly to allow instantiating hr trait objects when upcasting)

we need tests for all of this though 😁

[compiler-errors]

Consequence of this is what? That we can't do:

trait Supertrait<'a, 'b> {}
trait Subtrait: for<'a, 'b> Supertrait<'a, 'b> {}
let upcast: &dyn for<'a> Supertrait<'a, 'a> = todo!() as &dyn Subtrait;

Because of higher-ranked eq?

[lcnr]

Nope, the consequence is that it fixes a previously unknown unsoundness 🤣

#![feature(trait_upcasting)]
trait Supertrait<'a, 'b> {
    fn cast(&self, x: &'a str) -> &'b str;
}

trait Subtrait<'a, 'b>: Supertrait<'a, 'b> {}

impl<'a> Supertrait<'a, 'a> for () {
    fn cast(&self, x: &'a str) -> &'a str {
        x
    }
}
impl<'a> Subtrait<'a, 'a> for () {}
fn unsound(x: &dyn for<'a> Subtrait<'a, 'a>) -> &dyn for<'a, 'b> Supertrait<'a, 'b> {
    x
}

fn transmute<'a, 'b>(x: &'a str) -> &'b str {
    unsound(&()).cast(x)
}

fn main() {
    let x;
    {
        let mut temp = String::from("hello there");
        x = transmute(temp.as_str());
    }
    println!("{x}");
}

[lcnr]

@bors try

[bors]

⌛ Trying commit 23e69c0 with merge 21bc403...

[bors]

☀️ Try build successful - checks-actions
Build commit: 21bc403 (21bc403d557a2516df70ea80fb19b94177beede5)

[lcnr]

@rust-timer build 21bc403

@craterbot check

[craterbot]

👌 Experiment pr-119849 created and queued.
🤖 Automatically detected try build 21bc403
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rust-timer]

Finished benchmarking commit (21bc403): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.5%	[0.2%, 0.9%]	13
Regressions ❌ (secondary)	1.2%	[0.3%, 1.5%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.5%, -1.8%]	6
All ❌✅ (primary)	0.5%	[0.2%, 0.9%]	13

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)	1.3%	[1.3%, 1.3%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	1.3%	[1.3%, 1.3%]	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)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 666.39s -> 665.793s (-0.09%)
Artifact size: 308.38 MiB -> 308.37 MiB (-0.00%)

[craterbot]

🚧 Experiment pr-119849 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-119849 is completed!
📊 1539 regressed and 5 fixed (407280 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[compiler-errors]

r=me when green

[compiler-errors]

@bors r+

[bors]

📌 Commit c8f0f17 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit c8f0f17 with merge fd27e87...

[bors]

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

[rust-timer]

Finished benchmarking commit (fd27e87): 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.7%	[0.2%, 1.1%]	14
Regressions ❌ (secondary)	0.5%	[0.4%, 1.0%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.4%, -1.1%]	6
All ❌✅ (primary)	0.7%	[0.2%, 1.1%]	14

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)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-3.8%	[-3.8%, -3.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.8%	[-3.8%, -3.8%]	1

Cycles

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

Binary size

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

Bootstrap: 668s -> 668.42s (0.06%)
Artifact size: 311.58 MiB -> 311.47 MiB (-0.04%)

[Kobzol]

@lcnr I assume that the perf. hit was expected, given that this is a trait solver fix?

[lcnr]

yeah. I think so. By being able to replace more associated types with inference vars, the trait solver has to do more work

[Kobzol]

Ok, marking as triaged.

@rustbot label: +perf-regression-triaged