-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable a default implementation of an interface property/indexer to be provided as part of its declaration. #18110
Enable a default implementation of an interface property/indexer to be provided as part of its declaration. #18110
Conversation
…e provided as part of its declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
{ | ||
// PROTOTYPE(DefaultInterfaceImplementation): Should we check language version as well? | ||
// Usually, it is done based on specific syntax that targets a new feature, but in this case | ||
// no special syntax is used. Also, partial types can have declarations coming from different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use CSharpCompilation.LanguageVersion. Partial types cannot have declarations coming from different trees with different language versions. The compiler checks up front that all input trees have the same language version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still there is no special syntax at this site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that there is no special syntax at this site. But there is a new language feature being used. In any case I agree with your decision to document it as an open issue with a PROTOTYPE comment.
@@ -755,7 +781,8 @@ private DeclarationModifiers MakeModifiers(SyntaxTokenList modifiers, bool isExp | |||
// Proper errors must have been reported by now. | |||
if (isInterface) | |||
{ | |||
mods = (mods & ~DeclarationModifiers.AccessibilityMask) | DeclarationModifiers.Abstract | DeclarationModifiers.Public; | |||
mods = (mods & ~DeclarationModifiers.AccessibilityMask) | DeclarationModifiers.Public | | |||
(accessorsHaveImplementation ? DeclarationModifiers.Virtual : DeclarationModifiers.Abstract); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct if one accessor is abstract and one virtual?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see that is left as a future work item.
@dotnet/roslyn-compiler Please review, need a second sign-off. |
Consider naming helper methods with "Assert" or "Verify" to distinguish them from tests. Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:1250 in 59a8ffa. [](commit_id = 59a8ffa, deletion_comment = False) |
I see that you used "Validate" below. That works. In reply to: 288867427 [](ancestors = 288867427) Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:1250 in 59a8ffa. [](commit_id = 59a8ffa, deletion_comment = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks! I'll address feedback in the next PR. |
@dotnet/roslyn-compiler Please review.