Support re-abstraction of interface members in derived interfaces #35756

AlekseyTs · 2019-05-16T04:50:00Z

No description provided.

…essorSymbol

AlekseyTs · 2019-05-16T16:51:56Z

@dotnet/roslyn-compiler Please review #Closed

jcouv · 2019-05-16T22:44:23Z

FWIW, I collected the LDM notes that discuss reabstraction:

The proposal mentions it, but needs to be updated:
https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md#reabstraction

The design doc doesn't mention it:
https://github.com/dotnet/roslyn/blob/master/docs/features/DefaultInterfaceImplementation.md

AlekseyTs · 2019-05-17T17:04:54Z

@dotnet/roslyn-compiler Please review #Closed

jcouv · 2019-05-17T17:43:31Z

I'm looking

jcouv · 2019-05-17T17:45:16Z

docs/features/DefaultInterfaceImplementation.md

+class Test1 : I2 // This type must implement all members of I1
+{
+}
+```


nit: If I recall, this is getting a non-obvious representation in metadata. Consider documenting too (helps in case of cross-language support). #Closed

jcouv · 2019-05-17T17:55:33Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedFieldLikeEventAccessorSymbol.cs

-                SourceEventSymbol.GetAccessorName(@event.Name, isAdder);
-        }
-
-        public override string Name


Name [](start = 31, length = 4)

This removal seems surprising, as it means the Name will be empty (implementation from Symbol, I think). Wouldn't that at least affect some public APIs (ISymbol.Name now gives a different answer)?

Sorry. It's moved to a base class. :-S #Closed

gafter · 2019-05-17T20:01:23Z

docs/features/DefaultInterfaceImplementation.md

+
+public interface I2 : I1
+{
+    abstract void I1.M1();


abstract [](start = 4, length = 8)

In an interface, when a body is missing abstract is implied. So the abstract keyword is optional here. However I see no tests for that. Please add an example and/or comment here and tests covering this. This is called out explicitly in the specification at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md#reabstraction and https://github.com/dotnet/csharplang/blob/master/meetings/2017/LDM-2017-03-21.md#reabstraction #Resolved

In an interface, when a body is missing abstract is implied.

It is not implied for interface implementations and I am not sure if it is a good idea to do that. The behavior is captured in MethodImplementationInDerived_04 and similar unit-tests. I am not planning to change that in the current PR, instead I'll bring that for discussion to LDM.

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

Added to https://github.com/dotnet/csharplang/blob/master/meetings/2019/README.md under "Schedule ASAP"

In reply to: 285274070 [](ancestors = 285274070,285270178)

gafter · 2019-05-17T20:16:53Z

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

@@ -1289,7 +1289,8 @@ internal enum ErrorCode
        // available 8041-8049
        ERR_InitializerOnNonAutoProperty = 8050,
        ERR_AutoPropertyMustHaveGetAccessor = 8051,
-        ERR_AutoPropertyInitializerInInterface = 8052,
+        ERR_AbstractEventHasAccessors = 8052,
+        // available 8053


Please place all new C# 8 diagnostics in the region diagnostics introduced for C# 8.0 #Closed

Please place all new C# 8 diagnostics in the region diagnostics introduced for C# 8.0

I am not aware of a requirement to do that.

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

However, I was under impression that ERR_AutoPropertyInitializerInInterface was added as part of the DIM work, but that is not the case. Since we cannot reuse error numbers for errors that were shipped, I will not reuse this number for ERR_AbstractEventHasAccessors.

In reply to: 285274982 [](ancestors = 285274982,285274626)

Placing a C# 8 diagnostic in a region labeled diagnostics introduced in C# 4 and earlier is misleading, which is why I asked you to move it.

In reply to: 285274982 [](ancestors = 285274982,285274626)

gafter · 2019-05-17T21:14:28Z

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

@@ -1059,7 +1059,16 @@ private static Symbol FindMostSpecificImplementation(Symbol interfaceMember, Nam
                                                                 ref useSiteDiagnostics,
                                                                 out var _, out var _);


Are we possibly missing use-site diagnostics? #Closed

Are we possibly missing use-site diagnostics?

This method is not interested in any diagnostics, it doesn't report any and doesn't return any to its caller.

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

How do we know that any use site diagnostics that are deposited into useSiteDiagnostics here would have been previously reported so that they are not missed?

In reply to: 285293979 [](ancestors = 285293979,285291041)

Opened #35851 to follow up

In reply to: 286216141 [](ancestors = 286216141,285293979,285291041)

gafter · 2019-05-17T21:20:43Z

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

-                       ((!this.IsMetadataNewSlot() && (object)_containingType.BaseTypeNoUseSiteDiagnostics != null) || this.IsExplicitClassOverride);
+            this._containingType.IsInterface ?
+                false :
+                (this.IsMetadataVirtual() && !this.IsDestructor &&


Clearer perhaps !_containingType.IsInterface && #Closed

gafter · 2019-05-17T21:26:18Z

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

+            else if ((mods & (DeclarationModifiers.Static | DeclarationModifiers.Private | DeclarationModifiers.Partial | DeclarationModifiers.Virtual | DeclarationModifiers.Abstract)) == 0)
+            {
+                Debug.Assert(!isExplicitInterfaceImplementation);
+


Given that this is in the else of an if (isExplicitInterfaceImplementation), the assert seems unnecessary. #Closed

Given that this is in the else of an if (isExplicitInterfaceImplementation), the assert seems unnecessary.

It is here for the benefit of the reader and to prevent unintentional break in the future.

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

gafter · 2019-05-17T21:28:41Z

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

+            else if (isInterface)
+            {
+                Debug.Assert(explicitInterfaceImplementation);
+                allowedModifiers |= DeclarationModifiers.Abstract;


This is the else part of an if (!explicitInterfaceImplementation), so this is a bit redundant. #Closed

This is the else part of an if (!explicitInterfaceImplementation), so this is a bit redundant.

It is here for the benefit of the reader and to prevent unintentional break in the future.

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

gafter · 2019-05-17T21:49:13Z

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

+                            throw ExceptionUtilities.UnexpectedValue(accessor.Kind());
+                    }
+
+                    if (checkBody && !IsAbstract && accessor.Body == null && accessor.ExpressionBody == null && accessor.SemicolonToken.Kind() == SyntaxKind.SemicolonToken)


accessor.SemicolonToken.Kind() == SyntaxKind.SemicolonToken [](start = 112, length = 59)

This seems to depend on how the parser handles error recovery in a fragile way. Why isn't the previous accessor.Body == null sufficient? #Closed

This seems to depend on how the parser handles error recovery in a fragile way. Why isn't the previous accessor.Body == null sufficient?

This duplicates exact conditions under which the parser used to reported this error before. I do not intend to report it under any new conditions. Different conditions would cause different errors reported in the parser. For example, if semicolon is missing, the parser would complain about missing '''{```.

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

