Item bounds can reference self projections and still be object safe

[compiler-errors]

Background

Currently, we have some interesting rules about where Self is allowed to be mentioned in objects. Specifically, we allow mentioning Self behind associated types (e.g. fn foo(&self) -> Self::Assoc) only if that Self type comes from the trait we're defining or its supertraits:

trait Foo {
  fn good() -> Self::Assoc; // GOOD :)

  fn bad() -> <Self as OtherTrait>::Assoc; // BAD!
}

And more specifically, these Self::Assoc projections are only allowed to show up in:

• (A1) Method signatures
• (A2) Where clauses on traits, GATs and methods

But Self::Assoc projections are not allowed to show up in:

• (B1) Supertrait bounds (specifically: all super-predicates, which includes the projections that come from elaboration, and not just the traits themselves).
• (B2) Item bounds of associated types

The reason for (B1) is interesting: specifically, it arises from the fact that we currently eagerly elaborate all projection predicates into the object, so if we had the following code:

trait Sub<Assoc = Self::SuperAssoc> {}
trait Super {
    type SuperAssoc;
}

Then given dyn Sub<SuperAssoc = i32> we would need to have a type that is substituted into itself an infinite number of times¹, like dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <... as Super>::SuperAssoc> as Super>::SuperAssoc> as Super>::SuperAssoc>, i.e. the fixed-point of: type T = dyn Sub<SuperAssoc = i32, Assoc = <T as Super>::SuperAssoc>.

Similarly for (B2), we restrict mentioning Self::Assoc in associated type item bounds, which is the cause for #122798. However, there is no reason for us to do so, since item bounds never show up structurally in the dyn Trait object type.

What?

This PR relaxes the check for item bounds so that Self may be mentioned behind associated types in the same cases that they currently work for method signatures (A1) and where clauses (A2).

Why?

Fixes #122798. Removes a subtle and confusing inconsistency for the code mentioned in that issue.

This is sound because we only assemble alias bounds for rigid projections, and all projections coming from an object self type are not rigid, since all associated types should be specified by the type.

This is also desirable because we can do this via supertraits already. In #122789, it is noted that an item bound of Eq already works, just not PartialEq because of the default item bound. This is weird and should be fixed.

Future work

We could make the check for Self in super-predicates more sophisticated as well, only erroring if Self shows up in a projection super-predicate.

Footnotes

1. This could be fixed by some sort of structural replacement or eager normalization, but I don't think it's necessary currently. ↩

[BoxyUwU]

would like to have an fcp for this tho. probably should mention about the fact that you can already kind of do this via supertraits, and also that we only assemble alias bound candidates for rigid aliases and <dyn Trait as Trait>::Assoc is never going to be a rigid alias so the item bounds shouldn't affect the trait object

[compiler-errors]

Ya, I realized during discussion I should probably explain exactly why this is sound, specifically discussing the way we do Object candidate confirmation, etc. I'll write that up.

[bors]

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

[compiler-errors]

This is ready I believe :3

@rfcbot fcp merge

[rfcbot]

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

• @BoxyUwU
• @aliemjay
• @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.

[nikomatsakis]

@rfcbot reviewed

[rfcbot]

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

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[BoxyUwU]

@bors r+

[bors]

📌 Commit 1dcf764 has been approved by BoxyUwU

It is now in the queue for this repository.

[jieyouxu]

Failed in #125956 (comment)
@bors r-

[lqd]

hehe, race condition, I was writing the same rollup failure message that this PR needed a rebase

[bors]

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

[BoxyUwU]

@compiler-errors needs a rebase :-)

[compiler-errors]

@BoxyUwU @lcnr: y'all have a strong opinion about if #126090 should land before this?

I discovered an unsoundness in (EXISTING!) object safety that this PR technically makes easier to exploit with this change. I kinda don't think it matters to wait for that PR, but I did want to mention it.

[BoxyUwU]

I would be perhaps a little worried about the other PR landing then causing more regressions than expected and it getting reverted. That'd mean this PR would wind up making the problem A Bit worse on a long term basis potentially although I imagine we'd wind up with a fcw in that scenario 🤷‍♀️. I think trying to hit that unsoundness is also pretty hard in practice, I remember trying to find an example that'd hit it a few times and didn't manage to. I don't think it matters to land that PR before this one, we should just go ahead and merge this imo

[lcnr]

Given that #126090 should be in FCP soon and feels quite straight forward, I think we should wait until it's through FCP and then merge both PRs at once (as that the fix builds on this one).

[compiler-errors]

This was merged with #126090