Skip to content

Add support for static default interface methods #72661

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.

Sign up for GitHub

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

Merged
MichalStrehovsky merged 2 commits into from
Jul 22, 2022
Merged

Add support for static default interface methods #72661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf1412d
Add support for static default interface methods
MichalStrehovsky Jul 22, 2022
9a3715a
Couple fixes
MichalStrehovsky Jul 22, 2022
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
58 changes: 38 additions & 20 deletions src/coreclr/tools/Common/Compiler/TypeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Runtime.CompilerServices;
Copy link
Copy Markdown
Member

@jkotas jkotas Jul 22, 2022

Choose a reason for hiding this comment

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

Is this using really needed?

Copy link
Copy Markdown
Member Author

@MichalStrehovsky MichalStrehovsky Jul 22, 2022

Choose a reason for hiding this comment

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

Yup, Unsafe.NullRef so that users who don't need static method support av if they accidentally pass static methods.


using Internal.IL;
using Internal.TypeSystem;

Expand Down Expand Up @@ -410,14 +412,19 @@ private static bool ImplementsEquivalentInterface(this TypeDesc type, TypeDesc i
return false;
}

public static MethodDesc TryResolveConstraintMethodApprox(this TypeDesc constrainedType, TypeDesc interfaceType, MethodDesc interfaceMethod, out bool forceRuntimeLookup)
{
return TryResolveConstraintMethodApprox(constrainedType, interfaceType, interfaceMethod, out forceRuntimeLookup, ref Unsafe.NullRef<DefaultInterfaceMethodResolution>());
}

