Item bounds can reference self projections and still be object safe by compiler-errors · Pull Request #122804 · rust-lang/rust

@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.

labels

Mar 21, 2024

@workingjubilee workingjubilee 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 21, 2024

@BoxyUwU BoxyUwU added needs-fcp

This change is insta-stable, or significant enough to need a team FCP to proceed.

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

Apr 1, 2024

@compiler-errors compiler-errors removed the needs-fcp

This change is insta-stable, or significant enough to need a team FCP to proceed.

label

May 21, 2024

@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-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jun 3, 2024

Noratrieb added a commit to Noratrieb/rust that referenced this pull request

Jun 4, 2024
…ference-self, r=BoxyUwU

Item bounds can reference self projections and still be object safe

### Background

Currently, we have some interesting rules about where `Self` is allowed to be mentioned in objects. Specifically, we allow mentioning `Self` behind associated types (e.g. `fn foo(&self) -> Self::Assoc`) only if that `Self` type comes from the trait we're defining or its supertraits:

```
trait Foo {
  fn good() -> Self::Assoc; // GOOD :)

  fn bad() -> <Self as OtherTrait>::Assoc; // BAD!
}
```

And more specifically, these `Self::Assoc` projections are *only* allowed to show up in:
  * (A1) Method signatures
  * (A2) Where clauses on traits, GATs and methods

But `Self::Assoc` projections are **not** allowed to show up in:
  * (B1) Supertrait bounds (specifically: all *super-predicates*, which includes the projections that come from elaboration, and not just the traits themselves).
  * (B2) Item bounds of associated types

The reason for (B1) is interesting: specifically, it arises from the fact that we currently eagerly elaborate all projection predicates into the object, so if we had the following code:

```
trait Sub<Assoc = Self::SuperAssoc> {}
trait Super {
    type SuperAssoc;
}
```

Then given `dyn Sub<SuperAssoc = i32>` we would need to have a type that is substituted into itself an infinite number of times[^1], like `dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <... as Super>::SuperAssoc> as Super>::SuperAssoc> as Super>::SuperAssoc>`, i.e. the fixed-point of: `type T = dyn Sub<SuperAssoc = i32, Assoc = <T as Super>::SuperAssoc>`.

Similarly for (B2), we restrict mentioning `Self::Assoc` in associated type item bounds, which is the cause for rust-lang#122798. However, there is **no reason** for us to do so, since item bounds never show up structurally in the `dyn Trait` object type.

#### What?

This PR relaxes the check for item bounds so that `Self` may be mentioned behind associated types in the same cases that they currently work for method signatures (A1) and where clauses (A2).

#### Why?

Fixes rust-lang#122798. Removes a subtle and confusing inconsistency for the code mentioned in that issue.

This is sound because we only assemble alias bounds for rigid projections, and all projections coming from an object self type are not rigid, since all associated types should be specified by the type.

This is also desirable because we can do this via supertraits already. In rust-lang#122789, it is noted that an item bound of `Eq` already works, just not `PartialEq` because of the default item bound. This is weird and should be fixed.

#### Future work

We could make the check for `Self` in super-predicates more sophisticated as well, only erroring if `Self` shows up in a projection super-predicate.

[^1]: This could be fixed by some sort of structural replacement or eager normalization, but I don't think it's necessary currently.

Noratrieb added a commit to Noratrieb/rust that referenced this pull request

Jun 4, 2024
…ference-self, r=BoxyUwU

Item bounds can reference self projections and still be object safe

### Background

Currently, we have some interesting rules about where `Self` is allowed to be mentioned in objects. Specifically, we allow mentioning `Self` behind associated types (e.g. `fn foo(&self) -> Self::Assoc`) only if that `Self` type comes from the trait we're defining or its supertraits:

```
trait Foo {
  fn good() -> Self::Assoc; // GOOD :)

  fn bad() -> <Self as OtherTrait>::Assoc; // BAD!
}
```

And more specifically, these `Self::Assoc` projections are *only* allowed to show up in:
  * (A1) Method signatures
  * (A2) Where clauses on traits, GATs and methods

But `Self::Assoc` projections are **not** allowed to show up in:
  * (B1) Supertrait bounds (specifically: all *super-predicates*, which includes the projections that come from elaboration, and not just the traits themselves).
  * (B2) Item bounds of associated types

The reason for (B1) is interesting: specifically, it arises from the fact that we currently eagerly elaborate all projection predicates into the object, so if we had the following code:

```
trait Sub<Assoc = Self::SuperAssoc> {}
trait Super {
    type SuperAssoc;
}
```

Then given `dyn Sub<SuperAssoc = i32>` we would need to have a type that is substituted into itself an infinite number of times[^1], like `dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <... as Super>::SuperAssoc> as Super>::SuperAssoc> as Super>::SuperAssoc>`, i.e. the fixed-point of: `type T = dyn Sub<SuperAssoc = i32, Assoc = <T as Super>::SuperAssoc>`.

Similarly for (B2), we restrict mentioning `Self::Assoc` in associated type item bounds, which is the cause for rust-lang#122798. However, there is **no reason** for us to do so, since item bounds never show up structurally in the `dyn Trait` object type.

#### What?

This PR relaxes the check for item bounds so that `Self` may be mentioned behind associated types in the same cases that they currently work for method signatures (A1) and where clauses (A2).

#### Why?

Fixes rust-lang#122798. Removes a subtle and confusing inconsistency for the code mentioned in that issue.

This is sound because we only assemble alias bounds for rigid projections, and all projections coming from an object self type are not rigid, since all associated types should be specified by the type.

This is also desirable because we can do this via supertraits already. In rust-lang#122789, it is noted that an item bound of `Eq` already works, just not `PartialEq` because of the default item bound. This is weird and should be fixed.

