-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Fixes insert_trait_implementaion for nested generics. #6827
base: master
Are you sure you want to change the base?
Conversation
The unify_checker was returning false when comparing MyOption<T> with T. And filter_by_type_inner type substitution when adding methods from MyOption<T> to MyOption<MyOption<T>> was replacing MyOption<T> in its own type, ending up by inserting into type MyOption<MyOption<MyOption<T>>> instead of MyOption<MyOption<T>>. Fixes #6825
CodSpeed Performance ReportMerging #6827 will not alter performanceComparing Summary
|
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 changes are fine. I just don't understand why the code that's being removed was introduced in the first place.
I've added a nit about the length of one of the methods. Another nit is this:
The unify_checker was returning false when comparing `MyOption<T>` with `T`.
Surely those two types don't unify (unless the two T
s are independent type variables)? AFAICT the test you've introduced also doesn't test that unification.
type_id.subst(&SubstTypesContext::new( | ||
engines, | ||
&type_mapping, | ||
matches!(code_block_first_pass, CodeBlockFirstPass::No), | ||
)); |
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.
It seems reasonable to remove this bit of code, but why was it there in the first place? It seems like an odd thing to write by accident, so surely there was a point to it?
matches!(self.mode, ConstraintSubset) | ||
|| !OccursCheck::new(self.engines).check(right, left) |
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.
Nit: We really should refactor this method. It's extremely long, and it's hard to follow what's going on when the entire method doesn't fit on the screen.
Description
The unify_checker was returning false when comparing
MyOption<T>
withT
. And filter_by_type_inner type substitution when adding methods fromMyOption<T>
toMyOption<MyOption<T>>
was replacing MyOption in its own type, ending up by inserting into typeMyOption<MyOption<MyOption<T>>>
instead ofMyOption<MyOption<T>>
.Fixes #6825
Checklist
Breaking*
orNew Feature
labels where relevant.