Skip to content
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

Covariant Returns Feature #35308

Merged
merged 21 commits into from
Jun 1, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
bf7501f
Covariant Returns Feature: Allowing return types on MethodImpl to be …
Mar 23, 2020
fef49b2
Adding the missing .ctor methods to the various types
Apr 22, 2020
f10119d
Perform covariant return signature matching only after an exact match…
Apr 22, 2020
8d5d87e
Add test coverage for implicit override with less derived return type
Apr 23, 2020
a14af6f
Change ValidateMethodImplRemainsInEffect attribute to apply to methods
Apr 23, 2020
0e4e472
Moving covariant return type checking to the final stage of type load…
Apr 29, 2020
698eafb
Rename attribute and reference it from System.Runtime instead of S.P.C
Apr 29, 2020
6848c40
Add test coverage for interface cases.
Apr 29, 2020
5a85da8
Handling for struct cases, and adding test coverage for it.
Apr 30, 2020
89bb5c3
Small test fixes
Apr 30, 2020
cbecf78
Fix consistency issue with the way the RequireMethodImplToRemainInEff…
Apr 30, 2020
1154b90
Support covariant returns and slot unifications in crossgen2
May 4, 2020
f97b9de
some CR feedback
May 5, 2020
1329fea
Add unit test coverage for delegates
May 7, 2020
997074f
Fix handling of covariant and contravariant generic interfaces
janvorli May 14, 2020
ed9fb4c
Added test cases to interfaces unit test
janvorli May 18, 2020
6eaaa1a
Fix Nullable handling and add more tests
janvorli May 20, 2020
688c263
Reflect Fadi's feedback - limit the GCX_COOP scope
janvorli May 22, 2020
3636d03
Reflect PR feedback
janvorli May 26, 2020
c05f075
Rename the attribute to PreserveBaseOverridesAttribute
janvorli Jun 1, 2020
7ccd3ed
Disable covariant returns tests on Mono for now
janvorli Jun 1, 2020
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
83 changes: 83 additions & 0 deletions docs/design/features/covariant-return-methods.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
# Covariant Return Methods

Covariant return methods is a runtime feature designed to support the [covariant return types](https://github.com/dotnet/csharplang/blob/master/proposals/covariant-returns.md) and [records](https://github.com/dotnet/csharplang/blob/master/proposals/records.md) C# language features posed for C# 9.0.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a chapter on this to https://github.com/dotnet/runtime/blob/master/docs/design/specs/Ecma-335-Augments.md ? Try to match the style and language used by ECMA-335 (e.g. in the ideal case - we would just copy&paste it over once we figure out how to make edits to ECMA-335).


This feature allows an overriding method to have a more derived reference type than the method it overrides. Covariant return methods can only be described through MethodImpl records, and as an initial implementation, will have the following limitations:
1. Covariant return methods will only be applicable to methods on reference types: the MethodDecl and MethodImpl records can only be on reference types. Methods on interfaces will not be supported.
2. Return types in covariant return methods can only be reference types: covariant interface return types are not supported.

Supporting interfaces comes with many complications (ex: interface equivalence, default interface methods, variance on generic interfaces, etc...), which is why the feature will initially only support classes.

MethodImpl checking will allow a return type to vary as long as the override is compatible with the return type of the method overriden (i.e. a derived type).

If a language wishes for the override to be semantically visible such that users of the more derived type may rely on the covariant return type it shall make the override a newslot method with appropriate visibility AND name to be used outside of the class.
Copy link
Contributor

Choose a reason for hiding this comment

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

This statement sounds like newslot is not required if the method is not semantically visible (BTW, what exactly does it mean to be semantically visible). Is this correct?


For virtual method slot MethodImpl overrides, each slot shall be checked for compatible signature on type load. (Implementation note: This behavior can be triggered only if the type has a covariant return type override in its hierarchy, so as to make this pay for play.)

A new `ValidateMethodImplRemainsInEffectAttribute` shall be added. The presence of this attribute is to require the type loader to ensure that the MethodImpl records specified on the method have not lost their slot unifying behavior due to other actions. In other words, when a MethodImpl on type A overrides some method using a derived return type in the signature, any type deriving from A will be allowed to have a MethodImpl record that overrides the same method as long as the return type used in the signature is the same or more derived than the return type used in the MethodImpl signature on type A. This is used to allow the C# language to require that overrides have the consistent behaviors expected. The expectation is that C# would place this attribute on covariant override methods in classes.
fadimounir marked this conversation as resolved.
Show resolved Hide resolved

## Implementation Notes

### Signature Checking

Signature checking for MethodImpl is done through the `MetaSig::CompareElementType` method, which is called from various places in the runtime when comparing method signatures. This method compares the signatures of two types, and will now take a boolean flag that would allow for derived type checking behavior. The boolean flag will be set to `TRUE` appropriately during comparison of the return type signatures between a MethodImpl and MethodDecl records.

The type signature checking algorithm will perform the following:
1. Traverse and compare the signatures for `type1` and `type2` recursively.
2. If the signatures mismatch at any given point, and the current element type for `type2` is `ELEMENT_TYPE_CLASS` or `ELEMENT_TYPE_GENERICINST`:
+ Check if base type comparison is allowed
+ Compute the parent type's signature and parent type's generic substitution of `type2`
+ Perform a recursive call to re-compare `type1` with the new parent type signature of `type2`.

Note: if `ELEMENT_TYPE_INTERNAL` is encountered in either of the type signatures, both types will be fully loaded and compared for compatibility.

### VTable Slot Validation

Validation will only be performed if the `ValidateMethodImplRemainsInEffectAttribute` exists on the type that needs validation. Validation is only performed on the type where the attribute is added, and does not propagate to its base or derived types (every type that needs to be validated needs to have this attribute).

Validation is performed at the very last step of `CLASS_LOAD_EXACTPARENTS`, after all base types have been loaded, and the vtable has been fully built.

The validation algorithm will perform the following:
1. For each slot in the vtable that also exists in the vtable of the base type:
+ Check if the `MethodDesc` on that slot has a different original slot value. If not, continue.
+ Let `hType1` be the `TypeHandle` of the return type of the method on base type.
+ Let `hType2` be the `TypeHandle` of the return type of the method on the current type being validated.
+ Verify that `hType2` is the same type as, or a derived type of `hType1`

### [Future] Interface Support

An interface method may be both non-final and have a MethodImpl that declares that it overrides another interface method. If it does, NO other interface method may .override it. Instead further overrides must override the method that it overrode. Also the overriding method may only override 1 method.

The default interface method resolution algorithm shall change from:

``` console
Given interface method M and type T.
Let MSearch = M
Let MFound = Most specific implementation within the interfaces for MSearch within type T. If multiple implementations are found, throw Ambiguous match exception.
Return MFound
```

To:

``` console
Given interface method M and type T.
Let MSearch = M

If (MSearch overrides another method MBase)
Let MSearch = MBase

Let MFound = Most specific implementation within the interfaces for MSearch within type T. If multiple implementations are found, throw Ambiguous match exception.
Let M Code = NULL

If ((MFound != Msearch) and (MFound is not final))
Let M ClassVirtual = ResolveInterfaceMethod for MFound to virtual override on class T without using Default interface method implementation or return NULL if not found.
If (M ClassVirtual != NULL)
Let M Code= ResolveVirtualMethod for MFound on class T to implementation method

If (M Code != NULL)
Let M Code = MFound

Check M Code For signature <compatible-with> interface method M.

Return M Code
```
1 change: 1 addition & 0 deletions src/coreclr/src/dlls/mscorrc/mscorrc.rc
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,7 @@ BEGIN
IDS_CLASSLOAD_MI_BODY_DECL_MISMATCH "Signature of the body and declaration in a method implementation do not match. Type: '%1'. Assembly: '%2'."
IDS_CLASSLOAD_MI_MISSING_SIG_BODY "Signature for the body in a method implementation cannot be found. Type: '%1'. Assembly: '%2'."
IDS_CLASSLOAD_MI_MISSING_SIG_DECL "Signature for the declaration in a method implementation cannot be found. Type: '%1'. Assembly: '%2'."
IDS_CLASSLOAD_MI_BADRETURNTYPE "Return type in method '%1' on type '%2' from assembly '%3' is not compatible with base type method '%4'."

IDS_CLASSLOAD_EQUIVALENTSTRUCTMETHODS "Could not load the structure '%1' from assembly '%2'. The structure is marked as eligible for type equivalence, but it has a method."
IDS_CLASSLOAD_EQUIVALENTSTRUCTFIELDS "Could not load the structure '%1' from assembly '%2'. The structure is marked as eligible for type equivalence, but it has a static or non-public field."
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/dlls/mscorrc/resource.h
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@
#define IDS_CLASSLOAD_MI_BODY_DECL_MISMATCH 0x17a5
#define IDS_CLASSLOAD_MI_MISSING_SIG_BODY 0x17a6
#define IDS_CLASSLOAD_MI_MISSING_SIG_DECL 0x17a7
#define IDS_CLASSLOAD_MI_BADRETURNTYPE 0x17a8

#define IDS_CLASSLOAD_TOOMANYGENERICARGS 0x17ab
#define IDS_ERROR 0x17b0
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/inc/sigbuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ class SigBuilder
}

