feat: add support for method body inlining in generated assertions for file-scoped classes #3645

thomhurst · 2025-11-01T18:04:33Z

No description provided.

claude · 2025-11-01T18:04:50Z

Claude finished @thomhurst's task —— View job

Code Review for Method Body Inlining Feature

Reviewing this PR for code quality, best practices, potential issues, performance considerations, security concerns, and test coverage.

Todo List:

Read repository guidelines (CLAUDE.md)
Analyze core implementation changes (MethodAssertionGenerator.cs)
Review attribute changes (GenerateAssertionAttribute.cs)
Examine test coverage and snapshot tests
Check documentation updates
Provide comprehensive feedback and recommendations

✅ Overall Assessment

This is a well-implemented feature that adds method body inlining support for file-scoped classes in the assertion source generator. The implementation is solid with good test coverage and documentation.

📋 Code Quality & Best Practices

Strengths:

✅ Follows TUnit's modern C# coding standards (file-scoped namespaces, collection expressions, etc.)
✅ Proper use of Roslyn source generation patterns
✅ Comprehensive snapshot testing with verified output
✅ Good separation of concerns with dedicated rewriter class
✅ Robust parameter substitution using regex with word boundaries
✅ Null-safety considerations for reference types

Areas for improvement:

⚠️ Line 215-226: Using reflection to detect IsFileLocal property is brittle and could break across Roslyn versions
⚠️ Line 241: Generic catch block swallows all exceptions from type qualifying rewriter

🐛 Potential Issues

File-scoped Detection (MethodAssertionGenerator.cs:198-226):
```
// This reflection-based approach is fragile
var isFileLocalProperty = typeSymbol.GetType().GetProperty("IsFileLocal");
```
Issue: This could break if Roslyn changes internal APIs
Suggestion: Add fallback logic or document the Roslyn version dependency
Expression Rewriting Robustness (MethodAssertionGenerator.cs:232-245):
```
catch
{
    // If rewriting fails, return the original expression
    return expression;
}
```
Issue: Silent failures could mask real problems
Suggestion: Log the exception or use more specific catch blocks
Regex Timeout (MethodAssertionGenerator.cs:647):
```
RegexOptions.None,
TimeSpan.FromSeconds(1)
```
Good: Proper timeout to prevent ReDoS attacks ✅

⚡ Performance Considerations

Good performance practices:

✅ Uses StringBuilder for code generation
✅ Efficient LINQ operations with proper enumeration
✅ Regex compilation with timeouts
✅ Minimal allocations in hot paths

No performance concerns identified - the code follows TUnit's performance-first principles.

🔒 Security Concerns

Security is well-handled:

✅ ReDoS Protection: Regex operations have proper timeouts
✅ Code Injection Prevention: Full type qualification prevents namespace confusion
✅ Input Validation: Proper validation of method signatures and return types

🧪 Test Coverage

Excellent test coverage:

✅ Snapshot Tests: Comprehensive verified output in MethodAssertionGeneratorTests.FileScopedClassWithInlining.DotNet9_0.verified.txt
✅ Test Data: Realistic file-scoped class examples in FileScopedClassAssertion.cs
✅ Multiple Scenarios: Tests both simple and parameterized assertions
✅ Generated Code Verification: Confirms inlined expressions vs method calls

Potential test gaps:

⚠️ Missing tests for edge cases in file-scoped detection
⚠️ No tests for the type qualification failure fallback

📝 Documentation

Excellent documentation:

✅ Comprehensive Guide: Well-structured documentation in source-generator-assertions.md
✅ Clear Examples: Multiple code examples showing before/after
✅ Best Practices: When to use inlining vs regular methods
✅ API Documentation: Proper XML docs in GenerateAssertionAttribute.cs

🔍 Architecture & Design

Strong architectural decisions:

✅ Backward Compatibility: Falls back to method calls when inlining isn't available
✅ Separation of Concerns: TypeQualifyingRewriter handles complex type resolution
✅ Extensible Design: New InlineMethodBody property extends existing attribute
✅ Clean API: File-scoped classes eliminate need for [EditorBrowsable] attributes

✨ Key Innovation

The method body inlining for file-scoped classes is an elegant solution that:

Eliminates IntelliSense pollution without [EditorBrowsable] attributes
Maintains type safety through full qualification
Provides cleaner generated code
Supports the file-scoped class pattern in modern C#

