Covariant Returns Feature #35308

fadimounir · 2020-04-22T20:25:17Z

Allowing return types on MethodImpl to be derived types of the return type on the MethodDecl.

Feature limitations:

Only supports MethodImpls and MethodDecls on classes (no interfaces/valuetypes).
MethodImpl and MethodDecl cannot be on the same type.

MethodImpls where the signature matches for all parameters except for the return type are conditionally treated as valid during enumerating/processing, but flagged for further return type checking at a later stage of type load.

For every type being loaded LOAD_EXACTPARENTS stage, check every flagged overriding method for return type compatibility with the overriden method (using CanCastTo()), and process the MethodImpls with the RequireMethodImplToRemainInEffect attribute.

Inludes unit tests to cover positive/negative scenarios:

Non-generics
Generics, with various levels of complexities on generic instantiations and substitutions
GVMs
Tests for validation of the existence/absence of the RequireMethodImplToRemainInEffect attribute

TODOs:

Rename attribute to RequireMethodImplToRemainInEffectAttribute and add it to System.Runtime reference assembly (and update tests to reference it from there instead of S.P.C)
Crossgen2 work to support feature (some unit tests are failing today with cg2)
Add unit tests to cover interface -> interface compatibility
Add unit tests to cover class -> interface compatibility
Add unit tests to cover delegate invokes using base type/derived type signatures
Add coverage for covariant/contravariant generic interfaces used as covariant return types (and verify it works with crossgen2 as well)
Add unit test to ensure struct -> interface casts are treated as incompatible
Visit the uncommon castable scenarios and determine if they should be supported, and add test coverage (pointers, byrefs, arrays->IList/ICollection..., int[]->uint[], etc...)
Add a chapter on this to https://github.com/dotnet/runtime/blob/master/docs/design/specs/Ecma-335-Augments.md

cc @dotnet/crossgen-contrib

…derived types of the return type on the MethodDecl. Feature limitations: - Only supports MethodImpls and MethodDecls on classes (no interfaces/valuetypes). - MethodImpl and MethodDecl cannot be on the same type. - Interface/valuetypes not supported as covariant return types. Changes are mostly a boolean flag being passed around to allow for covariant type checking in method signatures. Generics are handled by correctly keeping track of the substitution chain while traversing the base type hierarchy for type comparison. All method signature comparisons uses metadata for checking, without loading any type (with some exceptions). Validation for the `ValidateMethodImplRemainsInEffectAttribute` is performed at the very last step of `CLASS_LOAD_EXACTPARENTS`. Inludes unit tests to cover positive/negative scenarios: - Non-generics - Generics, with various levels of complexities on generic instantiations and substitutions - GVMs - Delegates - Tests for validation of the existance/absence of the ValidateMethodImplRemainsInEffectAttribute - Interface and valuetype scenarios (negative scenarios)

fadimounir · 2020-04-22T20:28:51Z

Posting this to start getting feedback while working on the Crossgen2 changes.

@davidwrighton @jkotas Who can I talk to regarding the Mono side of the work?

...te.CoreLib/src/System/Runtime/CompilerServices/ValidateMethodImplRemainsInEffectAttribute.cs

...tests/src/Loader/classloader/MethodImpl/CovariantReturns/Validation/OverrideSameSigAsDecl.il

jkotas · 2020-04-22T21:01:24Z

docs/design/features/covariant-return-methods.md

@@ -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.


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).

src/coreclr/src/vm/siginfo.cpp

src/coreclr/src/vm/methodtablebuilder.cpp

docs/design/features/covariant-return-methods.md

src/coreclr/src/vm/siginfo.cpp

jkotas · 2020-04-22T21:21:01Z

Who can I talk to regarding the Mono side of the work?

@marek-safar ?

We should have the Mono side figured out before starting the work next time.

… fails Fix a couple of issues in IL tests

marek-safar · 2020-04-23T06:57:19Z

We should have the Mono side figured out before starting the work next time.

I agree such change has to be coordinated from the beginning as we now need to re-prioritize other work otherwise we ship with incompatible runtimes for net5. Please loop @lambdageek and @vargaz on type-system design decisions from the beginning.

fadimounir · 2020-04-23T21:52:51Z

The validation around the ValidateMethodImplRemainsInEffectAttribute is a bit incorrect here. My understanding was a bit off. I'm reworking this part now and updating the unit tests to verify the correct behavior

…ing, and use CanCastTo instead of signature-based checking, to be allow for type compatibility based on ECMA I.8.7.1. Fixed behavior of the ValidateMethodImplRemainsInEffectAttribute. Updates to tests

Fixed an assert

docs/design/features/covariant-return-methods.md

davidwrighton · 2020-05-15T20:29:19Z

@janvorli,
It looks right to pass NULL to CanCastTo as the last argument.
For the load level assert change, you need to test a case where the generic arg load level could be a problem.

For instance something like the following

class Base<T> {}
class Derived : Base<Derived> {}
class Derived2 : Base<Derived2> {}
interface IVariant<out V>
class BaseClassWithVirtualMethod
{
    public virtual IVariant<Base<Derived>> Func() {...}
}

class DerivedClassPass1
{
    public override IVariant<Derived> Func() {} // This should work
}

