Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For a rigid projection, recursively look at the self type's item bounds to fix the associated_type_bounds feature #120584

Merged
merged 4 commits into from
Feb 10, 2024

Conversation

compiler-errors
Copy link
Member

Given a deeply nested rigid projection like <<<T as Trait1>::Assoc1 as Trait2>::Assoc2 as Trait3>::Assoc3, this PR adjusts both trait solvers to look at the item bounds for all of Assoc3, Assoc2, and Assoc1 in order to satisfy a goal. We do this because the item bounds for projections may contain relevant bounds for other nested projections when the associated_type_bounds (ATB) feature is enabled. For example:

#![feature(associated_type_bounds)]

trait Trait1 {
    type Assoc1: Trait2<Assoc2: Foo>;
    // Item bounds for `Assoc1` are:
    // `<Self as Trait1>::Assoc1: Trait2`
    // `<<Self as Trait1>::Assoc1 as Trait2>::Assoc2: Foo`
}

trait Trait2 {
    type Assoc2;
}

trait Foo {}

fn hello<T: Trait1>(x: <<T as Trait1>::Assoc1 as Trait2>::Assoc2) {
    fn is_foo(_: impl Foo) {}
    is_foo(x);
    // Currently fails with:
    // ERROR the trait bound `<<Self as Trait1>::Assoc1 as Trait2>::Assoc2: Foo` is not satisfied
}

This has been a long-standing place of brokenness for ATBs, and is also part of the reason why ATBs currently desugar so differently in various positions (i.e. sometimes desugaring to param-env bounds, sometimes desugaring to RPITs, etc). For example, in RPIT and TAIT position, impl Foo<Bar: Baz> currently desugars to impl Foo<Bar = impl Baz> because we do not currently take advantage of these nested item bounds if we desugared them into a single set of item bounds on the opaque. This is obviously both strange and unnecessary if we just take advantage of these bounds as we should.

Approach

This PR repeatedly peels off each projection of a given goal's self type and tries to match its item bounds against a goal, repeating with the self type of the projection. This is pretty straightforward to implement in the new solver, only requiring us to loop on the self type of a rigid projection to discover inner rigid projections, and we also need to introduce an extra probe so we can normalize them.

In the old solver, we can do essentially the same thing, however we rely on the fact that projections should be normalized already. This is obviously not always the case -- however, in the case that they are not fully normalized, such as a projection which has both infer vars and, we bail out with ambiguity if we hit an infer var for the self type.

Caveats

⚠️ In the old solver, this has the side-effect of actually stalling some higher-ranked trait goals of the form for<'a> <?0 as Tr<'a>>: Tr2. Because we stall them, they no longer are eagerly treated as error -- this cause some existing known-bug tests to go from fail -> pass.

I'm pretty unconvinced that this is a problem since we make code that we expect to pass in the new solver also pass in the old solver, though this obviously doesn't solve the full problem.

And then also...

We also adjust the desugaring of ATB to always desugar to a regular associated bound, rather than sometimes to an impl Trait except for when the ATB is present in a dyn Trait. We need to lower dyn Trait<Assoc: Bar> to dyn Trait<Assoc = impl Bar> because object types need all of their associated types specified.

I would also be in favor of splitting out the ATB feature and/or removing support for object types in order to stabilize just the set of positions for which the ATB feature is consistent (i.e. always elaborates to a bound).

@rustbot
Copy link
Collaborator

rustbot commented Feb 2, 2024

r? @nnethercote

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 2, 2024

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@compiler-errors
Copy link
Member Author

r? lcnr

Needs FCP too. Would be open to landing the new trait solver changes separately since they don't affect as much as the old trait solver.

@rustbot rustbot assigned lcnr and unassigned nnethercote Feb 2, 2024
@@ -1,5 +1,4 @@
// NOTE: rustc cannot currently handle bounds of the form `for<'a> <Foo as Bar<'a>>::Assoc: Baz`.
// This should hopefully be fixed with Chalk.
// check-pass
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was already not even well-formed. I removed the items that were actually missing, and fixed the test to actually make sense.

--> $DIR/normalizing-self-auto-trait-issue-109924.rs:22:11
|
LL | build(Bar);
| ----- ^^^ `impl Future<Output = ()> { <_ as Foo>::bar() }` cannot be sent between threads safely
Copy link
Member Author

Choose a reason for hiding this comment

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

Side-effect: Due to stalling the <_ as Foo>::Bar projection.

@@ -1,11 +1,10 @@
// check-pass
Copy link
Member Author

Choose a reason for hiding this comment

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

Side-effect due to stalling a projection w/ an infer self type.

// known-bug: #88460

// This should pass, but has a missed normalization due to HRTB.
// check-pass
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here, side-effect due to stalling a projection w/ an infer self type.

@@ -1,5 +1,4 @@
// check-fail
// known-bug: #90950
// check-pass
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here, side-effect due to stalling a projection w/ an infer self type.

@@ -1,5 +1,4 @@
// check-fail
// known-bug: #89196
// check-pass
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here, side-effect due to stalling a projection w/ an infer self type.

@compiler-errors compiler-errors added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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 Feb 2, 2024
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

please forbid its use in trait objects, they are fundamentally different from other positions, user should write dyn Trait<Item = impl Trait> instead. Either in this PR or a separate one.

some style nits, then r=me. From what I can tell this cannot affect stable code.

However; I would propose that we try to extend the "where-bound to item_bound transform" to also consider where bounds whose self type is a projection whose self type is an associated type of the trait:

trait Trait
where
    Self::Assoc: Clone
{
    type Assoc;
}

fn foo<T: Trait>(x: &T::Assoc) -> T::Assoc {
    x.clone()
}

trait Trait2
where
    Self::Assoc: Iterator,
    <Self::Assoc as Iterator>::Item: Clone
{
    type Assoc;
}

fn foo2<T: Trait2>(x: &<T::Assoc as Iterator>::Item) -> <T::Assoc as Iterator>::Item {
    x.clone()
}

This feels very much desirable to me

compiler/rustc_trait_selection/src/solve/assembly/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/solve/assembly/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/project.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Feb 6, 2024

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

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

r=me after nits

return;
}

match self.try_normalize_ty(goal.param_env, alias_ty.self_ty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we can get a stackoverflow/hang here for Alias<T> which normalizes to Alias<Alias<T>>. Either add a FIXME or try to get a test and add an overflow counter here

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't seem to get it to ICE :|

compiler/rustc_trait_selection/src/solve/assembly/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/project.rs Outdated Show resolved Hide resolved
@compiler-errors compiler-errors removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Feb 8, 2024
@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Feb 8, 2024

📌 Commit f515201 has been approved by lcnr

It is now in the queue for this repository.

@bors 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 Feb 8, 2024
@bors
Copy link
Contributor

bors commented Feb 8, 2024

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 8, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 9, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 9, 2024
For a rigid projection, recursively look at the self type's item bounds to fix the `associated_type_bounds` feature

Given a deeply nested rigid projection like `<<<T as Trait1>::Assoc1 as Trait2>::Assoc2 as Trait3>::Assoc3`, this PR adjusts both trait solvers to look at the item bounds for all of `Assoc3`, `Assoc2`, and `Assoc1` in order to satisfy a goal. We do this because the item bounds for projections may contain relevant bounds for *other* nested projections when the `associated_type_bounds` (ATB) feature is enabled. For example:

```rust
#![feature(associated_type_bounds)]

trait Trait1 {
    type Assoc1: Trait2<Assoc2: Foo>;
    // Item bounds for `Assoc1` are:
    // `<Self as Trait1>::Assoc1: Trait2`
    // `<<Self as Trait1>::Assoc1 as Trait2>::Assoc2: Foo`
}

trait Trait2 {
    type Assoc2;
}

trait Foo {}

fn hello<T: Trait1>(x: <<T as Trait1>::Assoc1 as Trait2>::Assoc2) {
    fn is_foo(_: impl Foo) {}
    is_foo(x);
    // Currently fails with:
    // ERROR the trait bound `<<Self as Trait1>::Assoc1 as Trait2>::Assoc2: Foo` is not satisfied
}
```

This has been a long-standing place of brokenness for ATBs, and is also part of the reason why ATBs currently desugar so differently in various positions (i.e. sometimes desugaring to param-env bounds, sometimes desugaring to RPITs, etc). For example, in RPIT and TAIT position, `impl Foo<Bar: Baz>` currently desugars to `impl Foo<Bar = impl Baz>` because we do not currently take advantage of these nested item bounds if we desugared them into a single set of item bounds on the opaque. This is obviously both strange and unnecessary if we just take advantage of these bounds as we should.

## Approach

This PR repeatedly peels off each projection of a given goal's self type and tries to match its item bounds against a goal, repeating with the self type of the projection. This is pretty straightforward to implement in the new solver, only requiring us to loop on the self type of a rigid projection to discover inner rigid projections, and we also need to introduce an extra probe so we can normalize them.

In the old solver, we can do essentially the same thing, however we rely on the fact that projections *should* be normalized already. This is obviously not always the case -- however, in the case that they are not fully normalized, such as a projection which has both infer vars and, we bail out with ambiguity if we hit an infer var for the self type.

## Caveats

⚠️ In the old solver, this has the side-effect of actually stalling some higher-ranked trait goals of the form `for<'a> <?0 as Tr<'a>>: Tr2`. Because we stall them, they no longer are eagerly treated as error -- this cause some existing `known-bug` tests to go from fail -> pass.

I'm pretty unconvinced that this is a problem since we make code that we expect to pass in the *new* solver also pass in the *old* solver, though this obviously doesn't solve the *full* problem.

## And then also...

We also adjust the desugaring of ATB to always desugar to a regular associated bound, rather than sometimes to an impl Trait **except** for when the ATB is present in a `dyn Trait`. We need to lower `dyn Trait<Assoc: Bar>` to `dyn Trait<Assoc = impl Bar>` because object types need all of their associated types specified.

I would also be in favor of splitting out the ATB feature and/or removing support for object types in order to stabilize just the set of positions for which the ATB feature is consistent (i.e. always elaborates to a bound).
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#120584 (For a rigid projection, recursively look at the self type's item bounds to fix the `associated_type_bounds` feature)
 - rust-lang#120589 (std::thread::available_parallelism merging linux/android/freebsd version)
 - rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages)
 - rust-lang#120629 (Move some test files)
 - rust-lang#120846 (Update jobserver-rs to 0.1.28)
 - rust-lang#120850 (ast_lowering: Fix regression in `use ::{}` imports.)
 - rust-lang#120853 (Avoid a collection and iteration on empty passes)

