Allow `abstract` and `sealed` modifiers on static methods/properties/events in interfaces. by AlekseyTs · Pull Request #52228 · dotnet/roslyn

AlekseyTs · 2021-03-29T21:31:27Z

Spec: https://github.com/dotnet/csharplang/blob/9a0dddbf0643993db0da8f48436f2aac60bc20b1/proposals/statics-in-interfaces.md

Related to #52202.

Relevant quotes from the spec:

Abstract virtual members

Static interface members other than fields are allowed to also have the abstract modifier. Abstract static members are not allowed to have a body (or in the case of properties, the accessors are not allowed to have a body).

interface I<T> where T : I<T>
{
    static abstract void M();
    static abstract T P { get; set; }
    static abstract event Action E;
    static abstract T operator +(T l, T r);
}

Explicitly non-virtual static members

For symmetry with non-virtual instance members, static members should be allowed an optional sealed modifier, even though they are non-virtual by default:

interface I0
{
    static sealed void M() => Console.WriteLine("Default behavior");
    
    static sealed int f = 0;
    
    static sealed int P1 { get; set; }
    static sealed int P2 { get => f; set => f = value; }
    
    static sealed event Action E1;
    static sealed event Action E2 { add => E1 += value; remove => E1 -= value; }
    
    static sealed I0 operator +(I0 l, I0 r) => l;
}

…events in interafces. Related to dotnet#52202.

AlekseyTs · 2021-03-30T14:02:02Z

@333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review.

333fred · 2021-03-30T17:10:20Z

        {
-            var defaultAccess = DeclarationModifiers.Private;
+            // PROTOTYPE(StaticAbstractMembersInInterfaces): Do we need to check the lack of access modifier in an interface against language version?
+            //                                               See https://github.com/dotnet/roslyn/issues/52202


IMO I agree with Jared on the issue, this feels like a bug fix. #Closed

333fred · 2021-03-30T17:16:32Z

+                        var requiredVersionArgument = new CSharpRequiredLanguageVersion(requiredVersion);
+                        var availableVersionArgument = availableVersion.ToDisplayString();
+
+                        reportModifier(result, DeclarationModifiers.Abstract, location, diagnostics, requiredVersionArgument, availableVersionArgument);


Nit: this reads a bit awkwardly. I think it would be more clear if you just checked which of the flags to report up front, rather than appearing to report both modifiers (until the reader goes and reads the body of reportModifier and pieces it together with the previous logic that ensures only one will be reported). #WontFix

I think it would be more clear if you just checked which of the flags to report up front, rather than appearing to report both modifiers (until the reader goes and reads the body of reportModifier and pieces it together with the previous logic that ensures only one will be reported).

It sounds like you are suggesting to move the bit test out from the shared helper to here. I do not think this is the right thing to do. Also, I believe it is pretty clear that we can get here in a situation when only one of the bits is set (that is even ignoring the code that conditionally clears the DeclarationModifiers.Sealed bit above). So, it shoudl be obvious that this code doesn't report both modifiers all the time.

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

333fred · 2021-03-30T17:26:34Z

        Assert.False(m03.IsSealed);

This feels odd to me, and somewhat goes back to the same argument about IsStatic on local functions. We eventually made static local functions return true for IsStatic, but this feels even more different because sealed implies that it was virtual, but is no longer allowed to be overridden, whereas this method really isn't virtual to begin with. To me, this raises a question of whether we should even allow sealed in this location regardless of language version, as we don't allow it for non-virtual instance members either (in reference types). Can you take a note that it should be asked in LDM? #Resolved

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/StaticAbstractMembersInInterfacesTests.cs:171 in 1cd7bb4. [](commit_id = 1cd7bb4, deletion_comment = False)

333fred · 2021-03-30T17:30:56Z

            // (19,33): error CS0106: The modifier 'sealed' is not valid for this item

This error feels a bit wrong. The equivalent instance method error is 'symbol' cannot be sealed because it is not an override. By the current spec, this modifier is valid for the item, just not in conjunction with abstract, and also not without a body. I think we'd benefit from a more specific error. #WontFix

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/StaticAbstractMembersInInterfacesTests.cs:95 in 1cd7bb4. [](commit_id = 1cd7bb4, deletion_comment = False)

333fred · 2021-03-30T18:02:26Z

        m01 = m01.PartialImplementationPart;

I think it would be useful to validate that ReferenceEquals(m01.PartialImplementationPart.PartialDefinitionPart, m01). #Closed

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/StaticAbstractMembersInInterfacesTests.cs:1079 in 1cd7bb4. [](commit_id = 1cd7bb4, deletion_comment = False)

333fred · 2021-03-30T18:25:52Z

Done review pass (commit 1). A couple of minor comments.

AlekseyTs · 2021-03-30T19:04:27Z

        Assert.False(m03.IsSealed);

This feels odd to me, and somewhat goes back to the same argument about IsStatic on local functions. We eventually made static local functions return true for IsStatic, but this feels even more different because sealed implies that it was virtual, but is no longer allowed to be overridden, whereas this method really isn't virtual to begin with. To me, this raises a question of whether we should even allow sealed in this location regardless of language version, as we don't allow it for non-virtual instance members either (in reference types). Can you take a note that it should be asked in LDM?

I am not sure I am following. The behavior is consistent with instance methods today (the method is not virtual):

interface I1
{
    sealed void M() {}
}

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

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/StaticAbstractMembersInInterfacesTests.cs:171 in 1cd7bb4. [](commit_id = 1cd7bb4, deletion_comment = False)

AlekseyTs · 2021-03-30T19:10:16Z

            // (19,33): error CS0106: The modifier 'sealed' is not valid for this item

I think we'd benefit from a more specific error.

I believe the error is sufficiently good, it is intuitively clear that removing the modifier is the way out. If we get feedback from real users, we can improve it in the future.

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

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/StaticAbstractMembersInInterfacesTests.cs:95 in 1cd7bb4. [](commit_id = 1cd7bb4, deletion_comment = False)

333fred · 2021-03-30T19:33:42Z

        Assert.False(m03.IsSealed);

The behavior is consistent with instance methods today (the method is not virtual):

Well, M() is virtual by default, and (I'm expecting) will return true for this check. A static method is not virtual by default, and won't return true for this call regardless of whether it's legal or not (as in MethodModifiers_04). There is certainly a question of whether we should be consistent with classes or interface types here, and while I originally thought we should have consistency with interface instance methods here, I feel that, after looking at the consequence, I change my mind.

In reply to: 810504598 [](ancestors = 810504598,810441501)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/StaticAbstractMembersInInterfacesTests.cs:171 in 1cd7bb4. [](commit_id = 1cd7bb4, deletion_comment = False)

333fred

LGTM (commit 2)

AlekseyTs · 2021-03-31T15:15:18Z

@RikkiGibson, @dotnet/roslyn-compiler Please review, need a second sign-off.

AlekseyTs · 2021-04-01T14:14:15Z

@RikkiGibson, @dotnet/roslyn-compiler Please review, need a second sign-off.

RikkiGibson · 2021-04-01T23:52:27Z

+        {
+            var source1 =
+@"
+class C1


Was this meant to be a struct? #Resolved

Was this meant to be a struct?

Good catch, I'll adjust this in th enext PR.

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

Allow abstract and sealed modifiers on static methods/properties/…

1cd7bb4

…events in interafces. Related to dotnet#52202.

AlekseyTs added the Area-Compilers label Mar 29, 2021

AlekseyTs requested review from a team, 333fred and RikkiGibson March 29, 2021 21:31

333fred reviewed Mar 30, 2021

View reviewed changes

PR feedback

6f36030

333fred approved these changes Mar 30, 2021

View reviewed changes

RikkiGibson self-assigned this Apr 1, 2021

RikkiGibson reviewed Apr 1, 2021

View reviewed changes

RikkiGibson approved these changes Apr 1, 2021

View reviewed changes

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

jmarolf mentioned this pull request Feb 7, 2022

Add repo files to Roslyn.sln and Compilers.sln as solution items #59328

Closed

Conversation

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

Uh oh!

Abstract virtual members

Explicitly non-virtual static members

Uh oh!

AlekseyTs commented Mar 30, 2021

Uh oh!

333fred Mar 30, 2021 • 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!

333fred Mar 30, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 30, 2021

Choose a reason for hiding this comment

Uh oh!

333fred commented Mar 30, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Mar 30, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Mar 30, 2021

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs commented Mar 30, 2021

Uh oh!

333fred commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Mar 31, 2021

Uh oh!

AlekseyTs commented Apr 1, 2021

Uh oh!

RikkiGibson Apr 1, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 2, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AlekseyTs commented Mar 29, 2021 •

edited

Loading

333fred Mar 30, 2021 •

edited

Loading

333fred Mar 30, 2021 •

edited by AlekseyTs

Loading

333fred commented Mar 30, 2021 •

edited by AlekseyTs

Loading

333fred commented Mar 30, 2021 •

edited by AlekseyTs

Loading

333fred commented Mar 30, 2021 •

edited

Loading

AlekseyTs commented Mar 30, 2021 •

edited

Loading

333fred commented Mar 30, 2021 •

edited

Loading

RikkiGibson Apr 1, 2021 •

edited by AlekseyTs

Loading