fix: add defensive syntax fallbacks to migration code fixers #4369
Conversation
Adds support for converting xUnit's Record.Exception() and
Record.ExceptionAsync() patterns to try-catch blocks during migration.
The conversion transforms:
```csharp
var ex = Record.Exception(() => SomeMethod());
```
To:
```csharp
Exception? ex = null;
try
{
SomeMethod();
}
catch (Exception e)
{
ex = e;
}
```
Fixes #4332
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add tests verifying that Assert.Throws<T>() with property access works correctly: - Assert_Throws_With_Property_Access_Can_Be_Converted: tests capturing exception and accessing properties like .ParamName - Assert_Throws_With_Message_Contains_Can_Be_Converted: tests using Assert.Contains with ex.Message Both tests pass, confirming issue #4334 is working correctly. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Instead of removing the NUnit [Platform] attribute, convert it to TUnit's equivalent attributes: - [Platform(Include = "Win")] -> [RunOn(OS.Windows)] - [Platform(Exclude = "Linux")] -> [ExcludeOn(OS.Linux)] - [Platform(Include = "Win,Linux")] -> [RunOn(OS.Windows | OS.Linux)] Supports mapping NUnit platform strings to TUnit OS enum: - Win, Win32, Windows -> OS.Windows - Linux, Unix -> OS.Linux - MacOsX, MacOS, OSX, Mac -> OS.MacOs Unknown platforms are removed (cannot convert). Co-Authored-By: Claude Opus 4.5 <[email protected]>
Address potential `dotnet format` crashes on multi-targeted projects by adding deterministic syntax-based fallbacks when semantic analysis fails or produces different results across TFMs. Changes: - IsLikelyComparerArgument: Now tries semantic analysis first, then uses syntax-based detection (checking for variable/type names containing "comparer") as fallback - IsFrameworkAssertion: Now uses syntax-based fallback to check for known assertion type patterns (Assert, ClassicAssert, etc.) - Added IsKnownAssertionTypeBySyntax abstract method to each framework rewriter to provide framework-specific assertion type detection The key insight is that when semantic analysis fails on one TFM but succeeds on another, it produces different code transformations that can't be merged during dotnet format. By using deterministic syntax-based fallbacks, we ensure consistent behavior across all TFMs. Related to #4344 Co-Authored-By: Claude Opus 4.5 <[email protected]>
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
SummaryThis PR adds defensive syntax-based fallbacks to migration code fixers to prevent crashes during dotnet format on multi-targeted projects when semantic analysis fails across TFMs. Critical Issues1. Return Type Change Needs VerificationThe IsLikelyComparerArgument method changed from returning bool? to bool. The call sites in NUnitMigrationCodeFixProvider.cs at lines 1372 and 1393 still use IsLikelyComparerArgument(arguments[2]) == true, which suggests they were expecting nullable behavior. While this still works with the new bool return, the intent has changed: Before: null meant unknown/ambiguous resulting in conservative default behavior The change to non-nullable is correct IF the syntax fallback is reliable. However, verify that all call sites handle the new behavior correctly, especially when semantic analysis previously returned null. 2. Syntax-Based Comparer Detection May Be Too BroadIn IsLikelyComparerArgumentBySyntax (AssertionRewriter.cs line 376 in diff), the check lowerExpressionText.EndsWith("comparer") could match unintended patterns like myStringComparerFactory or isComparer (boolean variable). Consider being more specific by checking for complete tokens rather than substrings. This is not blocking if tests pass, but worth considering for robustness. 3. Record.Exception Try-Catch Indentation Logic Is ComplexThe new XUnit ConvertRecordException method generates try-catch blocks with manual indentation handling. This is fragile with mixed tabs/spaces, different indentation widths, and editorconfig settings. Have you tested this with various indentation styles (2-space, 4-space, tabs, mixed)? Suggestions4. Platform Attribute Mapping IncompletenessThe MapNUnitPlatformToTUnitOS method maps platform names, but NUnit supports many more platforms (net variants, mono). Currently unmapped platforms are silently ignored (returns null), which removes the attribute. Consider adding a TODO comment when an unknown platform is encountered. 5. Missing Abstract Method DocumentationThe new IsKnownAssertionTypeBySyntax abstract method needs XML documentation explaining when derived classes should return true vs false, with examples showing NUnit returning true for Assert, CollectionAssert, StringAssert but NOT ClassicAssert. Previous Review StatusNo previous comments found. VerdictTesting Note: The PR states All 520 analyzer tests pass which is excellent. However, please verify: (1) Tests with multi-targeted projects (the original issue scenario), (2) Edge cases for syntax-based fallbacks (unusual variable names, nested expressions), (3) Various indentation styles for Record.Exception conversion. |
…igrator
The GetTypeInfo().Type! could cause NullReferenceException if semantic analysis
fails for implicit object creation expressions (new() { ... }). This adds a
helper method that safely handles null/error types and catches exceptions.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
SummaryAdds defensive syntax-based fallbacks to migration code fixers to prevent crashes during Critical IssuesNone found ✅ The previous review raised valid concerns about the return type change from
The semantic change (null → false with syntax fallback) is acceptable because the fallback provides deterministic behavior when semantic analysis fails across TFMs, which is the core issue being addressed. Suggestions1. Syntax-Based Comparer Detection Could Match Unintended PatternsIn if (lowerExpressionText.EndsWith("comparer") || ...)Could match edge cases like:
However, since:
This is acceptable. If false positives occur in practice, they can be refined incrementally. 2. Record.Exception Indentation HandlingThe
Recommendation: Consider using Roslyn's 3. Platform Attribute Mapping Silent Failures
_ => null // TODO: TUnit migration - Unknown platform '{nunitPlatform}' could not be converted4. Missing XML DocumentationThe new
Minor: Add XML doc comments for maintainability. Previous Review StatusThe previous review flagged:
TUnit Rules Compliance✅ Dual-Mode: N/A (analyzers only, not core engine) Verdict✅ APPROVE - No critical issues. The previous concerns were addressed (return type) or are acceptable trade-offs (syntax detection robustness vs. determinism). The PR successfully solves the multi-TFM crash issue with 520 passing tests. |
Summary
dotnet formaton multi-targeted projectsChanges
IsLikelyComparerArgument(AssertionRewriter.cs)bool?toboolfor consistencyIsFrameworkAssertion(AssertionRewriter.cs)IsKnownAssertionTypeBySyntaxabstract methodIsKnownAssertionTypeBySyntax(new abstract method)Added to each framework's assertion rewriter with framework-specific patterns:
Assert,CollectionAssert,StringAssert,FileAssert,DirectoryAssertAssert,CollectionAssert,StringAssert,FileAssert,DirectoryAssertAssertNote:
ClassicAssertis explicitly excluded from NUnit's syntax-based detection because it's inNUnit.Framework.Legacywhich should not be auto-converted.Problem Addressed
Related to #4344 -
dotnet formatcrashes on multi-targeted projects with error "Changes must be within bounds of SourceText".The root cause is that SemanticModel can produce different results for different TFMs, leading to divergent code transformations that can't be merged during linked file processing.
Test plan
🤖 Generated with Claude Code