Skip to content

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

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Member

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.

{
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
Copy link
Member

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we actually want to do this:

Suggested change
MethodDesc runtimeInterfaceMethod = TryResolveInterfaceMethodOnVariantCompatibleInterface(runtimeInterfaceType, interfaceMethod);
MethodDesc runtimeInterfaceMethod = runtimeInterfaceType.FindMethodOnExactTypeWithMatchingTypicalMethod(interfaceMethod);


if (runtimeInterfaceMethod != null)
Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll need to compare with the interfaceMethod.GetMethodDefinition(). The code below assumes interfaceMethod is already instantiated if it's generic but the MethodImpl records you get from the constrained type will not be instantiated.

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (interfaceMethod.HasInstantiation || methodImpl.Body.HasInstantiation || constrainedType.HasInstantiation)
if (interfaceMethod.HasInstantiation)

If interfaceMethod.HasInstantiation != methodImpl.Body.HasInstantiation, terrible things will happen below, so we might as well assume that and consider the Body check redundant.

I believe InstantiateSignature is going to be a no-op if the constrained type has an instantiation because that part is already instantiated.

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resolvedMethodImpl = resolvedMethodImpl.InstantiateSignature(constrainedType.Instantiation, interfaceMethod.Instantiation);
resolvedMethodImpl = resolvedMethodImpl.MakeInstantiatedMethod(interfaceMethod.Instantiation);

Copy link
Member Author

Choose a reason for hiding this comment

The 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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,11 @@ public static MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(this
return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveVariantInterfaceMethodToVirtualMethodOnType(interfaceMethod, type);
}

public static MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(this TypeDesc type, MethodDesc interfaceMethod, Func<TypeDesc, bool> inVersionBubble)
{
return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod, type, inVersionBubble);
}

public static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultImplementationOnType(this TypeDesc type, MethodDesc interfaceMethod, out MethodDesc implMethod)
{
return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out implMethod);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;

namespace Internal.TypeSystem
Expand All @@ -24,6 +25,8 @@ public abstract class VirtualMethodAlgorithm

public abstract MethodDesc ResolveVariantInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc currentType);

public abstract MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, TypeDesc currentType, Func<TypeDesc, bool> inVersionBubble);

public abstract DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultImplementationOnType(MethodDesc interfaceMethod, TypeDesc currentType, out MethodDesc impl);

/// <summary>
Expand Down
Loading