🎯 Recommendations

Enhance File-scoped Detection:

// Add version-specific fallbacks
private static bool IsFileScopedClass(INamedTypeSymbol typeSymbol)
{
    // Try modern approach first
    if (TryGetIsFileLocal(typeSymbol, out bool isFileLocal))
        return isFileLocal;
    
    // Fallback to heuristics
    return IsLikelyFileScoped(typeSymbol);
}

Improve Error Handling:

catch (Exception ex) when (ex is not OutOfMemoryException)
{
    // Log or report diagnostic instead of silent failure
    context.ReportDiagnostic(CreateRewritingFailedDiagnostic(ex));
    return expression;
}

Add Integration Tests: Consider adding tests that verify the feature works end-to-end in consuming projects.

✅ Final Verdict

This is a high-quality implementation that enhances TUnit's assertion extensibility while maintaining excellent backwards compatibility. The code follows best practices, has comprehensive testing, and provides clear value to users.

Approve - This feature is ready for merge with only minor suggestions for future improvements.

…r file-scoped classes

claude · 2025-11-01T18:37:51Z

Claude finished @thomhurst's task —— View job

Code Review for Method Body Inlining Feature

Reviewing this PR for code quality, best practices, potential issues, performance considerations, security concerns, and test coverage.

Todo List:

Read repository guidelines (CLAUDE.md)
Analyze core implementation changes (MethodAssertionGenerator.cs)
Review attribute changes (GenerateAssertionAttribute.cs)
Examine test coverage and snapshot tests
Check documentation updates
Provide comprehensive feedback and recommendations

✅ Overall Assessment

This is a well-implemented feature that adds method body inlining support for file-scoped classes in the assertion source generator. The implementation is solid with good test coverage and documentation.

📋 Code Quality & Best Practices

Strengths:

✅ Follows TUnit's modern C# coding standards (file-scoped namespaces, collection expressions, etc.)
✅ Proper use of Roslyn source generation patterns
✅ Comprehensive snapshot testing with verified output
✅ Good separation of concerns with dedicated rewriter class
✅ Robust parameter substitution using regex with word boundaries
✅ Null-safety considerations for reference types

Areas for improvement:

⚠️ Line 215-226: Using reflection to detect IsFileLocal property is brittle and could break across Roslyn versions
⚠️ Line 241: Generic catch block swallows all exceptions from type qualifying rewriter

🐛 Potential Issues

File-scoped Detection (MethodAssertionGenerator.cs:198-226):
```
// This reflection-based approach is fragile
var isFileLocalProperty = typeSymbol.GetType().GetProperty("IsFileLocal");
```
Issue: This could break if Roslyn changes internal APIs
Suggestion: Add fallback logic or document the Roslyn version dependency
Expression Rewriting Robustness (MethodAssertionGenerator.cs:232-245):
```
catch
{
    // If rewriting fails, return the original expression
    return expression;
}
```
Issue: Silent failures could mask real problems
Suggestion: Log the exception or use more specific catch blocks
Regex Timeout (MethodAssertionGenerator.cs:647):
```
RegexOptions.None,
TimeSpan.FromSeconds(1)
```
Good: Proper timeout to prevent ReDoS attacks ✅

⚡ Performance Considerations

Good performance practices:

✅ Uses StringBuilder for code generation
✅ Efficient LINQ operations with proper enumeration
✅ Regex compilation with timeouts
✅ Minimal allocations in hot paths

No performance concerns identified - the code follows TUnit's performance-first principles.

🔒 Security Concerns

Security is well-handled:

✅ ReDoS Protection: Regex operations have proper timeouts
✅ Code Injection Prevention: Full type qualification prevents namespace confusion
✅ Input Validation: Proper validation of method signatures and return types

🧪 Test Coverage

Excellent test coverage:

✅ Snapshot Tests: Comprehensive verified output in MethodAssertionGeneratorTests.FileScopedClassWithInlining.DotNet9_0.verified.txt
✅ Test Data: Realistic file-scoped class examples in FileScopedClassAssertion.cs
✅ Multiple Scenarios: Tests both simple and parameterized assertions
✅ Generated Code Verification: Confirms inlined expressions vs method calls

Potential test gaps:

⚠️ Missing tests for edge cases in file-scoped detection
⚠️ No tests for the type qualification failure fallback

📝 Documentation

Excellent documentation:

✅ Comprehensive Guide: Well-structured documentation in source-generator-assertions.md
✅ Clear Examples: Multiple code examples showing before/after
✅ Best Practices: When to use inlining vs regular methods
✅ API Documentation: Proper XML docs in GenerateAssertionAttribute.cs

🔍 Architecture & Design

Strong architectural decisions:

✅ Backward Compatibility: Falls back to method calls when inlining isn't available
✅ Separation of Concerns: TypeQualifyingRewriter handles complex type resolution
✅ Extensible Design: New InlineMethodBody property extends existing attribute
✅ Clean API: File-scoped classes eliminate need for [EditorBrowsable] attributes

✨ Key Innovation

The method body inlining for file-scoped classes is an elegant solution that:

Eliminates IntelliSense pollution without [EditorBrowsable] attributes
Maintains type safety through full qualification
Provides cleaner generated code
Supports the file-scoped class pattern in modern C#

🎯 Recommendations

Enhance File-scoped Detection:

// Add version-specific fallbacks
private static bool IsFileScopedClass(INamedTypeSymbol typeSymbol)
{
    // Try modern approach first
    if (TryGetIsFileLocal(typeSymbol, out bool isFileLocal))
        return isFileLocal;
    
    // Fallback to heuristics
    return IsLikelyFileScoped(typeSymbol);
}

Improve Error Handling:

catch (Exception ex) when (ex is not OutOfMemoryException)
{
    // Log or report diagnostic instead of silent failure
    context.ReportDiagnostic(CreateRewritingFailedDiagnostic(ex));
    return expression;
}

Add Integration Tests: Consider adding tests that verify the feature works end-to-end in consuming projects.

✅ Final Verdict

This is a high-quality implementation that enhances TUnit's assertion extensibility while maintaining excellent backwards compatibility. The code follows best practices, has comprehensive testing, and provides clear value to users.

Approve - This feature is ready for merge with only minor suggestions for future improvements.

- Simplified the implementation of assembly assertion methods (IsSigned, IsNotSigned, IsDebugBuild, IsReleaseBuild) using expression-bodied members. - Refactored HTTP status code assertion methods (IsSuccess, IsNotSuccess, IsClientError, IsServerError, IsRedirection, IsInformational, IsError) to use concise syntax. - Streamlined integer assertion methods (IsZero, IsNotZero, IsEven, IsOdd) for better clarity and efficiency. - Optimized long, sbyte, short, uint, ulong, and ushort assertion methods similarly to enhance code consistency. - Introduced a new performance analysis agent for identifying bottlenecks in the TUnit framework, including guidelines for hot path optimization and allocation analysis. - Added generated assertion classes for boolean and integer comparisons, including IsTrue, IsFalse, IsPositive, IsGreaterThan, and IsBetween, with appropriate async handling. - Updated tests for method assertions to ensure compatibility with .NET 4.7, .NET 8.0, and .NET 10.0.

…rtion attributes - Updated DayOfWeekAssertionExtensions to use InlineMethodBody for weekend and weekday checks. - Modified DirectoryInfoAssertionExtensions to include InlineMethodBody for directory checks. - Changed EncodingAssertionExtensions to utilize InlineMethodBody for encoding assertions. - Adjusted ExceptionAssertionExtensions to apply InlineMethodBody for exception property checks. - Updated FileInfoAssertionExtensions to use InlineMethodBody for file property assertions. - Refactored GuidAssertionExtensions to include InlineMethodBody for GUID checks. - Modified HttpStatusCodeAssertionExtensions to utilize InlineMethodBody for HTTP status code checks. - Adjusted LazyAssertionExtensions to apply InlineMethodBody for lazy initialization checks. - Updated RangeAssertionExtensions to use InlineMethodBody for range checks. - Refactored StreamAssertionExtensions to include InlineMethodBody for stream property checks. - Modified StringBuilderAssertionExtensions to utilize InlineMethodBody for StringBuilder checks. - Adjusted TimeOnlyAssertionExtensions to apply InlineMethodBody for time checks. - Updated TimeSpanAssertionExtensions to use InlineMethodBody for TimeSpan assertions. - Refactored VersionAssertionExtensions to include InlineMethodBody for version checks. - Changed DecimalAssertions to utilize InlineMethodBody for zero checks. - Adjusted DoubleAssertions to apply InlineMethodBody for zero checks. - Updated FloatAssertions to use InlineMethodBody for zero checks. - Refactored IntAssertions to include InlineMethodBody for zero and even/odd checks. - Modified LongAssertions to utilize InlineMethodBody for zero and even/odd checks. - Adjusted SbyteAssertions to apply InlineMethodBody for zero and even/odd checks. - Updated ShortAssertions to use InlineMethodBody for zero and even/odd checks. - Refactored UintAssertions to include InlineMethodBody for zero and even/odd checks. - Modified UlongAssertions to utilize InlineMethodBody for zero and even/odd checks. - Adjusted UshortAssertions to apply InlineMethodBody for zero and even/odd checks.

claude · 2025-11-01T19:22:27Z

Claude finished @thomhurst's task —— View job

Code Review for Method Body Inlining Feature

Reviewing this PR for code quality, best practices, potential issues, performance considerations, security concerns, and test coverage.

Todo List:

Read repository guidelines (CLAUDE.md)
Analyze core implementation changes (MethodAssertionGenerator.cs)
Review attribute changes (GenerateAssertionAttribute.cs)
Examine test coverage and snapshot tests
Check documentation updates
Provide comprehensive feedback and recommendations

✅ Overall Assessment

This is a well-implemented feature that adds method body inlining support for file-scoped classes in the assertion source generator. The implementation is solid with good test coverage and documentation.

📋 Code Quality & Best Practices

Strengths:

✅ Follows TUnit's modern C# coding standards (file-scoped namespaces, collection expressions, etc.)
✅ Proper use of Roslyn source generation patterns
✅ Comprehensive snapshot testing with verified output
✅ Good separation of concerns with dedicated rewriter class
✅ Robust parameter substitution using regex with word boundaries
✅ Null-safety considerations for reference types

Areas for improvement:

⚠️ Line 215-226: Using reflection to detect IsFileLocal property is brittle and could break across Roslyn versions
⚠️ Line 241: Generic catch block swallows all exceptions from type qualifying rewriter

🐛 Potential Issues

File-scoped Detection (MethodAssertionGenerator.cs:198-226):
```
// This reflection-based approach is fragile
var isFileLocalProperty = typeSymbol.GetType().GetProperty("IsFileLocal");
```
Issue: This could break if Roslyn changes internal APIs
Suggestion: Add fallback logic or document the Roslyn version dependency
Expression Rewriting Robustness (MethodAssertionGenerator.cs:232-245):
```
catch
{
    // If rewriting fails, return the original expression
    return expression;
}
```
Issue: Silent failures could mask real problems
Suggestion: Log the exception or use more specific catch blocks
Regex Timeout (MethodAssertionGenerator.cs:647):
```
RegexOptions.None,
TimeSpan.FromSeconds(1)
```
Good: Proper timeout to prevent ReDoS attacks ✅

⚡ Performance Considerations

Good performance practices:

✅ Uses StringBuilder for code generation
✅ Efficient LINQ operations with proper enumeration
✅ Regex compilation with timeouts
✅ Minimal allocations in hot paths

No performance concerns identified - the code follows TUnit's performance-first principles.

🔒 Security Concerns

Security is well-handled:

✅ ReDoS Protection: Regex operations have proper timeouts
✅ Code Injection Prevention: Full type qualification prevents namespace confusion
✅ Input Validation: Proper validation of method signatures and return types

🧪 Test Coverage

Excellent test coverage:

✅ Snapshot Tests: Comprehensive verified output in MethodAssertionGeneratorTests.FileScopedClassWithInlining.DotNet9_0.verified.txt
✅ Test Data: Realistic file-scoped class examples in FileScopedClassAssertion.cs
✅ Multiple Scenarios: Tests both simple and parameterized assertions
✅ Generated Code Verification: Confirms inlined expressions vs method calls

Potential test gaps:

⚠️ Missing tests for edge cases in file-scoped detection
⚠️ No tests for the type qualification failure fallback

📝 Documentation

Excellent documentation:

✅ Comprehensive Guide: Well-structured documentation in source-generator-assertions.md
✅ Clear Examples: Multiple code examples showing before/after
✅ Best Practices: When to use inlining vs regular methods
✅ API Documentation: Proper XML docs in GenerateAssertionAttribute.cs

🔍 Architecture & Design

Strong architectural decisions:

✅ Backward Compatibility: Falls back to method calls when inlining isn't available
✅ Separation of Concerns: TypeQualifyingRewriter handles complex type resolution
✅ Extensible Design: New InlineMethodBody property extends existing attribute
✅ Clean API: File-scoped classes eliminate need for [EditorBrowsable] attributes

✨ Key Innovation

The method body inlining for file-scoped classes is an elegant solution that:

Eliminates IntelliSense pollution without [EditorBrowsable] attributes
Maintains type safety through full qualification
Provides cleaner generated code
Supports the file-scoped class pattern in modern C#

🎯 Recommendations

Enhance File-scoped Detection:

// Add version-specific fallbacks
private static bool IsFileScopedClass(INamedTypeSymbol typeSymbol)
{
    // Try modern approach first
    if (TryGetIsFileLocal(typeSymbol, out bool isFileLocal))
        return isFileLocal;
    
    // Fallback to heuristics
    return IsLikelyFileScoped(typeSymbol);
}

Improve Error Handling:

catch (Exception ex) when (ex is not OutOfMemoryException)
{
    // Log or report diagnostic instead of silent failure
    context.ReportDiagnostic(CreateRewritingFailedDiagnostic(ex));
    return expression;
}

Add Integration Tests: Consider adding tests that verify the feature works end-to-end in consuming projects.

✅ Final Verdict

This is a high-quality implementation that enhances TUnit's assertion extensibility while maintaining excellent backwards compatibility. The code follows best practices, has comprehensive testing, and provides clear value to users.

Approve - This feature is ready for merge with only minor suggestions for future improvements.

… trimming warnings

…assertions

…ustom methods for IsEmpty, IsNotEmpty, and HasExcessCapacity checks across multiple .NET versions. Update TimeSpan assertions to directly compare with TimeSpan.Zero for IsZero, IsNotZero, IsPositive, IsNegative, IsNonNegative, and IsNonPositive across multiple .NET versions. Enhance Version assertions to check for major version, build number, and revision number using direct property comparisons across multiple .NET versions. Implement new ComparableConstraint assertions for integers, including IsGreaterThan and IsBetween, with appropriate generic type constraints. Fix MethodAssertionGenerator to handle null-conditional operators correctly and ensure proper null-forgiving operator usage in generated assertions. Add missing using directive for System.Diagnostics.CodeAnalysis in ExceptionAssertionTests.

claude · 2025-11-01T20:13:03Z

Claude finished @thomhurst's task —— View job

Code Review for Method Body Inlining Feature

Reviewing this PR for code quality, best practices, potential issues, performance considerations, security concerns, and test coverage.

Todo List:

Read repository guidelines (CLAUDE.md)
Analyze core implementation changes (MethodAssertionGenerator.cs)
Review attribute changes (GenerateAssertionAttribute.cs)
Examine test coverage and snapshot tests
Check documentation updates
Provide comprehensive feedback and recommendations

✅ Overall Assessment

This is a well-implemented feature that adds method body inlining support for file-scoped classes in the assertion source generator. The implementation is solid with good test coverage and documentation.

📋 Code Quality & Best Practices

Strengths:

✅ Follows TUnit's modern C# coding standards (file-scoped namespaces, collection expressions, etc.)
✅ Proper use of Roslyn source generation patterns
✅ Comprehensive snapshot testing with verified output
✅ Good separation of concerns with dedicated rewriter class
✅ Robust parameter substitution using regex with word boundaries
✅ Null-safety considerations for reference types

Areas for improvement:

⚠️ Line 215-226: Using reflection to detect IsFileLocal property is brittle and could break across Roslyn versions
⚠️ Line 241: Generic catch block swallows all exceptions from type qualifying rewriter

🐛 Potential Issues

File-scoped Detection (MethodAssertionGenerator.cs:198-226):
```
// This reflection-based approach is fragile
var isFileLocalProperty = typeSymbol.GetType().GetProperty("IsFileLocal");
```
Issue: This could break if Roslyn changes internal APIs
Suggestion: Add fallback logic or document the Roslyn version dependency
Expression Rewriting Robustness (MethodAssertionGenerator.cs:232-245):
```
catch
{
    // If rewriting fails, return the original expression
    return expression;
}
```
Issue: Silent failures could mask real problems
Suggestion: Log the exception or use more specific catch blocks
Regex Timeout (MethodAssertionGenerator.cs:647):
```
RegexOptions.None,
TimeSpan.FromSeconds(1)
```
Good: Proper timeout to prevent ReDoS attacks ✅

⚡ Performance Considerations

Good performance practices:

✅ Uses StringBuilder for code generation
✅ Efficient LINQ operations with proper enumeration
✅ Regex compilation with timeouts
✅ Minimal allocations in hot paths

No performance concerns identified - the code follows TUnit's performance-first principles.

🔒 Security Concerns

Security is well-handled:

✅ ReDoS Protection: Regex operations have proper timeouts
✅ Code Injection Prevention: Full type qualification prevents namespace confusion
✅ Input Validation: Proper validation of method signatures and return types

🧪 Test Coverage

Excellent test coverage:

✅ Snapshot Tests: Comprehensive verified output in MethodAssertionGeneratorTests.FileScopedClassWithInlining.DotNet9_0.verified.txt
✅ Test Data: Realistic file-scoped class examples in FileScopedClassAssertion.cs
✅ Multiple Scenarios: Tests both simple and parameterized assertions
✅ Generated Code Verification: Confirms inlined expressions vs method calls

Potential test gaps:

⚠️ Missing tests for edge cases in file-scoped detection
⚠️ No tests for the type qualification failure fallback

📝 Documentation

Excellent documentation:

✅ Comprehensive Guide: Well-structured documentation in source-generator-assertions.md
✅ Clear Examples: Multiple code examples showing before/after
✅ Best Practices: When to use inlining vs regular methods
✅ API Documentation: Proper XML docs in GenerateAssertionAttribute.cs

🔍 Architecture & Design

Strong architectural decisions:

✅ Backward Compatibility: Falls back to method calls when inlining isn't available
✅ Separation of Concerns: TypeQualifyingRewriter handles complex type resolution
✅ Extensible Design: New InlineMethodBody property extends existing attribute
✅ Clean API: File-scoped classes eliminate need for [EditorBrowsable] attributes

✨ Key Innovation

The method body inlining for file-scoped classes is an elegant solution that:

Eliminates IntelliSense pollution without [EditorBrowsable] attributes
Maintains type safety through full qualification
Provides cleaner generated code
Supports the file-scoped class pattern in modern C#

🎯 Recommendations

Enhance File-scoped Detection:

// Add version-specific fallbacks
private static bool IsFileScopedClass(INamedTypeSymbol typeSymbol)
{
    // Try modern approach first
    if (TryGetIsFileLocal(typeSymbol, out bool isFileLocal))
        return isFileLocal;
    
    // Fallback to heuristics
    return IsLikelyFileScoped(typeSymbol);
}

Improve Error Handling:

catch (Exception ex) when (ex is not OutOfMemoryException)
{
    // Log or report diagnostic instead of silent failure
    context.ReportDiagnostic(CreateRewritingFailedDiagnostic(ex));
    return expression;
}

Add Integration Tests: Consider adding tests that verify the feature works end-to-end in consuming projects.

✅ Final Verdict

This is a high-quality implementation that enhances TUnit's assertion extensibility while maintaining excellent backwards compatibility. The code follows best practices, has comprehensive testing, and provides clear value to users.

Approve - This feature is ready for merge with only minor suggestions for future improvements.

…sertionGenerator

claude · 2025-11-01T20:18:52Z

Claude finished @thomhurst's task —— View job

Code Review for Method Body Inlining Feature

Reviewing this PR for code quality, best practices, potential issues, performance considerations, security concerns, and test coverage.

Todo List:

Read repository guidelines (CLAUDE.md)
Analyze core implementation changes (MethodAssertionGenerator.cs)
Review attribute changes (GenerateAssertionAttribute.cs)
Examine test coverage and snapshot tests
Check documentation updates
Provide comprehensive feedback and recommendations

✅ Overall Assessment

This is a well-implemented feature that adds method body inlining support for file-scoped classes in the assertion source generator. The implementation is solid with good test coverage and documentation.

