Runtime: RequireMethodImplToRemainInEffectAttribute

[fadimounir]

This new attribute will be used by the covariant return types and records C# language features posed for C# 9.0.

Today when we have an explicit override to a method (using MethodImpls), this override gets a new vtable slot on the type, and the slot for the overriden method gets updated to point to the new overriding method. Example:
``` C#
// Pseudo code with some IL
class A {
void VirtualFunction() { }
}
class B : A {
void VirtualFunction() { .override A.VirtualFunction }
}

// VTable for type A:
// [0]: A.VirtualFunction

// VTable for type B:
// [0]: B.VirtualFunction
// [1]: B.VirtualFunction

Now when a new derived type explicitly overrides `A.VirtualFunction`, it would also get a new vtable slot, and only the vtable slot of the method being overriden gets updated with the new override. Example:
``` C#
class C : B {
    void VirtualFunction() { .override A.VirtualFunction }
}

// VTable for type A:
// [0]: A.VirtualFunction
//
// VTable for type B:
// [0]: B.VirtualFunction
// [1]: B.VirtualFunction
//
// VTable for type C:
// [0]: C.VirtualFunction
// [1]: B.VirtualFunction   -> This slot does not get overriden
// [2]: C.VirtualFunction

With the new records and covariant return feature, a change of behavior is required to vtable slot overrides when a method gets explictly overriden. In the example above, the second slot on type C would also need to be updated to point to the latest override of the method:
``` C#
// VTable for type C:
// [0]: C.VirtualFunction
// [1]: C.VirtualFunction -> This slot needs to be updated as well
// [2]: C.VirtualFunction

The reason for this change is to ensure that any virtual call to the method, whether it uses the base signature or derived signature of the method, we always execute the most derived override:
``` c#
// Given an object of type C:
callvirt A.VirtualFunction -> Executes C.VirtualFunction
callvirt B.VirtualFunction -> Executes C.VirtualFunction
callvirt C.VirtualFunction -> Executes C.VirtualFunction

To achieve this, a new attribute will be introduced and applied to the MethodImpls that need to propagate to all applicable vtable slots: RequireMethodImplToRemainInEffect attribute. Example:
``` C#
class A {
void VirtualFunction() { }
}
class B : A {
void VirtualFunction() { .override A.VirtualFunction }
}
class C : B {
[RequireMethodImplToRemainInEffect]
void VirtualFunction() { .override A.VirtualFunction }
}

This attribute will only be applicable to methods with explicit overrides (MethodImpls), and takes no arguments. The proposed namespace for this attribute is the `System.Runtime.CompilerServices` namespace. Here is the proposed implementation of the attribute:

``` C#
namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)]
    public sealed class RequireMethodImplToRemainInEffectAttribute : Attribute
    {
    }
}

[fadimounir]

Quick clarification as well: we do not expect any developer to directly use this attribute. It will be placed by the Roslyn compiler appropriately on methods.

[fadimounir]

cc @dotnet/crossgen-contrib

[davidwrighton]

@vargaz @lambdageek should be looped in on this issue.

[fadimounir]

cc @janvorli

[lambdageek]

We will need to teach the IL linker to keep this attribute /cc @vitek-karas

[MichalStrehovsky]

We will need to teach the IL linker to keep this attribute /cc @vitek-karas

IL Linker will not strip attributes it knows nothing about because it would be a bug farm. https://github.com/mono/linker/issues/952#issuecomment-603110388

[terrajobst]

Video

A few questions:

• We're not entirely sure what this attribute does:
  
  
  
  • The issue talks about covariant returns but all samples use void
  
  
  • None of the methdos use virtual functions
  
  
  • What are the behavior differences of with and without the attribute? If this can't be expressed in C#, then ILDASM code would help too
  
  
• RequireMethodImplToRemainInEffectAttribute this name seems a bit abstract. Can make this a bit clearer, like PreserveBaseOverridesAttribute?
• Does it have to be a new attribute or could be a new flag on MethodImplAttributes?
• How does this attribute play with default interface methods?
• How does this play with versioning where overrides are added to the middle of the hierarchy later?

[davidwrighton]

• This is a feature to adjust the behavior of MethodImpl records, but really this is designed to support the covariant scenario. The reason that the examples are written in terms of void is to emphasize that this is an independent feature at the runtime level from covariant returns, although it is likely that in the near term, it will only be used by the C# compiler when compiling covariant return overrides.
• The name was written as a placeholder as I am notoriously terrible at naming stuff. I find the PreserveBaseOverridesAttribute naming to be reasonable as well, and if that makes more sense to the api review board, I'm all for it.
• It does not have to be a new attribute. A new flag MethodImpl would also work if the question were merely about changes in the file format. However, we need to be slightly careful with that as it needs to be hidden from the C# developer, as the flag is NOT something that is acceptable to adjust by setting a flag on the attribute from the C# language. It would need to be set by the compiler itself. Also, using a MethodImpl flag would imply that we need to update ildasm, and ilasm, which otherwise do not require adjustment. Overall, a MethodImpl flag would be "fine".
• This only applies to MethodImpl records used to override a class virtual method with another class virtual method. It does not interact with default virtual methods.
• This method actually enhances that experience. It allows there to be considered a single slot that defines how the override works, and changes the override logic so that the covariance will be guaranteed to exist thus preventing a non-covariant override by a further derived type. For instance, consider the following. The new attribute is used to cause the runtime to produce a type load exception instead of allowing the TestClassDerived2 to override the original Method() on TestClassWithVirtualMethod.

class BaseType {}
class DerivedType : BaseType {}

class TestClassWithVirtualMethod
{
    public virtual BaseType Method() {}
}
class TestClassDerived1 : TestClassWithVirtualMethod
{
    public override DerivedType Method() {}
}
class TestClassDerived2 : TestClassDerived1
{
// ERROR!, TypeLoadException!!
    public override BaseType Method() {}
}

[jkotas]

MethodImpl flag

If we choose to encode this in metadata, it should be a bit in the MethodAttributes column where the existing bits that control virtual method resolution live (Virtual, NewSlot, Strict, etc.). MethodImplAttributes does not sound right for this.

[MichalStrehovsky]

If we choose to encode this in metadata, it should be a bit in the MethodAttributes column

We don't have bits in the MethodAttribute column anymore, unless we want to do creative combinations (mdVirtual + mdUnmanagedExport?), or rev the file format version and reinterpret legacy useless bits differently (mdHasSecurity or mdRequireSecObject).

However, we need to be slightly careful with that as it needs to be hidden from the C# developer, as the flag is NOT something that is acceptable to adjust by setting a flag on the attribute from the C# language. It would need to be set by the compiler itself

Doesn't the same concern apply to the custom attribute? Roslyn already enfoces that user can't set bits in the MethodImpl flag (e.g. you can't set bit 1 and say the method body is native code).

Also, using a MethodImpl flag would imply that we need to update ildasm, and ilasm, which otherwise do not require adjustment.

We'll need to touch ILASM/ILDASM for covariant returns anyway: #37045.

[terrajobst]

Video

Let's go with PreserveBaseOverridesAttribute then.

C# namespace System.Runtime.CompilerServices { [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)] public sealed class PreserveBaseOverridesAttribute : Attribute { } }

[jkotas]

Fixed by #35308