Collect relevant item bounds from trait clauses for nested rigid projections #120752

compiler-errors · 2024-02-07T20:46:13Z

Rust currently considers trait where-clauses that bound the trait's own associated types to act like an item bound:

trait Foo where Self::Assoc: Bar { type Assoc; }
// acts as if:
trait Foo { type Assoc: Bar; }

Background

This behavior has existed since essentially forever (i.e. before Rust 1.0), since we originally started out by literally looking at the where clauses written on the trait when assembling SelectionCandidate::ProjectionCandidate for projections. However, looking at the predicates of the associated type themselves was not sound, since it was unclear which predicates were assumed and which predicates were implied, and therefore this was reworked in #72788 (which added a query for the predicates we consider for ProjectionCandidates), and then finally item bounds and predicates were split in #73905.

Problem 1: GATs don't uplift bounds correctly

All the while, we've still had logic to uplift associated type bounds from a trait's where clauses. However, with the introduction of GATs, this logic was never really generalized correctly for them, since we were using simple equality to test if the self type of a trait where clause is a projection. This leads to shortcomings, such as:

trait Foo
where
    for<'a> Self::Gat<'a>: Debug,
{
    type Gat<'a>;
}

fn test<T: Foo>(x: T::Gat<'static>) {
    //~^ ERROR `<T as Foo>::Gat<'a>` doesn't implement `Debug`
    println!("{:?}", x);
}

Problem 2: Nested associated type bounds are not uplifted

We also don't attempt to uplift bounds on nested associated types, something that we couldn't really support until #120584. This can be demonstrated best with an example:

trait A
    where Self::Assoc: B,
    where <Self::Assoc as B>::Assoc2: C,
{
    type Assoc; // <~ The compiler *should* treat this like it has an item bound `B<Assoc2: C>`.
}

trait B { type Assoc2; }
trait C {}

fn is_c<T: C>() {}

fn test<T: A>() {
    is_c::<<Self::Assoc as B>::Assoc2>();
    //~^ ERROR the trait bound `<<T as A>::Assoc as B>::Assoc2: C` is not satisfied
}

Why does this matter?

Well, generalizing this behavior bridges a gap between the associated type bounds (ATB) feature and trait where clauses. Currently, all bounds that can be stably written on associated types can also be expressed as where clauses on traits; however, with the stabilization of ATB, there are now bounds that can't be desugared in the same way. This fixes that.

How does this PR fix things?

First, when scraping item bounds from the trait's where clauses, given a trait predicate, we'll loop of the self type of the predicate as long as it's a projection. If we find a projection whose trait ref matches, we'll uplift the bound. This allows us to uplift, for example <Self as Trait>::Assoc: Bound (pre-existing), but also <<Self as Trait>::Assoc as Iterator>::Item: Bound (new).

If that projection is a GAT, we will check if all of the GAT's own args are all unique late-bound vars. We then map the late-bound vars to early-bound vars from the GAT -- this allows us to uplift for<'a, 'b> Self::Assoc<'a, 'b>: Trait into an item bound, but we will leave for<'a> Self::Assoc<'a, 'a>: Trait and Self::Assoc<'static, 'static>: Trait alone.

Okay, but does this really matter?

I consider this to be an improvement of the status quo because it makes GATs a bit less magical, and makes rigid projections a bit more expressive.

lcnr · 2024-02-08T08:44:05Z

r=me after fcp

though after this PR, associated type bounds are always just a desugaring of otherwise writeable where-clauses on the trait, are they?

compiler-errors · 2024-03-02T18:50:57Z

though after this PR, associated type bounds are always just a desugaring of otherwise writeable where-clauses on the trait, are they?

So not exactly. We don't currently uplift associated types for GATs properly, e.g.

trait Foo where Self::Assoc<'a>: Iterator<Item = &'a ()> { type Assoc<'a>; }

IDK if we should. Currently that code even errors with E0582:

error[E0582]: binding for associated type `Item` references lifetime `'a`, which does not appear in the trait input types

lcnr · 2024-03-04T10:33:26Z

I feel like the following should compile: 'a is a higher-ranked region and constrained by the GAT. I would like that bound to also get hoisted to the item_bounds but idk how difficult this is to implemented.

trait Foo where for<'a> Self::Assoc<'a>: Iterator<Item = &'a ()> { type Assoc<'a>; }

compiler-errors · 2024-03-04T15:07:15Z

I would like that bound to also get hoisted to the item_bounds but idk how difficult this is to implemented.

Shouldn't be too hard, just need to map them to their early-bound indices.

