-
Notifications
You must be signed in to change notification settings - Fork 5k
Crossgen2 support for static virtual method resolution #54063
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
Changes from all commits
bbeb1f3
6191fa3
d5cd7a4
5eea0c5
b4e78cf
118e3fa
91e1805
9d5c7f5
c3248ec
147bf51
4815ff7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -572,6 +572,11 @@ public override MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(Me | |||||
return ResolveVariantInterfaceMethodToVirtualMethodOnType(interfaceMethod, (MetadataType)currentType); | ||||||
} | ||||||
|
||||||
public override MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc currentType, Func<TypeDesc, bool> inVersionBubble) | ||||||
{ | ||||||
return ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, (MetadataType)currentType, inVersionBubble); | ||||||
} | ||||||
|
||||||
//////////////////////// INTERFACE RESOLUTION | ||||||
//Interface function resolution | ||||||
// Interface function resolution follows the following rules | ||||||
|
@@ -826,5 +831,125 @@ public static IEnumerable<MethodDesc> EnumAllVirtualSlots(MetadataType type) | |||||
} while (type != null); | ||||||
} | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Try to resolve a given virtual static interface method on a given constrained type and its base types. | ||||||
/// </summary> | ||||||
/// <param name="interfaceMethod">Interface method to resolve</param> | ||||||
/// <param name="currentType">Type to attempt virtual static method resolution on</param> | ||||||
/// <returns>MethodDesc of the resolved virtual static method, null when not found (runtime lookup must be used)</returns> | ||||||
public static MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType, Func<TypeDesc, bool> inVersionBubble) | ||||||
{ | ||||||
TypeDesc interfaceType = interfaceMethod.OwningType; | ||||||
if (!inVersionBubble(interfaceType)) | ||||||
{ | ||||||
return null; | ||||||
} | ||||||
|
||||||
// Search for match on a per-level in the type hierarchy | ||||||
for (TypeDesc typeToCheck = currentType; typeToCheck != null; typeToCheck = typeToCheck.BaseType) | ||||||
{ | ||||||
if (!inVersionBubble(typeToCheck)) | ||||||
{ | ||||||
return null; | ||||||
} | ||||||
|
||||||
MethodDesc resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, interfaceMethod); | ||||||
if (resolvedMethodOnType != null) | ||||||
{ | ||||||
return resolvedMethodOnType; | ||||||
} | ||||||
|
||||||
// Variant interface dispatch | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The static virtual methods don't do a two pass variance check? (First try to find an exact match and only then do a second pass looking for variant match.) If so could you please model these APIs the same as the existing "ResolveVariantInterfaceMethodToVirtualMethodOnType" and "ResolveInterfaceMethodToVirtualMethodOnType"? (i.e. add "ResolveInterfaceMethodToStaticVirtualMethodOnType" that "ResolveVariantInterfaceMethodToStaticVirtualMethodOnType" can call into as the first thing). |
||||||
foreach (DefType runtimeInterfaceType in typeToCheck.RuntimeInterfaces) | ||||||
{ | ||||||
if (runtimeInterfaceType == interfaceType) | ||||||
{ | ||||||
// This is the variant interface check logic, skip this | ||||||
continue; | ||||||
} | ||||||
|
||||||
if (!runtimeInterfaceType.HasSameTypeDefinition(interfaceType)) | ||||||
{ | ||||||
// Variance matches require a typedef match | ||||||
// Equivalence isn't sufficient, and is uninteresting as equivalent interfaces cannot have static virtuals. | ||||||
continue; | ||||||
} | ||||||
|
||||||
if (runtimeInterfaceType.CanCastTo(interfaceType)) | ||||||
{ | ||||||
if (!inVersionBubble(runtimeInterfaceType)) | ||||||
{ | ||||||
// Fail the resolution if a candidate variant interface match is outside of the version bubble | ||||||
return null; | ||||||
} | ||||||
|
||||||
// Attempt to resolve on variance matched interface | ||||||
MethodDesc runtimeInterfaceMethod = TryResolveInterfaceMethodOnVariantCompatibleInterface(runtimeInterfaceType, interfaceMethod); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we actually want to do this:
Suggested change
|
||||||
|
||||||
if (runtimeInterfaceMethod != null) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Under what condition could this be null? |
||||||
{ | ||||||
resolvedMethodOnType = TryResolveVirtualStaticMethodOnThisType(typeToCheck, runtimeInterfaceMethod); | ||||||
} | ||||||
if (resolvedMethodOnType != null) | ||||||
{ | ||||||
return resolvedMethodOnType; | ||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
return null; | ||||||
} | ||||||
|
||||||
private static MethodDesc TryResolveInterfaceMethodOnVariantCompatibleInterface(TypeDesc compatibleInterfaceType, MethodDesc interfaceMethod) | ||||||
{ | ||||||
Debug.Assert(compatibleInterfaceType.CanCastTo(interfaceMethod.OwningType)); | ||||||
if (compatibleInterfaceType is MetadataType mdType) | ||||||
{ | ||||||
foreach (MethodDesc runtimeInterfaceMethod in mdType.GetVirtualMethods()) | ||||||
{ | ||||||
if (runtimeInterfaceMethod.Name == interfaceMethod.Name && | ||||||
runtimeInterfaceMethod.Signature == interfaceMethod.Signature) | ||||||
{ | ||||||
return runtimeInterfaceMethod; | ||||||
} | ||||||
} | ||||||
} | ||||||
return null; | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Try to resolve a given virtual static interface method on a given constrained type and return the resolved method or null when not found. | ||||||
/// </summary> | ||||||
/// <param name="constrainedType">Type to attempt method resolution on</param> | ||||||
/// <param name="interfaceType">Interface declaring the method</param> | ||||||
/// <param name="interfaceMethod">Method to resolve</param> | ||||||
/// <returns>MethodDesc of the resolved method or null when not found (runtime lookup must be used)</returns> | ||||||
private static MethodDesc TryResolveVirtualStaticMethodOnThisType(TypeDesc constrainedType, MethodDesc interfaceMethod) | ||||||
{ | ||||||
if (constrainedType is MetadataType mdType) | ||||||
{ | ||||||
foreach (MethodImplRecord methodImpl in mdType.FindMethodsImplWithMatchingDeclName(interfaceMethod.Name) ?? Array.Empty<MethodImplRecord>()) | ||||||
{ | ||||||
if (methodImpl.Decl == interfaceMethod) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you'll need to compare with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in the latest commit, thanks for explaining! |
||||||
{ | ||||||
MethodDesc resolvedMethodImpl = methodImpl.Body; | ||||||
if (resolvedMethodImpl.OwningType != mdType) | ||||||
{ | ||||||
ThrowHelper.ThrowMissingMethodException(constrainedType, resolvedMethodImpl.Name, resolvedMethodImpl.Signature); | ||||||
} | ||||||
if (interfaceMethod.HasInstantiation || methodImpl.Body.HasInstantiation || constrainedType.HasInstantiation) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
If I believe InstantiateSignature is going to be a no-op if the constrained type has an instantiation because that part is already instantiated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in the latest commit, thanks for the suggestion. |
||||||
{ | ||||||
resolvedMethodImpl = resolvedMethodImpl.InstantiateSignature(constrainedType.Instantiation, interfaceMethod.Instantiation); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in the latest commit, thanks for the suggestion. |
||||||
} | ||||||
if (resolvedMethodImpl != null) | ||||||
{ | ||||||
return resolvedMethodImpl; | ||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
return null; | ||||||
} | ||||||
} | ||||||
} |
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
inVersionBubble
parameter should probably go under#if READYTORUN
.We don't ifdef things in the type system, but also we don't put version bubble stuff here. So maybe an ifdef is less evil.
I do wonder why we were able to get by without a bool like this for the other virtual method resolution scenarios.