void AppendBlob(const PVOID pBlob, SIZE_T cbBlob);

void AppendSignature(const PCCOR_SIGNATURE pSig, const PCCOR_SIGNATURE pSigEnd);
};

#endif // _SIGBUILDER_H_
16 changes: 16 additions & 0 deletions src/coreclr/src/utilcode/sigbuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,22 @@ void SigBuilder::AppendBlob(const PVOID pBlob, SIZE_T cbBlob)
m_dwLength += (DWORD)cbBlob;
}

void SigBuilder::AppendSignature(const PCCOR_SIGNATURE pSig, const PCCOR_SIGNATURE pSigEnd)
{
STANDARD_VM_CONTRACT;

// Overflow checks
if (pSigEnd < pSig)
ThrowOutOfMemory();

DWORD cbSig = (DWORD)(pSigEnd - pSig);

Ensure(cbSig);
memcpy(&m_pBuffer[m_dwLength], pSig, cbSig);

m_dwLength += cbSig;
}

void SigBuilder::Grow(SIZE_T cbMin)
{
STANDARD_VM_CONTRACT;
Expand Down
84 changes: 84 additions & 0 deletions src/coreclr/src/vm/class.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -967,12 +967,96 @@ void ClassLoader::LoadExactParents(MethodTable *pMT)

MethodTableBuilder::CopyExactParentSlots(pMT, pApproxParentMT);

ValidateMethodImplRemainsInEffect(pMT);

// We can now mark this type as having exact parents
pMT->SetHasExactParent();

RETURN;
}

/*static*/
void ClassLoader::ValidateMethodImplRemainsInEffect(MethodTable* pMT)
{
CONTRACT_VOID
{
STANDARD_VM_CHECK;
PRECONDITION(CheckPointer(pMT));
}
CONTRACT_END;

// Validation not applicable to interface types and value types, since these are not currently
// supported with the covariant return feature
if (pMT->IsInterface() || pMT->IsValueType())
RETURN;

MethodTable* pParentMT = pMT->GetParentMethodTable();
if (pParentMT == NULL)
RETURN;

// If the ValidateMethodImplRemainsInEffect attribute does not exist on the type, it will not be validated.
BYTE* pVal = NULL;
ULONG cbVal = 0;
HRESULT hr = pMT->GetCustomAttribute(WellKnownAttribute::ValidateMethodImplRemainsInEffectAttribute, (const void**)&pVal, &cbVal);
if (hr != S_OK)
RETURN;

for (WORD i = 0; i < pParentMT->GetNumVirtuals(); i++)
{
MethodDesc* pMD = pMT->GetMethodDescForSlot(i);
MethodDesc* pParentMD = pParentMT->GetMethodDescForSlot(i);

DWORD originalIndex = pMD->GetSlot();
if (originalIndex == i)
continue;

DWORD originalIndexParent = pParentMD->GetSlot();
if (originalIndex == originalIndexParent)
continue;

// If we reach this point, it means we have a MethodImpl override. With the covariant return feature, the presense of the
// ValidateMethodImplRemainsInEffect attribute is used to validate that if a method gets overridden, the return type
// in the new override is the same type or a more derived type as the return type of the method being overriden.

// The context used to load the return type of the parent method has to use the generic method arguments
// of the overriding method, otherwise the type comparison below will not work correctly
SigTypeContext context1(pParentMD->GetClassInstantiation(), pMD->GetMethodInstantiation());
MetaSig methodSig1(pParentMD);
TypeHandle hType1 = methodSig1.GetReturnProps().GetTypeHandleThrowing(pParentMD->GetModule(), &context1, ClassLoader::LoadTypesFlag::LoadTypes, CLASS_LOAD_EXACTPARENTS);

SigTypeContext context2(pMD);
MetaSig methodSig2(pMD);
TypeHandle hType2 = methodSig2.GetReturnProps().GetTypeHandleThrowing(pMD->GetModule(), &context2, ClassLoader::LoadTypesFlag::LoadTypes, CLASS_LOAD_EXACTPARENTS);

// Type1 has to be equal to Type2, or a base type of Type2 (covariant returns)

if (!MetaSig::CompareTypeHandles(hType1, hType2, TRUE /* allowDerivedClass */ ))
{
SString strAssemblyName;
pMD->GetAssembly()->GetDisplayName(strAssemblyName);

SString strInvalidTypeName;
TypeString::AppendType(strInvalidTypeName, TypeHandle(pMD->GetMethodTable()));

SString strInvalidMethodName;
TypeString::AppendMethod(strInvalidMethodName, pMD, pMD->GetMethodInstantiation());

SString strParentMethodName;
TypeString::AppendMethod(strParentMethodName, pParentMD, pParentMD->GetMethodInstantiation());

COMPlusThrow(
kTypeLoadException,
IDS_CLASSLOAD_MI_BADRETURNTYPE,
strInvalidMethodName,
strInvalidTypeName,
strAssemblyName,
strParentMethodName);
}
}

RETURN;
}

//*******************************************************************************
// This is the routine that computes the internal type of a given type. It normalizes
// structs that have only one field (of int/ptr sized values), to be that underlying type.
Expand Down
5 changes: 3 additions & 2 deletions src/coreclr/src/vm/classcompat.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1405,7 +1405,7 @@ VOID MethodTableBuilder::BuildInteropVTable_PlaceVtableMethods(
bmtType->pModule, NULL,
pInterfaceMethodSig,
cInterfaceMethodSig,
pInterfaceMD->GetModule(), NULL))
pInterfaceMD->GetModule(), NULL, FALSE))
{ // Found match, break from loop
break;
}
Expand Down Expand Up @@ -2263,7 +2263,8 @@ VOID MethodTableBuilder::EnumerateMethodImpls()
pSigBody,
cbSigBody,
bmtType->pModule,
NULL))
NULL,
FALSE))
{
BuildMethodTableThrowException(IDS_CLASSLOAD_MI_BODY_DECL_MISMATCH);
}
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/src/vm/clsload.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -969,10 +969,12 @@ class ClassLoader

// Phase CLASS_LOAD_EXACTPARENTS of class loading
// Load exact parents and interfaces and dependent structures (generics dictionary, vtable fixes)
static void LoadExactParents(MethodTable *pMT);
static void LoadExactParents(MethodTable* pMT);

static void LoadExactParentAndInterfacesTransitively(MethodTable *pMT);

static void ValidateMethodImplRemainsInEffect(MethodTable* pMT);

// Create a non-canonical instantiation of a generic type based off the canonical instantiation
// (For example, MethodTable for List<string> is based on the MethodTable for List<__Canon>)
static TypeHandle CreateTypeHandleForNonCanonicalGenericInstantiation(TypeKey *pTypeKey,
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/vm/ecall.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ static INT FindECIndexForMethod(MethodDesc *pMD, const LPVOID* impls)

//@GENERICS: none of these methods belong to generic classes so there is no instantiation info to pass in
if (!MetaSig::CompareMethodSigs(pMethodSig, cbMethodSigLen, pModule, NULL,
sig.GetRawSig(), sig.GetRawSigLen(), MscorlibBinder::GetModule(), NULL))
sig.GetRawSig(), sig.GetRawSigLen(), MscorlibBinder::GetModule(), NULL, FALSE))
{
continue;
}
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/src/vm/memberload.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ void MemberLoader::GetDescFromMemberRef(Module * pModule,

pMethodDef->GetSig(&pMethodSig, &cMethodSig);
if (!MetaSig::CompareMethodSigs(pSig, cSig, pModule, NULL, pMethodSig,
cMethodSig, pModule, NULL))
cMethodSig, pModule, NULL, FALSE))
{
// If the signatures do not match, then the correct MethodDesc has not been found.
fMissingMethod = TRUE;
Expand Down Expand Up @@ -1043,7 +1043,7 @@ BOOL CompareMethodSigWithCorrectSubstitution(

pCurDeclMD->GetSig(&pCurMethodSig, &cCurMethodSig);
return MetaSig::CompareMethodSigs(pSignature, cSignature, pModule, NULL, pCurMethodSig,
cCurMethodSig, pCurDeclMD->GetModule(), pDefSubst);
cCurMethodSig, pCurDeclMD->GetModule(), pDefSubst, FALSE);
}
else
{
Expand Down Expand Up @@ -1443,7 +1443,7 @@ MemberLoader::FindConstructor(MethodTable * pMT, PCCOR_SIGNATURE pSignature,DWOR
DWORD cCurMethodSig;
pCurMethod->GetSig(&pCurMethodSig, &cCurMethodSig);

if (MetaSig::CompareMethodSigs(pSignature, cSignature, pModule, NULL, pCurMethodSig, cCurMethodSig, pCurMethod->GetModule(), NULL))
if (MetaSig::CompareMethodSigs(pSignature, cSignature, pModule, NULL, pCurMethodSig, cCurMethodSig, pCurMethod->GetModule(), NULL, FALSE))
{
return pCurMethod;
}
Expand Down
Loading