feel like the following should compile: 'a is a higher-ranked region and constrained by the GAT.

IDK much about the motivation behind E0582.

compiler-errors · 2024-03-05T17:56:43Z

Ok this is ready for a review at least :>

lcnr

please add a test where a GAT is inside of another projection, for<'a, 'b> <<Self::Gat<'a> as Bound>::OtherGat<'b> or sth

compiler/rustc_hir_analysis/src/collect/item_bounds.rs

lcnr · 2024-03-05T18:13:44Z

If I am not wrong, this PR ends up causing associated type bounds to be a simple desugaring. This seems valuable as they can be ignored when explaining the language.

I also think that uplifting gat where-bounds to item bounds is very clearly desirable and doing so for nested also seems like an improvement, even if it likely only infrequently used.

With this

@rfcbot fcp merge

rfcbot · 2024-03-05T18:13:47Z

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

compiler-errors · 2024-03-05T18:18:27Z

If I am not wrong, this PR ends up causing associated type bounds to be a simple desugaring. This seems valuable as they can be ignored when explaining the language.

It's a simple desugaring in where clauses, APIT, and in associated type's item bounds. The only place it remains not a simple deusgaring is for opaques, i.e. impl Iterator<Item: Bound> remains unexpressible with surface Rust w/o ATB. Kind of a shame that that remains unexpressible, but I'd rather not block this on that.

lcnr · 2024-03-05T18:34:04Z

What's the meaming of impl Iterator<Item: Bound>. It is not impl Iterator<Item = impl Bound>, but instead existential type RPIT: Iterator where Self::Item: Bound;? For RPIT, is there an observable difference between using an nested opaque and using an (currently not otherwise expressable) item bound?

compiler-errors · 2024-03-05T18:38:48Z

It is not impl Iterator<Item = impl Bound>, but instead existential type RPIT: Iterator where Self::Item: Bound;?

Yes; this was what #120584 achieved by always lowering associated type bounds as bounds and never as nested RPITs.

is there an observable difference between using an nested opaque and using an (currently not otherwise expressable) item bound?

Yep, since we're not introducing a new RPIT here, we don't need to care about captures (especially higher-ranked ones). See the test:

rust/tests/ui/associated-type-bounds/higher-ranked.rs

Lines 1 to 17 in 96561a8

 //@ check-pass 

 #![feature(associated_type_bounds)] 

 trait A<'a> { 

 type Assoc: ?Sized; 

 } 

 impl<'a> A<'a> for () { 

 type Assoc = &'a (); 

 } 

 fn hello() -> impl for<'a> A<'a, Assoc: Sized> { 

 () 

 } 

 fn main() {}

Specifically, impl for<'a> A<'a, Assoc: Sized> is more powerful in impl for<'a> A<'a, Assoc = impl Sized + 'a>. I consider this to be a good thing, for the record.

oli-obk · 2024-03-07T11:24:54Z

I think it's somewhat obvious, but, uh, maybe mention in the main post that this is an insta-stable change 😆

So... this change can't break any code, because either it was erroring before due to the bound not being satisfied, or it already had the bound explicitly. There is no situation where the new implied bound in param env is now preferred over looking at global information?

lcnr · 2024-03-07T11:37:12Z

There is no situation where the new implied bound in param env is now preferred over looking at global information?

🤔 it only affects rigid associated types, and at this point impls don't really apply to it. I guess it can impact inference for <<This as Trait>::Assoc as Other>::Assoc: Bound<?x> where there's now an item bound constraining ?x, but also an always applicable impl for impl<T, U> Trait<U> for T.

I don't expect this to ever happen in practice, but it would be theoretically breaking:

trait Bound<T> {}
impl<T, U> Bound<T> for U {}

trait Trait
where
    <<Self as Trait>::Assoc as Other>::Assoc: Bound<u32>,
{
    type Assoc: Other;
}

trait Other {
    type Assoc;
}

fn impls_trait<T: Bound<U>, U>() -> Vec<U> { vec![] }

fn foo<T: Trait>() {
    let mut vec_u = impls_trait::<<<T as Trait>::Assoc as Other>::Assoc, _>();
    vec_u.sort();
    drop::<Vec<u8>>(vec_u);
}

fn main() {}

please add this as a test 😁

oli-obk · 2024-03-07T11:44:00Z

I don't expect this to ever happen in practice, but it would be theoretically breaking:

thanks. I imagined something like that but was unable to actually build the test 😆

compiler-errors · 2024-09-21T18:53:09Z

OK, I've finally quit procrastinating rebasing this.

@rustbot ready

lcnr · 2024-09-25T11:53:00Z

