Item bounds can reference self projections and still be object safe by compiler-errors · Pull Request #122804 · rust-lang/rust
rustbot
added
S-waiting-on-review
labels
Mar 21, 2024
workingjubilee
added
T-types
and removed T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.labels
Mar 21, 2024and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.labels
Apr 1, 2024
compiler-errors
removed
the
needs-fcp
label
May 21, 2024
bors
added
S-waiting-on-bors
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, 2024Noratrieb 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, 2024Rollup 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
added
S-waiting-on-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, 2024bors 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
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, 2024Rollup 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters