Fix dyn incompleteness with multiple supertraits with different substitutions #133397
Conversation
rustbot
added
S-waiting-on-review
|
@bors try |
…ss, r=<try> Fix dyn incompleteness with multiple supertraits with different substitutions So much to write about this. Fixes rust-lang#133361 r? `@ghost`
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
compiler-errors
force-pushed
the
fix-dyn-incompleteness
branch
from
88fa8f4 to
22cb94a
Compare
|
@bors try for later: |
…ss, r=<try> Fix dyn incompleteness with multiple supertraits with different substitutions So much to write about this. Fixes rust-lang#133361 r? `@ghost`
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot cancel |
|
🗑️ Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
compiler-errors
force-pushed
the
fix-dyn-incompleteness
branch
from
e56f8d3 to
956f324
Compare
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (fb57ace): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -0.0%, secondary -0.2%)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.
CyclesResults (primary -2.2%, secondary -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.
Binary sizeResults (primary -0.0%)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.
Bootstrap: 767.015s -> 768.319s (0.17%) |
|
☔ The latest upstream changes (presumably #133978) made this pull request unmergeable. Please resolve the merge conflicts. |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
Fix ICE when multiple supertrait substitutions need assoc but only one is provided
Dyn traits must have all of their associated types constrained either by:
1. writing them in the dyn trait itself as an associated type bound, like `dyn Iterator<Item = u32>`,
2. A supertrait bound, like `trait ConstrainedIterator: Iterator<Item = u32> {}`, then you may write `dyn ConstrainedIterator` which doesn't need to mention `Item`.
However, the object type lowering code did not consider the fact that there may be multiple supertraits with different substitutions, so it just used the associated type's *def id* as a key for keeping track of which associated types are missing:
https://github.com/rust-lang/rust/blob/1fc691e6ddc24506b5234d586a5c084eb767f1ad/compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs#L131
This means that we can have missing associated types when there are mutliple supertraits with different substitutions and only one of them is constrained, like:
```rust
trait Sup<T> {
type Assoc: Default;
}
impl<T: Default> Sup<T> for () {
type Assoc = T;
}
impl<T: Default, U: Default> Dyn<T, U> for () {}
trait Dyn<A, B>: Sup<A, Assoc = A> + Sup<B> {}
```
The above example allows you to name `<dyn Dyn<i32, u32> as Sup<u32>>::Assoc` even though it is not possible to project since it's neither constrained by a manually written projection bound or a supertrait bound. This successfully type-checks, but leads to a codegen ICE since we are not able to project the associated type.
This PR fixes the validation for checking that a dyn trait mentions all of its associated type bounds. This is theoretically a breaking change, since you could technically use that `dyn Dyn<A, B>` type mentionedin the example above without actually *projecting* to the bad associated type, but I don't expect it to ever be relevant to a user since it's almost certainly a bug. This is corroborated with the crater results[^crater], which show no failures[^unknown].
Crater: rust-lang#133392 (comment)
Fixes rust-lang#133388
[^crater]: I cratered this originally with rust-lang#133397, which is a PR that is stacked on top, then re-ran crater with just the failures from that PR.
[^unknown]: If you look at the crater results, it shows all of the passes as "unknown". I believe this is a crater bug, since looking at the results manually shows them as passes.
Rollup merge of rust-lang#133392 - compiler-errors:object-sup, r=lcnr Fix ICE when multiple supertrait substitutions need assoc but only one is provided Dyn traits must have all of their associated types constrained either by: 1. writing them in the dyn trait itself as an associated type bound, like `dyn Iterator<Item = u32>`, 2. A supertrait bound, like `trait ConstrainedIterator: Iterator<Item = u32> {}`, then you may write `dyn ConstrainedIterator` which doesn't need to mention `Item`. However, the object type lowering code did not consider the fact that there may be multiple supertraits with different substitutions, so it just used the associated type's *def id* as a key for keeping track of which associated types are missing: https://github.com/rust-lang/rust/blob/1fc691e6ddc24506b5234d586a5c084eb767f1ad/compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs#L131 This means that we can have missing associated types when there are mutliple supertraits with different substitutions and only one of them is constrained, like: ```rust trait Sup<T> { type Assoc: Default; } impl<T: Default> Sup<T> for () { type Assoc = T; } impl<T: Default, U: Default> Dyn<T, U> for () {} trait Dyn<A, B>: Sup<A, Assoc = A> + Sup<B> {} ``` The above example allows you to name `<dyn Dyn<i32, u32> as Sup<u32>>::Assoc` even though it is not possible to project since it's neither constrained by a manually written projection bound or a supertrait bound. This successfully type-checks, but leads to a codegen ICE since we are not able to project the associated type. This PR fixes the validation for checking that a dyn trait mentions all of its associated type bounds. This is theoretically a breaking change, since you could technically use that `dyn Dyn<A, B>` type mentionedin the example above without actually *projecting* to the bad associated type, but I don't expect it to ever be relevant to a user since it's almost certainly a bug. This is corroborated with the crater results[^crater], which show no failures[^unknown]. Crater: rust-lang#133392 (comment) Fixes rust-lang#133388 [^crater]: I cratered this originally with rust-lang#133397, which is a PR that is stacked on top, then re-ran crater with just the failures from that PR. [^unknown]: If you look at the crater results, it shows all of the passes as "unknown". I believe this is a crater bug, since looking at the results manually shows them as passes.
compiler-errors
force-pushed
the
fix-dyn-incompleteness
branch
from
956f324 to
1ea7bfd
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
So much to write about this.
Fixes #133361
r? @ghost