Fixing completion in async foreach/using #23960
Conversation
|
@dotnet/roslyn-ide for review. Thanks |
1 similar comment
|
@dotnet/roslyn-ide for review. Thanks |
|
@dotnet/roslyn-ide for review. Thanks |
|
|
||
| if (previous.IsKind(SyntaxKind.AwaitKeyword)) | ||
| { | ||
| var beforeAwait = previous.GetPreviousToken(includeSkipped: true); |
There was a problem hiding this comment.
I would assume this would check that the 'await' parsed as a part of a statement vs. just checking previous token. You sure don't want "using await" at the start of a file (i.e. namespace using) triggering this.
There was a problem hiding this comment.
(and yes, it looks like the previous check also has a bug. 😄)
There was a problem hiding this comment.
From another PR, I understand that recommending keywords a little loosely isn't bad.
#24089
Are you saying they both should be tightened?
There was a problem hiding this comment.
This seems like this shouldnt' be necessary. Instead of:
var previous = token.GetPreviousToken(includeSkipped: true);
if (previous.IsKind(SyntaxKind.ForKeyword) ||
previous.IsKind(SyntaxKind.ForEachKeyword) ||
previous.IsKind(SyntaxKind.UsingKeyword))
{
return true;
}We should jsut check the parent node kind.
There was a problem hiding this comment.
i.e. if we have an open paren, see if the parent of it is a BaseForeachStatementSyntax, ForStatementSyntax, or a UsingStatementSyntax.
| @@ -2363,6 +2375,14 @@ public static bool IsLabelContext(this SyntaxTree syntaxTree, int position, Canc | |||
| return true; | |||
| } | |||
|
|
|||
| // using await ( | | |||
| if (token.IsKind(SyntaxKind.OpenParenToken) && | |||
There was a problem hiding this comment.
we can just update the previous test to see if the open paren parent is a UsingStatementSyntax.
|
FYI: that code was written at a time when syntax trees were still in flux, and it wasn't clear if we had parent pointers or not. So it was trying to be as token oriented as possible. That's a lot less necessary now, and we can def go with the simpler forms where we just check the parent of a token. |
| @@ -51,6 +51,11 @@ protected override bool IsValidContext(int position, CSharpSyntaxContext context | |||
| return true; | |||
| } | |||
|
|
|||
| if (context.TargetToken.IsKind(SyntaxKind.UsingKeyword, SyntaxKind.ForEachKeyword)) | |||
There was a problem hiding this comment.
note: this will mean 'await' may be offered after 'using' at the top level. That's probably not waht we want. can you filter out that case and add a test. (note: maybe we do want it, because of script... not sure).
| public static bool IsParentKind(this SyntaxNode node, SyntaxKind kind1, SyntaxKind kind2, SyntaxKind kind3, SyntaxKind kind4) | ||
| { | ||
| return node != null && IsKind(node.Parent, kind1, kind2, kind3, kind4); | ||
| } |
|
@jasonmalinowski Any other feedback? |
|
Anybody from @dotnet/roslyn-ide can review this one. |
The keyword
awaitshould be offered for completion afterusingandforeach.Also,
varandnewshould be appropriately offered inusing await ($$andforeach await (var i in $$.