-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Use base type for generic argument matching #1316
Conversation
Oh, I also found a typo in some of the test classes' namespaces |
Codecov Report
@@ Coverage Diff @@
## develop #1316 +/- ##
===========================================
- Coverage 76.83% 76.80% -0.03%
===========================================
Files 188 188
Lines 5180 5191 +11
Branches 1061 1064 +3
===========================================
+ Hits 3980 3987 +7
- Misses 702 704 +2
- Partials 498 500 +2
Continue to review full report at Codecov.
|
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.
Approved, but with two minor queries if you're up for them. Generics are complicated....
{ | ||
// If it's not an interface, the implementation type MUST have derived from | ||
// the generic service type at some point or there's no way to cast. | ||
return Array.Empty<Type>(); |
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.
Can we test this path?
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.
I don't know if we can unless something had gone buggy in the checks where we determine if the thing in RegisterGeneric and As are compatible but I can try walking backwards through it to see if we'd ever get there.
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.
🐲
Thanks! I couldn't figure out a good way to test that one code path yet. My gut says it'll never get hit. Not sure I'll have time to figure a test in the super near future, so good to not hold it up. |
Yeah, probably don't need a test for what is sort of an Assert statement that only trips if something else has gone horribly wrong. |
Fix #1315.
When you call
GetInterfaces()
on aType
, it returns the set of implemented interfaces but has already swapped the generic type argument names in for the ones used on the implementingType
. For example, if you have:...then when you call
typeof(Thing<>).GetInterfaces()
the result will beIThing<TFirst>
- the name of the argument matches the implementer's name, not the one in the actual interface definition.We use this naming to line up the type parameters on generic types and service type interface implementations.
For classes, that's not what we were doing.
In the class case, we were basically doing
typeof(DervivedClass<>).GetGenericArguments()
andtypeof(BaseClass<>).GetGenericArguments()
and then trying to line up names. The problem is, the name had changed in the implementation so things wouldn't line up.Instead of doing
typeof(BaseClass<>).GetGenericArguments()
, we need to walk back in the class hierarchy to find whereDerivedClass<TFirst>
actually derives fromBaseClass<>
so we can make use of the compiler having already renamed all the generic arguments for us... then we can match on name.This PR does exactly that - walks back the inheritance hierarchy until it finds the derived class and then does the argument matching.
I added tests to show it'll work more than just one derived class deep, proving the updated name mapping works all the way down the stack.