-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: rename EmitAsyncVoidInterceptor to EmitAsyncTaskInterceptor #26
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
Conversation
The method generates `async Task` interceptors (not async void). The old name falsely implied it generated the async void antipattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello @ANcpLua, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the instrumentation source generator by updating the name of an internal method. The change ensures that the method's name accurately reflects its functionality of generating Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. WalkthroughA private helper method is renamed from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Code Review
This pull request correctly renames EmitAsyncVoidInterceptor to EmitAsyncTaskInterceptor to more accurately reflect that it generates interceptors for async Task methods. While reviewing this change, I discovered a critical logic bug in the switch statement that determines which interceptor to emit. The current logic would incorrectly handle methods returning a non-generic Task, and the branch containing the newly renamed method is unreachable. I've provided a detailed comment with a suggested fix to correct the code generation logic.
| break; | ||
| case true: | ||
| EmitAsyncVoidInterceptor(sb, target, methodId); | ||
| EmitAsyncTaskInterceptor(sb, target, methodId); |
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.
While the rename to EmitAsyncTaskInterceptor is correct, this case block is currently unreachable due to a logic flaw in the switch statement. The preceding condition case true when !isVoid: on line 83 incorrectly matches both Task<T> and non-generic Task, routing both to EmitAsyncInterceptor. This is only correct for Task<T> and would cause a compilation error if a method returning a non-generic Task were intercepted.
The entire switch block needs to be refactored to correctly differentiate between generic and non-generic tasks. Here is a suggested implementation:
// Before the switch statement, you can determine if the task is generic:
var isGenericTask = target.ReturnType.Contains('<');
// Then, replace the switch block (lines 81-92) with:
switch (isTask)
{
case true when isGenericTask:
EmitAsyncInterceptor(sb, target, methodId);
break;
case true:
EmitAsyncTaskInterceptor(sb, target, methodId);
break;
default:
EmitSyncInterceptor(sb, target, methodId);
break;
}This ensures Task<T> and Task are handled by their respective emitter methods, fixing the bug.
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.
Pull request overview
Renames an internal helper in the instrumentation source generator to reflect that it emits async Task (not async void) interceptors.
Changes:
- Rename
EmitAsyncVoidInterceptortoEmitAsyncTaskInterceptor. - Update the call site in
EmitInterceptorto use the new method name.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case true when !isVoid: | ||
| EmitAsyncInterceptor(sb, target, methodId); | ||
| break; | ||
| case true: | ||
| EmitAsyncVoidInterceptor(sb, target, methodId); | ||
| EmitAsyncTaskInterceptor(sb, target, methodId); | ||
| break; |
Copilot
AI
Feb 8, 2026
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.
The EmitAsyncTaskInterceptor branch is likely unreachable: isVoid only checks for void/System.Void, so any Task/Task<T> return type will have isVoid == false and hit EmitAsyncInterceptor. For non-generic Task (and ValueTask), EmitAsyncInterceptor will generate var result = await ..., which does not compile because await Task returns void. Consider explicitly detecting non-generic Task (and optionally ValueTask) and routing those to EmitAsyncTaskInterceptor, while keeping EmitAsyncInterceptor for Task<T>/ValueTask<T>.
Summary
EmitAsyncVoidInterceptortoEmitAsyncTaskInterceptorin the instrumentation source generatorasync Taskinterceptors, notasync void— the old name falsely implied the async void antipatternTest plan
dotnet buildpasses with zero warnings🤖 Generated with Claude Code
Summary by CodeRabbit