-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix await completion changing void to Task for event handler methods #81347
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
Fix await completion changing void to Task for event handler methods #81347
Conversation
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| await VerifyAbsenceAsync(code); | ||
| } | ||
|
|
||
| [Fact] |
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.
@copilot Add WorkItem for 77848 for all tests you added
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.
@copilot move all tests into the CSharpCompletionCommandHandlerTests_Await file instead.
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.
Added WorkItem attribute to all 8 tests (5 in AwaitCompletionProviderTests.cs and 3 in CSharpCompletionCommandHandlerTests_Await.vb) in commit 54690f6.
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.
Moved all 8 tests to CSharpCompletionCommandHandlerTests_Await.vb as integration tests. Removed tests from AwaitCompletionProviderTests.cs. Commit 4f4fe8c.
…eyword-event-handler
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
…hub.com/dotnet/roslyn into copilot/fix-await-keyword-event-handler
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/77848")] | ||
| public Task TestEventHandlerMethod_DoesNotChangeVoidToTask() |
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.
@copilot move all tests you added here into CSharpCompletionCommandHandlerTests_Await (unless you have aan equivalent test there). do not add any tests to this file at all.
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.
This was already completed in commit 4f4fe8c. All tests have been removed from AwaitCompletionProviderTests.cs and moved to CSharpCompletionCommandHandlerTests_Await.vb (8 tests total, all with WorkItem attributes).
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
|
|
||
| // Get the syntax root for the containing type | ||
| var documents = containingType.DeclaringSyntaxReferences.Select(r => solution.GetDocument(r.SyntaxTree)).WhereNotNull().ToImmutableHashSet(); | ||
| var references = await SymbolFinder.FindReferencesAsync( |
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 not a little expensive to be doing in completion? Looks like it happens on commit as well - any concern of UI delays here?
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 can add a comment, but the thinking here was this was limited to just the docs that the containing type is defined in. And it's a FAR for a method name. so we literally jsut need to bind the name of the containing method within those files (normally just a single file or a small couple). So this should not be a problem perf-wise.
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.
to be very cautious, we now limit the FAR search to teh same document that hte method is contained in. we may miss things. but it will catch the common case.
…eyword-event-handler
Fix: Prevent changing void to Task for event handler methods in await completion
Issue Summary
Regression where typing
awaitin an event handler method changed the return type fromvoidtoTask, making the code uncompilable. Event handlers in C# must havevoidreturn type.Before (BROKEN):
After (FIXED):
Solution Implemented
voidfor event handlersDetection Logic
The fix searches the containing type for patterns like:
MyEvent += OnMyEvent(event registration)MyEvent -= OnMyEvent(event unregistration)When detected, it:
asyncmodifier (required forawait)voidreturn type (required for event handlers)void → Taskfor non-event-handler methodsTest Coverage
All tests are integration tests in
CSharpCompletionCommandHandlerTests_Await.vb:Event Handler Cases (void preserved):
+=and-=operatorsNon-Event Handler Cases (void → Task):
Build Results
✅ All projects build successfully with 0 warnings
✅ No security vulnerabilities detected
Files Changed
AwaitCompletionProvider.cs(+55 lines)CSharpCompletionCommandHandlerTests_Await.vb(+257 lines) - all integration tests with WorkItem attributesTotal: 312 lines added, 2 files modified
Fixes #77848
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.