Skip to content

Commit

Permalink
Fix runtime dispatch to static virtuals on interface types (#91440)
Browse files Browse the repository at this point in the history
We were not generating information about static virtuals on interface types. Information about default interface methods normally goes to the class, but if the T we're dispatching on is an interface, this information wasn't generated. The fix is to put this information into dispatch maps and sealed vtables, same way we do for classes.

The test shows what the problem is - if we change `IBar` to be a class, things would work even before this PR.

Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
  • Loading branch information
github-actions[bot] and MichalStrehovsky authored Sep 1, 2023
1 parent 4d6f6bc commit 2134e62
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,8 @@ private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc
{
Debug.Assert(!interfaceMethod.Signature.IsStatic);

// This would be a default interface method resolution. The algorithm below would sort of work, but doesn't handle
// things like diamond cases and it's better not to let it resolve as such.
if (currentType.IsInterface)
return null;

Expand Down Expand Up @@ -781,7 +783,7 @@ private static DefaultInterfaceMethodResolution ResolveInterfaceMethodToDefaultI
// If we're asking about an interface, include the interface in the list.
consideredInterfaces = new DefType[currentType.RuntimeInterfaces.Length + 1];
Array.Copy(currentType.RuntimeInterfaces, consideredInterfaces, currentType.RuntimeInterfaces.Length);
consideredInterfaces[consideredInterfaces.Length - 1] = (DefType)currentType.InstantiateAsOpen();
consideredInterfaces[consideredInterfaces.Length - 1] = currentType.IsGenericDefinition ? (DefType)currentType.InstantiateAsOpen() : currentType;
}

foreach (MetadataType runtimeInterface in consideredInterfaces)
Expand Down Expand Up @@ -921,6 +923,11 @@ public static IEnumerable<MethodDesc> EnumAllVirtualSlots(MetadataType type)
/// <returns>MethodDesc of the resolved virtual static method, null when not found (runtime lookup must be used)</returns>
public static MethodDesc ResolveInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType)
{
// This would be a default interface method resolution. The algorithm below would sort of work, but doesn't handle
// things like diamond cases and it's better not to let it resolve as such.
if (currentType.IsInterface)
return null;

// Search for match on a per-level in the type hierarchy
for (MetadataType typeToCheck = currentType; typeToCheck != null; typeToCheck = typeToCheck.MetadataBaseType)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -372,16 +372,8 @@ public sealed override IEnumerable<CombinedDependencyListEntry> GetConditionalSt

DefType defType = _type.GetClosestDefType();

// Interfaces don't have vtables and we don't need to track their slot use.
// The only exception are those interfaces that provide IDynamicInterfaceCastable implementations;
// those have slots and we dispatch on them.
bool needsDependenciesForVirtualMethodImpls = !defType.IsInterface
|| ((MetadataType)defType).IsDynamicInterfaceCastableImplementation();

// If we're producing a full vtable, none of the dependencies are conditional.
needsDependenciesForVirtualMethodImpls &= !factory.VTable(defType).HasFixedSlots;

if (needsDependenciesForVirtualMethodImpls)
if (!factory.VTable(defType).HasFixedSlots)
{
bool isNonInterfaceAbstractType = !defType.IsInterface && ((MetadataType)defType).IsAbstract;

Expand Down Expand Up @@ -436,6 +428,12 @@ public sealed override IEnumerable<CombinedDependencyListEntry> GetConditionalSt
((System.Collections.IStructuralEquatable)defType.RuntimeInterfaces).Equals(_type.RuntimeInterfaces,
EqualityComparer<DefType>.Default));

// Interfaces don't have vtables and we don't need to track their instance method slot use.
// The only exception are those interfaces that provide IDynamicInterfaceCastable implementations;
// those have slots and we dispatch on them.
bool needsDependenciesForInstanceInterfaceMethodImpls = !defType.IsInterface
|| ((MetadataType)defType).IsDynamicInterfaceCastableImplementation();

// Add conditional dependencies for interface methods the type implements. For example, if the type T implements
// interface IFoo which has a method M1, add a dependency on T.M1 dependent on IFoo.M1 being called, since it's
// possible for any IFoo object to actually be an instance of T.
Expand All @@ -456,6 +454,9 @@ public sealed override IEnumerable<CombinedDependencyListEntry> GetConditionalSt

bool isStaticInterfaceMethod = interfaceMethod.Signature.IsStatic;

if (!isStaticInterfaceMethod && !needsDependenciesForInstanceInterfaceMethodImpls)
continue;

MethodDesc implMethod = isStaticInterfaceMethod ?
defType.ResolveInterfaceMethodToStaticVirtualMethodOnType(interfaceMethod) :
defType.ResolveInterfaceMethodToVirtualMethodOnType(interfaceMethod);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public static bool MightHaveInterfaceDispatchMap(TypeDesc type, NodeFactory fact
if (!type.IsArray && !type.IsDefType)
return false;

// Interfaces don't have a dispatch map because we dispatch them based on the
// Interfaces don't have a dispatch map for instance methods because we dispatch them based on the
// dispatch map of the implementing class.
// The only exception are IDynamicInterfaceCastable scenarios that dispatch
// using the interface dispatch map.
Expand All @@ -83,8 +83,9 @@ public static bool MightHaveInterfaceDispatchMap(TypeDesc type, NodeFactory fact
// wasn't marked as [DynamicInterfaceCastableImplementation]" and "we couldn't find an
// implementation". We don't want to use the custom attribute for that at runtime because
// that's reflection and this should work without reflection.
if (type.IsInterface)
return ((MetadataType)type).IsDynamicInterfaceCastableImplementation();
bool isInterface = type.IsInterface;
if (isInterface && ((MetadataType)type).IsDynamicInterfaceCastableImplementation())
return true;

DefType declType = type.GetClosestDefType();

Expand Down Expand Up @@ -112,6 +113,11 @@ public static bool MightHaveInterfaceDispatchMap(TypeDesc type, NodeFactory fact

Debug.Assert(declMethod.IsVirtual);

// Only static methods get placed in dispatch maps of interface types (modulo
// IDynamicInterfaceCastable we already handled above).
if (isInterface && !declMethod.Signature.IsStatic)
continue;

if (interfaceOnDefinitionType != null)
declMethod = factory.TypeSystemContext.GetMethodForInstantiatedType(declMethod.GetTypicalMethodDefinition(), interfaceOnDefinitionType);

Expand Down Expand Up @@ -154,6 +160,10 @@ private void EmitDispatchMap(ref ObjectDataBuilder builder, NodeFactory factory)
var staticImplementations = new List<(int InterfaceIndex, int InterfaceMethodSlot, int ImplMethodSlot, int Context)>();
var staticDefaultImplementations = new List<(int InterfaceIndex, int InterfaceMethodSlot, int ImplMethodSlot, int Context)>();

bool isInterface = declType.IsInterface;
bool needsEntriesForInstanceInterfaceMethodImpls = !isInterface
|| ((MetadataType)declType).IsDynamicInterfaceCastableImplementation();

// Resolve all the interfaces, but only emit non-static and non-default implementations
for (int interfaceIndex = 0; interfaceIndex < declTypeRuntimeInterfaces.Length; interfaceIndex++)
{
Expand All @@ -166,6 +176,10 @@ private void EmitDispatchMap(ref ObjectDataBuilder builder, NodeFactory factory)
for (int interfaceMethodSlot = 0; interfaceMethodSlot < virtualSlots.Count; interfaceMethodSlot++)
{
MethodDesc declMethod = virtualSlots[interfaceMethodSlot];

if (!declMethod.Signature.IsStatic && !needsEntriesForInstanceInterfaceMethodImpls)
continue;

if(!interfaceType.IsTypeDefinition)
declMethod = factory.TypeSystemContext.GetMethodForInstantiatedType(declMethod.GetTypicalMethodDefinition(), (InstantiatedType)interfaceDefinitionType);

Expand Down Expand Up @@ -244,9 +258,17 @@ private void EmitDispatchMap(ref ObjectDataBuilder builder, NodeFactory factory)
// For default interface methods, the generic context is acquired by indexing
// into the interface list of the owning type.
Debug.Assert(providingInterfaceDefinitionType != null);
int indexOfInterface = Array.IndexOf(declTypeDefinitionRuntimeInterfaces, providingInterfaceDefinitionType);
Debug.Assert(indexOfInterface >= 0);
genericContext = StaticVirtualMethodContextSource.ContextFromFirstInterface + indexOfInterface;
if (declTypeDefinition.HasSameTypeDefinition(providingInterfaceDefinitionType) &&
providingInterfaceDefinitionType == declTypeDefinition.InstantiateAsOpen())
{
genericContext = StaticVirtualMethodContextSource.ContextFromThisClass;
}
else
{
int indexOfInterface = Array.IndexOf(declTypeDefinitionRuntimeInterfaces, providingInterfaceDefinitionType);
Debug.Assert(indexOfInterface >= 0);
genericContext = StaticVirtualMethodContextSource.ContextFromFirstInterface + indexOfInterface;
}
}
staticDefaultImplementations.Add((
interfaceIndex,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,17 +108,21 @@ public bool BuildSealedVTableSlots(NodeFactory factory, bool relocsOnly)

_sealedVTableEntries = new List<SealedVTableEntry>();

// Interfaces don't have any virtual slots with the exception of interfaces that provide
// Interfaces don't have any instance virtual slots with the exception of interfaces that provide
// IDynamicInterfaceCastable implementation.
// Normal interface don't need one because the dispatch is done at the class level.
// For IDynamicInterfaceCastable, we don't have an implementing class.
if (_type.IsInterface && !((MetadataType)_type).IsDynamicInterfaceCastableImplementation())
return true;
bool isInterface = declType.IsInterface;
bool needsEntriesForInstanceInterfaceMethodImpls = !isInterface
|| ((MetadataType)declType).IsDynamicInterfaceCastableImplementation();

IReadOnlyList<MethodDesc> virtualSlots = factory.VTable(declType).Slots;

for (int i = 0; i < virtualSlots.Count; i++)
{
if (!virtualSlots[i].Signature.IsStatic && !needsEntriesForInstanceInterfaceMethodImpls)
continue;

MethodDesc implMethod = declType.FindVirtualFunctionTargetMethodOnObjectType(virtualSlots[i]);

if (implMethod.CanMethodBeInSealedVTable())
Expand All @@ -143,6 +147,10 @@ public bool BuildSealedVTableSlots(NodeFactory factory, bool relocsOnly)
for (int interfaceMethodSlot = 0; interfaceMethodSlot < virtualSlots.Count; interfaceMethodSlot++)
{
MethodDesc declMethod = virtualSlots[interfaceMethodSlot];

if (!declMethod.Signature.IsStatic && !needsEntriesForInstanceInterfaceMethodImpls)
continue;

if (!interfaceType.IsTypeDefinition)
declMethod = factory.TypeSystemContext.GetMethodForInstantiatedType(declMethod.GetTypicalMethodDefinition(), (InstantiatedType)interfaceDefinitionType);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,8 @@ private static int GetNumberOfSlotsInCurrentType(NodeFactory factory, TypeDesc i
{
if (implType.IsInterface)
{
// We normally don't need to ask about vtable slots of interfaces. It's not wrong to ask
// that question, but we currently only ask it for IDynamicInterfaceCastable implementations.
Debug.Assert(((MetadataType)implType).IsDynamicInterfaceCastableImplementation());
// Interface types don't have physically assigned virtual slots, so the number of slots
// is always 0. They may have sealed slots.
return (implType.HasGenericDictionarySlot() && countDictionarySlots) ? 1 : 0;
}

Expand Down
73 changes: 73 additions & 0 deletions src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ public static int Run()
TestMoreConstraints.Run();
TestSimpleNonGeneric.Run();
TestSimpleGeneric.Run();
TestDefaultDynamicStaticNonGeneric.Run();
TestDefaultDynamicStaticGeneric.Run();
TestDynamicStaticGenericVirtualMethods.Run();

return Pass;
Expand Down Expand Up @@ -1502,6 +1504,77 @@ public static void Run()
}
}

class TestDefaultDynamicStaticNonGeneric
{
interface IFoo
{
abstract static string ImHungryGiveMeCookie();
}

interface IBar : IFoo
{
static string IFoo.ImHungryGiveMeCookie() => "IBar";
}

class Baz : IBar
{
}

class Gen<T> where T : IFoo
{
public static string GrabCookie() => T.ImHungryGiveMeCookie();
}

public static void Run()
{
var r = (string)typeof(Gen<>).MakeGenericType(typeof(Baz)).GetMethod("GrabCookie").Invoke(null, Array.Empty<object>());
if (r != "IBar")
throw new Exception(r);

r = (string)typeof(Gen<>).MakeGenericType(typeof(IBar)).GetMethod("GrabCookie").Invoke(null, Array.Empty<object>());
if (r != "IBar")
throw new Exception(r);
}
}

class TestDefaultDynamicStaticGeneric
{
class Atom1 { }
class Atom2 { }

interface IFoo
{
abstract static string ImHungryGiveMeCookie();
}

interface IBar<T> : IFoo
{
static string IFoo.ImHungryGiveMeCookie() => $"IBar<{typeof(T).Name}>";
}

class Baz<T> : IBar<T>
{
}

class Gen<T> where T : IFoo
{
public static string GrabCookie() => T.ImHungryGiveMeCookie();
}

public static void Run()
{
Activator.CreateInstance(typeof(Baz<>).MakeGenericType(typeof(Atom1)));

var r = (string)typeof(Gen<>).MakeGenericType(typeof(Baz<>).MakeGenericType(typeof(Atom1))).GetMethod("GrabCookie").Invoke(null, Array.Empty<object>());
if (r != "IBar<Atom1>")
throw new Exception(r);

r = (string)typeof(Gen<>).MakeGenericType(typeof(IBar<>).MakeGenericType(typeof(Atom2))).GetMethod("GrabCookie").Invoke(null, Array.Empty<object>());
if (r != "IBar<Atom2>")
throw new Exception(r);
}
}

class TestDynamicStaticGenericVirtualMethods
{
interface IEntry
Expand Down

0 comments on commit 2134e62

Please sign in to comment.