Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #2183

@claude
Copy link
Contributor

claude bot commented Oct 24, 2025

Claude finished @thomhurst's task —— View job


PR Review Complete

Review Checklist:

  • Read and analyze the changed file
  • Evaluate code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Review security concerns
  • Evaluate test coverage needs
  • Provide comprehensive feedback

Code Review Feedback

This PR adds essential error handling to prevent crashes in the source generator when dealing with invalid typeof() expressions and empty nameof() calls. The changes are well-implemented and follow defensive programming principles.

Positive Aspects

  1. Robust Error Handling: Both fixes properly handle edge cases that could crash the source generator:

    • VisitTypeOfExpression: Handles null symbols (invalid types)
    • VisitInvocationExpression: Handles nameof() with zero arguments
  2. Graceful Degradation: Instead of crashing, the code:

    • Falls back to base implementation for typeof() with invalid types
    • Returns empty string literal for invalid nameof() calls
    • Allows the C# compiler to report proper error messages
  3. Code Quality:

    • Clear, descriptive comments explaining the rationale
    • Consistent with existing error handling patterns in the codebase
    • Removes the risky null-forgiving operator (symbol!) on line 153
  4. Performance: No performance impact - these are edge case handlers that rarely execute

🔍 Detailed Analysis

VisitTypeOfExpression Fix (TUnit.Core.SourceGenerator/CodeGenerators/Helpers/FullyQualifiedWithGlobalPrefixRewriter.cs:143-148):

if (symbol is null)
{
    return base.VisitTypeOfExpression(node) ?? node;
}
  • Good: Prevents NullReferenceException when GetSymbolInfo returns null
  • Good: Falls back to base implementation, maintaining the syntax tree structure
  • Good: Uses null-coalescing to ensure non-null return value

VisitInvocationExpression Fix (TUnit.Core.SourceGenerator/CodeGenerators/Helpers/FullyQualifiedWithGlobalPrefixRewriter.cs:170-179):

if (argumentList.Arguments.Count == 0)
{
    return SyntaxFactory.LiteralExpression(
        SyntaxKind.StringLiteralExpression,
        SyntaxFactory.Literal(string.Empty)
    );
}
  • Good: Prevents IndexOutOfRangeException when accessing Arguments[0]
  • Good: Returns meaningful empty string instead of crashing
  • Good: Allows compiler to report proper error for invalid nameof() usage

🚨 Potential Concerns

  1. Test Coverage: No specific tests were added for these error conditions. Consider adding:

    // Test case for typeof() with invalid type
    [Test] public void TestTypeOfInvalidType() { ... }
    
    // Test case for nameof() with no arguments  
    [Test] public void TestNameOfNoArguments() { ... }
  2. Error Consistency: The fix returns different fallback values:

    • typeof(): Falls back to base implementation
    • nameof(): Returns empty string literal

    While both are reasonable, consider documenting this difference in behavior.

🔒 Security Assessment

  • No security concerns: The changes are purely defensive and don't introduce new attack vectors
  • Improves stability: Prevents potential DoS through malformed code that crashes the generator

🏃 Performance Assessment

  • No performance impact: These are guard clauses that execute only on malformed input
  • Maintains efficiency: Falls back to existing implementations where possible

📋 Recommendations

  1. Add Unit Tests: Create tests to verify the error handling works correctly:

    [Test]
    public void TypeOfExpression_WithInvalidType_DoesNotCrash() 
    {
        // Test source generator with typeof(NonExistentType)
    }
    
    [Test]  
    public void NameOfExpression_WithNoArguments_ReturnsEmptyString()
    {
        // Test source generator with nameof()
    }
  2. Consider Logging: For diagnostic purposes, consider adding debug logging when these fallbacks are triggered

  3. Documentation: Update any relevant documentation about error handling in the source generator

Overall Assessment

This is a solid defensive fix that addresses real crash scenarios in the source generator. The implementation is clean, well-commented, and follows established patterns. The only improvement would be adding explicit test coverage for these error conditions.

Recommendation: ✅ Approve with suggestion to add tests


@thomhurst thomhurst merged commit 452b512 into main Oct 24, 2025
13 checks passed
@thomhurst thomhurst deleted the bug/2183 branch October 24, 2025 18:45
This was referenced Oct 24, 2025
This was referenced Oct 27, 2025
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.

Invalid attribute arguments break the generator and cause multiple Roslyn service crashes in VS

2 participants