Do not deduplicate list of associated types provided by dyn principal

[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 1: 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 2: 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 3: 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

Do not deduplicate after elaborating projections when lowering dyn types

The root cause of this issue has to do with mismatches of the deduplicated projection list before and after substitution or inference. This PR aims to avoid these issues by never deduplicating the projection list after elaborating the list of projections from the identity substituted principal trait ref.

For example,

trait Foo<T> {
    type Assoc;
}

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

When computing the projections for dyn Bar<(), ()>, before this PR we'd elaborate Bar<(), ()> to find a (deduplicated) projection list of [Foo<()>::Assoc = ()].

After this PR, we take the principal trait and use its identity substitutions Bar<A, B> during elaboration, giving us projections [Foo<A>::Assoc = A, Foo<B>::Assoc = B]. Only after this elaboration do we substitute A = (), B = () to get [Foo<()>::Assoc = (), Foo<()>::Assoc = ()]. This allows the type to be unified with the projections for dyn Bar<?A, ?B>, which are [Foo<?A>::Assoc = ?A, Foo<?B>::Assoc = ?B].

This helps us avoid shorcomings 1 noted above.

Do not deduplicate projections when relating dyn types

Similarly, we also do not call deduplicate when relating dyn types. This means that the list of projections does not differ depending on if the type has been normalized or not, which should avoid shortcomings 2 noted above.

Following from the example above, when relating projection lists like [Foo<()>::Assoc = (), Foo<()>::Assoc = ()] and [Foo<?A>::Assoc = ?A, Foo<?B>::Assoc = ?B], the latter won't be deduplicated to a list of length 1 which would immediately fail to relate to the latter which is a list of length 2.

Implement proper precedence between supertrait and user-written projection bounds when lowering dyn types

trait Foo {
    type Assoc;
}

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

Given a type like dyn Foo<Assoc = _>, we used to previously include both the supertrait and user-written associated type bounds in the projection list, giving us [Foo::Assoc = (), Foo::Assoc = _]. This would never unify with dyn Foo. However, this PR implements a strategy which overwrites the supertrait associated type bound with the one provided by the user, giving us a projection list of [Foo::Assoc = _].

Why is this OK? Well, if a user wrote an associated type bound that is unsatisfiable (e.g. dyn Bar<Assoc = i32>) then the dyn type would never implement Bar or Foo anyways. If the user wrote something that is either structurally equal or equal modulo normalization to the supertrait bound, then it should be unaffected. And if the user wrote something that needs inference guidance (e.g. dyn Bar<Assoc = _>), then it'll be constrained when proving dyn Bar<Assoc = _>: Bar.

Importantly, this differs from the strategy in #133397, which preferred the supertrait bound and ignored the user-written bound. While that's also theoretically justifiable in its own way, it does lead to code which does not (and probably should not) compile either today or after this PR, like:

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;
}

Conclusion

This is a far less invasive change compared to #133397, and doesn't necessarily necessitate the addition of new lints or any breakage of existing code. While we could (and possibly should) eventually introduce lints to warn users of redundant or mismatched associated type bounds, we don't need to do so as part of fixing this unsoundness, which leads me to believe this is a much safer solution.

[compiler-errors]

@bors try @rust-timer queue

[bors]

⌛ Trying commit a55ea23 with merge 654bc80...

[bors]

☀️ Try build successful - checks-actions
Build commit: 654bc80 (654bc80249a2b2ba8bd16392b5829d87ed96c92d)

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-136458 created and queued.
🤖 Automatically detected try build 654bc80
🔍 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-136458 is now running

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

[rust-timer]

Finished benchmarking commit (654bc80): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking 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
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary 0.8%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.3%	[4.3%, 4.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 4.9%, secondary 2.8%)

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.

	mean	range	count
Regressions ❌ (primary)	4.9%	[4.9%, 4.9%]	1
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.9%	[4.9%, 4.9%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 778.369s -> 778.801s (0.06%)
Artifact size: 328.65 MiB -> 328.87 MiB (0.07%)

[lcnr]

please add a debug assert to Ty:new_dyn checking that the existential predicates list matches the list of the identity trait ref

[craterbot]

🎉 Experiment pr-136458 is completed!
📊 4 regressed and 1 fixed (577338 total)
📰 Open the full report.

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

[rustbot]

changes to the core type system

cc @compiler-errors, @lcnr

HIR ty lowering was modified

cc @fmease

[lcnr]

This is a far simpler and less breaking solution than #133397. I still feel quite surprised why we did not arrive at this solution originally. It results in no breakage found via crater while fixing the incompleteness when relating trait objects.

@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 #136751) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[jackh726]

Checked my box. I didn't dig in-depth into this this, but at a high level seems correct to me. I also think there's relatively little risk given no crater breakage and that we're in theory only keeping around more info (so, I think it's unlikely we could accidentally allow more code).