Port CoreRT fix to System.Reflection.MetadataLoadContext

[MichalStrehovsky]

Port this to MetadataLoadContext and add test coverage:

https://github.com/dotnet/corert/pull/6638/commits/93364da1a71588650e1e4013bb481d4745dc5d9d

[joshfree]

Marking as up-for-grabs

[ShreyasJejurkar]

I have open corresponding PR for this issue with fix. #37204

[steveharter]

@MichalStrehovsky do you recall the scenario? I was unable to find a repro with that code path.

I assume some combination of a base class and derived class with open and closed generic parameters.

[steveharter]

Moving to future until we can get a repro.

The existing test cases in System.Reflection.Tests.TypeTests.TestMethodSelection1 do exercise similar code paths but do not have a case where:

• Base class and derived class both have generic methods of same name and sig
• One generic is open, and the other closed

Adding variants of this was unable to repro.

[MichalStrehovsky]

If I'm reading this code correctly, this code path is only reachable from GetImplicitlyOverriddenBaseClassMember in MetadataLoadContext and that method is dead code.

Please double check and if so, we can probably close and open an issue to deleted dead code instead. I assume MetadataLoadContext is not able to do things like get a list of inherited custom attributes as a result because this code is used for that on the CoreRT side.

[steveharter]

See the linked PR -- I can hit that branch, just not trigger the exception you reported. I'll create another PR to remove that dead code. Thanks

[MichalStrehovsky]

See the linked PR -- I can hit that branch, just not trigger the exception you reported

Looking at the linked PR, if you say we're hitting the branch with t1 = some generic type and t2 = int - that should throw an exception when calling GetGenericDefinition on the int. If we're not hitting the code path in that order, try reordering the methods or throwing in some extras whose parameter is not a generic type...

Also,could it be that the equivalent of typeof(int).GetGenericDefinition() doesn't throw in MetadataLoadContext? That might be another MetadataLoadContext bug then that could paper over this bug.

I don't recall the details anymore. It was a customer reported issue and all context is lost to GDPR compliance.

[steveharter]

Looking at the linked PR, if you say we're hitting the branch with t1 = some generic type and t2 = int - that should throw an exception when calling GetGenericDefinition on the int. If we're not hitting the code path in that order, try reordering the methods or throwing in some extras whose parameter is not a generic type...

The GenericMethodAwareAreParameterTypesEqual logic only runs when standard Type.Equals determines the types are not equal, so one can't be generic and another non-generic:

        //
        // This helper compares the types of the corresponding parameters of two methods to see if one method is signature equivalent to the other.
        // This is needed when comparing the signatures of two generic methods as Type.Equals() is not up to that job.
        //
        private static bool GenericMethodAwareAreParameterTypesEqual(Type t1, Type t2)
        {
            // Fast-path - if Reflection has already deemed them equivalent, we can trust its result.
            if (t1.Equals(t2))
                return true;

            // If we got here, Reflection determined the types not equivalent. Most of the time, that's the result we want.
            // There is however, one wrinkle. If the type is or embeds a generic method parameter type, Reflection will always report them
            // non-equivalent, since generic parameter type comparison always compares both the position and the declaring method. For our purposes, though,
            // we only want to consider the position.

So I was unable to construct a case that hits the line in question.

FWIW the call stack when that line in question is hit:

>   System.Reflection.MetadataLoadContext.dll!System.Reflection.Runtime.BindingFlagSupport.MemberPolicies<System.Reflection.MethodInfo>.GenericMethodAwareAreParameterTypesEqual(System.Type t1, System.Type t2) Line 152   C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.Runtime.BindingFlagSupport.MemberPolicies<System.Reflection.MethodInfo>.AreNamesAndSignaturesEqual(System.Reflection.MethodInfo method1, System.Reflection.MethodInfo method2) Line 113 C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.Runtime.BindingFlagSupport.MethodPolicies.ImplicitlyOverrides(System.Reflection.MethodInfo baseMember, System.Reflection.MethodInfo derivedMember) Line 37  C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.Runtime.BindingFlagSupport.MethodPolicies.IsSuppressedByMoreDerivedMember(System.Reflection.MethodInfo member, System.Reflection.MethodInfo[] priorMembers, int startIndex, int endIndex) Line 54   C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.Runtime.BindingFlagSupport.QueriedMemberList<System.Reflection.MethodInfo>.Create(System.Reflection.TypeLoading.RoType type, string filter, bool ignoreCase, bool immediateTypeOnly) Line 135   C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.TypeLoading.RoType.TypeComponentsCache.PerNameQueryCache<System.Reflection.MethodInfo>.Factory(string key) Line 138 C#
    System.Collections.Concurrent.dll!System.Collections.Concurrent.ConcurrentDictionary<string, System.Reflection.Runtime.BindingFlagSupport.QueriedMemberList<System.Reflection.MethodInfo>>.GetOrAdd(string key, System.Func<string, System.Reflection.Runtime.BindingFlagSupport.QueriedMemberList<System.Reflection.MethodInfo>> valueFactory) Line 1137   C#
    System.Reflection.MetadataLoadContext.dll!System.Collections.Concurrent.ConcurrentUnifier<string, System.Reflection.Runtime.BindingFlagSupport.QueriedMemberList<System.Reflection.MethodInfo>>.GetOrAdd(string key) Line 52    C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.TypeLoading.RoType.TypeComponentsCache.GetQueriedMembers<System.Reflection.MethodInfo>(string name, bool ignoreCase, bool immediateTypeOnly) Line 53    C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.TypeLoading.RoType.Query<System.Reflection.MethodInfo>(string optionalName, System.Reflection.BindingFlags bindingAttr, System.Func<System.Reflection.MethodInfo, bool> optionalPredicate) Line 191 C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.TypeLoading.RoType.Query<System.Reflection.MethodInfo>(string name, System.Reflection.BindingFlags bindingAttr) Line 176    C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.TypeLoading.RoType.GetMethodImplCommon(string name, int genericParameterCount, System.Reflection.BindingFlags bindingAttr, System.Reflection.Binder binder, System.Reflection.CallingConventions callConvention, System.Type[] types, System.Reflection.ParameterModifier[] modifiers) Line 82  C#
    System.Reflection.MetadataLoadContext.dll!System.Reflection.TypeLoading.RoType.GetMethodImpl(string name, System.Reflection.BindingFlags bindingAttr, System.Reflection.Binder binder, System.Reflection.CallingConventions callConvention, System.Type[] types, System.Reflection.ParameterModifier[] modifiers) Line 57   C#

[MichalStrehovsky]

It's possible this was only reachable from GetImplicitlyOverriddenBaseClassMember (I vaguely remember we hit this while trying to get inherited custom attributes on CoreRT). Since MetadataLoadContext decided it doesn't want to support APIs that retrieve inherited attributes and you're deleting the vestigial support for it, it's possible the bug is unreachable (still there, for someone to hit in the future, but not reachable right now). I don't have enough context on MetadataLoadContext to really sign off. I hope there's a co-owner of the component who can do that. Thanks for looking into it.

I only filed this bug because I got it as a feedback review for the CoreRT reflection stack fix. I don't intend to ramp up on MetadataLoadContext.

[steveharter]

Closing this. Please re-open if we have a repro.