-
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
Don't place 'async' after 'partial' modifier #76168
Conversation
@@ -177,7 +180,21 @@ private static bool IsIEnumerator(ITypeSymbol returnType, KnownTaskTypes knownTy | |||
private static (SyntaxTokenList newModifiers, TypeSyntax newReturnType) AddAsyncModifierWithCorrectedTrivia(SyntaxTokenList modifiers, TypeSyntax returnType) | |||
{ | |||
if (modifiers.Any()) | |||
return (modifiers.Add(s_asyncKeywordWithSpace), returnType); | |||
{ | |||
// 'partial' modifier must say at the end of the modifiers arrays. |
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 think await
completion that I implemented may have the exact same bug. The bug is more critical for IntelliSense I think.
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.
roslyn/src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs
Line 40 in 5213c18
protected override int GetAsyncKeywordInsertionPosition(SyntaxNode 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.
Thanks @Youssef1313 . I'm going to handle in a followup pr.
return (modifiers.Add(s_asyncKeywordWithSpace), returnType); | ||
{ | ||
// 'partial' modifier must say at the end of the modifiers arrays. | ||
var partialModifier = modifiers.FirstOrDefault(static m => m.IsKind(SyntaxKind.PartialKeyword)); |
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.
Nope!
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.
What about this, it's internal, but I thought it would be accessible? Works the way you have it, and I know the lists are small, but it seems weird to always search to find it.
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.
What about this, it's internal,
Nope. That's in compiler layer. We don't have IVT to them. We could make an extension at workspace layer, and update to that. i empower you :)
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.
Isn't it already exposed as an extension method by the compiler itself?
public static int IndexOf(this SyntaxTokenList list, SyntaxKind 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.
Good find
@@ -177,7 +180,21 @@ private static bool IsIEnumerator(ITypeSymbol returnType, KnownTaskTypes knownTy | |||
private static (SyntaxTokenList newModifiers, TypeSyntax newReturnType) AddAsyncModifierWithCorrectedTrivia(SyntaxTokenList modifiers, TypeSyntax returnType) | |||
{ | |||
if (modifiers.Any()) | |||
return (modifiers.Add(s_asyncKeywordWithSpace), returnType); | |||
{ | |||
// 'partial' modifier must say at the end of the modifiers arrays. |
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.
@genlu ptal. |
Fixes #63404