-
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
Support declaring types within interfaces. #19149
Support declaring types within interfaces. #19149
Conversation
**Protected** and **protected internal** accessibility is not supported.
@dotnet/roslyn-compiler Please review. |
void M1(); | ||
} | ||
|
||
class T2 |
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.
I don't expect a problem, but maybe test type nested in type.
Also consider generic type (with it's own T, or with the T from the containing interface). #Resolved
void M1(); | ||
} | ||
|
||
private class T2 |
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.
Maybe test partial class?
Also what happens if I put a partial method and also the partial methods implementation in an interface?
Maybe test static type?
Russian dolls: interface containing a type containing an interface... #Resolved
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.
Though, it is theoretically possible, given the nature of the changes in this PR, I believe it is very unlikely to find any problem that doesn't exist with types that, let's say, are nested into classes, but exists for types nested into interfaces. Therefore, I see very little benefit in doing that kind of ad-hoc testing. At the same time, redoing full-matrix testing for nested types is cost-prohibitive. If you have reason to believe that there is a problem with some scenarios and that reason has a real basis behind it, I'd be happy to add tests for scenarios like this. Otherwise, I prefer to not perform discovery testing in context of this PR.
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
var allowedModifiers = DeclarationModifiers.AccessibilityMask | DeclarationModifiers.Partial; | ||
|
||
if (ContainingSymbol is TypeSymbol) | ||
if (containingSymbol.Kind == SymbolKind.Namespace) |
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.
Would the following nested if
be more readable as a switch on containingSymbol.Kind
?
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.
Would the following nested if be more readable as a switch on containingSymbol.Kind?
Interfaces are not represented by different SymbolKind.
Protected and protected internal accessibility is not supported.
@dotnet/roslyn-compiler Please review.