Fix ICE when multiple supertrait substitutions need assoc but only one is provided

[compiler-errors]

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:

rust/compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs

Line 131 in 1fc691e

let mut associated_types: FxIndexMap<Span, FxIndexSet<DefId>> = FxIndexMap::default();

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:

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¹, which show no failures².

Crater: #133392 (comment)

Fixes #133388

Footnotes

1. I cratered this originally with Fix dyn incompleteness with multiple supertraits with different substitutions #133397, which is a PR that is stacked on top, then re-ran crater with just the failures from that PR. ↩

2. 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. ↩

[rustbot]

changes to the core type system

cc @compiler-errors, @lcnr

HIR ty lowering was modified

cc @fmease

[compiler-errors]

@bors try

[bors]

⌛ Trying commit 8a10f68 with merge c7377f6...

[bors]

☀️ Try build successful - checks-actions
Build commit: c7377f6 (c7377f633f2c785ef968ab6517e5218297777ae5)

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-133392 created and queued.
🤖 Automatically detected try build c7377f6
🔍 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

[compiler-errors]

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

[craterbot]

👌 Experiment pr-133392-1 created and queued.
🤖 Automatically detected try build c7377f6
⚠️ Try build based on commit 8a10f68, but latest commit is 2a0ede8. Did you forget to make a new try build?
🔍 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-133392-again is now running

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

[craterbot]

🎉 Experiment pr-133392-again is completed!
📊 0 regressed and 0 fixed (1598 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]

@rfcbot fcp merge

Hi @rust-lang/types. Please see the PR description. This fixes what I consider to be a minor issue in the way we validate the associated types that are required to be specified for a dyn Trait. While it's a theoretical breakage (and therefore @lcnr asked me to start an FCP), I don't believe this is a major decision.

[rfcbot]

Team member @compiler-errors 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 #133694) made this pull request unmergeable. Please resolve the merge conflicts.

[rfcbot]

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

[lcnr]

@bors r+ rollup

[bors]

📌 Commit 24da053 has been approved by lcnr

It is now in the queue for this repository.

[lcnr]

oh lel, i thought it finished the fcp period 😅

@bors r-

[BoxyUwU]

Kind of funny that you just cant specify the associated type equality bounds here

[compiler-errors]

Yeah, that's a fundamental limitation of Rust atm. I can't be arsed to improve the diagnostic, though I can open an issue for the diagnostic being confusing if you care.

[bors]

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