Fix dyn incompleteness with multiple supertraits with different substitutions

[compiler-errors]

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<T> {
    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<T> {
    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<T> 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.

1. Syntactically equal bound: user writes dyn Trait<Assoc = ()>. This associated type bound matches the supertrait associated type bound exactly.
2. 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.
3. 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<T> 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<Interner>;
}

impl<I: Interner> TypeFolder<I> 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<T> 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.

[compiler-errors]

@bors try

[bors]

⌛ Trying commit 88fa8f4 with merge 52ae244...

[bors]

☀️ Try build successful - checks-actions
Build commit: 52ae244 (52ae2445414b2a196b318d85c2819995c6e5f1aa)

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-133397 created and queued.
🤖 Automatically detected try build 52ae244
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-133397 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-133397 is completed!
📊 1065 regressed and 0 fixed (542435 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[compiler-errors]

@bors try

for later: craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-133397/retry-regressed-list.txt p=1

[bors]

⌛ Trying commit 22cb94a with merge 3165a83...

[bors]

☀️ Try build successful - checks-actions
Build commit: 3165a83 (3165a8314026cdff6b1df9de8759749f19d98623)

[compiler-errors]

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-133397/retry-regressed-list.txt p=1

[craterbot]

👌 Experiment pr-133397-1 created and queued.
🤖 Automatically detected try build 3165a83
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-133397-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[compiler-errors]

@craterbot cancel

[craterbot]

🗑️ Experiment pr-133397-1 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rust-log-analyzer]

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#22 exporting to docker image format
#22 sending tarball 27.9s done
#22 DONE 33.6s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 3.79s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
warning: target feature `x87` must be enabled to ensure that the ABI of the current target can be implemented correctly
  |
  = note: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #116344 <https://github.com/rust-lang/rust/issues/116344>
---
 finished in 2.878 seconds
##[endgroup]
Generating lint docs (x86_64-unknown-linux-gnu)
##[group]Running stage2 lint-docs (x86_64-unknown-linux-gnu)
error: failed to test example in lint docs for `dyn_assoc_redundant` in /checkout/compiler/rustc_lint_defs/src/builtin.rs:5125: lint docs should contain the line `### Example`
This error was generated by the lint-docs tool.
This tool extracts documentation for lints from the source code and places
This tool extracts documentation for lints from the source code and places
them in the rustc book. See the declare_lint! documentation
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint_defs/macro.declare_lint.html


To re-run these tests, run: ./x.py test --keep-stage=0 src/tools/lint-docs
The --keep-stage flag should be used if you have already built the compiler


Command has failed. Rerun with -v to see more details.
  local time: Fri Jan 31 00:09:41 UTC 2025
  network time: Fri, 31 Jan 2025 00:09:41 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[compiler-errors]

Oh right I need lint docs.

[lcnr]

While in theory we could alternative change type equality of trait objects to no longer be structural and simply make sure that there's a match for all existential predicates. This seems both bad for perf, additional complexity, and doesn't really feel worth it.

The long-term goal of "trait objects must not have explicit bounds for already constrained associated types" seems quite clean and desirable to me.

Thanks @compiler-errors for your impl and the writeup.

cc @rust-lang/lang for info, I consider this to be purely owned by t-types.

@rfcbot fcp merge

[rfcbot]

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[bors]

☔ The latest upstream changes (presumably #136371) made this pull request unmergeable. Please resolve the merge conflicts.

[compiler-errors]

Actually, I've boiled down the fix here to a much smaller set of changes around how we produce the list of existential projections.

It much more closely mirrors how we handle projections today rather than having to rework trait solving and WF checking too, and it obviates the need to introduce new lints, and should also break no code (I hope). I'll close this FCP and rewrite/restart one over on #136458. Worst case if that solution doesn't pan out, I'll start a new FCP over here.

@rfcbot fcp cancel

[rfcbot]

@compiler-errors proposal cancelled.