class DerivedClassFail1
{
    public override IVariant<Derived2> Func() {} // This should fail
}

Based on PR feedback, I've added test cases to verify the load level asserts change.

janvorli · 2020-05-18T21:40:34Z

@davidwrighton I've added a commit with a test based on your suggestion and tested it with debug build of runtime. It works as expected.

davidwrighton · 2020-05-19T00:57:09Z

The generic variance tests look good. Looking through the tests I think we need to have Nullable tests as well to ensure that CanCastTo isn't causing a problem. If you want to be particular, testing to see if int? and int are compatible-with. They should not be for this calculation.

The attempt to overload method with int32 return value by a method with Nullable<int32> was asserting in CanCastTo. This change fixes it by pre-checking this case before calling into CanCastTo. I've also slightly refactored the ValidateMethodsWithCovariantReturnTypes by extracting the compatible-with stuff into IsCompatibleWith method. Added tests to test various compatible-with differences from the originally used CanCastTo

janvorli · 2020-05-20T21:24:53Z

@davidwrighton I've tested and fixed the nullable case and refactored the code a tiny bit by introducing IsCompatibleWith method that wraps around the CanCastTo and does some of the pre-filtering as you've suggested.
I've added another set of tests for testing various interesting cases from the compatible-with list, like overriding return value of IList by uint32[] etc.

davidwrighton · 2020-05-20T22:54:17Z

Have we gotten api signoff on RequireMethodImplToRemainInEffectAttribute yet? Otherwise I think this is implemented and the logic looks good.

janvorli · 2020-05-20T23:06:25Z

Have we gotten api signoff on RequireMethodImplToRemainInEffectAttribute yet

Doesn't seem so, #35315.

fadimounir · 2020-05-21T15:34:37Z

src/coreclr/src/vm/class.cpp

@@ -1067,10 +1090,7 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT)
        {
            GCX_COOP();


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

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.

Thanks Jan, yes I’ll keep the branch around

MichalStrehovsky · 2020-05-26T10:08:36Z

refactored the code a tiny bit by introducing IsCompatibleWith method that wraps around the CanCastTo and does some of the pre-filtering as you've suggested.

Do we need to port that to crossgen2 as well?

In general I don't believe the CompatibleWithTest.il test that tests the CanCastTo pre-filtering is going to work with crossgen2 and optimizations enabled - I expect RyuJIT to optimize out the newobj/pop sequence and since crossgen2 doesn't actually "load" types, it won't see there's a problem.

It might be better to do newobj/callvirt to ensure the virtual method resolution in crossgen2 is actually hit and we test what we want to test.

janvorli · 2020-05-26T10:32:23Z

In general I don't believe the CompatibleWithTest.il test that tests the CanCastTo pre-filtering is going to work with crossgen2 and optimizations enabled - I expect RyuJIT to optimize out the newobj/pop sequence and since crossgen2 doesn't actually "load" types, it won't see there's a problem.

I've tested it with crossgen2 and optimizations enabled and the test was passing (that means the parts that should succeed were succeeding and the parts that should fail were failing). But it makes sense to change the newobj/pop to newobj / callvirt.

Do we need to port that to crossgen2 as well?

Right, I've missed that

- Change the CompatibleWithTest to call virtual methods on test classes instead of just instantiating the classes. - Update crossgen2 similar to the runtime to have IsCompatibleWith method doing the same pre-filtering before calling CanCastTo.

janvorli · 2020-05-26T19:08:53Z

@MichalStrehovsky I've made changes based on your feedback.

MichalStrehovsky

Thanks!

MichalStrehovsky · 2020-05-27T12:19:25Z

src/coreclr/src/tools/Common/TypeSystem/Common/CastingHelper.cs

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


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).

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

MichalStrehovsky · 2020-05-27T12:21:07Z

src/coreclr/src/tools/Common/TypeSystem/Common/CastingHelper.cs

+                return false;
+            }
+
+            return otherType.CanCastTo(thisType);


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.

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.

The mono doesn't implement the feature yet

davidwrighton

This looks good now. I believe we may have some small amount of cleanup around error conditions about pointers and function pointers, but we should handle those as bugs.

MichalStrehovsky · 2020-06-01T18:25:15Z

Are we happy about the perf of virtual method resolution after this change? #35308 (comment)

It feels like this attribute should be cached the same way we cache e.g. UnmanagedCallersOnlyAttribute...

davidwrighton · 2020-06-01T18:29:07Z

While not great, and I agree we should probably cache it, the perf impact isn't that significant in crossgen2 due to the relatively low amount of virtual resolution it performs. We'll probably need to adjust that when we port this to the CoreRT codebase where virtual resolution performance is much more important. However, that is bug fix level work that doesn't need to block checkin.

janvorli · 2020-06-01T18:32:32Z

I am going to create an issue tracking all the additional changes that were mentioned here so that they don't get lost.

Dotnet-GitSync-Bot added the area-Infrastructure label Apr 22, 2020

fadimounir changed the title ~~Covariant Returns Feature~~ [WIP] Covariant Returns Feature Apr 22, 2020

fadimounir requested review from jkotas and davidwrighton April 22, 2020 20:26

fadimounir added area-TypeSystem-coreclr and removed area-Infrastructure labels Apr 22, 2020