Disallow implicit implementation of non-public interface members and explicit implementation of inaccessible members. #19549

AlekseyTs · 2017-05-16T06:03:51Z

@dotnet/roslyn-compiler Please review.

…explicit implementation of inaccessible members.

AlekseyTs · 2017-05-16T16:53:48Z

@dotnet/roslyn-compiler Please review.

jcouv · 2017-05-16T19:24:19Z

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

+                case SymbolKind.Property:
+                    var propertySymbol = (PropertySymbol)symbol;
+
+                    bool AccessorIsNotPublic(MethodSymbol accessor)


Consider moving out of the switch statement, since used by both cases.
Also, consider using lower-case (accessorIsNotPublic) for local function.

Both were intentional choices. I prefer to keep them unless they violate some guidelines.

In reply to: 116834659 [](ancestors = 116834659)

I agree with @jcouv . Calling a method declared in one switch branch from another switch branch makes the code confusing to read.

jcouv · 2017-05-16T19:28:47Z

    /// <param name="interfaceMember">A non-null property on an interface type.</param>

Consider updating the comment. This method works on methods, properties and events, not just properties.

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs:778 in 7d4e4e3. [](commit_id = 7d4e4e3, deletion_comment = False)

AlekseyTs · 2017-05-17T06:14:31Z

    /// <param name="interfaceMember">A non-null property on an interface type.</param>

Consider updating the comment. This method works on methods, properties and events, not just properties.

Done.

In reply to: 301890039 [](ancestors = 301890039)

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs:778 in 7d4e4e3. [](commit_id = 7d4e4e3, deletion_comment = False)

AlekseyTs · 2017-05-17T17:24:18Z

@dotnet/roslyn-compiler Please review.

jcouv · 2017-05-17T18:07:28Z

        void ValidateEventNotImplemented_11(CSharpCompilation compilation, string className)

Having a method and a local function with the same name is rather confusing.
Their logic seems the same (except for hard-coded "Test2"), so one can be deleted.

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:20047 in 3a44513. [](commit_id = 3a44513, deletion_comment = False)

AlekseyTs · 2017-05-17T18:14:38Z

        void ValidateEventNotImplemented_11(CSharpCompilation compilation, string className)

Yes, meant to delete this function after extracting. Done.

In reply to: 302177324 [](ancestors = 302177324)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:20047 in 3a44513. [](commit_id = 3a44513, deletion_comment = False)

AlekseyTs · 2017-05-18T00:10:26Z

@dotnet/roslyn-compiler Please review.

AlekseyTs · 2017-05-18T17:40:10Z

@dotnet/roslyn-compiler Please review.

jcouv

LGTM
Sorry, I'd forgotten to approve after reviewing yesterday.

cston · 2017-05-18T21:39:42Z

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

+                        case SymbolKind.Property:
+                            var propertySymbol = (PropertySymbol)implementedMember;
+
+                            void CheckAccessorAccessible(MethodSymbol acessor)


Typo: accessor

Typo: accessor

Thanks, I'll address it in the next PR.

cston · 2017-05-18T21:56:28Z

    public void PropertyModifiers_23_51()

12?

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:13304 in 1399384. [](commit_id = 1399384, deletion_comment = False)

cston · 2017-05-18T21:58:10Z

LGTM

gafter

Partial review. I did not review the tests yet.

gafter · 2017-05-18T21:31:29Z

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 '{1}' because '{1}' is not public.</value>


I think it would be a good idea to find a way to reword this so that fill-ins are not repeated. I think the second sentence can be replaced by "Non-public methods of an interface may not be implemented implicitly."

I think there is a value in the second sentence. How about I change it to "'{2}' cannot implicitly implement non-public member" ?

gafter · 2017-05-18T21:33:36Z

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

+                case SymbolKind.Property:
+                    var propertySymbol = (PropertySymbol)symbol;
+
+                    bool AccessorIsNotPublic(MethodSymbol accessor)


I agree with @jcouv . Calling a method declared in one switch branch from another switch branch makes the code confusing to read.

gafter · 2017-05-18T21:37:24Z

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

+                        case SymbolKind.Property:
+                            var propertySymbol = (PropertySymbol)implementedMember;
+
+                            void CheckAccessorAccessible(MethodSymbol acessor)


Please move this local function out of the switch since it is used in more than one case block.

Please move this local function out of the switch since it is used in more than one case block.

Will do.

gafter · 2017-05-18T21:42:50Z

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

-                    //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


This comment is confusing. "But do this only if interface member is public because language doesn't allow implicit implementations for such members ..."

The language does allow implicit implementations for interface members that are public. Can you please reword?

How does "But do this only if interface member is public because language doesn't allow implicit implementations for non-public members ..." sound?

AlekseyTs · 2017-05-18T22:15:04Z

12?

I intentionally left a gap,in case I need to add more tests in between.

- Move local functions out of switch statement. - Rename parameter - Adjust comment - Adjust error message text

Disallow implicit implementation of non-public interface members and …

7d4e4e3

…explicit implementation of inaccessible members.

AlekseyTs added Area-Compilers Language-C# Feature - Default Interface Impl Default Interface Implementation labels May 16, 2017

dnfclas added the cla-already-signed label May 16, 2017

jcouv reviewed May 16, 2017

View reviewed changes

Adjust a comment

3a44513

Remove obsolete local function.

1399384

gafter self-assigned this May 17, 2017

jcouv approved these changes May 18, 2017

View reviewed changes

cston reviewed May 18, 2017

View reviewed changes

gafter reviewed May 18, 2017

View reviewed changes

Minor changes to address CR feedback

796e067

- Move local functions out of switch statement. - Rename parameter - Adjust comment - Adjust error message text

AlekseyTs merged commit f030051 into dotnet:features/DefaultInterfaceImplementation May 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow implicit implementation of non-public interface members and explicit implementation of inaccessible members. #19549

Disallow implicit implementation of non-public interface members and explicit implementation of inaccessible members. #19549

AlekseyTs commented May 16, 2017

AlekseyTs commented May 16, 2017

jcouv May 16, 2017

AlekseyTs May 17, 2017

gafter May 18, 2017

jcouv commented May 16, 2017

AlekseyTs commented May 17, 2017

AlekseyTs commented May 17, 2017

jcouv commented May 17, 2017

AlekseyTs commented May 17, 2017

AlekseyTs commented May 18, 2017

AlekseyTs commented May 18, 2017

jcouv left a comment

cston May 18, 2017

AlekseyTs May 18, 2017

cston commented May 18, 2017

cston commented May 18, 2017

gafter left a comment

gafter May 18, 2017

AlekseyTs May 18, 2017

gafter May 18, 2017

gafter May 18, 2017

AlekseyTs May 18, 2017

gafter May 18, 2017

AlekseyTs May 18, 2017

AlekseyTs commented May 18, 2017

Disallow implicit implementation of non-public interface members and explicit implementation of inaccessible members. #19549

Disallow implicit implementation of non-public interface members and explicit implementation of inaccessible members. #19549

Conversation

AlekseyTs commented May 16, 2017

AlekseyTs commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented May 16, 2017

AlekseyTs commented May 17, 2017

AlekseyTs commented May 17, 2017

jcouv commented May 17, 2017

AlekseyTs commented May 17, 2017

AlekseyTs commented May 18, 2017

AlekseyTs commented May 18, 2017

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented May 18, 2017

cston commented May 18, 2017

gafter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 18, 2017