Skip to content

Commit

Permalink
Add default method support to virtual statics (#64717)
Browse files Browse the repository at this point in the history
- The preview feature version of virtual statics implemented for .NET 6 does not allow for the interface methods to have a default implementation.
- With this change, we add support for the interface method having an actual implementation to CoreCLR. From what I can tell the Mono runtime already had such support
- There are some small ECMA spec updates to allow this change that are also included

In addition, I've taken the liberty to enable running the coreclr test suite on Mono on Windows. It needed a small amount of fixup.
  • Loading branch information
davidwrighton authored Feb 9, 2022
1 parent e48b179 commit 9e45de4
Show file tree
Hide file tree
Showing 21 changed files with 80,260 additions and 294 deletions.
5 changes: 2 additions & 3 deletions docs/design/specs/Ecma-335-Augments.md
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ https://www.ecma-international.org/publications-and-standards/standards/ecma-335

(Add second paragraph)

Static interface methods may be marked as virtual. Valid object types implementing such interfaces shall provide implementations
Static interface methods may be marked as virtual. Valid object types implementing such interfaces may provide implementations
for these methods by means of Method Implementations (II.15.1.4). Polymorphic behavior of calls to these methods is facilitated
by the constrained. call IL instruction where the constrained. prefix specifies the type to use for lookup of the static interface
method.
Expand Down Expand Up @@ -531,7 +531,6 @@ or static method actually implemented directly on the type.
(Add to the end of the 1st paragraph)

Interfaces may define static virtual methods that get resolved at runtime based on actual types involved.
These static virtual methods must be marked as abstract in the defining interfaces.

### II.12.2 Implementing virtual methods on interfaces

Expand Down Expand Up @@ -754,7 +753,7 @@ the call itself doesn't involve any instance or `this` pointer.
(Edit bulleted section "This contains informative text only" starting at the bottom of page
233):

Edit section *7.b*: Static | Virtual | !Abstract
Remove section *7.b*: ~~Static | Virtual~~

(Add new section 41 after the last section 40:)

Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/vm/genericdict.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1081,10 +1081,11 @@ Dictionary::PopulateEntry(
#if FEATURE_DEFAULT_INTERFACES
// If we resolved the constrained call on a value type into a method on a reference type, this is a
// default interface method implementation.
// If the method is a static method, this is ok, but for instance methods there are boxing issues.
// In such case we would need to box the value type before we can dispatch to the implementation.
// This would require us to make a "boxing stub". For now we leave the boxing stubs unimplemented.
// It's not clear if anyone would need them and the implementation complexity is not worth it at this time.
if (!pResolvedMD->GetMethodTable()->IsValueType() && constraintType.GetMethodTable()->IsValueType())
if (!pResolvedMD->IsStatic() && !pResolvedMD->GetMethodTable()->IsValueType() && constraintType.GetMethodTable()->IsValueType())
{
SString assemblyName;

Expand Down
9 changes: 7 additions & 2 deletions src/coreclr/vm/methodtable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7580,7 +7580,6 @@ MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc*
{
canonicalEquivalentFound = true;
break;
return NULL;
}
}
}
Expand Down Expand Up @@ -7644,6 +7643,12 @@ MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc*
}
}
}

// Default implementation logic, which only kicks in for default implementations when lookin up on an exact interface target
if (!pInterfaceMD->IsAbstract() && !(this == g_pCanonMethodTableClass) && !IsSharedByGenericInstantiations())
{
return pInterfaceMD->FindOrCreateAssociatedMethodDesc(pInterfaceMD, pInterfaceType, FALSE, pInterfaceMD->GetMethodInstantiation(), FALSE);
}
}

if (allowNullResult)
Expand Down Expand Up @@ -7818,7 +7823,7 @@ MethodTable::VerifyThatAllVirtualStaticMethodsAreImplemented()
MethodDesc *pMD = it.GetMethodDesc();
if (pMD->IsVirtual() &&
pMD->IsStatic() &&
!ResolveVirtualStaticMethod(pInterfaceMT, pMD, /* allowNullResult */ TRUE, /* verifyImplemented */ TRUE, /* allowVariantMatches */ FALSE))
(pMD->IsAbstract() && !ResolveVirtualStaticMethod(pInterfaceMT, pMD, /* allowNullResult */ TRUE, /* verifyImplemented */ TRUE, /* allowVariantMatches */ FALSE)))
{
IMDInternalImport* pInternalImport = GetModule()->GetMDImport();
GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, GetCl(), pMD->GetName(), IDS_CLASSLOAD_STATICVIRTUAL_NOTIMPL);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/methodtablebuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5076,7 +5076,7 @@ MethodTableBuilder::ValidateMethods()
// Virtual static methods are only allowed on interfaces and they must be abstract.
if (IsMdStatic(it.Attrs()) && IsMdVirtual(it.Attrs()))
{
if (!IsInterface() || !IsMdAbstract(it.Attrs()))
if (!IsInterface())
{
BuildMethodTableThrowException(IDS_CLASSLOAD_STATICVIRTUAL, it.Token());
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/typedesc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1613,7 +1613,7 @@ BOOL TypeVarTypeDesc::SatisfiesConstraints(SigTypeContext *pTypeContextOfConstra
MethodDesc *pMD = it.GetMethodDesc();
if (pMD->IsVirtual() &&
pMD->IsStatic() &&
!thElem.AsMethodTable()->ResolveVirtualStaticMethod(pInterfaceMT, pMD, /* allowNullResult */ TRUE, /* verifyImplemented */ TRUE))
(pMD->IsAbstract() && !thElem.AsMethodTable()->ResolveVirtualStaticMethod(pInterfaceMT, pMD, /* allowNullResult */ TRUE, /* verifyImplemented */ TRUE)))
{
virtualStaticResolutionCheckFailed = true;
break;
Expand Down
3 changes: 2 additions & 1 deletion src/tests/Common/Directory.Build.targets
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@
</ItemGroup>

<ItemGroup Condition="'$(RuntimeFlavor)' == 'mono' and '$(IsDesktopOS)' == 'true' " >
<RuntimeDependencyCopyLocal Include="$(MonoArtifactsPath)/libcoreclr$(LibSuffix)" TargetDir="" />
<RuntimeDependencyCopyLocal Condition="'$(TargetOS)' != 'windows'" Include="$(MonoArtifactsPath)/libcoreclr$(LibSuffix)" TargetDir="" />
<RuntimeDependencyCopyLocal Condition="'$(TargetOS)' == 'windows'" Include="$(MonoArtifactsPath)coreclr$(LibSuffix)" TargetDir="" />
<RuntimeDependencyCopyLocal Include="$(MonoArtifactsPath)/libmono-component-*" TargetDir="" />
<RuntimeDependencyCopyLocal Include="$(MonoArtifactsPath)/*.dll" TargetDir="/" />
</ItemGroup>
Expand Down
Loading

0 comments on commit 9e45de4

Please sign in to comment.