refactor: extract type validation logic to the common library #28
Conversation
Extract type validation logic from the source generator to the common library and create comprehensive unit tests to enable real-time analyzer implementation. This moves validation business logic to CompositeKey.Analyzers.Common where it can be shared between the source generator and upcoming analyzers that will provide real-time IDE diagnostics. Changes: - Extract TypeValidation class with comprehensive validation methods - Implement TypeValidationResult pattern for clean success/failure handling - Refactor source generator to use shared validation logic instead of inline code - Create extensive unit test suite with 95%+ code coverage for validation logic - Add test infrastructure with CompilationTestHelper for Roslyn testing - Maintain identical diagnostic behaviour with zero breaking changes The shared TypeValidation handles: - Record type validation and partial modifier requirements - Constructor accessibility and selection rules - Nested type declaration validation with precise error reporting - All edge cases including record structs, multiple constructors, and complex nesting All tests pass, including 18 new validation-specific tests, ensuring no regressions while establishing robust shared logic for analyzer implementation.
WalkthroughAdds a new shared TypeValidation API to CompositeKey.Analyzers.Common and rewires SourceGenerator.Parser to use it. Introduces a dedicated unit test project with Roslyn helpers and comprehensive tests for validation logic. Updates the solution to include the tests and adds corresponding project configuration and package lock. Changes
Sequence Diagram(s)sequenceDiagram
participant SG as SourceGenerator.Parser
participant TV as TypeValidation (Common)
participant Diag as DiagnosticReporter
SG->>TV: ValidateTypeForCompositeKey(typeSymbol, decl, model, attr, ct)
TV-->>SG: TypeValidationResult { IsSuccess, Descriptor?, MessageArgs?, Constructor?, Declarations? }
alt Validation failed
SG->>Diag: Report(validationResult.Descriptor, validationResult.MessageArgs)
SG-->>SG: Abort processing for this type
else Validation succeeded
SG-->>SG: Use Constructor and Declarations
SG->>SG: Parse parameters and properties
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
- Coverage 93.89% 93.88% -0.01%
==========================================
Files 27 28 +1
Lines 1064 1096 +32
Branches 158 160 +2
==========================================
+ Hits 999 1029 +30
- Misses 32 33 +1
- Partials 33 34 +1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
src/CompositeKey.Analyzers.Common.UnitTests/TestHelpers/CompilationTestHelper.cs (1)
13-14: Consider lazy initialization for System.Runtime assemblyThe System.Runtime assembly is loaded eagerly at static construction time. Consider lazy loading to avoid unnecessary overhead if the helper isn't used.
- private static readonly Assembly SystemRuntimeAssembly = Assembly.Load(new AssemblyName("System.Runtime")); + private static readonly Lazy<Assembly> SystemRuntimeAssembly = new(() => Assembly.Load(new AssemblyName("System.Runtime"))); private static readonly CSharpParseOptions DefaultParseOptions = new(LanguageVersion.CSharp11);And update the usage:
- MetadataReference.CreateFromFile(SystemRuntimeAssembly.Location), + MetadataReference.CreateFromFile(SystemRuntimeAssembly.Value.Location),src/CompositeKey.Analyzers.Common/Validation/TypeValidation.cs (3)
45-47: Complex LINQ filter could benefit from clarificationThe constructor filtering logic is dense and could be clearer with intermediate variables or comments explaining the exclusion criteria.
+ // Filter out: + // 1. Static constructors + // 2. Implicit parameterless constructors for value types + // 3. Copy constructors (single parameter of same type) var publicConstructors = typeSymbol.Constructors .Where(c => !c.IsStatic && !(c.IsImplicitlyDeclared && typeSymbol.IsValueType && c.Parameters.Length == 0)) .Where(c => !(c.Parameters.Length == 1 && SymbolEqualityComparer.Default.Equals(c.Parameters[0].Type, typeSymbol))) .ToArray();
58-59: Consider more specific error for multiple attributed constructorsWhen multiple constructors have the CompositeKeyConstructor attribute, returning false doesn't indicate the specific issue. Consider a more descriptive error.
Consider returning a specific error result or throwing an exception with a clear message when multiple constructors are marked with the attribute, rather than just returning false.
112-123: Consider using expression-bodied switch for consistencyThe switch expression could be more concise while maintaining readability.
- private static string GetTypeKindKeyword(TypeDeclarationSyntax typeDeclarationSyntax) => - typeDeclarationSyntax.Kind() switch - { - SyntaxKind.ClassDeclaration => "class", - SyntaxKind.InterfaceDeclaration => "interface", - SyntaxKind.StructDeclaration => "struct", - SyntaxKind.RecordDeclaration => "record", - SyntaxKind.RecordStructDeclaration => "record struct", - SyntaxKind.EnumDeclaration => "enum", - SyntaxKind.DelegateDeclaration => "delegate", - _ => throw new ArgumentOutOfRangeException(nameof(typeDeclarationSyntax)) - }; + private static string GetTypeKindKeyword(TypeDeclarationSyntax typeDeclarationSyntax) => typeDeclarationSyntax.Kind() switch + { + SyntaxKind.ClassDeclaration => "class", + SyntaxKind.InterfaceDeclaration => "interface", + SyntaxKind.StructDeclaration => "struct", + SyntaxKind.RecordDeclaration => "record", + SyntaxKind.RecordStructDeclaration => "record struct", + SyntaxKind.EnumDeclaration => "enum", + SyntaxKind.DelegateDeclaration => "delegate", + _ => throw new ArgumentOutOfRangeException(nameof(typeDeclarationSyntax), $"Unexpected type kind: {typeDeclarationSyntax.Kind()}") + };src/CompositeKey.SourceGeneration/SourceGenerator.Parser.cs (1)
72-74: Comment could be more precise about null-state guaranteesThe comment mentions "guaranteed non-null due to MemberNotNullWhen on IsSuccess" but could be clearer about which specific properties are guaranteed.
- // Use validated data from the validation result (guaranteed non-null due to MemberNotNullWhen on IsSuccess) + // Use validated data from the validation result (Constructor and TargetTypeDeclarations are guaranteed non-null when IsSuccess is true)src/CompositeKey.Analyzers.Common.UnitTests/Validation/TypeValidationTests.cs (10)
7-8: Consider making the test class non-static for better test organisation.Static test classes are unconventional in xUnit and can limit test extensibility. Consider removing the
staticmodifier to align with standard xUnit test class patterns.-public static class TypeValidationTests +public class TypeValidationTests
49-71: Extract repeated source generation logic to reduce duplication.The switch expression creates similar source code patterns with minor variations. Consider extracting a helper method to generate the source code template.
Add a helper method to generate the test source:
private static string GenerateTypeSource(string typeKind, string typeName) { var propertyDeclaration = typeKind == "interface" ? "Guid Id { get; set; }" : "public Guid Id { get; set; }"; return $$""" using System; using CompositeKey; [CompositeKey("{Id}")] public partial {{typeKind}} {{typeName}} { {{propertyDeclaration}} } """; }Then simplify the test:
- string source = typeKind switch - { - "interface" => $$""" - using System; - using CompositeKey; - - [CompositeKey("{Id}")] - public partial {{typeKind}} {{typeName}} - { - Guid Id { get; set; } - } - """, - _ => $$""" - using System; - using CompositeKey; - - [CompositeKey("{Id}")] - public partial {{typeKind}} {{typeName}} - { - public Guid Id { get; set; } - } - """ - }; + string source = GenerateTypeSource(typeKind, typeName);
126-127: Improve test comment clarity.The comment could be more descriptive about the specific validation rule being tested.
- // Arrange - Multiple constructors but none parameterless and none attributed + // Arrange - Multiple public constructors without a parameterless one or CompositeKeyConstructor attribute
199-200: Remove redundant comment as the test name is self-documenting.The comment "Record structs are supported" duplicates what the test name already conveys.
- // Arrange - Record structs are supported + // Arrange
221-222: Remove redundant comment that duplicates the assertion.The comment doesn't add value beyond what the assertion already verifies.
// Assert - // Record structs are valid composite key types result.IsSuccess.ShouldBeTrue();
325-326: Make test comment more precise.The comment could be clearer about the expected behaviour when no obvious constructor choice exists.
- // Arrange - Multiple constructors, no parameterless, no attributed + // Arrange - Multiple parameterised constructors without parameterless or attributed constructor
487-488: Clarify copy constructor test comment.The inline comment could better explain why copy constructors are excluded from consideration.
- // Copy constructor should be ignored + // Copy constructor (takes same type parameter) should be excluded from constructor selection
505-507: Improve assertion comment clarity.The comment could be more explicit about what constitutes a copy constructor.
- // Should return primary constructor, not copy constructor + // Should return the primary constructor (Guid parameter), not the copy constructor (TestKey parameter)
524-525: Clarify the null attribute test scenario.The comment could better explain the test's purpose - verifying graceful handling when the attribute type isn't available.
- // Act - Pass null for attribute symbol + // Act - Pass null for attribute symbol to test fallback constructor selection logic
568-569: Make preference assertion comment more specific.The comment could be clearer about the constructor selection priority.
- // Should prefer the parameterless constructor + // Should prefer parameterless constructor over parameterised constructors when multiple exist
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CompositeKey.slnx(1 hunks)src/CompositeKey.Analyzers.Common.UnitTests/CompositeKey.Analyzers.Common.UnitTests.csproj(1 hunks)src/CompositeKey.Analyzers.Common.UnitTests/TestHelpers/CompilationTestHelper.cs(1 hunks)src/CompositeKey.Analyzers.Common.UnitTests/Validation/TypeValidationTests.cs(1 hunks)src/CompositeKey.Analyzers.Common.UnitTests/packages.lock.json(1 hunks)src/CompositeKey.Analyzers.Common/Validation/TypeValidation.cs(1 hunks)src/CompositeKey.SourceGeneration/SourceGenerator.Parser.cs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/CompositeKey.Analyzers.Common/Validation/TypeValidation.cs (2)
src/CompositeKey.Analyzers.Common.UnitTests/TestHelpers/CompilationTestHelper.cs (2)
INamedTypeSymbol(59-62)SemanticModel(39-54)src/CompositeKey.Analyzers.Common/Diagnostics/DiagnosticDescriptors.cs (1)
DiagnosticDescriptors(10-101)
src/CompositeKey.SourceGeneration/SourceGenerator.Parser.cs (1)
src/CompositeKey.Analyzers.Common/Validation/TypeValidation.cs (1)
TypeValidation(10-124)
src/CompositeKey.Analyzers.Common.UnitTests/TestHelpers/CompilationTestHelper.cs (2)
src/CompositeKey.SourceGeneration.UnitTests/CompilationHelper.cs (1)
CSharpParseOptions(25-25)src/CompositeKey/CompositeKeyAttribute.cs (1)
CompositeKeyAttribute(13-44)
src/CompositeKey.Analyzers.Common.UnitTests/Validation/TypeValidationTests.cs (3)
src/CompositeKey.Analyzers.Common.UnitTests/TestHelpers/CompilationTestHelper.cs (1)
CompilationTestHelper(11-63)src/CompositeKey.Analyzers.Common/Validation/TypeValidation.cs (2)
TypeValidation(10-124)TryGetObviousOrExplicitlyMarkedConstructor(37-71)src/CompositeKey.Analyzers.Common/Diagnostics/DiagnosticDescriptors.cs (1)
DiagnosticDescriptors(10-101)
🔇 Additional comments (7)
src/CompositeKey.Analyzers.Common.UnitTests/packages.lock.json (1)
1-645: Package lock file looks goodThe package lock file correctly captures dependencies for both .NET 8.0 and .NET 9.0 target frameworks with appropriate testing and Roslyn packages.
src/CompositeKey.Analyzers.Common/Validation/TypeValidation.cs (2)
10-36: Well-structured validation APIThe
ValidateTypeForCompositeKeymethod provides a clean, centralised validation approach with clear success/failure paths using the result pattern. The sequential validation checks are logical and comprehensive.
126-150: Good use of result pattern with proper null-state annotationsThe
TypeValidationResultrecord effectively uses the result pattern withMemberNotNullWhenattributes to ensure compile-time null safety. The static factory methods provide a clean API.src/CompositeKey.SourceGeneration/SourceGenerator.Parser.cs (1)
58-74: Excellent refactoring to use shared validationThe replacement of inline validation logic with the centralised
TypeValidation.ValidateTypeForCompositeKeycall improves code maintainability and reusability. The error handling through the validation result pattern is clean and consistent.src/CompositeKey.Analyzers.Common.UnitTests/CompositeKey.Analyzers.Common.UnitTests.csproj (1)
1-40: LGTM!The project file is well-structured with appropriate target frameworks, test dependencies, and project references for testing the shared validation logic.
src/CompositeKey.Analyzers.Common.UnitTests/Validation/TypeValidationTests.cs (2)
11-260: Comprehensive test coverage for type validation scenarios.The test suite thoroughly covers the core validation scenarios including valid records, non-record types, partial requirements, constructor selection, nested types, and record structs. The tests are well-structured and follow AAA pattern consistently.
262-571: Excellent coverage of constructor selection logic.The test suite comprehensively covers all constructor selection scenarios including primary constructors, parameterless constructors, attributed constructors, copy constructor exclusion, and preference ordering. The edge cases are well-tested.
Extract type validation logic from the source generator to the common library and create comprehensive unit tests to enable real-time analyzer implementation.
This moves validation business logic to CompositeKey.Analyzers.Common where it can be shared between the source generator and upcoming analyzers that will provide real-time IDE diagnostics.
Changes:
The shared TypeValidation handles:
All tests pass, including 18 new validation-specific tests, ensuring no regressions while establishing robust shared logic for analyzer implementation.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores