Fix explicit interface method emission and diagnostics #2017
Conversation
Improves handling of explicit interface implementations in the source generator, ensuring that methods explicitly implemented from base interfaces are not redundantly emitted and that diagnostics are not incorrectly reported. Updates method model and emitter logic to track explicitness, normalizes method lookup keys, and adds polyfills for Index/Range for legacy targets. Updates tests and snapshots to reflect correct code generation.
Corrects 'installWorkflows' to 'installWorkloads' in both ci-build and release workflows. Also adds the CODECOV_TOKEN secret to the ci-build workflow for code coverage reporting.
The installWorkloads parameter was removed from both ci-build.yml and release.yml GitHub Actions workflows, simplifying the configuration.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2017 +/- ##
==========================================
- Coverage 87.73% 83.03% -4.71%
==========================================
Files 33 35 +2
Lines 2348 2652 +304
Branches 294 383 +89
==========================================
+ Hits 2060 2202 +142
- Misses 208 356 +148
- Partials 80 94 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves handling of explicit interface implementations in the Refit source generator, fixing issues where methods explicitly implemented from base interfaces were being redundantly emitted and incorrect diagnostics were being reported.
- Fixes explicit interface method emission to avoid duplicate method generation
- Updates method lookup logic to normalize method names by stripping interface qualifiers
- Adds support for synchronous return types in non-public and explicit interface members
Reviewed Changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Refit/ | Core library modernization with primary constructors, collection expressions, and synchronized method handling |
| Refit.Tests/ | New test file for explicit interface scenarios and updated snapshot tests reflecting corrected code generation |
| InterfaceStubGenerator.Shared/ | Major parser updates to handle explicit interface implementations and prevent duplicate emissions |
| Directory.Build.props | Updated target framework configuration for conditional Windows builds and .NET 10 support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(TargetFramework)' == 'net8.0' or '$(TargetFramework)' == 'net9.0'"> | ||
| <ItemGroup Condition="'$(TargetFramework)' == 'net8.0' or '$(TargetFramework)' == 'net9.0' or '$(TargetFramework)' == 'net10.0'"> |
There was a problem hiding this comment.
The condition logic is duplicated and should be consolidated. Consider using a common MSBuild property or function to define the framework condition once and reuse it.
Copilot uses AI. Check for mistakes.
| { | ||
| // Allow synchronous return types only for non-public or explicit interface members. | ||
| // This supports internal Refit methods and explicit interface members annotated with Refit attributes. | ||
| var isExplicitInterfaceMember = methodInfo.Name.IndexOf('.') >= 0; |
There was a problem hiding this comment.
[nitpick] Using IndexOf with >= 0 comparison can be replaced with the more readable Contains method for better code clarity.
| var isExplicitInterfaceMember = methodInfo.Name.IndexOf('.') >= 0; | |
| var isExplicitInterfaceMember = methodInfo.Name.Contains('.'); |
Copilot uses AI. Check for mistakes.
| if (type is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(type)); | ||
| } |
There was a problem hiding this comment.
[nitpick] This null check is redundant since the parameter already has nullable annotations and the framework will handle null reference validation appropriately.
| if (type is null) | |
| { | |
| throw new ArgumentNullException(nameof(type)); | |
| } |
Copilot uses AI. Check for mistakes.
| var lastDot = declaredBaseName.LastIndexOf('.'); | ||
| if (lastDot >= 0) | ||
| { | ||
| declaredBaseName = declaredBaseName.Substring(lastDot + 1); | ||
| } |
There was a problem hiding this comment.
This pattern for extracting the method name after the last dot is duplicated in multiple locations. Consider extracting this logic into a helper method to reduce code duplication.
Copilot uses AI. Check for mistakes.
What kind of change does this PR introduce?
fix
What is the current behavior?
#1951
What is the new behavior?
Improves handling of explicit interface implementations in the source generator, ensuring that methods explicitly implemented from base interfaces are not redundantly emitted and that diagnostics are not incorrectly reported. Updates method model and emitter logic to track explicitness, normalizes method lookup keys, and adds polyfills for Index/Range for legacy targets. Updates tests and snapshots to reflect correct code generation.
What might this PR break?
All tests pass, no known regressions
Please check if the PR fulfills these requirements
Other information: