Fix dyn incompleteness with multiple supertraits with different substitutions by compiler-errors · Pull Request #133397 · rust-lang/rust (original) (raw)

Background

The way that we handle a dyn trait type's projection bounds is very structural today. A dyn trait is represented as a list of PolyExistentialPredicates, which in most cases will be a principal trait (like Iterator) and a list of projections (like Item = u32). Importantly, the list of projections comes from user-written associated type bounds on the type and from elaborating the projections from the principal's supertraits.

For example, given a set of traits like:

trait Foo { type Assoc; }

trait Bar<A, B>: Foo<A, Assoc = A> + Foo<B, Assoc = B> {}

For the type dyn Bar<i32, u32>, the list of projections will be something like [Foo<i32>::Assoc = i32, Foo<u32>::Assoc = u32]. We deduplicate these projections when they're identical, so for dyn Bar<(), ()> would be something like [Foo<()>::Assoc = ()].

Shortcomings: inference

We face problems when we begin to mix this structural notion of projection bounds with inference and associated type normalization. For example, let's try calling a generic function that takes dyn Bar<A, B> with a value of type dyn Bar<(), ()>:

trait Foo { type Assoc; }

trait Bar<A, B>: Foo<A, Assoc = A> + Foo<B, Assoc = B> {}

fn call_bar<A, B>(_: &dyn Bar<A, B>) {}

fn test(x: &dyn Bar<(), ()>) { call_bar(x); // ^ ERROR mismatched types }

error[E0308]: mismatched types
  --> /home/mgx/test.rs:10:14
   |
10 |     call_bar(x);
   |     -------- ^ expected trait `Bar<_, _>`, found trait `Bar<(), ()>`

What's going on here? Well, when calling call_bar, the generic signature &dyn Bar<?A, ?B> does not unify with &dyn Bar<(), ()> because the list of projections differ -- [Foo<?A>::Assoc = ?A, Foo<?B>::Assoc = ?B] vs [Foo<()>::Assoc = ()].

A simple solution to this may be to unify the principal traits first, then attempt to deduplicate them after inference. In this case, if we constrain ?A = ?B = (), then we would be able to deduplicate those projections in the first list.

However, this idea is still pretty fragile, and it's not a complete solution.

Shortcomings: normalization

Consider a slightly modified example:

//@ compile-flags: -Znext-solver

trait Mirror { type Assoc; } impl Mirror for T { type Assoc = T; }

fn call_bar(_: &dyn Bar<(), <() as Mirror>::Assoc>) {}

fn test(x: &dyn Bar<(), ()>) { call_bar(x); }

This fails in the new solver. In this example, we try to unify dyn Bar<(), ()> and dyn Bar<(), <() as Mirror>::Assoc>. We are faced with the same problem even though there are no inference variables, and making this work relies on eagerly and deeply normalizing all projections so that they can be structurally deduplicated.

This is incompatible with how we handle associated types in the new trait solver, and while we could perhaps support it with some major gymnastics in the new solver, it suggests more fundamental shortcomings with how we deal with projection bounds in the new solver.

Shortcomings: redundant projections

Consider a final example:

trait Foo { type Assoc; }

trait Bar: Foo<Assoc = ()> {}

fn call_bar1(_: &dyn Bar) {}

fn call_bar2(_: &dyn Bar<Assoc = ()>) {}

fn main() { let x: &dyn Bar<Assoc = _> = todo!(); call_bar1(x); //~~^ ERROR mismatched types call_bar2(x); //~~^ ERROR mismatched types }

In this case, we have a user-written associated type bound (Assoc = _) which overlaps the bound that comes from the supertrait projection of Bar (namely, Foo<Assoc = ()>). In a similar way to the two examples above, this causes us to have a projection list mismatch that the compiler is not able to deduplicate.

Solution

The general strategy that this PR takes is to change the way we construct the PolyExistentialPredicates of the dyn Trait type. Specifically, we do not eagerly elaborate into it any associated type bounds that are implied by the supertraits of the dyn type's principal trait, and instead adjust projection and other code to compute these supertrait projections on-demand as needed from the principal trait.

Concretely, consider the following code:

trait Super { type Assoc; }

type Trait: Super<Assoc = ()> {}

When users write dyn Trait, the projections on that type used to be [Super::Assoc = ()], but now are an empty list. When projecting <dyn Trait as Super>::Assoc, we used to look through that list for a match, and now we look through the list and also re-elaborate the principal trait Trait to figure out matches for projecting Assoc.

Specifically, I had to change these places in the compiler:

Projection, which just needs to look at both the user-written associated type bounds and now also the supertrait projections.
Closure elaboration, much like above, we want to be able to deduce closure signatures from supertrait projections.
Well-formedness, since we require the type on the RHS of implied supertrait projections to be WF.

The weirdest of these changes is well-formedness. This is because we want to keep supporting, for example, code that looks like:

trait Foo<'a, T>: Super<Assoc = &'a T> {}

fn outlives<'a, T: 'a>() {}

fn wf<'a, T>(_: &dyn Foo<'a, T>) { outlives::<'a, T>(); }

Since previously, due to eagerly elaborating supertrait projections into the type dyn Foo<'a, T> meant that it actually acted as if it were dyn Foo<'a, T, Assoc = &'a T>, and the implied bounds code could deduce that for the type to be well-formed, then &'a T must also be well-formed, and thus we must also have that T: 'a. I don't think it's that big of a deal to keep supporting this code, since I'm mainly concerned with fixing this class of dyn Trait bugs and not reworking the way that dyn Trait interacts with implied bounds.

Caveat: Associated type bound shadowing

Since this PR changes the code to no longer explicitly treat associated type bounds that are elaborated from supertrait bounds the same as user-written associated type bounds, we must choose what to do when users write associated type bounds that overlap with ones from supertraits. Specifically, there are three distinct classes of associated type bounds.

Given code like:

trait Super { type Assoc; }

type Trait: Super<Assoc = ()> {}

// And also the Mirror trait definition from above.

Syntactically equal bound: user writes dyn Trait<Assoc = ()>. This associated type bound matches the supertrait associated type bound exactly.
Conflicting bound (semantically equal): dyn Trait<Assoc = <() as Mirror>::Assoc>. While this associated type bound is redundant above, we can't detect that it's redundant because normalizing while lowering dyn Trait types leads to trait cycles.
Conflicting bound (unequal): dyn Trait<Assoc = i32>. This bound is conflicting, and there are no types which may be coerced to this trait alias.

We have several choices for how to handle these.

Option A: Never deduplicate redundant associated type bounds

Always treat super-written associated type bounds as user-written predicates. This means that dyn Trait and dyn Trait<Assoc = ()> are distinct types.

I did not even begin to consider this solution since it seems to me to be unnecessarily breaking. I expect it to break thousands of crates in crater.

Option B: Deduplicate only syntactically equal associated type bounds

Filter out syntactically equal bounds (i.e. bounds from (1.) above), but otherwise treat associated type bounds as user-written projections (i.e. (2.) and (3.) above). While the fallout from this is much less than option A, we run the risk of introducing a new set of type errors on previously valid code:

trait IteratorOfUnit: Iterator<Item = ()> {} impl IteratorOfUnit for T where T: Iterator<Item = ()> {}

fn test(x: &dyn IteratorOfUnit<Item = <() as Mirror>::Assoc>) { let x: &dyn IteratorOfUnit = x; }

which would lead to a new type error since the first type (&dyn IteratorOfUnit<Item = <() as Mirror>::Assoc>) has a projection list of [Iterator::Item = <() as Mirror>::Assoc], and the second type has an empty projection list.

(side-note: You may ask why we can't just normalize here to filter out cases where we can trivially normalize -- normalizing in astconv/hir-lowering is very likely to result in cycle errors since normalization requires calling the same queries that we're normalizing within, so I think using normalization here would be incredibly fragile).

I analyzed this solution and there's some moderate breakage, specifically ~10 crates that all rely on an old version of ra_ap_hir_ty which had redundant type bounds on a dyn Trait (which I fixed in rust-lang/rust-analyzer#18577):

pub trait TypeFolder<I: Interner>: FallibleTypeFolder<I, Error = Infallible> { fn as_dyn(&mut self) -> &mut dyn TypeFolder; }

impl<I: Interner> TypeFolder for Foo { fn as_dyn(&mut self) -> &mut dyn TypeFolder<Interner, Error = Self::Error> { self } }

... which results in a type mismatch because dyn TypeFolder and dyn TypeFolder<Error = Self::Error> are distinct even though Self::Error normalizes to Infallible, which is what is implied by the supertrait bounds of TypeFolder.

Option C: Always prefer supertrait associated type bounds (the choice this PR chooses)

In the implementation proposed by this PR, we always prefer supertrait bounds over associated type bounds written on the type. That is, we never consider user-written associated type bounds when they are provided by a supertrait, and instead drop them on the floor, and lint if it doesn't agree with the bound from the supertrait.

That means any type dyn Trait<Assoc = ...> is equal to dyn Trait, since we ignore the Assoc = ... bound since it's implied by a supertrait. I think that the behavioral changes associated with this PR don't matter much in practice -- if the associated type is in group (2.) and semantically equal (i.e. equal modulo normalization) then we're not changing behavior here, and if the associated type is not equal (group (3.)), then the dyn trait type was never possible to create in the first place, so I don't expect much code to be exercising that anyways.

As a consequence, this code goes from fail to pass (for now, until we make a lint discussed below into a hard error):

trait IteratorOfUnit: Iterator<Item = ()> {} impl IteratorOfUnit for T where T: Iterator<Item = ()> {}

fn main() { let iter = [()].into_iter(); let iter: &dyn IteratorOfUnit<Item = i32> = &iter; }

However, we probably want to strongly discourage or outright ban associated type bounds which may be shadowed in this way. To begin the transition to being much stricter about these associated type bounds, I've added two lints to detect these cases.

Lints

To help users detect cases where behavior may be changed, I've added two lints:

DYN_ASSOC_REDUNDANT - Warn, which covers the group (1.) of syntactically equal bounds.
DYN_ASSOC_SHADOWED - Deny (although I could be convinced to upgrade this one to warn for a release cycle), which covers the last two groups (2.) and (3.) of conflicting bounds.

I don't expect DYN_ASSOC_REDUNDANT to ever become a hard error, but DYN_ASSOC_SHADOWED probably should become a hard error soon since it'll ensure that the code sample above (which went from fail -> pass) goes back to being an error as it should.

Downsides

The only surprising downside of this is the change in semantics around user-written associated type bounds which shadow ones implied by supertraits. I am pretty well convinced that this is unavoidable, and I chose the behavior to avoid the fallout from crater that we observed.

Conclusion

I could be convinced with going with option (B.), but I'd probably want to keep the DYN_ASSOC_SHADOWED lint as Deny so that we can move to eventually reporting these mismatches as soon as possible in ty lowering, rather than delaying them until a type mismatch occurs.