Add support for implementations of interface static abstract methods in classes and structures #52969

AlekseyTs · 2021-04-27T18:25:53Z

In metadata an implementation should be a static, not newslot, not final, not virtual, not abstract method. Type should have a MethodImpl entry pointing to the ”body” method with a MethodDef index. The “body” must be a method declared within the type. There is no concept of an implicit interface implementation for static methods in metadata. I.e., if there is no corresponding MethodImpl entry, the method isn’t an implementation from runtime point of view.

Language supports both implicit and explicit interface implementation.

Relevant ECMA-335 changes PR - dotnet/runtime#49558

…in classes and structures In metadata an implementation should be a static, not newslot, not final, not virtual, not abstract method. Type should have a MethodImpl entry pointing to the ”body” method with a MethodDef index. The “body” must be a method declared within the type. There is no concept of an implicit interface implementation for static methods in metadata. I.e., if there is no corresponding MethodImpl entry, the method isn’t an implementation from runtime point of view. Language supports both implicit and explicit interface implementation.

RikkiGibson

Looks good. Had some questions, some doc change suggestions, some diagnostic wording suggestions, and some minor test suggestions.

src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs

RikkiGibson · 2021-04-27T22:29:13Z

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

@@ -1553,22 +1574,23 @@ private void CheckInterfaceUnification(BindingDiagnosticBag diagnostics)
        /// <param name="implementingMemberAndDiagnostics">Returned from FindImplementationForInterfaceMemberWithDiagnostics.</param>
        /// <param name="interfaceMember">The interface method or property that is being implemented.</param>
        /// <returns>Synthesized implementation or null if not needed.</returns>
-        private SynthesizedExplicitImplementationForwardingMethod SynthesizeInterfaceMemberImplementation(SymbolAndDiagnostics implementingMemberAndDiagnostics, Symbol interfaceMember)
+        private (SynthesizedExplicitImplementationForwardingMethod ForwardingMethod, (MethodSymbol Body, MethodSymbol Implemented) MethodImpl)


It would be helpful to have nullable annotations on this signature, and if it's not too much trouble to #nullable enable the implementation as well. #Resolved

RikkiGibson · 2021-04-27T22:29:37Z

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

@@ -1553,22 +1574,23 @@ private void CheckInterfaceUnification(BindingDiagnosticBag diagnostics)
        /// <param name="implementingMemberAndDiagnostics">Returned from FindImplementationForInterfaceMemberWithDiagnostics.</param>
        /// <param name="interfaceMember">The interface method or property that is being implemented.</param>
        /// <returns>Synthesized implementation or null if not needed.</returns>


Should this <returns> comment be updated? #Resolved

RikkiGibson · 2021-04-27T22:34:47Z

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

@@ -60,7 +60,7 @@ internal abstract class SourceOrdinaryMethodSymbolBase : SourceMemberMethodSymbo
            DeclarationModifiers declarationModifiers;
            (declarationModifiers, HasExplicitAccessModifier) = this.MakeModifiers(methodKind, isPartial, hasBody, location, diagnostics);

-            var isMetadataVirtualIgnoringModifiers = methodKind == MethodKind.ExplicitInterfaceImplementation; //explicit impls must be marked metadata virtual
+            var isMetadataVirtualIgnoringModifiers = methodKind == MethodKind.ExplicitInterfaceImplementation && (declarationModifiers & DeclarationModifiers.Static) == 0; //explicit impls must be marked metadata virtual unless static


Consider moving the trailing comment to above this line #Resolved

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

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

RikkiGibson · 2021-04-28T00:47:34Z

Incidentally, I found that reviewing many of the tests went easier for me when I rotated my monitor into portrait orientation and I could see the whole test without needing to scroll.

AlekseyTs · 2021-04-28T16:55:53Z

@333fred, @dotnet/roslyn-compiler For a second review

AlekseyTs · 2021-04-29T13:27:28Z

@333fred, @dotnet/roslyn-compiler For a second review

333fred · 2021-04-29T23:08:06Z

src/Compilers/Core/Portable/PEWriter/Members.cs

            switch (member.Visibility)
            {
                case TypeMemberVisibility.Private:
-                    return context.IncludePrivateMembers;
+                    acceptBasedOnVisibility = context.IncludePrivateMembers;


context.IncludePrivateMembers

When would this ever be anything but false? We already did a shortcut out at the beginning of this method if this was true. It seems like the only thing we need to check is IVT for Assembly and FamilyAndAssembly members. #Resolved

When would this ever be anything but false? We already did a shortcut out at the beginning of this method if this was true. It seems like the only thing we need to check is IVT for Assembly and FamilyAndAssembly members.

It looks like you are correct, but I prefer not changing the logic here.

333fred · 2021-04-30T00:28:02Z

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

+                // (27,20): error CS0539: 'I6.M01()' in explicit interface declaration is not found among members of the interface that can be implemented
+                //     static void I1.M01() {}
+                Diagnostic(ErrorCode.ERR_InterfaceMemberNotFound, "M01").WithArguments("I6.M01()").WithLocation(27, 20),
+                // (32,26): warning CS0108: 'I7.M01()' hides inherited member 'I1.M01()'. Use the new keyword if hiding was intended.


If this isn't marked as newslot, how does the runtime know this hides the base version? Consider also verifying the newslot behavior of this method. #Resolved

This is probably me just not understanding part of the spec, I guess: I would have expected that the runtime would be updated so newslot could apply to static methods, so that a future world where you can say static abstract new void M1(); would someday exist.

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

If this isn't marked as newslot, how does the runtime know this hides the base version?

This is how static methods work, this is part of runtime specification. New slot is set to specify that a virtual method is not an override, static methods never override anything.

Consider also verifying the newslot behavior of this method.

DefineAbstractStaticMethod_01 already covers that.

so that a future world where you can say static abstract new void M1(); would someday exist.

ie, something like this:

interface I1 { static abstract void M1(); } interface I1 : I2 { static void I1.M1() {} static abstract new void M1(); }

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

so that a future world where you can say static abstract new void M1(); would someday exist.

This future is already here. It exists. This is a warning, not an error.

333fred · 2021-04-30T00:58:05Z

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

+                    Assert.Equal("void modopt(I1) C1.I1.M01()", c1M01.ToTestDisplayString());
+                    Assert.Same(m01, c1M01.ExplicitInterfaceImplementations.Single());
+
+                    c1M01 = module.GlobalNamespace.GetMember<MethodSymbol>("C1.M01");


c1M01

Consider asserting the display for this method to show the modopt difference. #Resolved

333fred

LGTM (commit 1). A minor test comment that isn't blocking.

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

AlekseyTs added Area-Compilers Feature - StaticAbstractMembersInInterfaces labels Apr 27, 2021

AlekseyTs requested review from 333fred and RikkiGibson April 27, 2021 18:25

AlekseyTs requested a review from a team as a code owner April 27, 2021 18:25

RikkiGibson self-assigned this Apr 27, 2021

RikkiGibson reviewed Apr 28, 2021

View reviewed changes

RikkiGibson approved these changes Apr 28, 2021

View reviewed changes

333fred reviewed Apr 29, 2021

View reviewed changes

333fred reviewed Apr 30, 2021

View reviewed changes

333fred approved these changes Apr 30, 2021

View reviewed changes

PR feedback

a4f865f

AlekseyTs commented Apr 30, 2021

View reviewed changes

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs Outdated Show resolved Hide resolved

Style fixup

d03e9b3

AlekseyTs enabled auto-merge (squash) April 30, 2021 16:04

AlekseyTs merged commit 2de7aa4 into dotnet:features/StaticAbstractMembersInInterfaces Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for implementations of interface static abstract methods in classes and structures #52969

Add support for implementations of interface static abstract methods in classes and structures #52969

AlekseyTs commented Apr 27, 2021 •

edited

Loading

RikkiGibson left a comment

RikkiGibson Apr 27, 2021 •

edited by AlekseyTs

Loading

RikkiGibson Apr 27, 2021 •

edited by AlekseyTs

Loading

RikkiGibson Apr 27, 2021 •

edited by AlekseyTs

Loading

RikkiGibson commented Apr 28, 2021

AlekseyTs commented Apr 28, 2021

AlekseyTs commented Apr 29, 2021

333fred Apr 29, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Apr 29, 2021

333fred Apr 30, 2021 •

edited by AlekseyTs

Loading

333fred Apr 30, 2021

AlekseyTs Apr 30, 2021

333fred Apr 30, 2021

AlekseyTs Apr 30, 2021

333fred Apr 30, 2021 •

edited by AlekseyTs

Loading

333fred left a comment

Add support for implementations of interface static abstract methods in classes and structures #52969

Add support for implementations of interface static abstract methods in classes and structures #52969

Conversation

AlekseyTs commented Apr 27, 2021 • edited Loading

RikkiGibson left a comment

Choose a reason for hiding this comment

RikkiGibson Apr 27, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

RikkiGibson Apr 27, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

RikkiGibson Apr 27, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

RikkiGibson commented Apr 28, 2021

AlekseyTs commented Apr 28, 2021

AlekseyTs commented Apr 29, 2021

333fred Apr 29, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Apr 29, 2021

Choose a reason for hiding this comment

333fred Apr 30, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred Apr 30, 2021

Choose a reason for hiding this comment

AlekseyTs Apr 30, 2021

Choose a reason for hiding this comment

333fred Apr 30, 2021

Choose a reason for hiding this comment

AlekseyTs Apr 30, 2021

Choose a reason for hiding this comment

333fred Apr 30, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 27, 2021 •

edited

Loading

RikkiGibson Apr 27, 2021 •

edited by AlekseyTs

Loading

RikkiGibson Apr 27, 2021 •

edited by AlekseyTs

Loading

RikkiGibson Apr 27, 2021 •

edited by AlekseyTs

Loading

333fred Apr 29, 2021 •

edited by AlekseyTs

Loading

333fred Apr 30, 2021 •

edited by AlekseyTs

Loading

333fred Apr 30, 2021 •

edited by AlekseyTs

Loading