Enable a default implementation of an interface method to be provided as part of its declaration. by AlekseyTs · Pull Request #17927 · dotnet/roslyn

AlekseyTs · 2017-03-17T03:13:57Z

@dotnet/roslyn-compiler Please review.

… as part of its declaration.

AlekseyTs · 2017-03-17T16:45:32Z

@dotnet/roslyn-compiler Please review.

gafter · 2017-03-17T16:52:50Z

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

            }
        }

+        /// <summary>


runtime [](start = 26, length = 7)

Please clarify "target runtime"

Please clarify "target runtime"

Will do.

gafter · 2017-03-17T17:00:58Z

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

+                    implicitImpl = interfaceMember;
+                }
+            }
+


This is correct only for the simplest scenario. This doesn't take into account interface member overrides appearing in interfaces. Please add a PROTOTYPE comment.

Overrides aren't supported at all, there is no need to add prototype comment for such cases.

There is absolutely no risk to forget adjusting this place once the support is added, unless there would be no testing, which won't happen.

gafter · 2017-03-17T17:15:11Z

            if (!isInterface)

Per spec, private and static are now permitted on an interface method. Please add a .work. file, or a PROTOTYPE comment here.

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs:777 in 38176cb. [](commit_id = 38176cb, deletion_comment = False)

gafter · 2017-03-17T17:16:16Z

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

            if (containingTypeIsInterface)
            {
-                mods = (mods & ~DeclarationModifiers.AccessibilityMask) | DeclarationModifiers.Public | DeclarationModifiers.Abstract;
+                mods = (mods & ~DeclarationModifiers.AccessibilityMask) | DeclarationModifiers.Public | 


mods [](start = 16, length = 4)

This is no longer correct (a private interface method is not public or virtual, for example, and a static method is not virtual).

It is not a goal of this PR to enable private or static members in interfaces. No one is making a claim that the entire spec is implemented. Please focus on the subject of this PR.

I don't see either a PROTOTYPE or a .work. tracking this issue. Given that you are updating AddImpliedModifiers, you should probably do so in a way that complies with the feature spec. Or track what isn't done.

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

gafter · 2017-03-17T17:17:15Z

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

+                else
+                {
+                    diagnostics.Add(ErrorCode.ERR_InterfaceMemberHasBody, location, this);
+                }


This is not correct; an accessor is permitted to have a body.

As PR title clearly states, only methods are supported, other kinds of members are NYI. No one is making a claim that the entire spec is implemented. Please focus on the subject of this PR.

It might be helpful to add a quick note of what is supported and what is NYI. A small example might be enough. I was also under impression that the change enables more than literally stated in the subject.

The comment applies to the PR, not to a particular piece of code.

The comment applies to the PR, not to a particular piece of code.

I will make the current level of support clear in the feature markdown file.

gafter · 2017-03-17T17:23:08Z

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs

+                // (13,23): error CS0737: 'Derived' does not implement interface member 'I1.M2()'. 'Derived.M2()' cannot implement an interface member because it is not public.
+                // class Derived : Base, I1
+                Diagnostic(ErrorCode.ERR_CloseUnimplementedInterfaceMemberNotPublic, "I1").WithArguments("Derived", "I1.M2()", "Derived.M2()").WithLocation(13, 23),
+                // (13,23): error CS0737: 'Derived' does not implement interface member 'I1.M1()'. 'Base.M1()' cannot implement an interface member because it is not public.


This diagnostic does not appear to be correct. Derived does implement I1.M2() by inheriting the default implementation.

This behavior is in agreement with the decision made at the LDM. I specifically presented this scenario and the decision was that for now we continue reporting an error in this situation.

gafter · 2017-03-17T17:23:36Z

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs

+                Diagnostic(ErrorCode.ERR_CloseUnimplementedInterfaceMemberNotPublic, "I1").WithArguments("Derived", "I1.M2()", "Derived.M2()").WithLocation(13, 23),
+                // (13,23): error CS0737: 'Derived' does not implement interface member 'I1.M1()'. 'Base.M1()' cannot implement an interface member because it is not public.
+                // class Derived : Base, I1
+                Diagnostic(ErrorCode.ERR_CloseUnimplementedInterfaceMemberNotPublic, "I1").WithArguments("Derived", "I1.M1()", "Base.M1()").WithLocation(13, 23)


Diagnostic [](start = 16, length = 10)

Same here.

This behavior is in agreement with the decision made at the LDM. I specifically presented this scenario and the decision was that for now we continue reporting an error in this situation.

gafter · 2017-03-17T17:25:30Z

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs

+                // (7,15): error CS0736: 'Test1' does not implement interface member 'I1.M1()'. 'Test1.M1()' cannot implement an interface member because it is static.
+                // class Test1 : I1
+                Diagnostic(ErrorCode.ERR_CloseUnimplementedInterfaceMemberStatic, "I1").WithArguments("Test1", "I1.M1()", "Test1.M1()").WithLocation(7, 15)
+                );


The diagnostic is misleading at best, and possibly incorrect. The second sentence is true, but the first is not. It isn't clear that an error is required under these circumstances (though a warning might be helpful).

gafter · 2017-03-17T17:26:34Z

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs

+            compilation1.VerifyDiagnostics(
+                // (8,15): error CS8152: 'Test1' does not implement interface member 'I1.M2()'. 'Test1.M2()' cannot implement 'I1.M2()' because it does not return by value
+                // class Test1 : I1
+                Diagnostic(ErrorCode.ERR_CloseUnimplementedInterfaceMemberWrongRefReturn, "I1").WithArguments("Test1", "I1.M2()", "Test1.M2()", "value").WithLocation(8, 15),


Diagnostic [](start = 16, length = 10)

While I agree that there is an error here, the first sentence is misleading and should not be reported.

This behavior is in agreement with the decision made at the LDM. I specifically presented this scenario and the decision was that for now we continue reporting an error in this situation.

@AlekseyTs You do not appear to have read my comment. I agree that it is an error. What I said is that the specific diagnostic being reported is very misleading.

What I said is that the specific diagnostic being reported is very misleading.

To me, it doesn't look misleading or more misleading than in scenarios, in which it was reported before. I will capture your concern in the feature file.

This scenario should probably not be an error, by analogy with the following, which is not an error:

interface IA { int M(); } class Base: IA { int IA.M() => 1; } class Derived: Base, IA { static int x; public ref int M() => ref x; }

(I edited the above response)

In this case the base class explicitly states that it implements the interface, so it doesn't look similar to me. In any case, I've made a note to go over these scenarios at LDM again.

A method that is an explicit interface implementation "implements the interface" with respect to that method.

gafter · 2017-03-17T17:27:17Z

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs

+                Diagnostic(ErrorCode.ERR_CloseUnimplementedInterfaceMemberWrongRefReturn, "I1").WithArguments("Test1", "I1.M2()", "Test1.M2()", "value").WithLocation(8, 15),
+                // (8,15): error CS0738: 'Test1' does not implement interface member 'I1.M1()'. 'Test1.M1()' cannot implement 'I1.M1()' because it does not have the matching return type of 'void'.
+                // class Test1 : I1
+                Diagnostic(ErrorCode.ERR_CloseUnimplementedInterfaceMemberWrongReturnType, "I1").WithArguments("Test1", "I1.M1()", "Test1.M1()", "void").WithLocation(8, 15)


ERR_CloseUnimplementedInterfaceMemberWrongReturnType [](start = 37, length = 52)

Ditto.

This behavior is in agreement with the decision made at the LDM. I specifically presented this scenario and the decision was that for now we continue reporting an error in this situation.

@AlekseyTs You do not appear to have read my comment. I agree that it is an error. What I said is that the specific diagnostic being reported is very misleading.

What I said is that the specific diagnostic being reported is very misleading.

Doesn't look like this to me, but I will capture your concern in the feature file.

This should probably not be an error, by analogy with the following, which is accepted today:

interface IA { void M(); } class Base: IA { void IA.M() { } } class Derived: Base, IA { public int M() => 1; }

AlekseyTs · 2017-03-17T17:27:21Z

Per spec, private and static are now permitted on an interface method. Please add a .work. file, or a PROTOTYPE comment here.

No one is making a claim that the entire spec is implemented. Please focus on the subject of this PR. Private or static members are not supported in interfaces at all. There is no need to sprinkle code with prototype comments about that fact.

jcouv · 2017-03-17T17:28:17Z

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

+                    {
+                        // PROTOTYPE(DefaultInterfaceImplementation): Implement real detection if the feature is supported by the runtime
+                        // For now we assume that it is supported by default
+                        _lazyRuntimeSupportsDefaultInterfaceImplementation = ThreeState.True;


Just to confirm, we don't need Interlocked.CompareExchange for integer types?

That is correct.

Don't we want this write to be immediately visible to other threads?

Don't we want this write to be immediately visible to other threads?

In case of a very small chance of that not happening, the behavior is still going to be correct. This is a common pattern in both compilers around structures that are guaranteed to be written atomically.

"Immediately visible" is an inconvenient term. Memory writes are immediately visible, but that is not enough in many cases because of read/write ordering.

The important part here is consensus - all readers read the same value. We have that in this pattern.

Yes, per our memory model, write/read of an int is atomic. And since we deal with a single value assigned the same value, the ordering is irrelevant.

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs

jcouv · 2017-03-17T17:31:01Z

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

        ERR_InvalidPreprocessingSymbol = 8301,
        ERR_FeatureNotAvailableInVersion7_1 = 8302,
+
+        ERR_RuntimeDoesNotSupportDefaultInterfaceImplementation = 8401,


You may want to pick a different range (8500 probably) to avoid conflict with Omar/Vlad's branch on readonly refs.

You may want to pick a different range (8500 probably) to avoid conflict with Omar/Vlad's branch on readonly refs.

Thanks, will do.

Thanks. We have learned already that reserving a noncontiguous block of error ids while developing is good. We can "compress" the error id's later when merging back to master.

gafter · 2017-03-17T17:32:12Z

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs

+            compilation1.VerifyDiagnostics(
+                // (4,17): error CS0106: The modifier 'static' is not valid for this item
+                //     static void M1() 
+                Diagnostic(ErrorCode.ERR_BadMemberFlag, "M1").WithArguments("static").WithLocation(4, 17),


Diagnostic [](start = 16, length = 10)

This is not correct per the feature spec.

No one is making a claim that the entire spec is implemented. Please focus on the subject of this PR. Private or static members are not supported in interfaces at all. The behavior is correct given the current state of the product.

gafter · 2017-03-17T17:34:27Z

src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs

-        public async Task TestDoNotReorderComImportMembers()
+        public async Task TestDoNotReorderComImportMembers_01()
+        {
+            await TestInRegularAndScriptAsync(


await [](start = 12, length = 5)

Can you please explain the relationship of this test to the rest of the PR?

Can you please explain the relationship of this test to the rest of the PR?

There used to be one test for implementing an interface, I cloned it to cover scenarios when there is and there isn't default implementation for interface methods.

jcouv · 2017-03-17T17:40:14Z

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

-            {
-                interfaceLocation = implementingType.Locations[0];
-            }
+            Location interfaceLocation = GetInterfaceLocation(interfaceMember, implementingType);


The code here previously handled null implementingType. But now it asserts that is it non-null.
I looked at the one caller for the ReportImplicitImplementationMismatchDiagnostics, but didn't see how it guarantees non-null implementingType here.

The code here previously handled null implementingType.

You mean it did it by dereferencing it (look what the else block was doing)?

But now it asserts that is it non-null.
I looked at the one caller for the ReportImplicitImplementationMismatchDiagnostics, but didn't see how it guarantees non-null implementingType here.

The lookup is always performed in some type, that is the implementingType.

Ah. Makes sense.
Consider duplicating or moving the assert up into ReportImplicitImplementationMismatchDiagnostics or even its caller?

jcouv · 2017-03-17T17:41:29Z

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

+        {
+            Debug.Assert((object)implementingType != null);
+            var @interface = interfaceMember.ContainingType;
+            SourceMemberContainerTypeSymbol snt = implementingType as SourceMemberContainerTypeSymbol;


Nit: What's the n in this acronym? #Closed

Nit: What's the n in this acronym?

This was an extract method refactoring. I didn't change the line, or the names.

jcouv · 2017-03-17T17:47:19Z

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

        /// If it is not, a diagnostic is added with the current type.
        /// </summary>
-        protected void CheckModifiersForBody(Location location, DiagnosticBag diagnostics)
+        protected void CheckModifiersForBody(SyntaxNode syntax, Location location, DiagnosticBag diagnostics)


syntax [](start = 56, length = 6)

Nit: could you leave a comment or maybe rename the parameters to clarify what is syntax pointing out versus location?

Nit: could you leave a comment or maybe rename the parameters to clarify what is syntax pointing out versus location?

Will rename.

jaredpar · 2017-03-17T17:52:24Z

No one is making a claim that the entire spec is implemented. Please focus on the subject of this PR. Private or static members are not supported in interfaces at all.

I agree we need to keep our focus on the intent of the PR.

At the same time though feature branches should always be in a known state. That way absent minded managers can quickly check where a given feature stands. Once this PR is merged it's going to be hard to tell what the current state of the feature is by looking at the branch.

I think we should either add comments for the unimplemented state or clearly document the present state of the implementation in the feature markdown file / feature issue. Either is fine by me.

jcouv · 2017-03-17T17:52:59Z

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs

+            compilation1.VerifyDiagnostics();
+            Assert.True(m1.IsMetadataVirtual());
+
+            CompileAndVerify(compilation1, verify:false,  


It looks like PEVerify fails, so it is skipped for now. Should we file a follow-up issue? #Closed

It looks like PEVerify fails, so it is skipped for now. Should we file a follow-up issue?

Regular runtime doesn't support the feature. Once we have runtime we can test against, new tests will have to be written. I will make it clear in the feature markdown file. #Resolved

AlekseyTs · 2017-03-17T17:54:23Z

I think we should either add comments for the unimplemented state or clearly document the present state of the implementation in the feature markdown file / feature issue. Either is fine by me.

I will make it clear in the feature markdown file.

VSadov · 2017-03-17T18:07:38Z

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

+            if (implicitImpl.ContainingType.IsInterface && implementingType.ContainingModule != implicitImpl.ContainingModule)
+            {
+                // PROTOTYPE(DefaultInterfaceImplementation): Should we check language version as well?
+                // Usually, it is done based on specific syntax that targets a new feature, but in this case


I think we use language version in binding, in rare cases.

I think we use language version in binding, in rare cases.

That is true, but it still in response to specific syntax that wouldn't be valid for prior version of the language. In this case, there is no special syntax used.

jcouv

LGTM

gafter · 2017-03-17T19:05:24Z

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

            switch (feature)
            {
+                // C# 7.1 features.
+                case MessageID.IDS_DefaultInterfaceImplementation:


This is not correct; this feature is targeting 8.0 or later per LDM. Probably a PROTOTYPE comment is good enough.

Will make a note in the feature file.

AlekseyTs · 2017-03-17T19:56:49Z

@dotnet/roslyn-compiler I believe all feedback provided so far has been addressed. Please take a look.

gafter · 2017-03-17T22:43:02Z

I asked @MadsTorgersen to check on what his LDM notes and memory say about this situation:

interface IA
{
    void M() {}
}
class Derived: IA
{
    private void M() { }
}

Mads said that he does not recall it being discussed, and his LDM notes do not show it being discussed. However, he believes it should behave the same way that this behaves:

interface IA
{
    void M();
}
class Base: IA
{
    void IA.M() { }
}
class Derived: Base, IA
{
    private void M() { }
}

I agree with that: in either case Derived has a most specific implementation of IA.M, and so IA is fully implemented by Derived. The method Derived.M is simply not a candidate to implement IA.M and is unrelated to it.

This treatment is also desirable to minimize the break that could be introduced by adding a new default method to an existing interface.

If there is any controversy regarding this treatment we could bring it to the LDM for an explicit decision, but I don't see any need for that at this time.

gafter

Tests should be modified to pass if each interface method has a most specific implementation in each non-abstract type. This is true even if the type declares a method that isn't a candidate because it is static, private, has the wrong return type, ref return mismatch, etc.

AlekseyTs · 2017-03-18T02:11:00Z

The method Derived.M is simply not a candidate to implement IA.M and is unrelated to it.

This doesn't agree with my recollection of the LDM events. However, since I don't have a strong opinion about this behavior, I am going to adjust implementation to follow this logic for now and will bring the scenarios to LDM to get an official confirmation.

AlekseyTs · 2017-03-18T04:22:24Z

@gafter I believe all your feedback has been addressed. Please take a look.

agocke

LGTM although I'd like us to record our memory consistency decisions in a more concrete manner

VSadov

LGTM

gafter

LGTM

Enable a default implementation of an interface method to be provided…

38176cb

… as part of its declaration.

dnfclas added the cla-already-signed label Mar 17, 2017

AlekseyTs added Area-Compilers Feature - Default Interface Impl Default Interface Implementation labels Mar 17, 2017

gafter reviewed Mar 17, 2017

View reviewed changes

jcouv reviewed Mar 17, 2017

View reviewed changes

gafter reviewed Mar 17, 2017

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs Show resolved Hide resolved

jcouv reviewed Mar 17, 2017

View reviewed changes

gafter reviewed Mar 17, 2017

View reviewed changes

jcouv reviewed Mar 17, 2017

View reviewed changes

VSadov reviewed Mar 17, 2017

View reviewed changes

jcouv approved these changes Mar 17, 2017

View reviewed changes

gafter reviewed Mar 17, 2017

View reviewed changes

Address revie feedback.

c1b9eae

gafter suggested changes Mar 17, 2017

View reviewed changes

More PR feedback and additional tests.

2c6aa17

More tests.

6d1ee2a

agocke reviewed Mar 19, 2017

View reviewed changes

agocke approved these changes Mar 19, 2017

View reviewed changes

VSadov approved these changes Mar 19, 2017

View reviewed changes

AlekseyTs merged commit 50210ff into dotnet:features/DefaultInterfaceImplementation Mar 20, 2017

gafter reviewed Mar 20, 2017

View reviewed changes

jcouv mentioned this pull request May 12, 2022

Unify nint and IntPtr #60913

Merged

jasonmalinowski mentioned this pull request Sep 26, 2022

Disallow methods with bodies in ComImport interfaces. #64257

Merged

Conversation

AlekseyTs commented Mar 17, 2017

Uh oh!

AlekseyTs commented Mar 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter commented Mar 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Mar 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AlekseyTs Mar 17, 2017 •

edited

Loading

gafter Mar 17, 2017 •

edited

Loading

agocke Mar 18, 2017 •

edited

Loading

VSadov Mar 19, 2017 •

edited

Loading