#### Future work

We could make the check for `Self` in super-predicates more sophisticated as well, only erroring if `Self` shows up in a projection super-predicate.

[^1]: This could be fixed by some sort of structural replacement or eager normalization, but I don't think it's necessary currently.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jun 4, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#122804 (Item bounds can reference self projections and still be object safe)
 - rust-lang#124486 (Add tracking issue and unstable book page for `"vectorcall"` ABI)
 - rust-lang#125504 (Change pedantically incorrect OnceCell/OnceLock wording)
 - rust-lang#125608 (Avoid follow-up errors if the number of generic parameters already doesn't match)
 - rust-lang#125690 (ARM Target Docs Update)
 - rust-lang#125750 (Align `Term` methods with `GenericArg` methods, add `Term::expect_*`)
 - rust-lang#125818 (Handle no values cfgs with `--print=check-cfg`)
 - rust-lang#125909 (rustdoc: add a regression test for a former blanket impl synthesis ICE)
 - rust-lang#125919 (Remove stray "this")

r? `@ghost`
`@rustbot` modify labels: rollup

@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

Jun 4, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jun 6, 2024
…unsoundness, r=<try>

Fix supertrait associated type unsoundness

This is built on top of rust-lang#122804 though that's really not related, it's just easier to make this modification with the changes to the object safety code that I did in that PR. The only thing is that PR may make this unsoundness slightly easier to abuse, since there are more positions that allow self-associated-types.

This PR only looks for supertrait associated types *without* normalizing. I'm gonna crater this initially -- preferably we don't need to normalize unless there's a lot of breakage. I assume that most people are writing `Self::Assoc` so they don't really care about the trait ref being normalized, so somewhat tempted to believe that this will just work out and we can extend it later if needed.

r? lcnr

@compiler-errors

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jul 25, 2024
…y-unsoundness, r=lcnr

Fix supertrait associated type unsoundness

### What?

Object safety allows us to name `Self::Assoc` associated types in certain positions if they come from our trait or one of our supertraits. When this check was implemented, I think it failed to consider that supertraits can have different args, and it was only checking def-id equality.

This is problematic, since we can sneak different implementations in by implementing `Supertrait<NotActuallyTheSupertraitSubsts>` for a `dyn` type. This can be used to implement an unsound transmute function. See the committed test.

### How do we fix it?

We consider the whole trait ref when checking for supertraits. Right now, this is implemented using equality *without* normalization. We erase regions since those don't affect trait selection.

This is a limitation that could theoretically affect code that should be accepted, but doesn't matter in practice -- there are 0 crater regression. We could make this check stronger, but I would be worried about cycle issues. I assume that most people are writing `Self::Assoc` so they don't really care about the trait ref being normalized.

---

### What is up w the stacked commit

This is built on top of rust-lang#122804 though that's really not related, it's just easier to make this modification with the changes to the object safety code that I did in that PR. The only thing is that PR may make this unsoundness slightly easier to abuse, since there are more positions that allow self-associated-types -- I am happy to stall that change until this PR merges.

---

Fixes rust-lang#126079

r? lcnr

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Jul 26, 2024
Rollup merge of rust-lang#126090 - compiler-errors:supertrait-assoc-ty-unsoundness, r=lcnr

Fix supertrait associated type unsoundness

### What?

Object safety allows us to name `Self::Assoc` associated types in certain positions if they come from our trait or one of our supertraits. When this check was implemented, I think it failed to consider that supertraits can have different args, and it was only checking def-id equality.

This is problematic, since we can sneak different implementations in by implementing `Supertrait<NotActuallyTheSupertraitSubsts>` for a `dyn` type. This can be used to implement an unsound transmute function. See the committed test.

### How do we fix it?

We consider the whole trait ref when checking for supertraits. Right now, this is implemented using equality *without* normalization. We erase regions since those don't affect trait selection.

This is a limitation that could theoretically affect code that should be accepted, but doesn't matter in practice -- there are 0 crater regression. We could make this check stronger, but I would be worried about cycle issues. I assume that most people are writing `Self::Assoc` so they don't really care about the trait ref being normalized.

---

### What is up w the stacked commit

This is built on top of rust-lang#122804 though that's really not related, it's just easier to make this modification with the changes to the object safety code that I did in that PR. The only thing is that PR may make this unsoundness slightly easier to abuse, since there are more positions that allow self-associated-types -- I am happy to stall that change until this PR merges.

---

Fixes rust-lang#126079

r? lcnr

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Jul 26, 2024
…ness, r=lcnr

Fix supertrait associated type unsoundness

### What?

Object safety allows us to name `Self::Assoc` associated types in certain positions if they come from our trait or one of our supertraits. When this check was implemented, I think it failed to consider that supertraits can have different args, and it was only checking def-id equality.

This is problematic, since we can sneak different implementations in by implementing `Supertrait<NotActuallyTheSupertraitSubsts>` for a `dyn` type. This can be used to implement an unsound transmute function. See the committed test.

### How do we fix it?

We consider the whole trait ref when checking for supertraits. Right now, this is implemented using equality *without* normalization. We erase regions since those don't affect trait selection.

This is a limitation that could theoretically affect code that should be accepted, but doesn't matter in practice -- there are 0 crater regression. We could make this check stronger, but I would be worried about cycle issues. I assume that most people are writing `Self::Assoc` so they don't really care about the trait ref being normalized.

---

### What is up w the stacked commit

This is built on top of rust-lang/rust#122804 though that's really not related, it's just easier to make this modification with the changes to the object safety code that I did in that PR. The only thing is that PR may make this unsoundness slightly easier to abuse, since there are more positions that allow self-associated-types -- I am happy to stall that change until this PR merges.

---

Fixes #126079

r? lcnr