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.

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

Jump to bottom

Covariant Returns Feature #35308

Merged
merged 21 commits into from
Jun 1, 2020
Merged

Covariant Returns Feature #35308

Show file tree
Hide file tree
Changes from 1 commit
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 24 additions & 4 deletions src/coreclr/src/vm/class.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,29 @@ void ClassLoader::LoadExactParents(MethodTable *pMT)
RETURN;
}

// Checks if two types are compatible according to compatible-with as described in ECMA 335 I.8.7.1
// Most of the checks are performed by the CanCastTo, but with some cases pre-filtered out.
//
/*static*/
bool ClassLoader::IsCompatibleWith(TypeHandle hType1, TypeHandle hType2)
{
// Structs can be casted to the interfaces they implement, but they are not compatible according to ECMA I.8.7.1
bool isCastFromValueTypeToReferenceType = hType2.IsValueType() && !hType1.IsValueType();
if (isCastFromValueTypeToReferenceType)
{
return false;
}

// Nullable<T> can be casted to T, but this is not compatible according to ECMA I.8.7.1
bool isCastFromNullableOfTtoT = hType1.GetMethodTable()->IsNullable() && hType2.IsEquivalentTo(hType1.GetMethodTable()->GetInstantiation()[0]);
if (isCastFromNullableOfTtoT)
{
return false;
}

return hType2.GetMethodTable()->CanCastTo(hType1.GetMethodTable(), NULL);
}

/*static*/
void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT)
{
Expand Down Expand Up @@ -1067,10 +1090,7 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT)
{
GCX_COOP();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be better to move GCX_COOP inside the IsCompatibleWith helper when needed (not all checks need this)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Fadi, it makes sense. I've made that change. Btw, I am still using your branch for finishing this work, so I'd like to ask you to keep it until it is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Jan, yes I’ll keep the branch around


// Structs can be casted to the interfaces they implement, but they are not compatible according to ECMA I.8.7.1
bool isCastFromValueTypeToReferenceType = hType2.IsValueType() && !hType1.IsValueType();

if (isCastFromValueTypeToReferenceType || !hType2.GetMethodTable()->CanCastTo(hType1.GetMethodTable(), NULL))
if (!IsCompatibleWith(hType1, hType2))
{
SString strAssemblyName;
pMD->GetAssembly()->GetDisplayName(strAssemblyName);
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/vm/clsload.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,8 @@ class ClassLoader

static void ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT);

static bool IsCompatibleWith(TypeHandle hType1, TypeHandle hType2);

// 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