fix: resolve multiple migration code fixer bugs

[thomhurst]

Summary

• Fix using directive removal in namespace blocks: Extended RemoveFrameworkUsings() to also remove using directives from inside namespace blocks (both classic and file-scoped namespaces)

• Fix preprocessor directive corruption: Updated CleanupClassMemberLeadingTrivia() to preserve #region, #if, #endif, #pragma, and other preprocessor directives. Only removes excessive consecutive blank lines, not all newlines.

• Fix FixtureLifeCycle attribute handling: Added FixtureLifeCycle to recognized and removable attributes. TUnit uses instance-per-test by default, matching NUnit's InstancePerTestCase.

• Fix Assert.Throws tracking: Updated ConvertThrows/ConvertThrowsAsync to return the original expression instead of null, ensuring assertions are properly tracked in the conversion plan.

• Fix async return type wrapping: Added WrapReturnTypeInTask support for non-void, non-Task return types. Methods returning object, int, etc. now correctly become async Task<object>, async Task<int>, etc.

Closes

Fixes #4481
Fixes #4482
Fixes #4483
Fixes #4486
Fixes #4487
Fixes #4488
Fixes #4490
Fixes #4491
Fixes #4492

Note

Issue #4489 (Platform attribute conversion) was found to be invalid - TUnit does have ExcludeOnAttribute, so the existing conversion is correct.

Test plan

• Run existing analyzer tests to verify no regressions
• Verify using directives inside namespace blocks are removed
• Verify preprocessor directives (#region, #if) are preserved
• Verify FixtureLifeCycle attributes are removed without generating broken code
• Verify Assert.Throws assertions are tracked in the conversion plan
• Verify non-void async methods get correct Task return type

🤖 Generated with Claude Code

[thomhurst]

Summary

Fixes multiple migration code fixer bugs related to trivia handling, attribute conversion, assertion tracking, and async return type wrapping.

Critical Issues

None found ✅

Suggestions

1. Potential infinite loop in CleanupClassMemberLeadingTrivia (Low Risk)

In BaseMigrationCodeFixProvider.cs:396, the while loop relies on FindClassWithExcessiveLeadingTrivia detecting "excessive" trivia. However, the new logic in FindClassWithExcessiveLeadingTrivia only counts consecutive newlines without considering preprocessor directives or comments that reset the counter.

Consider this edge case:

class Test {
    #region Test

    [Test]
    void Method() { }
}

After the first pass, if a single newline remains after the #region, the method won't detect it as "excessive" (needs >1 consecutive), but CleanupClassMemberLeadingTrivia preserves one newline after preprocessor directives. This should be safe, but verify the loop terminates correctly.

Recommendation: Add a unit test with preprocessor directives to verify the cleanup loop terminates and preserves formatting correctly.

2. AnalyzeReturnTypeForAsync string comparison robustness

In MigrationAnalyzer.cs:567-588, the return type analysis uses string comparisons (returnTypeText.StartsWith("Task<")). This may fail for fully qualified types like System.Threading.Tasks.Task<int>.

Recommendation: Consider using semantic model or more robust pattern matching if this becomes an issue in testing. The current approach should work for most cases since ToString() on ReturnType typically gives the short form.

3. Assert.Throws tracking expression consistency

In XUnitTwoPhaseAnalyzer.cs:336-369, the new code reconstructs the assertion expression for tracking purposes. The reconstructed expression uses ToString() on arguments, which may not preserve exact formatting (e.g., line breaks, comments in lambdas).

This is fine for tracking purposes, but be aware the tracked text may differ slightly from the original source.

Previous Review Status

Unable to fetch previous comments (API permissions), but this appears to be a new PR addressing multiple issues (#4486, #4482, #4481, #4487, #4488, #4492, #4489, #4490, #4483, #4491).

Verdict

✅ APPROVE - No critical issues

---

TUnit Rules Checklist:

• ✅ Not touching core engine metadata collection (no dual-mode requirement)
• ✅ Analyzer changes (snapshot tests would be good to run but not blocking)
• ✅ No VSTest usage
• ✅ No obvious performance concerns (trivia manipulation is not hot path)
• ✅ No reflection requiring AOT annotations
• ✅ No security issues (no injection, XSS, or secrets)

[thomhurst]

Summary

Fixes multiple migration code fixer bugs: using directive removal in namespaces, preprocessor directive preservation, FixtureLifeCycle attribute handling, Assert.Throws tracking, and async return type wrapping.

Critical Issues

None found ✅

Suggestions

1. Preprocessor directive trivia logic complexity

In BaseMigrationCodeFixProvider.cs:401-458, the new trivia handling logic is more sophisticated but also more complex. The loop that builds triviaToKeep tracks state with consecutiveNewlines and lastWasPreprocessorOrComment.

Edge case concern: After a preprocessor directive like #region, the code allows one newline (consecutiveNewlines == 1), but then resets lastWasPreprocessorOrComment = false on the newline. This means a second newline would be considered excessive and removed. However, the detection logic in FindClassWithExcessiveLeadingTrivia requires consecutiveNewlines > 1 to detect excessive trivia. This creates a subtle interaction where first pass removes excessive newlines and after first pass may leave preprocessor + 1 newline which won't trigger another pass. This appears intentional and correct, but worth verifying with tests that have multiple preprocessor directives.

2. String-based return type analysis

In MigrationAnalyzer.cs:567-592, the AnalyzeReturnTypeForAsync method uses string comparisons like returnTypeText.StartsWith(Task<). While this works for typical cases, it may not handle fully qualified types or type aliases. The current approach should work for 99% of cases since ReturnType.ToString() typically gives the short form.

3. Assert.Throws expression reconstruction

In XUnitTwoPhaseAnalyzer.cs:332-369, the code now reconstructs expressions for tracking purposes using .ToString() on arguments. Lambda expressions with line breaks will be flattened and formatting may differ slightly from original. This is acceptable since it's only for tracking purposes.

Verdict

✅ APPROVE - No critical issues

TUnit Rules Applied: No core engine changes, no snapshot test changes needed, no VSTest usage, no performance concerns, no reflection requiring AOT annotations, no security vulnerabilities detected.