Make partial interface members virtual #77379

jjonescz · 2025-02-28T16:34:05Z

The classic (not extended) partial methods are always private (hence also not virtual), and that is also true in interfaces. However, the extended partial methods feature allows partial methods to be public and virtual, but in interfaces, they are not implicitly public nor virtual unlike their non-partial counterparts which I think was just an oversight (all speclet wording I could find in the mentioned features supports that). Since partial properties and events are based on extended partial methods, they have the same bug. This PR fixes all that.

RikkiGibson

Impl and testing seems good, we should just seek confirmation that we want to do this as a bug-fix level change, and figure out if we need to update the breaking change doc.

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

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

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

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md

AlekseyTs · 2025-03-05T15:41:57Z

we should just seek confirmation that we want to do this as a bug-fix level change

It is unfortunate that we have this behavior with already shipped features. Changing virtual-ness of interface members feels like a big change to me. #Closed

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs

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

AlekseyTs · 2025-03-05T16:11:12Z

Done with review pass (commit 3) #Resolved

RikkiGibson

LGTM modulo additional suggested tests and confirmation on moving forward with the break.

jjonescz · 2025-04-01T10:17:15Z

@RikkiGibson @AlekseyTs for another look, thanks
@jaredpar said he is okay with moving forward with this break but that I should also send an e-mail to LDT which I did and also got no pushback #Closed

jaredpar · 2025-04-01T17:26:29Z

Good to go from my perspective. #Closed

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md

AlekseyTs · 2025-04-01T20:27:24Z

@jjonescz It looks like there are conflicts that should be resolved #Closed

AlekseyTs · 2025-04-02T16:54:17Z

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs

@@ -794,7 +794,7 @@ partial C() { }
            }
            """;
        CreateCompilation(source).VerifyDiagnostics(
-            // (3,13): error CS9276: Partial member 'C.C()' must have a definition part.
+            // (3,13): error CS9401: Partial member 'C.C()' must have a definition part.
            //     partial C() { }
            Diagnostic(ErrorCode.ERR_PartialMemberMissingDefinition, "C").WithArguments("C.C()").WithLocation(3, 13),


nostic(ErrorCode.ERR_PartialMemberMissingDefinition, "C").WithArguments("C.C(

~~I think this error is unexpected. sealed on interface members means non-virtual.~~
I guess the behavior is consistent with non-partial method. #Resolved

Let's cover other accessibility modifiers (it looks like we cover public and private)

This comment is placed on unchanged code but judging by the quoted text // (3,48): error CS0238: 'I.E' cannot be sealed because it is not an override, I assume this comment was intended for InInterface_Sealed_Private.

That is correct (the location), this is a brand new test, so I am not sure why you consider it an unchanged code. Even though I "dismissed" my comment in the first message of this thread, the second message is still relevant.

this is a brand new test, so I am not sure why you consider it an unchanged code.

Sorry, I meant the location that this comment appears on GitHub and my CodeFlow is on unchanged test. I guess CodeFlow doesn't play nicely with merge commits.

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs

AlekseyTs · 2025-04-02T17:04:12Z

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs

+            using System;
+
+            M(new C1());
+            M(new C2());


"public"

Let's cover other accessibility modifiers

AlekseyTs · 2025-04-02T17:05:45Z

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs

+
+        static void validateEvent(EventSymbol e)
+        {
+            Assert.False(e.IsAbstract);


"public"

Let's cover other accessibility modifiers

AlekseyTs · 2025-04-02T18:59:59Z

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs

            //     partial event System.Action E { add { } remove { } }
            Diagnostic(ErrorCode.ERR_PartialMemberDuplicateImplementation, "E").WithArguments("C.E").WithLocation(9, 33),
            // (9,33): error CS0102: The type 'C' already contains a definition for 'E'
            //     partial event System.Action E { add { } remove { } }
            Diagnostic(ErrorCode.ERR_DuplicateNameInClass, "E").WithArguments("C", "E").WithLocation(9, 33),
-            // (11,13): error CS9278: Partial member 'C.C()' may not have multiple implementing declarations.
+            // (11,13): error CS9403: Partial member 'C.C()' may not have multiple implementing declarations.
            //     partial C() { }
            Diagnostic(ErrorCode.ERR_PartialMemberDuplicateImplementation, "C").WithArguments("C.C()").WithLocation(11, 13),


rCode.

It would be good to test situations when only one partial has sealed or virtual modifier. For other members too. #Resolved

Not sure what would you like to see tested in such scenarios. We already have test for the error that will occur "Both partial member declarations must have identical combinations of 'virtual', 'override', 'sealed', and 'new' modifiers." (That's tested for class, can extend that to interface if you want)

AlekseyTs · 2025-04-02T19:02:22Z

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

+            var source = $$"""
+                partial interface I
+                {
+                    public static {{modifier}} partial int M();


public

Let's cover remaining accessibility modifiers, including a situation with none #Resolved

AlekseyTs · 2025-04-02T19:11:23Z

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

+                """;
+            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net60)
+                .VerifyDiagnostics(
+                    // (3,32): error CS0238: 'I.M()' cannot be sealed because it is not an override


// (3,32): error CS0238: 'I.M()' cannot be sealed because it is not an override

Same comment as for events #Resolved

I guess same response as for events applies here - this is consistent with non-partial methods.

There was a second message on that thread, that is still relevant

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md

AlekseyTs · 2025-04-02T19:27:36Z

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

        {
+            // In C# 13 and earlier, interface partial members were not implicitly public and virtual and that is fixed only in C# 14 and later to avoid a breaking change.


// In C# 13 and earlier, interface partial members were not implicitly public and virtual and that is fixed only in C# 14 and later to avoid a breaking change.

I do not think we should change anything about implicit accessibility #Resolved

The breaking change email doesn't mention this aspect explicitly at all. It only talks about virtual-ness.

The breaking change email doesn't mention this aspect explicitly at all. It only talks about virtual-ness.

Yes, I narrowed down the focus of the email because I think the virtualness is the most important part, it can lead to silent behavior breaks. The accessibility break leads to source breaks only, and it doesn't affect methods. The email has link to this doc which has all the details.

Sorry, but I do not find this response satisfactory. We can discuss offline in more details

Scheduled for LDM: dotnet/csharplang#9282

AlekseyTs · 2025-04-02T19:38:42Z

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

+        }
+
+        [Theory, CombinatorialData, WorkItem("https://github.com/dotnet/roslyn/issues/77346")]
+        public void InInterface_NonVirtual_CSharp13_Private(


InInterface_NonVirtual_CSharp13_Private

Is this a dupe of InInterface_Private? #Resolved

Yes, will remove, thanks.

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

AlekseyTs · 2025-04-02T19:52:57Z

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

+        }
+
+        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/77346")]
+        public void InInterface_StaticVirtual_CSharp13()


InInterface_StaticVirtual_CSharp13

Is there a difference in behavior by comparison to InInterface_StaticVirtual #Resolved

Looks like there isn't, I will merge the tests, thanks.

Actually, I cannot merge them because the combination langVersion=CSharp13 + access="" has a different behavior. But it's correct that InInterface_StaticVirtual(langVersion=CSharp13, access="public") would have the same behavior as InInterface_StaticVirtual_CSharp13.

I find it hard to track differences in behavior across multiple "related" tests, hard to put back the entire matrix together and what/where the differences are in the matrix. It feels like it might be better to cover the entire matrix in one test, but adjust asserts to make expectations conditional based on the inputs. I think this way it will be easier to observe differences in behavior and when they happen. Consider restructuring tests this way.

If you decide to pursue this suggestion, we can work on it offline together

AlekseyTs · 2025-04-02T19:57:23Z

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs

            //     partial event System.Action E;
            Diagnostic(ErrorCode.ERR_PartialMemberDuplicateDefinition, "E").WithArguments("C.E").WithLocation(4, 33),
            // (4,33): error CS0102: The type 'C' already contains a definition for 'E'
            //     partial event System.Action E;
            Diagnostic(ErrorCode.ERR_DuplicateNameInClass, "E").WithArguments("C", "E").WithLocation(4, 33),
-            // (5,33): error CS9277: Partial member 'C.F' may not have multiple defining declarations.
+            // (5,33): error CS9402: Partial member 'C.F' may not have multiple defining declarations.
            //     partial event System.Action F;
            Diagnostic(ErrorCode.ERR_PartialMemberDuplicateDefinition, "F").WithArguments("C.F").WithLocation(5, 33),
            // (5,33): error CS0102: The type 'C' already contains a definition for 'F'


ror CS0102: The type

Is there a change in behavior around implicit accessibility in this scenario in this PR? #Resolved

Unfortunately, I cannot decipher where this comment was supposed to point to

Unfortunately, I cannot decipher where this comment was supposed to point to

For me, CodeFlow successfully places the comment on line 1013 in commit 6: Assert.Equal(Accessibility.Public, e.DeclaredAccessibility);

Then yes, previously the event was implicitly private now it is implicitly public.

AlekseyTs · 2025-04-02T20:01:04Z

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

        {
+            // In C# 13 and earlier, interface partial members were not implicitly public and virtual and that is fixed only in C# 14 and later to avoid a breaking change.
+            bool newPartialBehavior = (mods & DeclarationModifiers.Partial) == 0 || ((CSharpParseOptions)syntaxNode.SyntaxTree.Options).LanguageVersion > LanguageVersion.CSharp13;


newPartialBehavior

Setting this to true regardless of language version feels unintuitive and confusing. #Resolved

Perhaps changing the name of the local might improve things

AlekseyTs · 2025-04-02T20:32:32Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md

+***Introduced in Visual Studio 2022 version 17.15***
+
+We have fixed [an inconsistency](https://github.com/dotnet/roslyn/issues/77346)
+where interface members would not be implicitly `virtual` and `public` if they were also marked `partial`.


I think this description is unclear because accessibility for methods isn't affected by the change #Resolved

AlekseyTs · 2025-04-02T20:33:04Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md

+partial interface I
+{
+    public partial void M();
+    public partial void M() // implicitly virtual now


// implicitly virtual now

I think we should add examples targeting changes around accessibility, preferable when virtual-ness is not involved #Resolved

AlekseyTs · 2025-04-02T20:42:23Z

Done with review pass (commit 6)

Make partial interface members virtual

f7eec84

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 28, 2025

jjonescz marked this pull request as ready for review February 28, 2025 18:18

jjonescz requested a review from a team as a code owner February 28, 2025 18:18

RikkiGibson self-assigned this Mar 4, 2025

RikkiGibson reviewed Mar 5, 2025

View reviewed changes

Simplify code

a3d86b0

jjonescz changed the base branch from features/PartialEventsCtors to main March 5, 2025 14:56

Document the breaking change

4b1b0da

jjonescz commented Mar 5, 2025

View reviewed changes

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 5, 2025

View reviewed changes

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 5, 2025

View reviewed changes

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

RikkiGibson reviewed Mar 5, 2025

View reviewed changes

jjonescz added 2 commits March 11, 2025 15:32

Add more tests

8036ef1

Gate on lang version

cab885e

jjonescz requested review from jaredpar, RikkiGibson and AlekseyTs March 17, 2025 09:37

jaredpar added the breaking-change https://github.com/dotnet/sdk/blob/main/documentation/project-docs/breaking-change-guidelines.md label Apr 1, 2025

jjonescz commented Apr 1, 2025

View reviewed changes

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md Outdated Show resolved Hide resolved

Merge branch 'main' into PartialEventsCtors-12-Interface

4c23368

AlekseyTs reviewed Apr 2, 2025

View reviewed changes

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Apr 2, 2025

View reviewed changes

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md Outdated Show resolved Hide resolved

AlekseyTs reviewed Apr 2, 2025

View reviewed changes

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

AlekseyTs reviewed Apr 2, 2025

View reviewed changes

jjonescz added 3 commits April 3, 2025 14:15

Merge branch 'main' into PartialEventsCtors-12-Interface

6995680

Improve code and doc

4a0022f

Improve wording

db64315

Make partial interface members virtual #77379

Are you sure you want to change the base?

Make partial interface members virtual #77379

Conversation

jjonescz commented Feb 28, 2025 • edited Loading

RikkiGibson left a comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 5, 2025 • edited Loading

AlekseyTs commented Mar 5, 2025 • edited by jjonescz Loading

RikkiGibson left a comment

Choose a reason for hiding this comment

jjonescz commented Apr 1, 2025 • edited Loading

jaredpar commented Apr 1, 2025 • edited by jjonescz Loading

AlekseyTs commented Apr 1, 2025 • edited Loading

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 3, 2025 • 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

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 2, 2025 • edited by jjonescz 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

Choose a reason for hiding this comment

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs Apr 2, 2025 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs commented Apr 2, 2025

jjonescz commented Feb 28, 2025 •

edited

Loading

AlekseyTs commented Mar 5, 2025 •

edited

Loading

AlekseyTs commented Mar 5, 2025 •

edited by jjonescz

Loading

jjonescz commented Apr 1, 2025 •

edited

Loading

jaredpar commented Apr 1, 2025 •

edited by jjonescz

Loading

AlekseyTs commented Apr 1, 2025 •

edited

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

jjonescz Apr 3, 2025 •

edited

Loading

AlekseyTs Apr 3, 2025 •

edited

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

jjonescz Apr 3, 2025 •

edited

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

jjonescz Apr 4, 2025 •

edited

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading

AlekseyTs Apr 2, 2025 •

edited by jjonescz

Loading