Enable declaration, implementation(in non-interfaces) and consumption of virtual static members. by AlekseyTs · Pull Request #60965 · dotnet/roslyn

AlekseyTs · 2022-04-26T20:28:29Z

https://github.com/dotnet/csharplang/blob/main/proposals/static-abstracts-in-interfaces.md

Test plan: #60968

… of virtual static members. https://github.com/dotnet/csharplang/blob/main/proposals/static-abstracts-in-interfaces.md

333fred

Reviewed everything except StaticAbstractMembersInInterfacesTests.cs. Will get to that tomorrow.

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

…terfaceImplementation_102

…ntation' into DefaultInterfaceImplementation_102

333fred · 2022-04-29T17:21:14Z

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

@@ -73,9 +73,9 @@ sealed override static void M10()
                // (4,26): error CS8703: The modifier 'abstract' is not valid for this item in C# 9.0. Please use language version 'preview' or greater.
                //     abstract static void M01()
                Diagnostic(ErrorCode.ERR_InvalidModifierForLanguageVersion, "M01").WithArguments("abstract", "9.0", "preview").WithLocation(4, 26),
-                // (7,25): error CS0112: A static member cannot be marked as 'virtual'
+                // (7,25): error CS8703: The modifier 'virtual' is not valid for this item in C# 9.0. Please use language version 'preview' or greater.


Should we update this and other tests that use C# 9 to 10? #Resolved

Should we update this and other tests that use C# 9 to 10?

Yes, I am planning to do this in a separate PR to reduce the noise.

333fred

LGTM (commit 9). Future reviews, I recommend turning off whitespace to avoid a decent chunk of changes in some of the test files.

AlekseyTs · 2022-04-29T18:53:27Z

@jcouv, @dotnet/roslyn-compiler For the second review.

In reply to: 1113626786

AlekseyTs · 2022-05-02T13:38:49Z

@dotnet/roslyn-compiler For the second review.

In reply to: 1114895031

cston · 2022-05-02T23:54:10Z

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

+                                        MessageID.IDS_FeatureStaticAbstractMembersInInterfaces.Localize(),
+                                        availableVersion.GetValueOrDefault().ToDisplayString(),
+                                        new CSharpRequiredLanguageVersion(requiredVersion));
+                    }


Please consider extracting this to a local function that can be reused for IDS_DefaultInterfaceImplementation above. #Resolved

Please consider extracting this to a local function that can be reused for IDS_DefaultInterfaceImplementation above.

I will follow up on this suggestion separately.

cston · 2022-05-03T00:41:43Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs

@@ -512,7 +512,8 @@ private BoundExpression MakeConversionNodeCore(
                        var mg = (BoundMethodGroup)rewrittenOperand;
                        Debug.Assert(oldNodeOpt.SymbolOpt is { });


oldNodeOpt.SymbolOpt

Consider extracting a local. #Resolved

Consider extracting a local.

I will follow up on this suggestion separately.

cston · 2022-05-03T23:55:33Z

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

                        var availableVersionArgument = availableVersion.ToDisplayString();

-                        reportModifierIfPresent(result, DeclarationModifiers.Abstract, location, diagnostics, requiredVersionArgument, availableVersionArgument);
+                        if ((result & DeclarationModifiers.Abstract) != 0)


if ((result & DeclarationModifiers.Abstract) != 0)

Why check this rather than calling reportModifierIfPresent() unconditionally, for DeclarationModifiers.Abstract and DeclarationModifiers.Sealed? #Resolved

Why check this rather than calling

Suppresses extra diagnostics for "abstract virtual" case

I think there is a dedicated error reported for this case elsewhere.

cston · 2022-05-04T03:52:15Z

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

Why is Test1 included in the compilation with Test2? Does that affect the test? (Is Test2 a copy of Test1 that is used for compiling in a separate compilation from the interface declaration?)

In reply to: 1116901341

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

AlekseyTs · 2022-05-04T13:17:37Z

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

Why is Test1 included in the compilation with Test2?

source2 includes only Test2, it doesn't include Test1. Test1 is defined in a referenced assembly.

Does that affect the test?
Presence of Test1 in the referenced assembly doesn't affect any validation around compilation2. As presence of hundreds of other types in other referenced assemblies.

Is Test2 a copy of Test1 that is used for compiling in a separate compilation from the interface declaration?

Yes, I think the only difference is the name of the type.

In reply to: 1116901341

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

cston · 2022-05-04T19:21:20Z

            // (50,22): error CS0539: 'C8.op_UnaryPlus(C8)' in explicit interface declaration is not found among members of the interface that can be implemented

Why is the C8 implementation invalid?

In reply to: 1117727936

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

AlekseyTs · 2022-05-04T19:29:04Z

            // (50,22): error CS0539: 'C8.op_UnaryPlus(C8)' in explicit interface declaration is not found among members of the interface that can be implemented

Why is the C8 implementation invalid?

It is not using operator syntax, it is trying to refer to the operator as to a regular method by name.

In reply to: 1117727936

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

cston · 2022-05-04T20:02:09Z

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

Why is static not valid for the implementation?

In reply to: 1117832578

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

cston · 2022-05-04T20:17:32Z

            // (11,37): warning CS0473: Explicit interface implementation 'Other.Interface<int, int>.Method(int)' matches more than one interface member. Which interface member is actually chosen is implementation-dependent. Consider using a non-explicit implementation instead.

Why a warning for new scenarios?

In reply to: 1117853635

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

cston · 2022-05-04T20:28:20Z

src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/StaticAbstractMembersInInterfacesTests.vb

+BC30275: 'Overridable' is not valid on an interface event declaration.
+    Overridable Shared Event E1 As System.Action
+    ~~~~~~~~~~~
+</errors>


errors

Please also test with Shared only, not Overridable. #Resolved

Please also test with Shared only, not Overridable.

Isn't this the previous test?

AlekseyTs · 2022-05-04T21:17:55Z

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

Why is static not valid for the implementation?

Because implementations inside interfaces are out of scope of this PR. The next PR is focused on this.

In reply to: 1117832578

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

AlekseyTs · 2022-05-04T21:23:28Z

            // (11,37): warning CS0473: Explicit interface implementation 'Other.Interface<int, int>.Method(int)' matches more than one interface member. Which interface member is actually chosen is implementation-dependent. Consider using a non-explicit implementation instead.

Why a warning for new scenarios?

Are you asking why we report it or why it is a warning?
The behavior matches existing behavior for instance methods and for static abstract methods (see tests above).

In reply to: 1117853635

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

cston · 2022-05-04T21:36:10Z

Are you asking why we report it or why it is a warning?

In reply to: 1117853635

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

I meant: why not report an error rather than a warning?

AlekseyTs · 2022-05-04T21:47:41Z

I meant: why not report an error rather than a warning?

That is a preexisting condition/behavior. We do the same for instance methods.

In reply to: 1117959391

Enable declaration, implementation(in non-interfaces) and consumption…

04470bf

… of virtual static members. https://github.com/dotnet/csharplang/blob/main/proposals/static-abstracts-in-interfaces.md

AlekseyTs added Area-Compilers Feature - Default Interface Impl Default Interface Implementation Feature - StaticAbstractMembersInInterfaces labels Apr 26, 2022

AlekseyTs requested review from 333fred and jcouv April 26, 2022 20:28

AlekseyTs requested review from a team as code owners April 26, 2022 20:28

AlekseyTs added 4 commits April 26, 2022 14:14

Fix formatting

359e290

Fixup

be05645

More tests

427c3f0

Disable possibly flaky test

eae5341

333fred reviewed Apr 28, 2022

View reviewed changes

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

AlekseyTs added 3 commits April 29, 2022 05:25

Merge branch 'features/DefaultInterfaceImplementation' into DefaultIn…

e3d16ec

…terfaceImplementation_102

More tests

17cb538

Rename

2a40e39

AlekseyTs requested review from a team and JoeRobich as code owners April 29, 2022 14:10

AlekseyTs requested a review from a team April 29, 2022 14:10

Merge remote-tracking branch 'dotnet/features/DefaultInterfaceImpleme…

a28883d

…ntation' into DefaultInterfaceImplementation_102

333fred reviewed Apr 29, 2022

View reviewed changes

333fred approved these changes Apr 29, 2022

View reviewed changes

cston reviewed May 2, 2022

View reviewed changes

cston reviewed May 3, 2022

View reviewed changes

cston reviewed May 4, 2022

View reviewed changes

cston approved these changes May 4, 2022

View reviewed changes

AlekseyTs merged commit a5f11b5 into dotnet:features/DefaultInterfaceImplementation May 4, 2022

jcouv mentioned this pull request Jun 1, 2022

Revise FindImplementationForInterfaceMember for abstract statics #60704

Closed

		@@ -512,7 +512,8 @@ private BoundExpression MakeConversionNodeCore(
		var mg = (BoundMethodGroup)rewrittenOperand;
		Debug.Assert(oldNodeOpt.SymbolOpt is { });

Conversation

AlekseyTs commented Apr 26, 2022 • 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!

Uh oh!

333fred Apr 29, 2022 • 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 29, 2022

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston May 2, 2022 • 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 May 4, 2022

Choose a reason for hiding this comment

Uh oh!

cston May 3, 2022 • 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 May 4, 2022

Choose a reason for hiding this comment

Uh oh!

cston May 3, 2022 • 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 May 3, 2022

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 3, 2022

Choose a reason for hiding this comment

Uh oh!

cston commented May 4, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented May 4, 2022

Uh oh!

cston commented May 4, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented May 4, 2022

Uh oh!

cston commented May 4, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented May 4, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston May 4, 2022 • 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 May 4, 2022

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented May 4, 2022

Uh oh!

cston commented May 4, 2022

Uh oh!

AlekseyTs commented May 4, 2022

Uh oh!

AlekseyTs commented Apr 26, 2022 •

edited

Loading

333fred Apr 29, 2022 •

edited by AlekseyTs

Loading

AlekseyTs commented Apr 29, 2022 •

edited

Loading

AlekseyTs commented May 2, 2022 •

edited

Loading

cston May 2, 2022 •

edited by AlekseyTs

Loading

cston May 3, 2022 •

edited by AlekseyTs

Loading

cston May 3, 2022 •

edited by AlekseyTs

Loading

cston commented May 4, 2022 •

edited by AlekseyTs

Loading

cston commented May 4, 2022 •

edited by AlekseyTs

Loading

cston commented May 4, 2022 •

edited by AlekseyTs

Loading

cston commented May 4, 2022 •

edited by AlekseyTs

Loading

cston May 4, 2022 •

edited by AlekseyTs

Loading

AlekseyTs commented May 4, 2022 •

edited

Loading