more eagerly instantiate binders #119849

lcnr · 2024-01-11T12:04:09Z

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 · 2024-01-11T12:04:15Z

@bors try

…<try> eagerly instantiate binders to avoid relying on `sub` 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

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

lcnr · 2024-01-11T12:39:37Z

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

+            obligation.cause.span,
+            HigherRankedType,
+            candidate,
+        );


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 · 2024-01-11T12:39:52Z

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

+            obligation.cause.span,
+            HigherRankedType,
+            unnormalized_upcast_trait_ref,
+        );


same here, can allow additional normalization

lcnr · 2024-01-11T12:40:00Z

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

+            obligation.cause.span,
+            HigherRankedType,
+            self_ty_trait_ref,
+        );


lcnr · 2024-01-11T12:40:19Z

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

-                }
-            })
-            .map_err(|_| ())
+            );


lcnr · 2024-01-11T12:45:57Z

tests/ui/generic-associated-types/issue-93340-1.rs

+
+fn build_expression<A: Scalar, B: Scalar, O: Scalar>(
+) -> impl Fn(A::RefType<'_>, B::RefType<'_>) -> O {
+    cmp_eq


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.

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.

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

lcnr · 2024-01-11T12:47:12Z

tests/ui/implied-bounds/gluon_salsa.rs

+    // implied bound due to the unconstrained lifetime.
+    //
+    // The `<Q as AsyncQueryFunction<'?y>>::SendDb`: 'a` bound then results in an error
+    // during typeck.
    pub fn get_async<'a>(&'a mut self) {


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

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(

Does that diff result in any other differences?

See the diagnostic changes in #121743

jackh726 · 2024-01-11T14:07:34Z

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 · 2024-01-11T15:03:29Z

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

@@ -2532,7 +2515,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
                    nested.extend(
                        self.infcx
                            .at(&obligation.cause, obligation.param_env)
-                            .sup(
+                            .eq(


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 😁

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?

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 · 2024-01-12T07:30:45Z

@bors try

…<try> eagerly instantiate binders to avoid relying on `sub` 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

bors · 2024-01-12T07:31:55Z

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

bors · 2024-01-12T08:58:31Z

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

lcnr · 2024-01-12T09:29:01Z

@rust-timer build 21bc403

@craterbot check

craterbot · 2024-01-12T09:29:12Z

👌 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 · 2024-01-12T12:57:57Z

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 · 2024-01-15T18:42:30Z

🚧 Experiment pr-119849 is now running

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

craterbot · 2024-01-17T14:44:12Z

🎉 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 after adding a test for the upcast case
r? compiler-errors

compiler-errors · 2024-03-12T19:46:24Z

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

@@ -2532,7 +2515,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
                    nested.extend(
                        self.infcx
                            .at(&obligation.cause, obligation.param_env)
-                            .sup(
+                            .eq(


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?

compiler-errors

r=me when green

compiler-errors · 2024-03-14T17:59:26Z

@bors r+

bors · 2024-03-14T17:59:29Z

📌 Commit c8f0f17 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-03-14T18:58:56Z

⌛ Testing commit c8f0f17 with merge fd27e87...

bors · 2024-03-14T21:04:29Z

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

rust-timer · 2024-03-14T23:04:02Z

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%)

…r=<try> Eagerly instantiate closure/coroutine-like bounds with placeholders to deal with binders correctly A follow-up to rust-lang#119849, however it aims to fix a different set of issues. Currently, we have trouble confirming goals where built-in closure/fnptr/coroutine signatures are compared against higher-ranked goals. Currently, we don't support goals like `for<'a> fn(&'a ()): Fn(&'a ())` because we don't expect the self type of goal to reference any bound regions from the goal, because we don't really know how to deal with the double binder of predicate + self type. However, this definitely can be reached (rust-lang#121653) -- and in fact, it results in post-mono errors in the case of rust-lang#112347 where the builtin type (e.g. a coroutine) is hidden behind a TAIT. The proper fix here is to instantiate the goal before trying to extract the signature from the self type. See final two commits. r? lcnr

Kobzol · 2024-03-19T08:35:00Z

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

lcnr · 2024-03-19T09:40:29Z

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

Kobzol · 2024-03-19T11:27:57Z

Ok, marking as triaged.

@rustbot label: +perf-regression-triaged

…r=lcnr Eagerly instantiate closure/coroutine-like bounds with placeholders to deal with binders correctly A follow-up to rust-lang#119849, however it aims to fix a different set of issues. Currently, we have trouble confirming goals where built-in closure/fnptr/coroutine signatures are compared against higher-ranked goals. Currently, we don't support goals like `for<'a> fn(&'a ()): Fn(&'a ())` because we don't expect the self type of goal to reference any bound regions from the goal, because we don't really know how to deal with the double binder of predicate + self type. However, this definitely can be reached (rust-lang#121653) -- and in fact, it results in post-mono errors in the case of rust-lang#112347 where the builtin type (e.g. a coroutine) is hidden behind a TAIT. The proper fix here is to instantiate the goal before trying to extract the signature from the self type. See final two commits. r? lcnr

rustbot assigned jackh726 Jan 11, 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 Jan 11, 2024

lcnr added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jan 11, 2024

This comment was marked as outdated.

Sign in to view

lcnr commented Jan 11, 2024

View reviewed changes

compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved

lcnr commented Jan 11, 2024

View reviewed changes

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

}

})

.map_err(|_| ())

);

Copy link

Contributor Author

lcnr Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

lcnr force-pushed the eagerly-instantiate-binders branch from c010b64 to 23e69c0 Compare January 11, 2024 12:41

lcnr commented Jan 11, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2024

rustbot added the perf-regression Performance regression. label Jan 12, 2024

craterbot removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jan 17, 2024

compiler-errors approved these changes Mar 12, 2024

View reviewed changes

rustbot assigned compiler-errors and unassigned BoxyUwU Mar 12, 2024

compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2024

apiraino removed the to-announce Announce this issue on triage meeting label Mar 14, 2024

lcnr added 3 commits March 14, 2024 17:19

eagerly instantiate binders to avoid relying on sub

24a1729

one must imagine tidy happy

6729e01

rebase

323069f

lcnr force-pushed the eagerly-instantiate-binders branch from 650afe8 to 323069f Compare March 14, 2024 16:19

add tests

c8f0f17

compiler-errors approved these changes Mar 14, 2024

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 14, 2024

bors merged commit fd27e87 into rust-lang:master Mar 14, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Mar 14, 2024

lcnr deleted the eagerly-instantiate-binders branch March 14, 2024 22:00

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 19, 2024

compiler-errors mentioned this pull request Apr 1, 2024

regression: type inference changes #123290

Closed

more eagerly instantiate binders #119849

more eagerly instantiate binders #119849

Conversation

lcnr commented Jan 11, 2024 • edited

lcnr commented Jan 11, 2024

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Jan 11, 2024 • edited

Choose a reason for hiding this comment

compiler-errors Jan 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Feb 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackh726 commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr commented Jan 12, 2024

bors commented Jan 12, 2024

bors commented Jan 12, 2024

lcnr commented Jan 12, 2024

This comment has been minimized.

craterbot commented Jan 12, 2024

rust-timer commented Jan 12, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

craterbot commented Jan 15, 2024

craterbot commented Jan 17, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors left a comment

Choose a reason for hiding this comment

compiler-errors commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 14, 2024

rust-timer commented Mar 14, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Kobzol commented Mar 19, 2024

lcnr commented Mar 19, 2024

Kobzol commented Mar 19, 2024

lcnr commented Jan 11, 2024 •

edited

lcnr Jan 11, 2024 •

edited

compiler-errors Jan 11, 2024 •

edited

lcnr Feb 27, 2024 •

edited