-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add more using-alias-to-type tests #66889
Add more using-alias-to-type tests #66889
Conversation
@jcouv this is ready for review. |
var newType = await Simplifier.ExpandAsync(usingDirective.Type, document, cancellationToken: cancellationToken).ConfigureAwait(false); | ||
return usingDirective.WithType(newType); | ||
var newType = await Simplifier.ExpandAsync(usingDirective.NamespaceOrType, document, cancellationToken: cancellationToken).ConfigureAwait(false); | ||
return usingDirective.WithNamespaceOrType(newType); |
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.
fallout of API review meeting. We decided to rename 'Type' to 'NamespaceOrType' (to match INamespaceOrTypeSymbol)
} | ||
"; | ||
var comp = CreateCompilation(csharp, options: TestOptions.UnsafeDebugDll); | ||
comp.VerifyDiagnostics(); |
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.
test validates taht even though the using is unsafe, it binds to a normal safe type. So you should be able to use that alias at a site that itself is not marked unsafe without error.
syntaxTree.IsAfterKeyword(position, SyntaxKind.ConstKeyword, cancellationToken) || | ||
syntaxTree.IsAfterKeyword(position, SyntaxKind.RefKeyword, cancellationToken) || | ||
syntaxTree.IsAfterKeyword(position, SyntaxKind.ReadOnlyKeyword, cancellationToken) || | ||
syntaxTree.IsUsingAliasContext(position, cancellationToken); |
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.
these other cases are subsumed by IsTypeCOntext
.
var leftUsing = (UsingStatementSyntax)leftNode; | ||
var rightUsing = (UsingStatementSyntax)rightNode; | ||
|
||
if (leftUsing.Declaration != null && rightUsing.Declaration != null) | ||
{ |
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.
view with whitespace off. This was just put in a block so that it and the next case section coudl both use leftUsing/rightUsing
.
if (token.IsKind(SyntaxKind.GreaterThanToken) && token.Parent.IsKind(SyntaxKind.FunctionPointerParameterList)) | ||
if (token.IsKind(SyntaxKind.GreaterThanToken) && | ||
token.Parent.IsKind(SyntaxKind.FunctionPointerParameterList) && | ||
token.Parent.Parent?.Parent is not UsingDirectiveSyntax) |
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.
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.
Then that won't matter as it won't be followed by the semicolon. e.g. delegate*<int,int>[];
will be a question about how >[
is normalized (which is a present issue, unrelated to my pr), and how ];
is normalized (which is covered by another test).
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.
Then that won't matter as it won't be followed by the semicolon.
Where do we check for the fact? Specifically: "followed by the semicolon."
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.
implied by: token.Parent.Parent?.Parent is not UsingDirectiveSyntax)
. we know (from line above) that token.Parent is FuncPointerParameterList, to token.Parent.Parent is FuncPointerSYntax. So if token.Parent.Parent.Parent is a UsingDirectiveSyntax, then the func-pointer showed up as the NamespaceOrType
of that using directive. So, in that case, we do not want a whitespace separator between it and the next token (which would be the semicolon of the using directive).
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.
So, in that case, we do not want a whitespace separator between it and the next token (which would be the semicolon of the using directive)
That is understood. The question however is why the same logic should not be applied to a case when we are in the "using" type, but not at the top level?
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.
In other words, are there more scenarios that we are missing?
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.
those scenarios would also arise for function-pointer prior to my PR/feature. So i view it as out of the scope of what this PR is trying to accomplish. IN other words, say there is an issue with an array of function pointers, where you get it as delegate*<int, int> []
instead of delegate*<int, int>[]
. That issue would exist regardless of the type being in a using directive or not. It's possible there is some issue, but it doesn't seem related/relevant to the feature i'm doing, so it's not something i'm looking into improving.
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.
Compiler changes LGTM (commit 132). I didn't review tests.
@CyrusNajmabadi Please squash commits during merging |
@AlekseyTs will do. |
oh, @cston too |
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.
Compiler changes look good (iteration 132), but if we don't think there's a circularity issue then we should remove the comment in BaseTypeAnalysis
.
Sure, i can do that. |
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.
Compiler changes LGTM Thanks (iteration 133)
Fallout from test-plan review.
Relates to test plan #56323