Covariant returns part 3

src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs

@@ -348,8 +348,11 @@ internal virtual IEnumerable<EventSymbol> GetEventsToEmit()
                         // If C# and the runtime don't agree on the overridden method, then 
                         // we will mark the method as newslot (see MethodSymbolAdapter) and
                         // specify the override explicitly.
-                        // This mostly affects accessors - C# ignores method interactions
+                        // This affects accessors - C# ignores method interactions
                         // between accessors and non-accessors, whereas the runtime does not.
+                        // It also affects covariant returns - C# ignores the return type in
+                        // determining if one method overrides another, while the runtime considers
+                        // the return type part of the signature.
                         yield return new Microsoft.Cci.MethodImplementation(method, moduleBeingBuilt.TranslateOverriddenMethodReference(method.OverriddenMethod, (CSharpSyntaxNode)context.SyntaxNodeOpt, context.Diagnostics));
                     }
                     else if (method.MethodKind == MethodKind.Destructor && this.SpecialType != SpecialType.System_Object)

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs

@@ -1166,13 +1166,7 @@ public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations
                     return explicitInterfaceImplementations;
                 }
 
-                var moduleSymbol = _containingType.ContainingPEModule;
-
-                // Context: we need the containing type of this method as context so that we can substitute appropriately into
-                // any generic interfaces that we might be explicitly implementing.  There is no reason to pass in the method
-                // context, however, because any method type parameters will belong to the implemented (i.e. interface) method,
-                // which we do not yet know.
-                var explicitlyOverriddenMethods = new MetadataDecoder(moduleSymbol, _containingType).GetExplicitlyOverriddenMethods(_containingType.Handle, _handle, this.ContainingType);
+                var explicitlyOverriddenMethods = ExplicitlyOverriddenMethods();
 
                 //avoid allocating a builder in the common case
                 var anyToRemove = false;
@@ -1183,9 +1177,9 @@ public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations
                     {
                         anyToRemove = true;
                         sawObjectFinalize =
-                            (method.ContainingType.SpecialType == SpecialType.System_Object &&
-                             method.Name == WellKnownMemberNames.DestructorName && // Cheaper than MethodKind.
-                             method.MethodKind == MethodKind.Destructor);
+                           (method.ContainingType.SpecialType == SpecialType.System_Object &&
+                            method.Name == WellKnownMemberNames.DestructorName && // Cheaper than MethodKind.
+                            method.MethodKind == MethodKind.Destructor);
                     }
 
                     if (anyToRemove && sawObjectFinalize)
@@ -1221,6 +1215,38 @@ public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations
             }
         }
 
+        private ImmutableArray<MethodSymbol> ExplicitlyOverriddenMethods()
+        {
+            var moduleSymbol = _containingType.ContainingPEModule;
+
+            // Context: we need the containing type of this method as context so that we can substitute appropriately into
+            // any generic interfaces that we might be explicitly implementing.  There is no reason to pass in the method
+            // context, however, because any method type parameters will belong to the implemented (i.e. interface) method,
+            // which we do not yet know.
+            return new MetadataDecoder(moduleSymbol, _containingType).GetExplicitlyOverriddenMethods(_containingType.Handle, _handle, this.ContainingType);
+        }
+
+        internal MethodSymbol ExplicitlyOverriddenClassMethod
+        {
+            get
+            {
+                if (IsExplicitClassOverride)
+                {
+                    foreach (MethodSymbol method in ExplicitlyOverriddenMethods())
+                    {
+                        if (method.ContainingType.IsClassType())
+                        {
+                            return method;
+                        }
+                    }
+
+                    throw ExceptionUtilities.Unreachable;
+                }
+
+                return null;
+            }
+        }
+
         internal override bool IsDeclaredReadOnly
         {
             get

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs

@@ -536,6 +536,21 @@ public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences
             }
         }
 
+        internal PropertySymbol ExplicitlyOverriddenClassProperty
+        {
+            get
+            {
+                // A covariant return readonly property override is indicated in metadata by
+                // a get accessor with an explicit override (methodimpl).
+                if (GetMethod is PEMethodSymbol { ExplicitlyOverriddenClassMethod: { AssociatedSymbol: PropertySymbol overriddenProperty } })
+                {
+                    return overriddenProperty;
+                }
+
+                return null;
+            }
+        }
+
         public override ImmutableArray<CSharpAttributeData> GetAttributes()
         {
             if (_lazyCustomAttributes.IsDefault)

src/Compilers/CSharp/Portable/Symbols/OverriddenOrHiddenMembersHelpers.cs

@@ -6,8 +6,7 @@
 using System.Collections.Immutable;
 using System.Diagnostics;
 using System.Linq;
-using Microsoft.CodeAnalysis.Collections;
-using Microsoft.CodeAnalysis.CSharp.Symbols;
+using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Roslyn.Utilities;
 
@@ -132,6 +131,14 @@ private static OverriddenOrHiddenMembersResult MakeOverriddenOrHiddenMembersWork
             Symbol bestMatch = null;
             ArrayBuilder<Symbol> hiddenBuilder = null;
 
+            // If the metadata indicates a specific override, use that.
+            Symbol explicitOverride = member switch
+            {
+                PEMethodSymbol method => method.ExplicitlyOverriddenClassMethod,
+                PEPropertySymbol property => property.ExplicitlyOverriddenClassProperty,
+                _ => null
+            };
+
             for (NamedTypeSymbol currType = containingType.BaseTypeNoUseSiteDiagnostics;
                 (object)currType != null && (object)bestMatch == null && hiddenBuilder == null;
                 currType = currType.BaseTypeNoUseSiteDiagnostics)
@@ -141,6 +148,7 @@ private static OverriddenOrHiddenMembersResult MakeOverriddenOrHiddenMembersWork
                     member,
                     memberIsFromSomeCompilation,
                     containingType,
+                    explicitOverride,
                     currType,
                     out bestMatch,
                     out unused,
@@ -238,16 +246,30 @@ private static OverriddenOrHiddenMembersResult MakePropertyAccessorOverriddenOrH
             ImmutableArray<Symbol> overriddenAccessors = ImmutableArray<Symbol>.Empty;
             ImmutableArray<Symbol> runtimeOverriddenAccessors = ImmutableArray<Symbol>.Empty;
             if ((object)overriddenAccessor != null && IsOverriddenSymbolAccessible(overriddenAccessor, accessor.ContainingType) &&
-                    (accessorIsFromSomeCompilation
-                        ? MemberSignatureComparer.CSharpAccessorOverrideComparer.Equals(accessor, overriddenAccessor) //NB: custom comparer
-                        : MemberSignatureComparer.RuntimeSignatureComparer.Equals(accessor, overriddenAccessor)))
+                isAccessorOverride(accessor, overriddenAccessor))
             {
                 FindRelatedMembers(
                     accessor.IsOverride, accessorIsFromSomeCompilation, accessor.Kind, overriddenAccessor, out overriddenAccessors, out runtimeOverriddenAccessors, ref hiddenBuilder);
             }
 
             ImmutableArray<Symbol> hiddenMembers = hiddenBuilder == null ? ImmutableArray<Symbol>.Empty : hiddenBuilder.ToImmutableAndFree();
             return OverriddenOrHiddenMembersResult.Create(overriddenAccessors, hiddenMembers, runtimeOverriddenAccessors);
+
+            bool isAccessorOverride(MethodSymbol accessor, MethodSymbol overriddenAccessor)
+            {
+                if (accessorIsFromSomeCompilation)
+                {
+                    return MemberSignatureComparer.CSharpAccessorOverrideComparer.Equals(accessor, overriddenAccessor); //NB: custom comparer
+                }
+
+                if (accessor is PEMethodSymbol { ExplicitlyOverriddenClassMethod: MethodSymbol peOverriddenAccessor } &&
+                    overriddenAccessor.Equals(peOverriddenAccessor, TypeCompareKind.AllIgnoreOptions))
+                {
+                    return true;
+                }
+
+                return MemberSignatureComparer.RuntimeSignatureComparer.Equals(accessor, overriddenAccessor);
+            }
         }
 
         /// <summary>
@@ -377,6 +399,7 @@ internal static OverriddenOrHiddenMembersResult MakeInterfaceOverriddenOrHiddenM
                     member,
                     memberIsFromSomeCompilation,
                     containingType,
+                    explicitOverride: null,
                     currType,
                     out currTypeBestMatch,
                     out currTypeHasSameKindNonMatch,
@@ -460,6 +483,7 @@ internal static OverriddenOrHiddenMembersResult MakeInterfaceOverriddenOrHiddenM
         /// <param name="member">Member that is hiding or overriding.</param>
         /// <param name="memberIsFromSomeCompilation">True if member is from the current compilation.</param>
         /// <param name="memberContainingType">The type that contains member (member.ContainingType).</param>
+        /// <param name="explicitOverride">An explicitly overridden class member.</param>
         /// <param name="currType">The type to search.</param>
         /// <param name="currTypeBestMatch">
         /// A member with the same signature if currTypeHasExactMatch is true,
@@ -477,6 +501,7 @@ private static void FindOverriddenOrHiddenMembersInType(
             Symbol member,
             bool memberIsFromSomeCompilation,
             NamedTypeSymbol memberContainingType,
+            Symbol explicitOverride,
             NamedTypeSymbol currType,
             out Symbol currTypeBestMatch,
             out bool currTypeHasSameKindNonMatch,
@@ -540,7 +565,8 @@ private static void FindOverriddenOrHiddenMembersInType(
                             break;
 
                         default:
-                            if (exactMatchComparer.Equals(member, otherMember))
+                            if (otherMember.Equals(explicitOverride, TypeCompareKind.AllIgnoreOptions) ||
+                                exactMatchComparer.Equals(member, otherMember))
                             {
                                 currTypeHasExactMatch = true;
                                 currTypeBestMatch = otherMember;
@@ -891,9 +917,12 @@ private static int CustomModifierCount(Symbol member)
             }
         }
 
-        // CONSIDER: we could cache this on MethodSymbol
+        /// <summary>
+        /// Determine if this method requires a methodimpl table entry to inform the runtime of the override relationship.
+        /// </summary>
         internal static bool RequiresExplicitOverride(this MethodSymbol method)
         {
+            // CONSIDER: we could cache this on MethodSymbol
             if (method.IsOverride)
             {
                 MethodSymbol csharpOverriddenMethod = method.OverriddenMethod;
@@ -905,7 +934,8 @@ internal static bool RequiresExplicitOverride(this MethodSymbol method)
                 // We can ignore interface implementation changes since the method is already metadata virtual (since override).
                 // TODO: do we want to add more sophisticated handling for the case where there are multiple runtime-overridden methods?
                 MethodSymbol runtimeOverriddenMethod = method.GetFirstRuntimeOverriddenMethodIgnoringNewSlot(ignoreInterfaceImplementationChanges: true);
-                return csharpOverriddenMethod != runtimeOverriddenMethod &&
+                return runtimeOverriddenMethod is null ||
+                    csharpOverriddenMethod != runtimeOverriddenMethod &&
                     method.IsAccessor() != runtimeOverriddenMethod.IsAccessor();
             }