Match generic type parameters by position, not by name #465
Conversation
stakx
force-pushed
the
generic-type-parameter-name-mismatch
branch
from
8ca481e
to
f8366d6
Compare
| @@ -303,7 +303,7 @@ public Type[] GetGenericArgumentsFor(MethodInfo genericMethod) | |||
| var types = new List<Type>(); | |||
| foreach (var genType in genericMethod.GetGenericArguments()) | |||
| { | |||
| types.Add(name2GenericType[genType.Name].AsType()); | |||
| types.Add(genericTypeParams[genType.GenericParameterPosition].AsType()); | |||
There was a problem hiding this comment.
This makes sense. I wonder if it would be a good idea to check if genericTypeParams isn't null and the array contains the element before accessing it since it is provided externally by SetGenericTypeParameters, then provide a useful exception, so we don't get something useless like the previous KeyNotFoundException one.
Have you looked at the previous GetGenericArgumentsFor overload to see if that should also be changed?
There was a problem hiding this comment.
I wonder if it would be a good idea to check if
genericTypeParamsisn't null and the array contains the element before accessing it since it is provided externally bySetGenericTypeParameters
Having an assertion to that effect might be good, but it shouldn't be any more necessary than checking whether name2GenericType contains the key genType.Name. Note how the only place where SetGenericTypeParameters is called from initializes both genericTypeParams and populates name2GenericType:
I'll give it some more thought.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Have you looked at the previous
GetGenericArgumentsForoverload to see if that should also be changed?
@jonorossi, it turns out that other overload is most probably unreachable code. See the new commit for details. Without any test cases, it's hard to know whether or not it is faulty. But given that it's public and there may in theory be external consumers, I think the safest option is to leave it as is, but mark it [Obsolete].
We have yet another method, Type GetGenericArgument(string genericArgumentName), which precedes the two that we've discussed so far. The code paths surrounding generic type arguments are rather convoluted, and I don't understand them well enough (yet) to know whether or not there needs to be a change here, too.
stakx
force-pushed
the
generic-type-parameter-name-mismatch
branch
from
f8366d6
to
dc09dd7
Compare
It turns out that beside the name-to-parameter map `name2GenericType`, we also have a position-to-parameter map available that we can use in- stead of a costly LINQ query: `genericTypeParams`.
... but let's not turn into a debug-only assertion, because if it *is* reachable against our assumptions we need to find out about it so that we can add a test case.
stakx
force-pushed
the
generic-type-parameter-name-mismatch
branch
from
dc09dd7
to
3acb4d5
Compare
|
I think I'll need to take some time to study the (rather convoluted) code paths that are involved here in much more depth. I'd like to feel more confident in the change I'm proposing here... if possible. |
No problem. |
@stakx any change in your position? Should we merge or close this pull request? |
|
I haven't got around to really study this through as the code surrounding this is rather convoluted. Let's close this for now. When I eventually get this fully done, I'll reopen. |
Fixes #106.
I didn't (and still don't quite) trust this bug fix, mainly because it seems far too easy for DP standards (+-1 LoC!? 馃槤). However, it makes good sense to match generic type parameters by position instead of by name; in IL, for instance, generic type parameters are referred to by position (e.g.
``1). The execution environment doesn't really care about the names.To give us some added safety, I ran a few additional checks: