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
25 changes: 25 additions & 0 deletions src/coreclr/src/tools/Common/TypeSystem/Common/CastingHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,31 @@ public static bool CanCastTo(this TypeDesc thisType, TypeDesc otherType)
return thisType.CanCastToInternal(otherType, null);
}

public static bool IsCompatibleWith(this TypeDesc thisType, TypeDesc otherType)
{
// Structs can be cast to the interfaces they implement, but they are not compatible according to ECMA I.8.7.1
bool isCastFromValueTypeToReferenceType = otherType.IsValueType && !thisType.IsValueType;
if (isCastFromValueTypeToReferenceType)
{
return false;
}

// Nullable<T> can be cast to T, but this is not compatible according to ECMA I.8.7.1
bool isCastFromNullableOfTtoT = thisType.IsNullable && otherType.IsEquivalentTo(thisType.Instantiation[0]);
if (isCastFromNullableOfTtoT)
{
return false;
}

return otherType.CanCastTo(thisType);
Copy link
Member

Choose a reason for hiding this comment

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

This is still going to throw if thisType or otherType is a pointer or function pointer. Do we need to handle that? (I had that feedback in #35308 (comment) but it probably got lost.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, let me add those to the test suite. It seems pointers cannot be compatible-with each other unless they are the same. I am actually not sure if the function pointers correspond to "method-signature compatible-with" case in the ECMA spec.

}

private static bool IsEquivalentTo(this TypeDesc thisType, TypeDesc otherType)
{
// TODO: Once type equivalence is implemented, this implementation needs to be enhanced to honor it.
Copy link
Member

Choose a reason for hiding this comment

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

Is type equivalence used for anything outside COM?

We don't intend to model COM in the managed type system so if this is exclusive to COM, the use of this can be replaced with == (we can safely use refence equality in the type system; no need to call Equals).

Copy link
Member

Choose a reason for hiding this comment

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

I think that it is used just by COM, @davidwrighton can you please confirm that?

return thisType.Equals(otherType);
}

private static bool CanCastToInternal(this TypeDesc thisType, TypeDesc otherType, StackOverflowProtect protect)
{
if (thisType == otherType)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/tools/Common/TypeSystem/Common/MethodDesc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ private bool Equals(MethodSignature otherSignature, bool allowCovariantReturn)
if (!allowCovariantReturn)
return false;

if (!this._returnType.CanCastTo(otherSignature._returnType))
if (!otherSignature._returnType.IsCompatibleWith(this._returnType))
return false;
}

Expand Down
27 changes: 27 additions & 0 deletions ...r/tests/src/Loader/classloader/MethodImpl/CovariantReturns/UnitTest/CompatibleWithTest.il
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,31 +13,38 @@

.method public hidebysig newslot virtual instance int32[] M1()
{
ldnull
ret
}

.method public hidebysig newslot virtual instance class [System.Runtime]System.Collections.Generic.IList`1<int32> M2()
{
ldnull
ret
}

.method public hidebysig newslot virtual instance int32 M3()
{
ldc.i4.0
ret
}

.method public hidebysig newslot virtual instance valuetype [mscorlib]System.Nullable`1<int32> M4()
{
ldc.i4.0
newobj instance void valuetype [mscorlib]System.Nullable`1<int32>::.ctor(!0)
ret
}

.method public hidebysig newslot virtual instance class [System.Runtime]System.Collections.Generic.IList`1<class Base> M5()
{
ldnull
ret
}

.method public hidebysig newslot virtual instance native int[] M6()
{
ldnull
ret
}
}
Expand All @@ -49,6 +56,7 @@
.method public hidebysig newslot virtual instance class [System.Runtime]System.Collections.Generic.IList`1<int32> M1()
{
.override method instance int32[] C1::M1();
ldnull
ret
}
}
Expand All @@ -60,6 +68,7 @@
.method public hidebysig newslot virtual instance int32[] M2()
{
.override method instance class [System.Runtime]System.Collections.Generic.IList`1<int32> C1::M2();
ldnull
ret
}
}
Expand All @@ -71,6 +80,8 @@
.method public hidebysig virtual instance valuetype [mscorlib]System.Nullable`1<int32> M3()
{
.override method instance int32 C1::M3();
ldc.i4.0
newobj instance void valuetype [mscorlib]System.Nullable`1<int32>::.ctor(!0)
ret
}
}
Expand All @@ -82,6 +93,7 @@
.method public hidebysig virtual instance int32 M4()
{
.override method instance valuetype [mscorlib]System.Nullable`1<int32> C1::M4();
ldc.i4.0
ret
}
}
Expand All @@ -93,6 +105,7 @@
.method public hidebysig virtual instance uint32[] M1()
{
.override method instance int32[] C1::M1();
ldnull
ret
}
}
Expand All @@ -104,6 +117,7 @@
.method public hidebysig newslot virtual instance uint32[] M2()
{
.override method instance class [System.Runtime]System.Collections.Generic.IList`1<int32> C1::M2();
ldnull
ret
}
}
Expand All @@ -115,6 +129,7 @@
.method public hidebysig newslot virtual instance class Derived[] M5()
{
.override method instance class [System.Runtime]System.Collections.Generic.IList`1<class Base> C1::M5();
ldnull
ret
}
}
Expand All @@ -126,6 +141,7 @@
.method public hidebysig virtual instance uint64[] M6()
{
.override method instance native int[] C1::M6();
ldnull
ret
}
}
Expand All @@ -137,6 +153,7 @@
.method public hidebysig virtual instance uint32[] M6()
{
.override method instance native int[] C1::M6();
ldnull
ret
}
}
Expand All @@ -146,6 +163,7 @@
.method public static void RunTestC1() noinlining
{
newobj instance void class C1::.ctor()
callvirt instance int32[] class C1::M1()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand All @@ -155,6 +173,7 @@
.method public static void RunTestC2() noinlining
{
newobj instance void class C2::.ctor()
callvirt instance int32[] class C1::M1()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand All @@ -164,6 +183,7 @@
.method public static void RunTestC3() noinlining
{
newobj instance void class C3::.ctor()
callvirt instance class [System.Runtime]System.Collections.Generic.IList`1<int32> class C1::M2()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand All @@ -173,6 +193,7 @@
.method public static void RunTestC4() noinlining
{
newobj instance void class C4::.ctor()
callvirt instance int32 class C1::M3()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand All @@ -182,6 +203,7 @@
.method public static void RunTestC5() noinlining
{
newobj instance void class C5::.ctor()
callvirt instance valuetype [mscorlib]System.Nullable`1<int32> C1::M4()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand All @@ -191,6 +213,7 @@
.method public static void RunTestC6() noinlining
{
newobj instance void class C6::.ctor()
callvirt instance int32[] class C1::M1()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand All @@ -200,6 +223,7 @@
.method public static void RunTestC7() noinlining
{
newobj instance void class C7::.ctor()
callvirt instance class [System.Runtime]System.Collections.Generic.IList`1<int32> class C1::M2()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand All @@ -209,6 +233,7 @@
.method public static void RunTestC8() noinlining
{
newobj instance void class C8::.ctor()
callvirt instance class [System.Runtime]System.Collections.Generic.IList`1<class Base> class C1::M5()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand All @@ -218,6 +243,7 @@
.method public static void RunTestC9() noinlining
{
newobj instance void class C9::.ctor()
callvirt instance native int[] class C1::M6()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand All @@ -227,6 +253,7 @@
.method public static void RunTestC10() noinlining
{
newobj instance void class C10::.ctor()
callvirt instance native int[] class C1::M6()
pop
ldstr "Succeeded"
call void [System.Console]System.Console::WriteLine(string)
Expand Down