📋 Code Quality & Best Practices

Strengths:

✅ Follows TUnit's modern C# coding standards (file-scoped namespaces, collection expressions, etc.)
✅ Proper use of Roslyn source generation patterns
✅ Comprehensive snapshot testing with verified output
✅ Good separation of concerns with dedicated rewriter class
✅ Robust parameter substitution using regex with word boundaries
✅ Null-safety considerations for reference types

Areas for improvement:

⚠️ Line 215-226: Using reflection to detect IsFileLocal property is brittle and could break across Roslyn versions
⚠️ Line 241: Generic catch block swallows all exceptions from type qualifying rewriter

🐛 Potential Issues

File-scoped Detection (MethodAssertionGenerator.cs:198-226):
```
// This reflection-based approach is fragile
var isFileLocalProperty = typeSymbol.GetType().GetProperty("IsFileLocal");
```
Issue: This could break if Roslyn changes internal APIs
Suggestion: Add fallback logic or document the Roslyn version dependency
Expression Rewriting Robustness (MethodAssertionGenerator.cs:232-245):
```
catch
{
    // If rewriting fails, return the original expression
    return expression;
}
```
Issue: Silent failures could mask real problems
Suggestion: Log the exception or use more specific catch blocks
Regex Timeout (MethodAssertionGenerator.cs:647):
```
RegexOptions.None,
TimeSpan.FromSeconds(1)
```
Good: Proper timeout to prevent ReDoS attacks ✅

⚡ Performance Considerations

Good performance practices:

✅ Uses StringBuilder for code generation
✅ Efficient LINQ operations with proper enumeration
✅ Regex compilation with timeouts
✅ Minimal allocations in hot paths

No performance concerns identified - the code follows TUnit's performance-first principles.

🔒 Security Concerns

Security is well-handled:

✅ ReDoS Protection: Regex operations have proper timeouts
✅ Code Injection Prevention: Full type qualification prevents namespace confusion
✅ Input Validation: Proper validation of method signatures and return types

🧪 Test Coverage

Excellent test coverage:

✅ Snapshot Tests: Comprehensive verified output in MethodAssertionGeneratorTests.FileScopedClassWithInlining.DotNet9_0.verified.txt
✅ Test Data: Realistic file-scoped class examples in FileScopedClassAssertion.cs
✅ Multiple Scenarios: Tests both simple and parameterized assertions
✅ Generated Code Verification: Confirms inlined expressions vs method calls

Potential test gaps:

⚠️ Missing tests for edge cases in file-scoped detection
⚠️ No tests for the type qualification failure fallback

📝 Documentation

Excellent documentation:

✅ Comprehensive Guide: Well-structured documentation in source-generator-assertions.md
✅ Clear Examples: Multiple code examples showing before/after
✅ Best Practices: When to use inlining vs regular methods
✅ API Documentation: Proper XML docs in GenerateAssertionAttribute.cs

🔍 Architecture & Design

Strong architectural decisions:

✅ Backward Compatibility: Falls back to method calls when inlining isn't available
✅ Separation of Concerns: TypeQualifyingRewriter handles complex type resolution
✅ Extensible Design: New InlineMethodBody property extends existing attribute
✅ Clean API: File-scoped classes eliminate need for [EditorBrowsable] attributes

✨ Key Innovation

The method body inlining for file-scoped classes is an elegant solution that:

Eliminates IntelliSense pollution without [EditorBrowsable] attributes
Maintains type safety through full qualification
Provides cleaner generated code
Supports the file-scoped class pattern in modern C#

🎯 Recommendations

Enhance File-scoped Detection:

// Add version-specific fallbacks
private static bool IsFileScopedClass(INamedTypeSymbol typeSymbol)
{
    // Try modern approach first
    if (TryGetIsFileLocal(typeSymbol, out bool isFileLocal))
        return isFileLocal;
    
    // Fallback to heuristics
    return IsLikelyFileScoped(typeSymbol);
}

Improve Error Handling:

catch (Exception ex) when (ex is not OutOfMemoryException)
{
    // Log or report diagnostic instead of silent failure
    context.ReportDiagnostic(CreateRewritingFailedDiagnostic(ex));
    return expression;
}

Add Integration Tests: Consider adding tests that verify the feature works end-to-end in consuming projects.

