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.

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

Allow implicit implementation of non-public interface members #54182

Merged
merged 5 commits into from
Jun 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6077,7 +6077,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<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>
<value>'{0}' does not implement interface member '{1}'. '{2}' cannot implicitly implement a non-public member in C# {3}. Please use language version '{4}' or greater.</value>
</data>
<data name="ERR_MostSpecificImplementationIsNotFound" xml:space="preserve">
<value>Interface member '{0}' does not have a most specific implementation. Neither '{1}', nor '{2}' are most specific.</value>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ internal enum MessageID
IDS_FeatureStaticAbstractMembersInInterfaces = MessageBase + 12803,
IDS_FeatureLambdaReturnType = MessageBase + 12804,
IDS_AsyncMethodBuilderOverride = MessageBase + 12805,
IDS_FeatureImplicitImplementationOfNonPublicMemebers = MessageBase + 12806,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -352,6 +353,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureLambdaReturnType: // semantic check
case MessageID.IDS_AsyncMethodBuilderOverride: // semantic check
case MessageID.IDS_FeatureConstantInterpolatedStrings: // semantic check
case MessageID.IDS_FeatureImplicitImplementationOfNonPublicMemebers: // semantic check
return LanguageVersion.CSharp10;

// C# 9.0 features.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1588,18 +1588,9 @@ private void CheckInterfaceUnification(BindingDiagnosticBag diagnostics)
private (SynthesizedExplicitImplementationForwardingMethod? ForwardingMethod, (MethodSymbol Body, MethodSymbol Implemented)? MethodImpl)
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 default;
}

foreach (Diagnostic diagnostic in implementingMemberAndDiagnostics.Diagnostics.Diagnostics)
{
if (diagnostic.Severity == DiagnosticSeverity.Error)
if (diagnostic.Severity == DiagnosticSeverity.Error && diagnostic.Code is not (int)ErrorCode.ERR_ImplicitImplementationOfNonPublicInterfaceMember)
{
return default;
}
Expand All @@ -1616,15 +1607,6 @@ private void CheckInterfaceUnification(BindingDiagnosticBag diagnostics)
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?.IsEventOrPropertyWithImplementableNonPublicAccessor() == true)
{
return default;
}

//explicit implementations are always respected by the CLR
if (implementingMethod.ExplicitInterfaceImplementations.Contains(interfaceMethod, ExplicitInterfaceImplementationTargetMemberEqualityComparer.Instance))
{
Expand Down
34 changes: 16 additions & 18 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ private static Symbol ComputeImplementationForInterfaceMember(Symbol interfaceMe

Symbol implicitImpl = null;
Symbol closestMismatch = null;
bool canBeImplementedImplicitly = interfaceMember.DeclaredAccessibility == Accessibility.Public && !interfaceMember.IsEventOrPropertyWithImplementableNonPublicAccessor();
bool canBeImplementedImplicitlyInCSharp9 = interfaceMember.DeclaredAccessibility == Accessibility.Public && !interfaceMember.IsEventOrPropertyWithImplementableNonPublicAccessor();
TypeSymbol implementingBaseOpt = null; // Calculated only if canBeImplementedImplicitly == false
bool implementingTypeImplementsInterface = false;
CSharpCompilation compilation = implementingType.DeclaringCompilation;
Expand Down Expand Up @@ -891,7 +891,7 @@ private static Symbol ComputeImplementationForInterfaceMember(Symbol interfaceMe
}

if (!seenTypeDeclaringInterface ||
(!canBeImplementedImplicitly && (object)implementingBaseOpt == null))
(!canBeImplementedImplicitlyInCSharp9 && (object)implementingBaseOpt == null))
{
if (currType.InterfacesAndTheirBaseInterfacesWithDefinitionUseSiteDiagnostics(ref useSiteInfo).ContainsKey(interfaceType))
{
Expand All @@ -905,7 +905,7 @@ private static Symbol ComputeImplementationForInterfaceMember(Symbol interfaceMe
{
implementingTypeImplementsInterface = true;
}
else if (!canBeImplementedImplicitly && (object)implementingBaseOpt == null)
else if (!canBeImplementedImplicitlyInCSharp9 && (object)implementingBaseOpt == null)
{
implementingBaseOpt = currType;
}
Expand Down Expand Up @@ -941,7 +941,7 @@ private static Symbol ComputeImplementationForInterfaceMember(Symbol interfaceMe
}
}

Debug.Assert(!canBeImplementedImplicitly || (object)implementingBaseOpt == null);
Debug.Assert(!canBeImplementedImplicitlyInCSharp9 || (object)implementingBaseOpt == null);

bool tryDefaultInterfaceImplementation = !interfaceMember.IsStatic;

Expand Down Expand Up @@ -1002,24 +1002,26 @@ private static Symbol ComputeImplementationForInterfaceMember(Symbol interfaceMe
{
if ((object)implicitImpl != null)
{
if (!canBeImplementedImplicitly)
if (!canBeImplementedImplicitlyInCSharp9)
{
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);
LanguageVersion requiredVersion = MessageID.IDS_FeatureImplicitImplementationOfNonPublicMemebers.RequiredVersion();
LanguageVersion? availableVersion = implementingType.DeclaringCompilation?.LanguageVersion;
if (requiredVersion > availableVersion)
{
diagnostics.Add(ErrorCode.ERR_ImplicitImplementationOfNonPublicInterfaceMember, GetInterfaceLocation(interfaceMember, implementingType),
implementingType, interfaceMember, implicitImpl,
availableVersion.GetValueOrDefault().ToDisplayString(), new CSharpRequiredLanguageVersion(requiredVersion));
}
Comment on lines +1011 to +1017
Copy link
Member

@Youssef1313 Youssef1313 Jun 17, 2021

Choose a reason for hiding this comment

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

Wondering why this is not using the usual "CheckFeatureAvailability" method? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering why this is not using the usual "CheckFeatureAvailability" method?

Because I am not interested in the generic error that helper would report.

}
}
else
{
ReportImplicitImplementationMatchDiagnostics(interfaceMember, implementingType, implicitImpl, diagnostics);
}

ReportImplicitImplementationMatchDiagnostics(interfaceMember, implementingType, implicitImpl, diagnostics);
}
else if ((object)closestMismatch != null)
{
Debug.Assert(interfaceMember.DeclaredAccessibility == Accessibility.Public);
Debug.Assert(!interfaceMember.IsEventOrPropertyWithImplementableNonPublicAccessor());
ReportImplicitImplementationMismatchDiagnostics(interfaceMember, implementingType, closestMismatch, diagnostics);
}
}
Expand Down Expand Up @@ -2063,11 +2065,7 @@ private static void FindPotentialImplicitImplementationMemberDeclaredInType(
}

// 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.IsEventOrPropertyWithImplementableNonPublicAccessor())
else if ((object)closeMismatch == null && implementingTypeIsFromSomeCompilation)
{
// 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.
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading