traits: Use a placeholder self type for escaping dyn principals in WF#157280
Open
Dnreikronos wants to merge 2 commits into
Open
traits: Use a placeholder self type for escaping dyn principals in WF#157280Dnreikronos wants to merge 2 commits into
Dnreikronos wants to merge 2 commits into
Conversation
Collaborator
|
changes to the core type system cc @lcnr |
rustbot
added
S-waiting-on-review
Collaborator
|
r? @mejrs rustbot has assigned @mejrs. Use Why was this reviewer chosen?The reviewer was selected based on:
|
WF-checking walks through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` bound reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Feeding that type into `ExistentialTraitRef::with_self_ty` violated its no-escaping-self precondition and tripped the assertion that guards it. The trait ref built there is only used to read off `ConstArgHasType` clauses, which constrain the trait's own const arguments and never mention the `Self` type. Substitute a fresh placeholder self type when the real one escapes: the assertion holds and the const-argument check is still performed. Re-enable the `with_self_ty` debug assertion now that its precondition is upheld.
One test covers the original ICE (a `dyn Trait` nested in a `for<'a>` bound, distilled from itertools). The other locks in that a const argument on such a nested `dyn` is still type-checked, so the placeholder-self-type fix cannot silently regress into dropping the `ConstArgHasType` check.
Dnreikronos
force-pushed
the
existential_trait_ref_escaping_bound_vars_assert
branch
from
87f6783 to
0676d4a
Compare
Collaborator
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Contributor
|
I'm not confident reviewing this. r? @fmease perhaps? |
Contributor
Author
Contributor
|
Oh it's not about what and how you implemented this, I'm very unfamiliar with this part of the compiler so I'm ill-suited to review PRs touching it. |
oli-obk
added
T-types
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #157122
WF-checking walks through higher-ranked binders without instantiating them, so a
dyn Traitnested in afor<'a>bound reaches thety::Dynamicarm ofWfPredicates::visit_tywhile it still carries escaping bound vars. That type got handed toExistentialTraitRef::with_self_ty, and itsdebug_assert!(!self_ty.has_escaping_bound_vars())fired while building the standard library. The assert was turned back on recently after sitting commented out since a 2018 refactor.The trait ref built there only exists to read off
ConstArgHasTypeclauses, which constrain the trait's own const arguments and never mentionSelf. So when the real self type escapes, swap in a fresh placeholder. The assert holds and the const-argument check still runs.Skipping the block when
tescapes was my first attempt, since it mirrors the guard on the projection bounds right below it. It's wrong. Skipping drops theConstArgHasTypeobligation, so an ill-typed const argument on adynnested in an HRTB compiles clean when it should error. The sibling check incheck.rsonly runs for type aliases, not fn params, so nothing else catches it. The second regression test pins that down.A binder-based fix doesn't work here either. You'd want to instantiate the escaping vars with placeholders or rebind the obligation under
t's binder, buttis a bareTy, not aBinder. Its escaping vars are bound by an ancestor the visitor already walked past, sinceWfPredicatesoverridesvisit_tyandvisit_constbut notvisit_binder. There's no binder in hand to instantiate. The placeholder gets around that: the clauses we keep don't care about the self type, so a stand-in is enough.The placeholder-self-type trick is the same one the pretty-printer uses for existential principals.