[WIP] Support param bounds on non-lifetime binders #115362

compiler-errors · 2023-08-29T21:52:30Z

This PR adds resolution and AST lowering for where clauses on binders. The meaning for for<T> now becomes for<T: Sized> like regular generics positions, and you are now able to write for<T: Trait> (and for<T: ?Sized>). Binder predicates are only considered in the new solver today. Since non-lifetime binders is an incomplete feature, I don't think we need to do any messaging to tell people that they don't work correctly in the old solver, but I'll see to that in a follow-up.

This PR then adds a new List<ty::Clause> to binders. Most places in the compiler shouldn't care about them existing, but eventually as support for non-lifetime binders gets fleshed out we should be more careful about asserting that they're handled. This will not happen in this PR.

Tracking:

Tracking Issue for non-lifetime binders #108185

compiler-errors · 2023-08-29T21:52:37Z

@bors try @rust-timer queue

bors · 2023-08-29T21:52:46Z

⌛ Trying commit 3471faa6dbd43943bb8dc2e3bdedf59fab546439 with merge ff357375eca19961c0e26a765585b8cf4b3a1b3c...

bors · 2023-08-29T21:59:57Z

💔 Test failed - checks-actions

bors · 2023-09-03T13:50:45Z

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

bors · 2023-09-14T06:31:36Z

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

bors · 2023-10-18T14:25:34Z

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

fmease · 2023-10-19T19:07:39Z

Obviously, this is still WIP. Here's one observation I made while playing around with this patch:

trait Trait<T> {}
fn f(_: impl for<T: ?Sized> Trait<T>) {}

This successfully compiles (-Ztrait-solver=next ofc) while I don't think it should. impl for<T: ?Sized> Trait<T> isn't WF since the contained Trait<T> requires T: Sized to hold to be WF which isn't the case here. Cf: fn g<T: ?Sized>(_: impl Trait<T>) (ofc this is far from semantically equivalent) doesn't compile, rightly so.

compiler-errors · 2023-10-19T19:11:22Z

@fmease: Yeah, that's pre-existing.

#![feature(non_lifetime_binders)]

trait Trait<T> {}
fn f<S: for<T> Trait<T>>(_: S) {}
//~^ Recall that `for<T>` on stable == `for<T: ?Sized>`

This is likely due to one of the many !obligation.has_escaping_bound_vars() in wf code.

compiler-errors · 2023-12-18T17:55:28Z

@bors try @rust-timer queue

bors · 2023-12-18T17:56:38Z

⌛ Trying commit 82ae54c with merge 2e849e3...

…where-clauses, r=<try> [WIP] Support param bounds on non-lifetime binders 👀 r? `@ghost`

bors · 2025-03-13T08:03:19Z

☀️ Try build successful - checks-actions
Build commit: abe0bfa (abe0bfa4da894ead245f7da89acdfcc10ab56bc9)

rust-timer · 2025-03-13T09:18:52Z

Finished benchmarking commit (abe0bfa): comparison URL.

Overall result: ❌ regressions - please read the text below

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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.2%, 5.1%]	82
Regressions ❌ (secondary)	0.8%	[0.2%, 2.3%]	50
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.2%, 5.1%]	82

Max RSS (memory usage)

Results (primary 0.9%, secondary 2.5%)

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.5%	[0.6%, 4.4%]	13
Regressions ❌ (secondary)	3.1%	[1.2%, 4.8%]	7
Improvements ✅ (primary)	-1.8%	[-2.5%, -0.7%]	3
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	0.9%	[-2.5%, 4.4%]	16

Cycles

