Skip to content

Conversation

@thomhurst
Copy link
Owner

Summary

Methods that implement interface members should NOT have their signatures converted to async Task because this would break the interface implementation contract.

The fix uses a two-pass approach:

  1. Before syntax modifications: Collect all methods that implement interface members using semantic analysis (via semantic model)
  2. During async signature rewriting: Skip methods identified as interface implementations

Also adds ref/out/in parameter check to prevent async conversion of methods with those parameter types (which cannot be async).

Changes

  • Modified AsyncMethodSignatureRewriter to accept a set of interface-implementing method keys
  • Added static method CollectInterfaceImplementingMethods that uses semantic analysis to identify interface implementations
  • Updated BaseMigrationCodeFixProvider to collect interface-implementing methods before modifications
  • Added test for interface implementation scenario

Test plan

  • Run NUnit migration tests (66 passed)
  • Run xUnit migration tests (39 passed, 1 skipped)
  • Run MSTest migration tests (23 passed)

Fixes #4342

🤖 Generated with Claude Code

… async

Methods that implement interface members should NOT have their signatures
converted to async Task because this would break the interface implementation
contract. The fix uses a two-pass approach:

1. Before syntax modifications, collect all methods that implement interface
   members using semantic analysis (via semantic model)
2. During async signature rewriting, skip methods identified as interface
   implementations

Also adds ref/out/in parameter check to prevent async conversion of methods
with those parameter types (which cannot be async).

Fixes #4342

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@thomhurst
Copy link
Owner Author

Summary

Fixes interface implementation methods being incorrectly converted to async by the migration code fixer.

Critical Issues

None found ✅

Suggestions

1. Potential Performance Concern - HashSet allocation

The CollectInterfaceImplementingMethods method is called during code fix application. While this is not as hot as test discovery/execution, it still allocates a new HashSet and iterates through all methods.

Location: TUnit.Analyzers.CodeFixers/Base/AsyncMethodSignatureRewriter.cs:19-66

Consider:

  • The implementation looks reasonable for a code fixer context (not a hot path)
  • The two-pass approach is necessary since semantic model becomes invalid after syntax changes
  • No action needed unless profiling shows issues

2. Method Key Stability

The GetMethodKey method uses string concatenation to create method signatures. The key format should remain stable across syntax tree modifications (before/after conversion), which appears to be the case.

Location: TUnit.Analyzers.CodeFixers/Base/AsyncMethodSignatureRewriter.cs:83-91

3. Test Coverage

The added test covers the implicit interface implementation case well. Consider adding edge cases for explicit interface implementations, generic interfaces, and multiple interface implementations, though the explicit case is already handled by checking ExplicitInterfaceSpecifier.

Technical Review

✅ Dual-mode: N/A - This is analyzer/code fixer code, not engine metadata collection
✅ Snapshot Testing: N/A - No source generator output or public API changes
✅ No VSTest: No VSTest references
✅ Performance: Acceptable for code fixer context (not hot path)
✅ AOT Compatibility: Uses Roslyn semantic model appropriately (analyzers run at compile-time)

The two-pass approach (collect interface implementations before modifications, then check during rewriting) is the correct solution since the semantic model becomes invalid after syntax tree modifications.

Verdict

✅ APPROVE - Well-implemented fix with appropriate test coverage. The solution correctly handles the semantic model invalidation problem and prevents breaking interface implementation contracts.

@thomhurst thomhurst merged commit 422a2c7 into main Jan 12, 2026
13 checks passed
@thomhurst thomhurst deleted the fix/interface-async-conversion-4342 branch January 12, 2026 23:29
This was referenced Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NUnit Migration: Interface implementation methods incorrectly converted to async

2 participants