AlekseyTs · 2019-05-21T16:44:33Z

@dotnet/roslyn-compiler, @gafter Please review #Closed

gafter · 2019-05-21T20:56:29Z

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

@@ -15,17 +15,64 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
    internal abstract class SourceEventAccessorSymbol : SourceMemberMethodSymbol
    {
        private readonly SourceEventSymbol _event;
+        private readonly string _name;
+        private readonly ImmutableArray<MethodSymbol> _explicitInterfaceImplementations;


ImmutableArray [](start = 25, length = 28)

This appears to be an ImmutableArray that is either empty (when there is no corresponding implemented event) or contains a single method (when there is a corresponding implemented event). If I am right, for this situation I would expect to see a field of type MethodSymbol that is either null or contains the implemented event's accessor. #Closed

Ah, I see this implements an API that handles imported symbols as well, which can implement multiple methods.

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

gafter

AlekseyTs · 2019-05-21T21:29:07Z

@dotnet/roslyn-compiler Please review, need a second sign-off #Closed

AlekseyTs · 2019-05-22T16:53:18Z

@dotnet/roslyn-compiler Please review, need a second sign-off #Closed

AlekseyTs · 2019-05-22T20:59:25Z

@dotnet/roslyn-compiler Please review, need a second sign-off #Closed

AlekseyTs · 2019-05-23T16:44:00Z

@dotnet/roslyn-compiler Please review, need a second sign-off #Closed

333fred · 2019-05-23T18:14:03Z

docs/features/DefaultInterfaceImplementation.md

+{
+}
+```
+In metadata, methods representing re-abstraction have all three flags `abstarct`, `virtual`, `sealed` set and do not have `newslot` flag set.


abstarct [](start = 71, length = 8)

abstract #Resolved

333fred · 2019-05-23T18:19:24Z

        foreach (var reference in new[] { compilation2.ToMetadataReference(), compilation2.EmitToImageReference() })

Not necessary to address in this PR, but considering the number of times we do this general pattern we should probably consider a helper on our compilation extensions to validate these scenarios.

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

333fred · 2019-05-23T18:21:38Z

        static void validate(ModuleSymbol m)

No calls to ValidateReabstraction here and in other following validate local functions? #Resolved

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

333fred · 2019-05-23T18:23:44Z

                // (2,15): error CS8705: Interface member 'I1.M1()' does not have a most specific implementation. Neither 'I2.I1.M1()', nor 'I3.I1.M1()' are most specific.

Interesting. I can't say I expected this error: neither I2 nor I3 have an implementation for I1.M1(). They both reabstract it. #Resolved

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

333fred · 2019-05-23T18:24:15Z

                // (2,15): error CS8705: Interface member 'I1.M1()' does not have a most specific implementation. Neither 'I2.I1.M1()', nor 'I3.I1.M1()' are most specific.

Is there any way to improve the message for this scenario or perhaps report a different error?

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

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

AlekseyTs · 2019-05-23T18:25:25Z

        static void validate(ModuleSymbol m)

No calls to ValidateReabstraction here and in other following validate local functions?

Prior tests already validated re-abstraction declaration. Tests validating various consumption scenarios don't need to do this over and over

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

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

AlekseyTs · 2019-05-23T18:28:22Z

                // (2,15): error CS8705: Interface member 'I1.M1()' does not have a most specific implementation. Neither 'I2.I1.M1()', nor 'I3.I1.M1()' are most specific.

Is there any way to improve the message for this scenario or perhaps report a different error?

It really doesn't matter if any of them is a re-abstraction. As long as there is more than one, it is an error. Nothing specific to re-abstraction

In reply to: 495331020 [](ancestors = 495331020,495330811)

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

AlekseyTs · 2019-05-23T18:39:26Z

                // (2,15): error CS8705: Interface member 'I1.M1()' does not have a most specific implementation. Neither 'I2.I1.M1()', nor 'I3.I1.M1()' are most specific.

Opened #35915 to follow up

In reply to: 495332460 [](ancestors = 495332460,495331020,495330811)

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

333fred · 2019-05-23T19:31:49Z

            // (9,22): error CS0501: 'C2.I1.M1()' must declare a body because it is not marked abstract, extern, or partial

This error is very unexpected imo. The method is marked abstract. The fact that it can't be has no bearing on that, that error was already reported. #WontFix

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

333fred · 2019-05-23T19:45:08Z

    get => throw null;

How are expression and block bodies meaningfully different in this scenario? #Resolved

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

AlekseyTs · 2019-05-23T19:47:56Z

            // (9,22): error CS0501: 'C2.I1.M1()' must declare a body because it is not marked abstract, extern, or partial

This error is very unexpected imo. The method is marked abstract. The fact that it can't be has no bearing on that, that error was already reported.

I believe there is no behavior change around this scenario. Feel free to open an issue.

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

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

AlekseyTs · 2019-05-23T19:50:44Z

    get => throw null;

How are expression and block bodies meaningfully different in this scenario?

They are likely not different, but it doesn't hurt to verify

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

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

333fred · 2019-05-23T20:00:09Z

int P1 {internal get => throw null; set => throw null;}

Consider adding some tests with the additional modifiers on I2, unless I just haven't gotten to them yet. #Resolved

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

AlekseyTs · 2019-05-23T20:15:39Z

int P1 {internal get => throw null; set => throw null;}

Consider adding some tests with the additional modifiers on I2, unless I just haven't gotten to them yet.

Since there was no change in the accessor's syntax and allowed modifiers on an accessor (which is none) for explicit interface implementation. I do not believe it is worth testing specifically in connection to DIM

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

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

AlekseyTs · 2019-05-23T20:22:12Z

int P1 {internal get => throw null; set => throw null;}

BTW, there is an existing test covering this for non-reabstracting scenario PropertyImplementationInDerived_18

In reply to: 495368645 [](ancestors = 495368645,495363704)

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

333fred

LGTM (commit 5)

AlekseyTs added the Area-Compilers label May 16, 2019

AlekseyTs requested review from a team as code owners May 16, 2019 04:50

AlekseyTs added 2 commits May 15, 2019 21:54

Support re-abstraction of interface members in derived interfaces

f06d878

Rename SynthesizedFieldLikeEventAccessorSymbol to SynthesizedEventAcc…

471afac

…essorSymbol

AlekseyTs force-pushed the ReAbstraction_01 branch from a69ce55 to 471afac Compare May 16, 2019 04:55

AlekseyTs mentioned this pull request May 16, 2019

Use of reabstraction of default interface methods causes System.TypeLoadException: VTable setup of type failed mono/mono#14495

Closed

Skip some failing tests on Mono.

63941df

jcouv added this to the 16.2.P2 milestone May 16, 2019

Update DefaultInterfaceImplementation.md

6eee43d

jcouv reviewed May 17, 2019

View reviewed changes

gafter reviewed May 17, 2019

View reviewed changes

PR feedback

2b8a937

gafter reviewed May 17, 2019

View reviewed changes

gafter reviewed May 21, 2019

View reviewed changes

gafter approved these changes May 21, 2019

View reviewed changes

333fred reviewed May 23, 2019

View reviewed changes

333fred approved these changes May 23, 2019

View reviewed changes

AlekseyTs merged commit 877853b into dotnet:master May 23, 2019

jcouv mentioned this pull request Jun 19, 2019

Support "reabstraction" in an interface #34591

Closed

		@@ -1059,7 +1059,16 @@ private static Symbol FindMostSpecificImplementation(Symbol interfaceMember, Nam
		ref useSiteDiagnostics,
		out var _, out var _);

Support re-abstraction of interface members in derived interfaces #35756

Support re-abstraction of interface members in derived interfaces #35756

Conversation

AlekseyTs commented May 16, 2019

AlekseyTs commented May 16, 2019 • edited Loading

jcouv commented May 16, 2019

AlekseyTs commented May 17, 2019 • edited Loading

jcouv commented May 17, 2019

jcouv May 17, 2019 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv May 17, 2019 • edited Loading

Choose a reason for hiding this comment

gafter May 17, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter May 17, 2019 • edited Loading

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

gafter May 17, 2019 • edited Loading

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

gafter May 17, 2019 • edited Loading

Choose a reason for hiding this comment

gafter May 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter May 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter May 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 21, 2019 • edited Loading

gafter May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

AlekseyTs commented May 21, 2019 • edited Loading

AlekseyTs commented May 22, 2019 • edited Loading

AlekseyTs commented May 22, 2019 • edited Loading

AlekseyTs commented May 23, 2019 • edited Loading

333fred May 23, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred commented May 23, 2019

333fred commented May 23, 2019 • edited by AlekseyTs Loading

333fred commented May 23, 2019 • edited by AlekseyTs Loading

333fred commented May 23, 2019

AlekseyTs commented May 23, 2019

AlekseyTs commented May 23, 2019

AlekseyTs commented May 23, 2019

333fred commented May 23, 2019 • edited by AlekseyTs Loading

333fred commented May 23, 2019 • edited by AlekseyTs Loading

AlekseyTs commented May 23, 2019

AlekseyTs commented May 23, 2019

333fred commented May 23, 2019 • edited by AlekseyTs Loading

AlekseyTs commented May 23, 2019

AlekseyTs commented May 23, 2019

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs commented May 16, 2019 •

edited

Loading

AlekseyTs commented May 17, 2019 •

edited

Loading

jcouv May 17, 2019 •

edited by gafter

Loading

jcouv May 17, 2019 •

edited

Loading

gafter May 17, 2019 •

edited by AlekseyTs

Loading

gafter May 17, 2019 •

edited

Loading

gafter May 17, 2019 •

edited

Loading

gafter May 17, 2019 •

edited

Loading

gafter May 17, 2019 •

edited

Loading

gafter May 17, 2019 •

edited

Loading

gafter May 17, 2019 •

edited

Loading

AlekseyTs commented May 21, 2019 •

edited

Loading

gafter May 21, 2019 •

edited

Loading

AlekseyTs commented May 21, 2019 •

edited

Loading

AlekseyTs commented May 22, 2019 •

edited

Loading

AlekseyTs commented May 22, 2019 •

edited

Loading

AlekseyTs commented May 23, 2019 •

edited

Loading

333fred May 23, 2019 •

edited by AlekseyTs

Loading

333fred commented May 23, 2019 •

edited by AlekseyTs

Loading

333fred commented May 23, 2019 •

edited by AlekseyTs

Loading

333fred commented May 23, 2019 •

edited by AlekseyTs

Loading

333fred commented May 23, 2019 •

edited by AlekseyTs

Loading

333fred commented May 23, 2019 •

edited by AlekseyTs

Loading