Results (primary 1.4%, secondary 1.9%)

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.4%	[0.9%, 3.4%]	16
Regressions ❌ (secondary)	1.9%	[1.7%, 2.4%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[0.9%, 3.4%]	16

Binary size

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

Bootstrap: 777.662s -> 789.655s (1.54%)
Artifact size: 365.21 MiB -> 365.67 MiB (0.13%)

compiler-errors · 2025-03-24T22:06:11Z

I'd like to land this is (mostly) the state that it's in, if only so that we now have a place to experiment with non-lifetime binders (both ty::Binder with clauses, but also the syntax itself).

This is IMO a necessary somewhat drastic hit to performance that I don't think is fixable, since we need to add new clauses to binders.

I'm happy to consider optimizations if people think that I'm missing something obvious here, though I'm not certain that the performance hit comes from anything other than just making binders larger in size and giving them more fields to have to be dealt with in common operations.

r? lcnr

rustbot · 2025-03-24T22:06:27Z

changes to the core type system

cc @compiler-errors, @lcnr

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

HIR ty lowering was modified

cc @fmease

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler-errors · 2025-03-24T22:07:17Z

cc @rust-lang/types rust-lang/types-team#81

oli-obk · 2025-03-25T11:40:25Z

I did some perf runs locally and it seems like most is inlining changes around type folders:

// nalgebra
10,911,801,289  ty::util::fold_list::<RegionEraserVisitor, &RawList<TypeInfo, Clause>, Clause, <&RawList<ty::list::TypeInfo, Clause> as TypeFoldable>::try_fold_with<RegionEraserVisitor>>::closure
-7,774,259,348  <Clause as TypeFoldable>::try_fold_with::<RegionEraserVisitor>

Similarly OpportunisticVarResolver and ArgFolder show up diesel, but they look like inlining changes.

While I can see us playing inliner golf, that's something we could have done before to find some more local optima. So 👍 on landing this and taking the perf hit (most of it is in instructions anyway, cycles is more on the 1% maximum regression side).

lcnr · 2025-03-26T18:00:37Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/bounds.rs

+                }
+                hir::GenericParamKind::Const { .. } => {
+                    let param_def_id = param.def_id.to_def_id();
+                    let ct_ty = tcx.type_of(param_def_id).instantiate_identity();


why do we shift types but not consts?

we don't support non lifetime consts :>

er, higher ranked consts.

lcnr · 2025-03-26T18:01:02Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/bounds.rs

+                    let ct = self.lower_const_param(param_def_id, param.hir_id);
+                    if let ty::ConstKind::Bound(..) = ct.kind() {
+                        // We don't allow const params in non-lifetime binders today.
+                        tcx.dcx().span_delayed_bug(


ì guess that's why xd 🤔 why not?

const eval is fucked up

specifically, given a situation like: for<const C: usize> [T; C]: Trait, that C in the array type will turn into an anon const that has an escaping bound ^C. If we were to support higher-ranked consts, then anon consts with bound consts in their scopes would need to insert additional early bound consts to capture them, kinda like how we do for late-bound regions and opaques.

See: https://github.com/rust-lang/rust/pull/115486/files#diff-a4c015afff6dad42c5509797bcc09a33e07fa6c5cef506e6e02374aed368c14c

this is actually not true anymore i think, we don't create anon consts for N arguments anymore

Yep, if we are doing the mGCA style of lowering for bound argument then it ""should""¹ just work. We probably still need to ban late-bound const params from any other anon consts that show up, but that can be a property of anon consts and not a property of binders.

Footnotes

well, we probably need to make sure that other operations on ty::Const will handle bound consts correctly, but they probably do? idk. ↩

for gce its fucked though yeah :3

lcnr · 2025-03-26T18:04:59Z

compiler/rustc_middle/src/ty/context.rs

@@ -2302,6 +2325,7 @@ nop_list_lift! {
    poly_existential_predicates; PolyExistentialPredicate<'a> => PolyExistentialPredicate<'tcx>
 }
 nop_list_lift! { bound_variable_kinds; ty::BoundVariableKind => ty::BoundVariableKind }
+nop_list_with_cached_type_info_lift! { clauses; Clause<'a> => Clause<'tcx> }


naming question.. what is the meaning of nop here? This lift is definitely not a noop, as we do have check that the list has actually been interned by the TyCtxt

That's what the nop means afaict; instead of re-interning the list, it's able to do the lifting by checking that the interned pointer originates from the tyctxt's arena. It's avoiding doing any actual work, hence "nop".

lcnr · 2025-03-26T18:09:17Z

compiler/rustc_middle/src/ty/sty.rs

@@ -2090,6 +2090,6 @@ mod size_asserts {
    use super::*;
    // tidy-alphabetical-start
    static_assert_size!(ty::RegionKind<'_>, 24);
-    static_assert_size!(ty::TyKind<'_>, 24);
+    static_assert_size!(ty::TyKind<'_>, 32);


I am personally not a big fan of supporting where-bounds inside of types and would like to only allow them in where-bounds. I also believe that this is where pretty much all of the perf regression is coming from.

I do expect that playing around with this feature is more annoying and ugly if we have to split binder for it, so 🤷 i guess i don't mind too much

lcnr · 2025-03-26T18:11:02Z

Currently don't have the mental capacity to review changes related to this feature in-depth

r? oli-obk maybe 🤔

nnethercote · 2025-03-26T21:41:51Z

w.r.t. perf: we have a lot of size assertions in the codebase, but the one on TyKind is one of the most important. There are many instances of that type and they are all interned, so if it gets bigger that's more memory usage and more hashing costs. It's up there with ast::Expr at the top of the list of "types I least want to get bigger" :(

compiler-errors · 2025-03-26T21:43:46Z

Maybe I can intern the pair of clauses lists into one or something 🤔 Ideas would be helpful

rustbot added S-waiting-on-review T-compiler WG-trait-system-refactor labels Aug 29, 2023