@bors r+ rollup=never

bors · 2024-09-25T11:53:04Z

📌 Commit 517208f has been approved by lcnr

It is now in the queue for this repository.

…, r=lcnr Collect relevant item bounds from trait clauses for nested rigid projections Rust currently considers trait where-clauses that bound the trait's *own* associated types to act like an item bound: ```rust trait Foo where Self::Assoc: Bar { type Assoc; } // acts as if: trait Foo { type Assoc: Bar; } ``` ### Background This behavior has existed since essentially forever (i.e. before Rust 1.0), since we originally started out by literally looking at the where clauses written on the trait when assembling `SelectionCandidate::ProjectionCandidate` for projections. However, looking at the predicates of the associated type themselves was not sound, since it was unclear which predicates were *assumed* and which predicates were *implied*, and therefore this was reworked in rust-lang#72788 (which added a query for the predicates we consider for `ProjectionCandidate`s), and then finally item bounds and predicates were split in rust-lang#73905. ### Problem 1: GATs don't uplift bounds correctly All the while, we've still had logic to uplift associated type bounds from a trait's where clauses. However, with the introduction of GATs, this logic was never really generalized correctly for them, since we were using simple equality to test if the self type of a trait where clause is a projection. This leads to shortcomings, such as: ```rust trait Foo where for<'a> Self::Gat<'a>: Debug, { type Gat<'a>; } fn test<T: Foo>(x: T::Gat<'static>) { //~^ ERROR `<T as Foo>::Gat<'a>` doesn't implement `Debug` println!("{:?}", x); } ``` ### Problem 2: Nested associated type bounds are not uplifted We also don't attempt to uplift bounds on nested associated types, something that we couldn't really support until rust-lang#120584. This can be demonstrated best with an example: ```rust trait A where Self::Assoc: B, where <Self::Assoc as B>::Assoc2: C, { type Assoc; // <~ The compiler *should* treat this like it has an item bound `B<Assoc2: C>`. } trait B { type Assoc2; } trait C {} fn is_c<T: C>() {} fn test<T: A>() { is_c::<<Self::Assoc as B>::Assoc2>(); //~^ ERROR the trait bound `<<T as A>::Assoc as B>::Assoc2: C` is not satisfied } ``` Why does this matter? Well, generalizing this behavior bridges a gap between the associated type bounds (ATB) feature and trait where clauses. Currently, all bounds that can be stably written on associated types can also be expressed as where clauses on traits; however, with the stabilization of ATB, there are now bounds that can't be desugared in the same way. This fixes that. ## How does this PR fix things? First, when scraping item bounds from the trait's where clauses, given a trait predicate, we'll loop of the self type of the predicate as long as it's a projection. If we find a projection whose trait ref matches, we'll uplift the bound. This allows us to uplift, for example `<Self as Trait>::Assoc: Bound` (pre-existing), but also `<<Self as Trait>::Assoc as Iterator>::Item: Bound` (new). If that projection is a GAT, we will check if all of the GAT's *own* args are all unique late-bound vars. We then map the late-bound vars to early-bound vars from the GAT -- this allows us to uplift `for<'a, 'b> Self::Assoc<'a, 'b>: Trait` into an item bound, but we will leave `for<'a> Self::Assoc<'a, 'a>: Trait` and `Self::Assoc<'static, 'static>: Trait` alone. ### Okay, but does this *really* matter? I consider this to be an improvement of the status quo because it makes GATs a bit less magical, and makes rigid projections a bit more expressive.

bors · 2024-09-25T17:03:16Z

⌛ Testing commit 517208f with merge 68386dc...

bors · 2024-09-25T17:09:41Z

💔 Test failed - checks-actions

…ections, GATs

compiler-errors · 2024-09-25T17:13:51Z

Needed reformat after the rustfmt bump.

@bors r=lcnr

bors · 2024-09-25T17:13:54Z

📌 Commit c591475 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-09-25T21:12:10Z

⌛ Testing commit c591475 with merge 9e394f5...

bors · 2024-09-25T23:47:56Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 9e394f5 to master...

rust-timer · 2024-09-26T01:05:01Z

Finished benchmarking commit (9e394f5): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 2.6%)

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.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	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: 772.322s -> 773.661s (0.17%)
Artifact size: 340.90 MiB -> 340.89 MiB (-0.00%)

rustbot assigned lcnr Feb 7, 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Feb 7, 2024

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

rust-cloud-vms bot force-pushed the more-relevant-bounds branch from fa551a1 to 35d8667 Compare March 4, 2024 19:59

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the more-relevant-bounds branch from 35d8667 to 98fcb15 Compare March 4, 2024 20:16

compiler-errors marked this pull request as ready for review March 5, 2024 17:56

lcnr approved these changes Mar 5, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/collect/item_bounds.rs Outdated Show resolved Hide resolved

compiler/rustc_hir_analysis/src/collect/item_bounds.rs Show resolved Hide resolved

lcnr added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 5, 2024

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 5, 2024

compiler-errors mentioned this pull request Mar 6, 2024

Stabilize associated type bounds (RFC 2289) #122055

Merged

oli-obk approved these changes Mar 7, 2024

View reviewed changes

compiler-errors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2024

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

compiler-errors mentioned this pull request Sep 24, 2024

Associated type bounds are not equivalent to de-sugared where clauses for supertraits and nested associated types #130805

Open

fmease mentioned this pull request Sep 24, 2024

Higher-ranked trait bounds on associated types are not elaborated #50346

Open

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2024

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 25, 2024

compiler-errors added 3 commits September 25, 2024 13:13

Collect relevant item bounds from trait clauses for nested rigid proj…

2dacf7a

…ections, GATs

Pull out into helper function

149bd87

Add a few more tests, comments

c591475

compiler-errors force-pushed the more-relevant-bounds branch from 517208f to c591475 Compare September 25, 2024 17:13

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 25, 2024

bors merged commit 9e394f5 into rust-lang:master Sep 25, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Sep 25, 2024

compiler-errors deleted the more-relevant-bounds branch September 26, 2024 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect relevant item bounds from trait clauses for nested rigid projections #120752

Collect relevant item bounds from trait clauses for nested rigid projections #120752

compiler-errors commented Feb 7, 2024 •

edited

Loading

lcnr commented Feb 8, 2024 •

edited

Loading

compiler-errors commented Mar 2, 2024

lcnr commented Mar 4, 2024

compiler-errors commented Mar 4, 2024

This comment has been minimized.

compiler-errors commented Mar 5, 2024

lcnr left a comment

lcnr commented Mar 5, 2024

rfcbot commented Mar 5, 2024 •

edited

Loading

compiler-errors commented Mar 5, 2024 •

edited

Loading

lcnr commented Mar 5, 2024 •

edited

Loading

compiler-errors commented Mar 5, 2024

oli-obk commented Mar 7, 2024

lcnr commented Mar 7, 2024

oli-obk commented Mar 7, 2024

compiler-errors commented Sep 21, 2024

lcnr commented Sep 25, 2024

bors commented Sep 25, 2024

bors commented Sep 25, 2024

This comment has been minimized.

bors commented Sep 25, 2024

compiler-errors commented Sep 25, 2024

bors commented Sep 25, 2024

bors commented Sep 25, 2024

bors commented Sep 25, 2024

rust-timer commented Sep 26, 2024

Collect relevant item bounds from trait clauses for nested rigid projections #120752

Collect relevant item bounds from trait clauses for nested rigid projections #120752

Conversation

compiler-errors commented Feb 7, 2024 • edited Loading

Background

Problem 1: GATs don't uplift bounds correctly

Problem 2: Nested associated type bounds are not uplifted

How does this PR fix things?

Okay, but does this really matter?

lcnr commented Feb 8, 2024 • edited Loading

compiler-errors commented Mar 2, 2024

lcnr commented Mar 4, 2024

compiler-errors commented Mar 4, 2024

This comment has been minimized.

compiler-errors commented Mar 5, 2024

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Mar 5, 2024

rfcbot commented Mar 5, 2024 • edited Loading

compiler-errors commented Mar 5, 2024 • edited Loading

lcnr commented Mar 5, 2024 • edited Loading

compiler-errors commented Mar 5, 2024

oli-obk commented Mar 7, 2024

lcnr commented Mar 7, 2024

oli-obk commented Mar 7, 2024

compiler-errors commented Sep 21, 2024

lcnr commented Sep 25, 2024

bors commented Sep 25, 2024

bors commented Sep 25, 2024

This comment has been minimized.

bors commented Sep 25, 2024

compiler-errors commented Sep 25, 2024

bors commented Sep 25, 2024

bors commented Sep 25, 2024

bors commented Sep 25, 2024

rust-timer commented Sep 26, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

compiler-errors commented Feb 7, 2024 •

edited

Loading

lcnr commented Feb 8, 2024 •

edited

Loading

rfcbot commented Mar 5, 2024 •

edited

Loading

compiler-errors commented Mar 5, 2024 •

edited

Loading

lcnr commented Mar 5, 2024 •

edited

Loading