Disallow implicit implementation of non-public interface members and …

…explicit implementation of inaccessible members. (#19549)

docs/features/DefaultInterfaceImplementation.md

@@ -44,3 +44,41 @@ class Test1 : I1
 - Declaring types within interfaces. **Protected** and **protected internal** accessibility is not supported.
 
 **Open issues and work items** are tracked in https://github.com/dotnet/roslyn/issues/17952.
+
+**Parts of ECMA-335 that become obsolete/inaccurate/incomplete**
+>I.8.5.3.2 Accessibility of members and nested types
+Members (other than nested types) defined by an interface shall be public.
+I.8.9.4 Interface type definition
+Similarly, an interface type definition shall not provide implementations for any methods on the
+values of its type.
+Interfaces can have static or virtual methods, but shall not have instance methods.
+However, since accessibility attributes are relative to the implementing type rather
+than the interface itself, all members of an interface shall have public accessibility, ...
+I.8.11.1 Method definitions
+All non-static methods of an interface definition are abstract.
+All non-static method definitions in interface definitions shall be virtual methods.
+II.10.4 Method implementat ion requirements
+II.12 Semantics of interfaces
+Interfaces can have static fields and methods, but they shall not have instance fields or 
+methods. Interfaces can define virtual methods, but only if those methods are abstract 
+(see Partition I and §II.15.4.2.4).
+II.12.2 Implement ing virtual methods on interfaces
+If the class defines any public virtual methods whose name and signature
+match a virtual method on the interface, then add these to the list for that
+method, in type declaration order (see above).
+If there are any public virtual methods available on this class (directly or inherited)
+having the same name and signature as the interface method, and whose generic type
+parameters do not exactly match any methods in the existing list for that interface
+method for this class or any class in its inheritance chain, then add them (in type
+declaration order) to the list for the corresponding methods on the interface.
+II.15.2 Static, instance, and virtual methods
+It follows that instance methods shall only be defined in classes or value types, 
+but not in interfaces or outside of a type (i.e., globally).
+II.22.27 MethodImpl : 0x19
+The method indexed by MethodBody shall be a member of Class or some base class
+of Class (MethodImpls do not allow compilers to ‘hook’ arbitrary method bodies)
+II.22.37 TypeDef : 0x02
+All of the methods owned by an Interface (Flags.Interface = 1) shall be abstract
+(Flags.Abstract = 1)
+IV.6 Implementation-specific modifications to the system libraries
+Interfaces and virtual methods shall not be added to an existing interface.

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5076,4 +5076,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
   <data name="ERR_DefaultInterfaceImplementationModifier" xml:space="preserve">
     <value>The modifier '{0}' is not valid for this item in C# {1}. Please use language version {2} or greater.</value>
   </data>
+  <data name="ERR_ImplicitImplementationOfNonPublicInterfaceMember" xml:space="preserve">
+    <value>'{0}' does not implement interface member '{1}'. '{2}' cannot implicitly implement a non-public member.</value>
+  </data>
 </root>

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -1485,6 +1485,7 @@ internal enum ErrorCode
         // PROTOTYPE(DefaultInterfaceImplementation): Should add this error code to the list monitored by the UpgradeProject code fixer.
         // This PR can be used to find the relevant code and test files: https://github.com/dotnet/roslyn/pull/18045.
         ERR_DefaultInterfaceImplementationModifier = 8503,
+        ERR_ImplicitImplementationOfNonPublicInterfaceMember = 8504,
 
         ERR_BadDynamicMethodArgDefaultLiteral = 9000,
         ERR_DefaultLiteralNotValid = 9001,

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

@@ -119,6 +119,27 @@ internal static bool HasUnsafeParameter(this Symbol member)
             return false;
         }
 
+        public static bool IsEventOrPropertyWithNonPublicAccessor(this Symbol symbol)
+        {
+            switch (symbol.Kind)
+            {
+                case SymbolKind.Property:
+                    var propertySymbol = (PropertySymbol)symbol;
+                    return AccessorIsNotPublic(propertySymbol.GetMethod) || AccessorIsNotPublic(propertySymbol.SetMethod);
+
+                case SymbolKind.Event:
+                    var eventSymbol = (EventSymbol)symbol;
+                    return AccessorIsNotPublic(eventSymbol.AddMethod) || AccessorIsNotPublic(eventSymbol.RemoveMethod);
+            }
+
+            return false;
+
+            bool AccessorIsNotPublic(MethodSymbol accessor)
+            {
+                return (object)accessor != null && accessor.DeclaredAccessibility != Accessibility.Public;
+            }
+        }
+
         public static bool IsAccessor(this MethodSymbol methodSymbol)
         {
             return (object)methodSymbol.AssociatedSymbol != null;

src/Compilers/CSharp/Portable/Symbols/Source/ExplicitInterfaceHelpers.cs

@@ -1,5 +1,6 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
+using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Diagnostics;
 using System.Text;
@@ -219,8 +220,6 @@ private static Symbol FindExplicitlyImplementedMember(
             {
                 // At this point, we know that explicitInterfaceNamedType is an interface.
                 // However, metadata interface members can be static - we ignore them, as does Dev10.
-                // PROTOTYPE(DefaultInterfaceImplementation): We might want to check accessibility of the members as well, since  
-                //                                            they don't have to be public any more.
                 if (interfaceMember.Kind != implementingMember.Kind || !interfaceMember.IsImplementableInterfaceMember())
                 {
                     continue;
@@ -261,6 +260,45 @@ private static Symbol FindExplicitlyImplementedMember(
                 diagnostics.Add(ErrorCode.ERR_InterfaceMemberNotFound, memberLocation, implementingMember);
             }
 
+            // Make sure implemented member is accessible
+            if ((object)implementedMember != null)
+            {
+                HashSet<DiagnosticInfo> useSiteDiagnostics = null;
+
+                if (!AccessCheck.IsSymbolAccessible(implementedMember, implementingMember.ContainingType, ref useSiteDiagnostics, throughTypeOpt: implementedMember.ContainingType))
+                {
+                    diagnostics.Add(ErrorCode.ERR_BadAccess, memberLocation, implementedMember);
+                }
+                else
+                {
+                    switch (implementedMember.Kind)
+                    {
+                        case SymbolKind.Property:
+                            var propertySymbol = (PropertySymbol)implementedMember;
+                            CheckAccessorAccessible(propertySymbol.GetMethod);
+                            CheckAccessorAccessible(propertySymbol.SetMethod);
+                            break;
+
+                        case SymbolKind.Event:
+                            var eventSymbol = (EventSymbol)implementedMember;
+                            CheckAccessorAccessible(eventSymbol.AddMethod);
+                            CheckAccessorAccessible(eventSymbol.RemoveMethod);
+                            break;
+                    }
+
+                    void CheckAccessorAccessible(MethodSymbol accessor)
+                    {
+                        if ((object)accessor != null &&
+                            !AccessCheck.IsSymbolAccessible(accessor, implementingMember.ContainingType, ref useSiteDiagnostics, throughTypeOpt: accessor.ContainingType))
+                        {
+                            diagnostics.Add(ErrorCode.ERR_BadAccess, memberLocation, accessor);
+                        }
+                    }
+                }
+
+                diagnostics.Add(memberLocation, useSiteDiagnostics);
+            }
+
             if (MemberSignatureComparer.ConsideringTupleNamesCreatesDifference(implementingMember, implementedMember))
             {
                 diagnostics.Add(ErrorCode.ERR_ImplBadTupleNames, memberLocation, implementingMember, implementedMember);

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs

@@ -1025,6 +1025,15 @@ private void CheckInterfaceUnification(DiagnosticBag diagnostics)
         /// <returns>Synthesized implementation or null if not needed.</returns>
         private SynthesizedExplicitImplementationForwardingMethod SynthesizeInterfaceMemberImplementation(SymbolAndDiagnostics implementingMemberAndDiagnostics, Symbol interfaceMember)
         {
+            if (interfaceMember.DeclaredAccessibility != Accessibility.Public)
+            {
+                // Non-public interface members cannot be implemented implicitly,
+                // appropriate errors are reported elsewhere. Let's not synthesize
+                // forwarding methods, or modify metadata virtualness of the
+                // implementing methods.
+                return null;
+            }
+
             foreach (Diagnostic diagnostic in implementingMemberAndDiagnostics.Diagnostics)
             {
                 if (diagnostic.Severity == DiagnosticSeverity.Error)
@@ -1044,6 +1053,15 @@ private SynthesizedExplicitImplementationForwardingMethod SynthesizeInterfaceMem
             MethodSymbol interfaceMethod = (MethodSymbol)interfaceMember;
             MethodSymbol implementingMethod = (MethodSymbol)implementingMember;
 
+            // Interface properties/events with non-public accessors cannot be implemented implicitly,
+            // appropriate errors are reported elsewhere. Let's not synthesize
+            // forwarding methods, or modify metadata virtualness of the
+            // implementing accessors, even for public ones.
+            if (interfaceMethod.AssociatedSymbol?.IsEventOrPropertyWithNonPublicAccessor() == true)
+            {
+                return null;
+            }
+
             //explicit implementations are always respected by the CLR
             if (implementingMethod.ExplicitInterfaceImplementations.Contains(interfaceMethod))
             {

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

@@ -775,7 +775,7 @@ private SymbolAndDiagnostics ComputeImplementationAndDiagnosticsForInterfaceMemb
         /// <remarks>
         /// CONSIDER: we could probably do less work in the metadata and retargeting cases - we won't use the diagnostics.
         /// </remarks>
-        /// <param name="interfaceMember">A non-null property on an interface type.</param>
+        /// <param name="interfaceMember">A non-null implementable member on an interface type.</param>
         /// <param name="implementingType">The type implementing the interface property (usually "this").</param>
         /// <param name="diagnostics">Bag to which to add diagnostics.</param>
         /// <returns>The implementing property or null, if there isn't one.</returns>
@@ -807,6 +807,8 @@ private static Symbol ComputeImplementationForInterfaceMember(Symbol interfaceMe
 
             Symbol implicitImpl = null;
             Symbol closestMismatch = null;
+            bool canBeImplementedImplicitly = interfaceMember.DeclaredAccessibility == Accessibility.Public && !interfaceMember.IsEventOrPropertyWithNonPublicAccessor();
+            TypeSymbol implementingBaseOpt = null; // Calculated only if canBeImplementedImplicitly == true
 
             for (TypeSymbol currType = implementingType; (object)currType != null; currType = currType.BaseTypeNoUseSiteDiagnostics)
             {
@@ -829,7 +831,20 @@ private static Symbol ComputeImplementationForInterfaceMember(Symbol interfaceMe
                     return currTypeExplicitImpl;
                 }
 
-                seenTypeDeclaringInterface = seenTypeDeclaringInterface || currType.InterfacesAndTheirBaseInterfacesNoUseSiteDiagnostics.Contains(interfaceType);
+                if (!seenTypeDeclaringInterface || 
+                    (!canBeImplementedImplicitly && (object)implementingBaseOpt == null))
+                {
+                    if (currType.InterfacesAndTheirBaseInterfacesNoUseSiteDiagnostics.Contains(interfaceType))
+                    {
+                        seenTypeDeclaringInterface = true;
+
+                        if (!canBeImplementedImplicitly && (object)implementingBaseOpt == null && (object)currType != implementingType)
+                        {
+                            implementingBaseOpt = currType;
+                        }
+                    }
+                }
+
 
                 // We want the implementation from the most derived type at or above the first one to
                 // include the interface (or a subinterface) in its interface list
@@ -859,11 +874,14 @@ private static Symbol ComputeImplementationForInterfaceMember(Symbol interfaceMe
                 }
             }
 
+            Debug.Assert(!canBeImplementedImplicitly || (object)implementingBaseOpt == null);
+
             // Dev10 has some extra restrictions and extra wiggle room when finding implicit
             // implementations for interface accessors.  Perform some extra checks and possibly
             // update the result (i.e. implicitImpl).
             if (interfaceMember.IsAccessor())
             {
+                // PROTOTYPE(DefaultInterfaceImplementation): Do we need to adjust behavior of this function in any way?
                 CheckForImplementationOfCorrespondingPropertyOrEvent((MethodSymbol)interfaceMember, implementingType, implementingTypeIsFromSomeCompilation, ref implicitImpl);
             }
 
@@ -878,10 +896,24 @@ private static Symbol ComputeImplementationForInterfaceMember(Symbol interfaceMe
 
             if ((object)implicitImpl != null)
             {
-                ReportImplicitImplementationMatchDiagnostics(interfaceMember, implementingType, implicitImpl, diagnostics);
+                if (!canBeImplementedImplicitly && !implicitImpl.ContainingType.IsInterface)
+                {
+                    if (interfaceMember.Kind == SymbolKind.Method && 
+                        (object)implementingBaseOpt == null) // Otherwise any approprite errors are going to be reported for the base.
+                    {
+                        diagnostics.Add(ErrorCode.ERR_ImplicitImplementationOfNonPublicInterfaceMember, GetInterfaceLocation(interfaceMember, implementingType),
+                                        implementingType, interfaceMember, implicitImpl);
+                    }
+                }
+                else
+                {
+                    ReportImplicitImplementationMatchDiagnostics(interfaceMember, implementingType, implicitImpl, diagnostics);
+                }
             }
             else if ((object)closestMismatch != null)
             {
+                Debug.Assert(interfaceMember.DeclaredAccessibility == Accessibility.Public);
+                Debug.Assert(!interfaceMember.IsEventOrPropertyWithNonPublicAccessor());
                 ReportImplicitImplementationMismatchDiagnostics(interfaceMember, implementingType, closestMismatch, diagnostics);
             }
 
@@ -1297,8 +1329,12 @@ private static void FindPotentialImplicitImplementationMemberDeclaredInType(
                         return;
                     }
 
-                    //if we haven't found a match, do a weaker comparison that ignores static-ness, accessibility, and return type
-                    if ((object)closeMismatch == null && implementingTypeIsFromSomeCompilation)
+                    // If we haven't found a match, do a weaker comparison that ignores static-ness, accessibility, and return type.
+                    // But do this only if interface member is public because language doesn't allow implicit implementations for
+                    // non-public members and, since candidate's signature doesn't match, runtime will never pick it up either. 
+                    else if ((object)closeMismatch == null && implementingTypeIsFromSomeCompilation &&
+                             interfaceMember.DeclaredAccessibility == Accessibility.Public &&
+                             !interfaceMember.IsEventOrPropertyWithNonPublicAccessor())
                     {
                         // We can ignore custom modifiers here, because our goal is to improve the helpfulness
                         // of an error we're already giving, rather than to generate a new error.