Failed merges:

 - rust-lang#120549 (modify alias-relate to also normalize ambiguous opaques)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#120584 (For a rigid projection, recursively look at the self type's item bounds to fix the `associated_type_bounds` feature)
 - rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages)
 - rust-lang#120629 (Move some test files)
 - rust-lang#120846 (Update jobserver-rs to 0.1.28)
 - rust-lang#120850 (ast_lowering: Fix regression in `use ::{}` imports.)
 - rust-lang#120853 (Avoid a collection and iteration on empty passes)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9ec287d into rust-lang:master Feb 10, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2024
Rollup merge of rust-lang#120584 - compiler-errors:u, r=lcnr

For a rigid projection, recursively look at the self type's item bounds to fix the `associated_type_bounds` feature

Given a deeply nested rigid projection like `<<<T as Trait1>::Assoc1 as Trait2>::Assoc2 as Trait3>::Assoc3`, this PR adjusts both trait solvers to look at the item bounds for all of `Assoc3`, `Assoc2`, and `Assoc1` in order to satisfy a goal. We do this because the item bounds for projections may contain relevant bounds for *other* nested projections when the `associated_type_bounds` (ATB) feature is enabled. For example:

```rust
#![feature(associated_type_bounds)]

trait Trait1 {
    type Assoc1: Trait2<Assoc2: Foo>;
    // Item bounds for `Assoc1` are:
    // `<Self as Trait1>::Assoc1: Trait2`
    // `<<Self as Trait1>::Assoc1 as Trait2>::Assoc2: Foo`
}

trait Trait2 {
    type Assoc2;
}

trait Foo {}

fn hello<T: Trait1>(x: <<T as Trait1>::Assoc1 as Trait2>::Assoc2) {
    fn is_foo(_: impl Foo) {}
    is_foo(x);
    // Currently fails with:
    // ERROR the trait bound `<<Self as Trait1>::Assoc1 as Trait2>::Assoc2: Foo` is not satisfied
}
```

This has been a long-standing place of brokenness for ATBs, and is also part of the reason why ATBs currently desugar so differently in various positions (i.e. sometimes desugaring to param-env bounds, sometimes desugaring to RPITs, etc). For example, in RPIT and TAIT position, `impl Foo<Bar: Baz>` currently desugars to `impl Foo<Bar = impl Baz>` because we do not currently take advantage of these nested item bounds if we desugared them into a single set of item bounds on the opaque. This is obviously both strange and unnecessary if we just take advantage of these bounds as we should.

## Approach

This PR repeatedly peels off each projection of a given goal's self type and tries to match its item bounds against a goal, repeating with the self type of the projection. This is pretty straightforward to implement in the new solver, only requiring us to loop on the self type of a rigid projection to discover inner rigid projections, and we also need to introduce an extra probe so we can normalize them.

In the old solver, we can do essentially the same thing, however we rely on the fact that projections *should* be normalized already. This is obviously not always the case -- however, in the case that they are not fully normalized, such as a projection which has both infer vars and, we bail out with ambiguity if we hit an infer var for the self type.

## Caveats

⚠️ In the old solver, this has the side-effect of actually stalling some higher-ranked trait goals of the form `for<'a> <?0 as Tr<'a>>: Tr2`. Because we stall them, they no longer are eagerly treated as error -- this cause some existing `known-bug` tests to go from fail -> pass.

I'm pretty unconvinced that this is a problem since we make code that we expect to pass in the *new* solver also pass in the *old* solver, though this obviously doesn't solve the *full* problem.

## And then also...

We also adjust the desugaring of ATB to always desugar to a regular associated bound, rather than sometimes to an impl Trait **except** for when the ATB is present in a `dyn Trait`. We need to lower `dyn Trait<Assoc: Bar>` to `dyn Trait<Assoc = impl Bar>` because object types need all of their associated types specified.

I would also be in favor of splitting out the ATB feature and/or removing support for object types in order to stabilize just the set of positions for which the ATB feature is consistent (i.e. always elaborates to a bound).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 10, 2024
Remove support for `associated_type_bound` nested in `dyn` types

These necessarily desugar to `impl Trait`, which is inconsistent with the `associated_type_bound` feature after rust-lang#120584.

This PR keeps the `is_in_dyn_type` hack, which kind of makes me sad. Ideally, we'd be validating that no object types have associated type bounds somewhere else. Unfortunately, we can't do this later during astconv (i think?), nor can we do it earlier during ast validation (i think?) because of the feature gating of ATB being a *warning* rather than an *error*. Let me know if you have thoughts about this.

r? lcnr
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 10, 2024
Remove support for `associated_type_bound` nested in `dyn` types

These necessarily desugar to `impl Trait`, which is inconsistent with the `associated_type_bound` feature after rust-lang#120584.

This PR keeps the `is_in_dyn_type` hack, which kind of makes me sad. Ideally, we'd be validating that no object types have associated type bounds somewhere else. Unfortunately, we can't do this later during astconv (i think?), nor can we do it earlier during ast validation (i think?) because of the feature gating of ATB being a *warning* rather than an *error*. Let me know if you have thoughts about this.

r? lcnr
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 10, 2024
Remove support for `associated_type_bound` nested in `dyn` types

These necessarily desugar to `impl Trait`, which is inconsistent with the `associated_type_bound` feature after rust-lang#120584.

This PR keeps the `is_in_dyn_type` hack, which kind of makes me sad. Ideally, we'd be validating that no object types have associated type bounds somewhere else. Unfortunately, we can't do this later during astconv (i think?), nor can we do it earlier during ast validation (i think?) because of the feature gating of ATB being a *warning* rather than an *error*. Let me know if you have thoughts about this.

r? lcnr
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2024
Rollup merge of rust-lang#120719 - compiler-errors:no-dyn-atb, r=lcnr

Remove support for `associated_type_bound` nested in `dyn` types

These necessarily desugar to `impl Trait`, which is inconsistent with the `associated_type_bound` feature after rust-lang#120584.

This PR keeps the `is_in_dyn_type` hack, which kind of makes me sad. Ideally, we'd be validating that no object types have associated type bounds somewhere else. Unfortunately, we can't do this later during astconv (i think?), nor can we do it earlier during ast validation (i think?) because of the feature gating of ATB being a *warning* rather than an *error*. Let me know if you have thoughts about this.

r? lcnr
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 18, 2024
…li-obk

Stabilize associated type bounds (RFC 2289)

This PR stabilizes associated type bounds, which were laid out in [RFC 2289]. This gives us a shorthand to express nested type bounds that would otherwise need to be expressed with nested `impl Trait` or broken into several `where` clauses.

### What are we stabilizing?

We're stabilizing the associated item bounds syntax, which allows us to put bounds in associated type position within other bounds, i.e. `T: Trait<Assoc: Bounds...>`. See [RFC 2289] for motivation.

In all position, the associated type bound syntax expands into a set of two (or more) bounds, and never anything else (see "How does this differ[...]" section for more info).

Associated type bounds are stabilized in four positions:
* **`where` clauses (and APIT)** - This is equivalent to breaking up the bound into two (or more) `where` clauses. For example, `where T: Trait<Assoc: Bound>` is equivalent to `where T: Trait, <T as Trait>::Assoc: Bound`.
* **Supertraits** - Similar to above, `trait CopyIterator: Iterator<Item: Copy> {}`. This is almost equivalent to breaking up the bound into two (or more) `where` clauses; however, the bound on the associated item is implied whenever the trait is used. See rust-lang#112573/rust-lang#112629.
* **Associated type item bounds** - This allows constraining the *nested* rigid projections that are associated with a trait's associated types. e.g. `trait Trait { type Assoc: Trait2<Assoc2: Copy>; }`.
* **opaque item bounds (RPIT, TAIT)** - This allows constraining associated types that are associated with the opaque without having to *name* the opaque. For example, `impl Iterator<Item: Copy>` defines an iterator whose item is `Copy` without having to actually name that item bound.

The latter three are not expressible in surface Rust (though for associated type item bounds, this will change in rust-lang#120752, which I don't believe should block this PR), so this does represent a slight expansion of what can be expressed in trait bounds.

### How does this differ from the RFC?

Compared to the RFC, the current implementation *always* desugars associated type bounds to sets of `ty::Clause`s internally. Specifically, it does *not* introduce a position-dependent desugaring as laid out in [RFC 2289], and in particular:
* It does *not* desugar to anonymous associated items in associated type item bounds.
* It does *not* desugar to nested RPITs in RPIT bounds, nor nested TAITs in TAIT bounds.

This position-dependent desugaring laid out in the RFC existed simply to side-step limitations of the trait solver, which have mostly been fixed in rust-lang#120584. The desugaring laid out in the RFC also added unnecessary complication to the design of the feature, and introduces its own limitations to, for example:
* Conditionally lowering to nested `impl Trait` in certain positions such as RPIT and TAIT means that we inherit the limitations of RPIT/TAIT, namely lack of support for higher-ranked opaque inference. See this code example: rust-lang#120752 (comment).
* Introducing anonymous associated types makes traits no longer object safe, since anonymous associated types are not nameable, and all associated types must be named in `dyn` types.

This last point motivates why this PR is *not* stabilizing support for associated type bounds in `dyn` types, e.g, `dyn Assoc<Item: Bound>`. Why? Because `dyn` types need to have *concrete* types for all associated items, this would necessitate a distinct lowering for associated type bounds, which seems both complicated and unnecessary compared to just requiring the user to write `impl Trait` themselves. See rust-lang#120719.

### Implementation history:

Limited to the significant behavioral changes and fixes and relevant PRs, ping me if I left something out--
* rust-lang#57428
* rust-lang#108063
* rust-lang#110512
* rust-lang#112629
* rust-lang#120719
* rust-lang#120584

Closes rust-lang#52662

[RFC 2289]: https://rust-lang.github.io/rfcs/2289-associated-type-bounds.html
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2024
…-obk

Stabilize associated type bounds (RFC 2289)

This PR stabilizes associated type bounds, which were laid out in [RFC 2289]. This gives us a shorthand to express nested type bounds that would otherwise need to be expressed with nested `impl Trait` or broken into several `where` clauses.

### What are we stabilizing?

We're stabilizing the associated item bounds syntax, which allows us to put bounds in associated type position within other bounds, i.e. `T: Trait<Assoc: Bounds...>`. See [RFC 2289] for motivation.

In all position, the associated type bound syntax expands into a set of two (or more) bounds, and never anything else (see "How does this differ[...]" section for more info).

Associated type bounds are stabilized in four positions:
* **`where` clauses (and APIT)** - This is equivalent to breaking up the bound into two (or more) `where` clauses. For example, `where T: Trait<Assoc: Bound>` is equivalent to `where T: Trait, <T as Trait>::Assoc: Bound`.
* **Supertraits** - Similar to above, `trait CopyIterator: Iterator<Item: Copy> {}`. This is almost equivalent to breaking up the bound into two (or more) `where` clauses; however, the bound on the associated item is implied whenever the trait is used. See rust-lang#112573/rust-lang#112629.
* **Associated type item bounds** - This allows constraining the *nested* rigid projections that are associated with a trait's associated types. e.g. `trait Trait { type Assoc: Trait2<Assoc2: Copy>; }`.
* **opaque item bounds (RPIT, TAIT)** - This allows constraining associated types that are associated with the opaque without having to *name* the opaque. For example, `impl Iterator<Item: Copy>` defines an iterator whose item is `Copy` without having to actually name that item bound.

The latter three are not expressible in surface Rust (though for associated type item bounds, this will change in rust-lang#120752, which I don't believe should block this PR), so this does represent a slight expansion of what can be expressed in trait bounds.

### How does this differ from the RFC?

Compared to the RFC, the current implementation *always* desugars associated type bounds to sets of `ty::Clause`s internally. Specifically, it does *not* introduce a position-dependent desugaring as laid out in [RFC 2289], and in particular:
* It does *not* desugar to anonymous associated items in associated type item bounds.
* It does *not* desugar to nested RPITs in RPIT bounds, nor nested TAITs in TAIT bounds.

This position-dependent desugaring laid out in the RFC existed simply to side-step limitations of the trait solver, which have mostly been fixed in rust-lang#120584. The desugaring laid out in the RFC also added unnecessary complication to the design of the feature, and introduces its own limitations to, for example:
* Conditionally lowering to nested `impl Trait` in certain positions such as RPIT and TAIT means that we inherit the limitations of RPIT/TAIT, namely lack of support for higher-ranked opaque inference. See this code example: rust-lang#120752 (comment).
* Introducing anonymous associated types makes traits no longer object safe, since anonymous associated types are not nameable, and all associated types must be named in `dyn` types.

This last point motivates why this PR is *not* stabilizing support for associated type bounds in `dyn` types, e.g, `dyn Assoc<Item: Bound>`. Why? Because `dyn` types need to have *concrete* types for all associated items, this would necessitate a distinct lowering for associated type bounds, which seems both complicated and unnecessary compared to just requiring the user to write `impl Trait` themselves. See rust-lang#120719.

### Implementation history:

Limited to the significant behavioral changes and fixes and relevant PRs, ping me if I left something out--
* rust-lang#57428
* rust-lang#108063
* rust-lang#110512
* rust-lang#112629
* rust-lang#120719
* rust-lang#120584

Closes rust-lang#52662

[RFC 2289]: https://rust-lang.github.io/rfcs/2289-associated-type-bounds.html
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 2, 2024
…ays, r=lcnr

Make inductive cycles always ambiguous

 This makes inductive cycles always result in ambiguity rather than be treated like a stack-dependent error.

This has some  interactions with specialization, and so breaks a few UI tests that I don't agree should've ever worked in the first place, and also breaks a handful of crates in a way that I don't believe is a problem.

On the bright side, it puts us in a better spot when it comes to eventually enabling coinduction everywhere.

## Results

This was cratered in rust-lang#116494 (comment), which boils down to two regressions:
* `lu_packets` - This code should have never compiled in the first place. More below.
* **ALL** other regressions are due to `[email protected]` (edit: and `[email protected]`) - This actually seems to be fixed in version `0.11.0-beta.5`, which is the *most* up to date version, but it's still prerelease on crates.io so I don't think cargo ends up picking `beta.5` when building dependent crates.

### `lu_packets`

Firstly, this crate uses specialization, so I think it's automatically worth breaking. However, I've minimized [the regression](https://crater-reports.s3.amazonaws.com/pr-116494-3/try%23d614ed876e31a5f3ad1d0fbf848fcdab3a29d1d8/gh/lcdr.lu_packets/log.txt) to:

```rust
// Upstream crate
pub trait Serialize {}
impl Serialize for &() {}
impl<S> Serialize for &[S] where for<'a> &'a S: Serialize {}

// ----------------------------------------------------------------------- //

// Downstream crate
#![feature(specialization)]
#![allow(incomplete_features, unused)]

use upstream::Serialize;

trait Replica {
    fn serialize();
}

impl<T> Replica for T {
    default fn serialize() {}
}

impl<T> Replica for Option<T>
where
    for<'a> &'a T: Serialize,
{
    fn serialize() {}
}
```

Specifically this fails when computing the specialization graph for the `downstream` crate.

The code ends up cycling on `&[?0]: Serialize` when we equate `&?0 = &[?1]` during impl matching, which ends up needing to prove `&[?1]: Serialize`, which since cycles are treated like ambiguity, ends up in a **fatal overflow**. For some reason this requires two crates, squashing them into one crate doesn't work.

Side-note: This code is subtly order dependent. When minimizing, I ended up having the code start failing on `nightly` very easily after removing and reordering impls. This seems to me all the more reason to remove this behavior altogether.

## Side-note: Item Bounds (edit: this was fixed independently in rust-lang#121123)

Due to the changes in rust-lang#120584 where we now consider an alias's item bounds *and* all the item bounds of the alias's nested self type aliases, I've had to add e6b64c6 which is a hack to make sure we're not eagerly normalizing bounds that have nothing to do with the predicate we're trying to solve, and which result in.

This is fixed in a more principled way in rust-lang#121123.

---

r? lcnr for an initial review
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 3, 2024
…ays, r=lcnr

Make inductive cycles always ambiguous

 This makes inductive cycles always result in ambiguity rather than be treated like a stack-dependent error.

This has some  interactions with specialization, and so breaks a few UI tests that I don't agree should've ever worked in the first place, and also breaks a handful of crates in a way that I don't believe is a problem.

On the bright side, it puts us in a better spot when it comes to eventually enabling coinduction everywhere.

## Results

This was cratered in rust-lang#116494 (comment), which boils down to two regressions:
* `lu_packets` - This code should have never compiled in the first place. More below.
* **ALL** other regressions are due to `[email protected]` (edit: and `[email protected]`) - This actually seems to be fixed in version `0.11.0-beta.5`, which is the *most* up to date version, but it's still prerelease on crates.io so I don't think cargo ends up picking `beta.5` when building dependent crates.

### `lu_packets`

Firstly, this crate uses specialization, so I think it's automatically worth breaking. However, I've minimized [the regression](https://crater-reports.s3.amazonaws.com/pr-116494-3/try%23d614ed876e31a5f3ad1d0fbf848fcdab3a29d1d8/gh/lcdr.lu_packets/log.txt) to:

```rust
// Upstream crate
pub trait Serialize {}
impl Serialize for &() {}
impl<S> Serialize for &[S] where for<'a> &'a S: Serialize {}

// ----------------------------------------------------------------------- //

// Downstream crate
#![feature(specialization)]
#![allow(incomplete_features, unused)]

use upstream::Serialize;

trait Replica {
    fn serialize();
}

impl<T> Replica for T {
    default fn serialize() {}
}

impl<T> Replica for Option<T>
where
    for<'a> &'a T: Serialize,
{
    fn serialize() {}
}
```

Specifically this fails when computing the specialization graph for the `downstream` crate.

The code ends up cycling on `&[?0]: Serialize` when we equate `&?0 = &[?1]` during impl matching, which ends up needing to prove `&[?1]: Serialize`, which since cycles are treated like ambiguity, ends up in a **fatal overflow**. For some reason this requires two crates, squashing them into one crate doesn't work.

Side-note: This code is subtly order dependent. When minimizing, I ended up having the code start failing on `nightly` very easily after removing and reordering impls. This seems to me all the more reason to remove this behavior altogether.

## Side-note: Item Bounds (edit: this was fixed independently in rust-lang#121123)

Due to the changes in rust-lang#120584 where we now consider an alias's item bounds *and* all the item bounds of the alias's nested self type aliases, I've had to add e6b64c6 which is a hack to make sure we're not eagerly normalizing bounds that have nothing to do with the predicate we're trying to solve, and which result in.

This is fixed in a more principled way in rust-lang#121123.

---

r? lcnr for an initial review
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2024
…jections, r=oli-obk

Check def id before calling `match_projection_projections`

When I "inlined" `assemble_candidates_from_predicates` into `for_each_item_bound` in rust-lang#120584, I forgot to copy over the check that actually made sure the def id of the candidate was equal to the def id of the obligation. This means that we normalize goal a bit too often even if it's not productive to do so.

This PR adds that def id check back.
Fixes rust-lang#123448
GuillaumeGomez pushed a commit to GuillaumeGomez/rust that referenced this pull request Jul 10, 2024
…-obk

Stabilize associated type bounds (RFC 2289)

This PR stabilizes associated type bounds, which were laid out in [RFC 2289]. This gives us a shorthand to express nested type bounds that would otherwise need to be expressed with nested `impl Trait` or broken into several `where` clauses.

### What are we stabilizing?

We're stabilizing the associated item bounds syntax, which allows us to put bounds in associated type position within other bounds, i.e. `T: Trait<Assoc: Bounds...>`. See [RFC 2289] for motivation.

In all position, the associated type bound syntax expands into a set of two (or more) bounds, and never anything else (see "How does this differ[...]" section for more info).

Associated type bounds are stabilized in four positions:
* **`where` clauses (and APIT)** - This is equivalent to breaking up the bound into two (or more) `where` clauses. For example, `where T: Trait<Assoc: Bound>` is equivalent to `where T: Trait, <T as Trait>::Assoc: Bound`.
* **Supertraits** - Similar to above, `trait CopyIterator: Iterator<Item: Copy> {}`. This is almost equivalent to breaking up the bound into two (or more) `where` clauses; however, the bound on the associated item is implied whenever the trait is used. See rust-lang#112573/rust-lang#112629.
* **Associated type item bounds** - This allows constraining the *nested* rigid projections that are associated with a trait's associated types. e.g. `trait Trait { type Assoc: Trait2<Assoc2: Copy>; }`.
* **opaque item bounds (RPIT, TAIT)** - This allows constraining associated types that are associated with the opaque without having to *name* the opaque. For example, `impl Iterator<Item: Copy>` defines an iterator whose item is `Copy` without having to actually name that item bound.

The latter three are not expressible in surface Rust (though for associated type item bounds, this will change in rust-lang#120752, which I don't believe should block this PR), so this does represent a slight expansion of what can be expressed in trait bounds.

### How does this differ from the RFC?

Compared to the RFC, the current implementation *always* desugars associated type bounds to sets of `ty::Clause`s internally. Specifically, it does *not* introduce a position-dependent desugaring as laid out in [RFC 2289], and in particular:
* It does *not* desugar to anonymous associated items in associated type item bounds.
* It does *not* desugar to nested RPITs in RPIT bounds, nor nested TAITs in TAIT bounds.

This position-dependent desugaring laid out in the RFC existed simply to side-step limitations of the trait solver, which have mostly been fixed in rust-lang#120584. The desugaring laid out in the RFC also added unnecessary complication to the design of the feature, and introduces its own limitations to, for example:
* Conditionally lowering to nested `impl Trait` in certain positions such as RPIT and TAIT means that we inherit the limitations of RPIT/TAIT, namely lack of support for higher-ranked opaque inference. See this code example: rust-lang#120752 (comment).
* Introducing anonymous associated types makes traits no longer object safe, since anonymous associated types are not nameable, and all associated types must be named in `dyn` types.

This last point motivates why this PR is *not* stabilizing support for associated type bounds in `dyn` types, e.g, `dyn Assoc<Item: Bound>`. Why? Because `dyn` types need to have *concrete* types for all associated items, this would necessitate a distinct lowering for associated type bounds, which seems both complicated and unnecessary compared to just requiring the user to write `impl Trait` themselves. See rust-lang#120719.

### Implementation history:

Limited to the significant behavioral changes and fixes and relevant PRs, ping me if I left something out--
* rust-lang#57428
* rust-lang#108063
* rust-lang#110512
* rust-lang#112629
* rust-lang#120719
* rust-lang#120584

Closes rust-lang#52662

[RFC 2289]: https://rust-lang.github.io/rfcs/2289-associated-type-bounds.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants