Check for dyn providing all of its projections is incomplete w.r.t. different supertrait substitutions
#133388
Assignees
Labels
A-trait-objects
Area: trait objects, vtable layout
C-bug
Category: This is a bug.
I-ICE
Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️
T-types
Relevant to the types team, which will review and decide on the PR/issue.
Comments
compiler-errors
added
A-trait-objects
rustbot
added
the
needs-triage
jieyouxu
removed
the
needs-triage
This was referenced Nov 23, 2024
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Nov 23, 2024
Fix ICE when multiple supertrait substitutions need assoc but only one is provided I'll write more here when the crate run is done. Fixes rust-lang#133388 r? lcnr `@rustbot` author
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Nov 25, 2024
Some minor dyn-related tweaks Each commit should be self-explanatory, but I'm happy to explain what's going on if not. These are tweaks I pulled out of rust-lang#133388, but they can be reviewed sooner than that. r? types
jhpratt
added a commit
to jhpratt/rust
that referenced
this issue
Nov 26, 2024
…=lcnr
Bail on more errors in dyn ty lowering
If we have more than one principal trait, or if we have a principal trait with errors in it, then bail with `TyKind::Error` rather than attempting lowering. Lowering a dyn trait with more than one principal just arbitrarily chooses the first one and drops the subsequent ones, and lowering a dyn trait path with errors in it is just kinda useless.
This suppresses unnecessary errors which I think is net-good, but also is important to make sure that we don't end up leaking `{type error}` in rust-lang#133388 error messaging :)
r? types
compiler-errors
added a commit
to compiler-errors/rust
that referenced
this issue
Nov 26, 2024
…=lcnr
Bail on more errors in dyn ty lowering
If we have more than one principal trait, or if we have a principal trait with errors in it, then bail with `TyKind::Error` rather than attempting lowering. Lowering a dyn trait with more than one principal just arbitrarily chooses the first one and drops the subsequent ones, and lowering a dyn trait path with errors in it is just kinda useless.
This suppresses unnecessary errors which I think is net-good, but also is important to make sure that we don't end up leaking `{type error}` in rust-lang#133388 error messaging :)
r? types
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
Nov 27, 2024
Rollup merge of rust-lang#133394 - compiler-errors:dyn-more-errors, r=lcnr Bail on more errors in dyn ty lowering If we have more than one principal trait, or if we have a principal trait with errors in it, then bail with `TyKind::Error` rather than attempting lowering. Lowering a dyn trait with more than one principal just arbitrarily chooses the first one and drops the subsequent ones, and lowering a dyn trait path with errors in it is just kinda useless. This suppresses unnecessary errors which I think is net-good, but also is important to make sure that we don't end up leaking `{type error}` in rust-lang#133388 error messaging :) r? types
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Nov 27, 2024
…pastorino Some minor dyn-related tweaks Each commit should be self-explanatory, but I'm happy to explain what's going on if not. These are tweaks I pulled out of rust-lang#133388, but they can be reviewed sooner than that. r? types
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
Dec 15, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I tried this code:
I expected to see this happen: It errors b/c we don't know what the associated type for
<dyn Dyn<i32, u32> as Sup<u32>>::Associs.Instead, this happened: Codegen ICE.
Meta
rustc --version --verbose:Backtrace
The text was updated successfully, but these errors were encountered: