Make partial interface members virtual#77379
Conversation
RikkiGibson
left a comment
There was a problem hiding this comment.
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/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs
Outdated
Show resolved
Hide resolved
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
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs
Outdated
Show resolved
Hide resolved
|
Done with review pass (commit 3) #Resolved |
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM modulo additional suggested tests and confirmation on moving forward with the break.
|
@RikkiGibson @AlekseyTs for another look, thanks |
|
Good to go from my perspective. #Closed |
|
@jjonescz It looks like there are conflicts that should be resolved #Closed |
src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs
Outdated
Show resolved
Hide resolved
|
Done with review pass (commit 6) |
src/Compilers/CSharp/Test/Symbol/Symbols/ExtendedPartialMethodsTests.cs
Outdated
Show resolved
Hide resolved
|
@jjonescz Does PR description accurately reflect the changes that this PR is making now? |
|
Done with review pass (commit 11) |
Updated the description, thanks. |
Likely caused by some unintentional git operation.
|
@RikkiGibson for another look, thanks |
| This inconsistency is however [preserved](./Deviations%20from%20Standard.md#interface-partial-methods) for partial interface methods to avoid a larger breaking change. | ||
| Note that Visual Basic and other languages not supporting default interface members will start requiring to implement implicitly virtual `partial` interface members. | ||
|
|
||
| To keep the previous behavior, explicitly mark `partial` interface members as `sealed` and `private`. |
There was a problem hiding this comment.
nit: private by itself should accomplish this, right? Since the member will be implicitly sealed in that case. It looks like private sealed is permitted on static interface methods but not instance ones.
There was a problem hiding this comment.
private will be enough for members like partial int P { get; }. But if you had a member like public partial int P { get; }, the workaround to preserve previous behavior is adding sealed modifier (you cannot add private since the member is already public). Let me try to clarify, thanks.
|
It looks like the PR is ready to merge |
Fixes #77346.
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 that inconsistency for partial properties and events but leaves it for extended partial methods per LDM decision.