/// <summary>
/// Attempts to resolve constrained call to <paramref name="interfaceMethod"/> into a concrete non-unboxing
/// method on <paramref name="constrainedType"/>.
/// The ability to resolve constraint methods is affected by the degree of code sharing we are performing
/// for generic code.
/// </summary>
/// <returns>The resolved method or null if the constraint couldn't be resolved.</returns>
public static MethodDesc TryResolveConstraintMethodApprox(this TypeDesc constrainedType, TypeDesc interfaceType, MethodDesc interfaceMethod, out bool forceRuntimeLookup)
public static MethodDesc TryResolveConstraintMethodApprox(this TypeDesc constrainedType, TypeDesc interfaceType, MethodDesc interfaceMethod, out bool forceRuntimeLookup, ref DefaultInterfaceMethodResolution staticResolution)
{
forceRuntimeLookup = false;

Expand Down Expand Up @@ -461,24 +468,23 @@ public static MethodDesc TryResolveConstraintMethodApprox(this TypeDesc constrai
interfaceType.ConvertToCanonForm(CanonicalFormKind.Specific))
{
potentialMatchingInterfaces++;

// The below code is just trying to prevent one of the matches from requiring boxing
// It doesn't apply to static virtual methods.
if (isStaticVirtualMethod)
continue;

MethodDesc potentialInterfaceMethod = genInterfaceMethod;
if (potentialInterfaceMethod.OwningType != potentialInterfaceType)
{
potentialInterfaceMethod = context.GetMethodForInstantiatedType(
potentialInterfaceMethod.GetTypicalMethodDefinition(), (InstantiatedType)potentialInterfaceType);
}

if (isStaticVirtualMethod)
{
method = canonType.ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(potentialInterfaceMethod);
}
else
{
method = canonType.ResolveInterfaceMethodToVirtualMethodOnType(potentialInterfaceMethod);
}
method = canonType.ResolveInterfaceMethodToVirtualMethodOnType(potentialInterfaceMethod);

// See code:#TryResolveConstraintMethodApprox_DoNotReturnParentMethod
if (!isStaticVirtualMethod && method != null && !method.OwningType.IsValueType)
if (method != null && !method.OwningType.IsValueType)
{
// We explicitly wouldn't want to abort if we found a default implementation.
// The above resolution doesn't consider the default methods.
Expand Down Expand Up @@ -512,6 +518,12 @@ public static MethodDesc TryResolveConstraintMethodApprox(this TypeDesc constrai
if (isStaticVirtualMethod)
{
method = constrainedType.ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(exactInterfaceMethod);
if (method == null)
{
staticResolution = constrainedType.ResolveVariantInterfaceMethodToDefaultImplementationOnType(exactInterfaceMethod, out method);
if (staticResolution != DefaultInterfaceMethodResolution.DefaultImplementation)
method = null;
}
}
else
{
Expand Down Expand Up @@ -542,6 +554,12 @@ public static MethodDesc TryResolveConstraintMethodApprox(this TypeDesc constrai
if (isStaticVirtualMethod)
{
method = constrainedType.ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(exactInterfaceMethod);
if (method == null)
{
staticResolution = constrainedType.ResolveVariantInterfaceMethodToDefaultImplementationOnType(exactInterfaceMethod, out method);
if (staticResolution != DefaultInterfaceMethodResolution.DefaultImplementation)
method = null;
}
}
else
{
Expand All @@ -562,16 +580,6 @@ public static MethodDesc TryResolveConstraintMethodApprox(this TypeDesc constrai
method = null;
}

// Default implementation logic, which only kicks in for default implementations when looking up on an exact interface target
if (isStaticVirtualMethod && method == null && !genInterfaceMethod.IsAbstract && !constrainedType.IsCanonicalSubtype(CanonicalFormKind.Any))
{
MethodDesc exactInterfaceMethod = genInterfaceMethod;
if (genInterfaceMethod.OwningType != interfaceType)
exactInterfaceMethod = context.GetMethodForInstantiatedType(
genInterfaceMethod.GetTypicalMethodDefinition(), (InstantiatedType)interfaceType);
method = exactInterfaceMethod;
}

if (method == null)
{
// Fall back to VSD
Expand All @@ -594,6 +602,16 @@ public static MethodDesc TryResolveConstraintMethodApprox(this TypeDesc constrai
method = method.MakeInstantiatedMethod(methodInstantiation);
}

// It's difficult to discern what runtime determined form the interface method
// is on later so fail the resolution if this would be that.
// This is pretty conservative and can be narrowed down.
Comment on lines +605 to +607
Copy link
Copy Markdown
Member Author

@MichalStrehovsky MichalStrehovsky Jul 22, 2022

Choose a reason for hiding this comment

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

I went down this rabbit hole and it lead to stuff like:

https://github.com/MichalStrehovsky/runtime/blob/c7e5c934159fc27b5c92a0c609a6756cf43a26fe/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs#L519-L622

I really didn't like it and had trouble getting it to work. The problem is that RyuJIT doesn't operate on runtime-determined things and we can't even properly resolve such constrained calls (the support for that was never needed in the type system because only universal shared code needs to resolve constrained calls on runtime determined types otherwise and that was always supported by the STS type system in NUTC).

if (method.IsCanonicalMethod(CanonicalFormKind.Any)
&& !method.OwningType.IsValueType)
{
Debug.Assert(method.Signature.IsStatic);
return null;
}

Debug.Assert(method != null);

return method;
Expand Down
47 changes: 45 additions & 2 deletions src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,7 @@ private static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultI
bool diamondCase = false;
impl = null;

MethodDesc interfaceMethodDefinition = interfaceMethod.GetMethodDefinition();
DefType[] consideredInterfaces;
if (!currentType.IsInterface)
{
Expand All @@ -778,7 +779,7 @@ private static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultI
if (mostSpecificInterface == null && !interfaceMethod.IsAbstract)
{
mostSpecificInterface = runtimeInterface;
impl = interfaceMethod;
impl = interfaceMethodDefinition;
}
}
else if (Array.IndexOf(runtimeInterface.RuntimeInterfaces, interfaceMethodOwningType) != -1)
Expand All @@ -789,7 +790,7 @@ private static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultI
{
foreach (MethodImplRecord implRecord in possibleImpls)
{
if (implRecord.Decl == interfaceMethod)
if (implRecord.Decl == interfaceMethodDefinition)
{
// This interface provides a default implementation.
// Is it also most specific?
Expand Down Expand Up @@ -822,12 +823,54 @@ private static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultI
}
else if (impl.IsAbstract)
{
impl = null;
return DefaultInterfaceMethodResolution.Reabstraction;
}

if (interfaceMethod != interfaceMethodDefinition)
impl = impl.MakeInstantiatedMethod(interfaceMethod.Instantiation);

return DefaultInterfaceMethodResolution.DefaultImplementation;
}

public override DefaultInterfaceMethodResolution ResolveVariantInterfaceMethodToDefaultImplementationOnType(MethodDesc interfaceMethod, TypeDesc currentType, out MethodDesc impl)
{
return ResolveVariantInterfaceMethodToDefaultImplementationOnType(interfaceMethod, (MetadataType)currentType, out impl);
}

public static DefaultInterfaceMethodResolution ResolveVariantInterfaceMethodToDefaultImplementationOnType(MethodDesc interfaceMethod, MetadataType currentType, out MethodDesc impl)
{
Debug.Assert(interfaceMethod.Signature.IsStatic);

MetadataType interfaceType = (MetadataType)interfaceMethod.OwningType;
bool foundInterface = IsInterfaceImplementedOnType(currentType, interfaceType);

if (foundInterface)
{
DefaultInterfaceMethodResolution resolution = ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, currentType, out impl);
if (resolution != DefaultInterfaceMethodResolution.None)
return resolution;
}

MethodDesc interfaceMethodDefinition = interfaceMethod.GetMethodDefinition();
foreach (TypeDesc iface in currentType.RuntimeInterfaces)
{
if (iface.HasSameTypeDefinition(interfaceType) && iface.CanCastTo(interfaceType))
{
MethodDesc variantMethod = iface.FindMethodOnTypeWithMatchingTypicalMethod(interfaceMethodDefinition);
Debug.Assert(variantMethod != null);
if (interfaceMethod != interfaceMethodDefinition)
variantMethod = variantMethod.MakeInstantiatedMethod(interfaceMethod.Instantiation);
DefaultInterfaceMethodResolution resolution = ResolveInterfaceMethodToDefaultImplementationOnType(variantMethod, currentType, out impl);
if (resolution != DefaultInterfaceMethodResolution.None)
return resolution;
}
}

impl = null;
return DefaultInterfaceMethodResolution.None;
}

public override IEnumerable<MethodDesc> ComputeAllVirtualSlots(TypeDesc type)
{
return EnumAllVirtualSlots((MetadataType)type);
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,11 @@ public static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultIm
return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out implMethod);
}

public static DefaultInterfaceMethodResolution ResolveVariantInterfaceMethodToDefaultImplementationOnType(this TypeDesc type, MethodDesc interfaceMethod, out MethodDesc implMethod)
{
return type.Context.GetVirtualMethodAlgorithmForType(type).ResolveVariantInterfaceMethodToDefaultImplementationOnType(interfaceMethod, type, out implMethod);
}

/// <summary>
/// Resolves a virtual method call.
/// </summary>
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Common/VirtualMethodAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public abstract class VirtualMethodAlgorithm

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

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

/// <summary>
/// Resolves a virtual method call.
/// </summary>
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,8 +382,10 @@ public sealed override IEnumerable<CombinedDependencyListEntry> GetConditionalSt
{
Debug.Assert(!implMethod.IsVirtual);

MethodDesc defaultIntfMethod = implMethod.GetCanonMethodTarget(CanonicalFormKind.Specific);

// If the interface method is used virtually, the implementation body is used
result.Add(new CombinedDependencyListEntry(factory.CanonicalEntrypoint(implMethod), factory.VirtualMethodUse(interfaceMethod), "Interface method"));
result.Add(new CombinedDependencyListEntry(factory.MethodEntrypoint(defaultIntfMethod), factory.VirtualMethodUse(interfaceMethod), "Interface method"));
}
else
{
Expand Down
10 changes: 8 additions & 2 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericLookupResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1454,9 +1454,15 @@ public override ISymbolNode GetTarget(NodeFactory factory, GenericLookupResultCo
if (instantiatedConstrainedMethod.Signature.IsStatic)
{
implMethod = instantiatedConstraintType.GetClosestDefType().ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(instantiatedConstrainedMethod);
if (implMethod == null && !instantiatedConstrainedMethod.IsAbstract)
if (implMethod == null)
{
implMethod = instantiatedConstrainedMethod;
DefaultInterfaceMethodResolution resolution =
instantiatedConstraintType.GetClosestDefType().ResolveVariantInterfaceMethodToDefaultImplementationOnType(instantiatedConstrainedMethod, out implMethod);
if (resolution != DefaultInterfaceMethodResolution.DefaultImplementation)
{
// TODO: diamond/reabstraction
ThrowHelper.ThrowInvalidProgramException();
}
}
}
else
Expand Down
22 changes: 18 additions & 4 deletions src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ private void ImportCall(ILOpcode opcode, int token)

bool resolvedConstraint = false;
bool forceUseRuntimeLookup = false;
DefaultInterfaceMethodResolution staticResolution = default;

MethodDesc methodAfterConstraintResolution = method;
if (_constrained != null)
Expand All @@ -398,7 +399,7 @@ private void ImportCall(ILOpcode opcode, int token)
if (constrained.IsRuntimeDeterminedSubtype)
constrained = constrained.ConvertToCanonForm(CanonicalFormKind.Specific);

MethodDesc directMethod = constrained.GetClosestDefType().TryResolveConstraintMethodApprox(method.OwningType, method, out forceUseRuntimeLookup);
MethodDesc directMethod = constrained.GetClosestDefType().TryResolveConstraintMethodApprox(method.OwningType, method, out forceUseRuntimeLookup, ref staticResolution);
if (directMethod == null && constrained.IsEnum)
{
// Constrained calls to methods on enum methods resolve to System.Enum's methods. System.Enum is a reference
Expand All @@ -419,7 +420,7 @@ private void ImportCall(ILOpcode opcode, int token)
|| methodAfterConstraintResolution.Signature.IsStatic);
resolvedConstraint = true;

exactType = constrained;
exactType = directMethod.OwningType;
}
else if (method.Signature.IsStatic)
{
Expand Down Expand Up @@ -525,7 +526,7 @@ private void ImportCall(ILOpcode opcode, int token)
}
else
{
_dependencies.Add(_factory.FatFunctionPointer(runtimeDeterminedMethod), reason);
_dependencies.Add(_factory.FatFunctionPointer(targetMethod), reason);
}
}
else if (directCall)
Expand Down Expand Up @@ -676,14 +677,27 @@ private void ImportCall(ILOpcode opcode, int token)
_dependencies.Add(GetMethodEntrypoint(targetMethod), reason);
}
}
else if (staticResolution is DefaultInterfaceMethodResolution.Diamond or DefaultInterfaceMethodResolution.Reabstraction)
{
Debug.Assert(targetMethod.OwningType.IsInterface && targetMethod.IsVirtual && _constrained != null);

ThrowHelper.ThrowBadImageFormatException();
}
else if (method.Signature.IsStatic)
{
// This should be an unresolved static virtual interface method call. Other static methods should
// have been handled as a directCall above.
Debug.Assert(targetMethod.OwningType.IsInterface && targetMethod.IsVirtual && _constrained != null);

var constrainedCallInfo = new ConstrainedCallInfo(_constrained, runtimeDeterminedMethod);
_dependencies.Add(GetGenericLookupHelper(ReadyToRunHelperId.ConstrainedDirectCall, constrainedCallInfo), reason);
var constrainedHelperId = ReadyToRunHelperId.ConstrainedDirectCall;

// Constant lookup doesn't make sense and we don't implement it. If we need constant lookup,
// the method wasn't implemented. Don't crash on it.
if (!_compilation.NeedsRuntimeLookup(constrainedHelperId, constrainedCallInfo))
ThrowHelper.ThrowTypeLoadException(_constrained);

_dependencies.Add(GetGenericLookupHelper(constrainedHelperId, constrainedCallInfo), reason);
}
else if (method.HasInstantiation)
{
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/DependencyAnalysis/MethodCodeNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public class MethodCodeNode : ObjectNode, IMethodBodyNode, INodeWithCodeInfo, IN
public MethodCodeNode(MethodDesc method)
{
Debug.Assert(!method.IsAbstract);
Debug.Assert(!method.IsGenericMethodDefinition && !method.OwningType.IsGenericDefinition);
Debug.Assert(method.GetCanonMethodTarget(CanonicalFormKind.Specific) == method);
_method = method;
}
Expand Down
Loading