-
Notifications
You must be signed in to change notification settings - Fork 14.1k
skip checking supertraits in object candidate for NormalizesTo goal
#149167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This already fixes the issue. I wonder whether skipping proving the trait goal in Oh, the ICE variant is not fixed yet. Let me have a look. |
|
Minimization 1 and 3 compile with the next solver. |
| // We can erase them since evaluation later doesn't care about regions anyway. | ||
| let parent_trait_pred = | ||
| self.tcx.erase_and_anonymize_regions(cause.derived.parent_trait_pred); | ||
| try_borrowing(parent_trait_pred, &[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old solver doesn't cause ICE because it doesn't eagerly resolve vars before canonicalizing the predicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, can you move this into a separate PR, makes it easier to approve the rest of this one
The old solver doesn't cause ICE because it doesn't eagerly resolve vars before canonicalizing the predicate.
so the parent_trait_pred has leaking infer vars in both solvers? ah well, please move this into a separate PR, I would like to potentially fix this closer to the root of the leaked infer vars.
This comment has been minimized.
This comment has been minimized.
| /// Consider a clause specifically for a `dyn Trait` self type. This requires | ||
| /// additionally checking all of the supertraits and object bounds to hold, | ||
| /// since they're not implied by the well-formedness of the object type. | ||
| /// `NormalizesTo` overrides this to not check the supertraits, for backwards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// `NormalizesTo` overrides this to not check the supertraits, for backwards | |
| /// | |
| /// `NormalizesTo` overrides this to not check the supertraits for backwards |
| then(ecx) | ||
| } | ||
|
|
||
| fn probe_and_consider_object_bound_candidate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment here as well that this differs from trait goals with a FIXME(-Zhigher-ranked-assumptions) that we should try to unify these again once we properly support binders, linking to the issue fixed by this change
| // The old solver doesn't check the supertraits of the principal trait | ||
| // when considering object candidate for normalization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and the new solver previously did, resulting in a placeholder error while normalizing inside of a generator witness
| // We can erase them since evaluation later doesn't care about regions anyway. | ||
| let parent_trait_pred = | ||
| self.tcx.erase_and_anonymize_regions(cause.derived.parent_trait_pred); | ||
| try_borrowing(parent_trait_pred, &[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, can you move this into a separate PR, makes it easier to approve the rest of this one
The old solver doesn't cause ICE because it doesn't eagerly resolve vars before canonicalizing the predicate.
so the parent_trait_pred has leaking infer vars in both solvers? ah well, please move this into a separate PR, I would like to potentially fix this closer to the root of the leaked infer vars.
|
@bors r+ rollup |
Rollup of 8 pull requests Successful merges: - #147736 (Stabilize `asm_cfg`) - #148652 (Cleanup and refactor FnCtxt::report_no_match_method_error) - #149167 (skip checking supertraits in object candidate for `NormalizesTo` goal) - #149210 (fix: Do not ICE on normalization failure of an extern static item) - #149268 (add implementation-internal namespace for globalasm) - #149274 (Fix invalid link generation for type alias methods) - #149302 (Fix comment wording in simplify_comparison_integral.rs) - #149305 (Simplify OnceCell Clone impl) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #149167 - adwinwhite:next-245, r=lcnr skip checking supertraits in object candidate for `NormalizesTo` goal Fixes rust-lang/trait-system-refactor-initiative#245. r? `@lcnr`
Rollup of 8 pull requests Successful merges: - rust-lang/rust#147736 (Stabilize `asm_cfg`) - rust-lang/rust#148652 (Cleanup and refactor FnCtxt::report_no_match_method_error) - rust-lang/rust#149167 (skip checking supertraits in object candidate for `NormalizesTo` goal) - rust-lang/rust#149210 (fix: Do not ICE on normalization failure of an extern static item) - rust-lang/rust#149268 (add implementation-internal namespace for globalasm) - rust-lang/rust#149274 (Fix invalid link generation for type alias methods) - rust-lang/rust#149302 (Fix comment wording in simplify_comparison_integral.rs) - rust-lang/rust#149305 (Simplify OnceCell Clone impl) r? `@ghost` `@rustbot` modify labels: rollup
Fixes rust-lang/trait-system-refactor-initiative#245.
r? @lcnr