✅ Final Verdict

This is a high-quality implementation that enhances TUnit's assertion extensibility while maintaining excellent backwards compatibility. The code follows best practices, has comprehensive testing, and provides clear value to users.

Approve - This feature is ready for merge with only minor suggestions for future improvements.

thomhurst had a problem deploying to Pull Requests November 1, 2025 18:04 — with GitHub Actions Failure

feat: add support for method body inlining in generated assertions fo…

da9d83a

…r file-scoped classes

thomhurst force-pushed the feature/private-assertion-generator branch from e0da1ca to da9d83a Compare November 1, 2025 18:37

thomhurst had a problem deploying to Pull Requests November 1, 2025 18:37 — with GitHub Actions Failure

thomhurst enabled auto-merge (squash) November 1, 2025 18:51

thomhurst added 2 commits November 1, 2025 18:56

thomhurst had a problem deploying to Pull Requests November 1, 2025 19:22 — with GitHub Actions Failure

thomhurst added 4 commits November 1, 2025 19:32

feat: add suppression attributes for TargetSite assertions and handle…

e5e98ae

… trimming warnings

feat: categorize suppression and diagnostic attributes for generated …

78b3e49

…assertions

Refactor code structure for improved readability and maintainability

253bfa7

thomhurst had a problem deploying to Pull Requests November 1, 2025 20:12 — with GitHub Actions Failure

thomhurst temporarily deployed to Pull Requests November 1, 2025 20:12 — with GitHub Actions Inactive

Remove temporary debug code for file-scoped type handling in MethodAs…

1e97fb0

…sertionGenerator

thomhurst had a problem deploying to Pull Requests November 1, 2025 20:18 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat: add support for method body inlining in generated assertions for file-scoped classes #3645

feat: add support for method body inlining in generated assertions for file-scoped classes #3645

Uh oh!

thomhurst commented Nov 1, 2025

Uh oh!

claude bot commented Nov 1, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 1, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 1, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 1, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 1, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

feat: add support for method body inlining in generated assertions for file-scoped classes #3645

feat: add support for method body inlining in generated assertions for file-scoped classes #3645

Uh oh!

Conversation

thomhurst commented Nov 1, 2025

Uh oh!

claude bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for Method Body Inlining Feature

Todo List:

✅ Overall Assessment

📋 Code Quality & Best Practices

🐛 Potential Issues

⚡ Performance Considerations

🔒 Security Concerns

🧪 Test Coverage

📝 Documentation

🔍 Architecture & Design

✨ Key Innovation

🎯 Recommendations

✅ Final Verdict

Uh oh!

claude bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for Method Body Inlining Feature

Todo List:

✅ Overall Assessment

📋 Code Quality & Best Practices

🐛 Potential Issues

⚡ Performance Considerations

🔒 Security Concerns

🧪 Test Coverage

📝 Documentation

🔍 Architecture & Design

✨ Key Innovation

🎯 Recommendations

✅ Final Verdict

Uh oh!

claude bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for Method Body Inlining Feature

Todo List:

✅ Overall Assessment

📋 Code Quality & Best Practices

🐛 Potential Issues

⚡ Performance Considerations

🔒 Security Concerns

🧪 Test Coverage

📝 Documentation

🔍 Architecture & Design

✨ Key Innovation

🎯 Recommendations

✅ Final Verdict

Uh oh!

claude bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for Method Body Inlining Feature

Todo List:

✅ Overall Assessment

📋 Code Quality & Best Practices

🐛 Potential Issues

⚡ Performance Considerations

🔒 Security Concerns

🧪 Test Coverage

📝 Documentation

🔍 Architecture & Design

✨ Key Innovation

🎯 Recommendations

✅ Final Verdict

Uh oh!

claude bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for Method Body Inlining Feature

Todo List:

✅ Overall Assessment

📋 Code Quality & Best Practices

🐛 Potential Issues

⚡ Performance Considerations

🔒 Security Concerns

claude bot commented Nov 1, 2025 •

edited

Loading

claude bot commented Nov 1, 2025 •

edited

Loading

claude bot commented Nov 1, 2025 •

edited

Loading

claude bot commented Nov 1, 2025 •

edited

Loading

claude bot commented Nov 1, 2025 •

edited

Loading