Fix ValidationsGenerator ITypeParameterSymbol handling#65419
Fix ValidationsGenerator ITypeParameterSymbol handling#65419ANcpLua wants to merge 2 commits intodotnet:mainfrom
Conversation
…eters When endpoint handlers use generic extension methods (e.g., MapCommand<TRequest>()), the resolved method has ITypeParameterSymbol parameters. These have DeclaredAccessibility == NotApplicable, which fails the accessibility check in TryExtractValidatableType, causing the type to be silently skipped without any diagnostic. Handle ITypeParameterSymbol before the accessibility check by walking constraint types to discover validatable concrete types. Add the type parameter to visitedTypes before iterating constraints to prevent stack overflow with circular constraints. Add ContainsTypeParameter guard in ExtractValidatableMembers to prevent invalid typeof() expressions in generated code for properties whose types contain unresolved type parameters (e.g., CRTP patterns).
|
Thanks for your PR, @@ANcpLua. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the ValidationsGenerator source generator where generic type parameters (like TRequest from a generic endpoint extension method) were either silently skipped or caused compilation errors when they leaked into generated typeof() expressions. The fix adds type parameter handling to discover validatable types through constraint types, while preventing type parameters from appearing in emitted code.
Changes:
- Added
ITypeParameterSymbolbranch inTryExtractValidatableTypeto walk constraint types and discover validatable types - Added
ContainsTypeParameterguards in both record and regular property extraction paths to prevent emittingtypeof(T)expressions - Added
ContainsTypeParameterhelper method to recursively detect type parameters in type trees
| // Type parameters (e.g., TRequest from a generic MapCommand<TRequest>() extension) | ||
| // have DeclaredAccessibility == NotApplicable. The concrete type is only known at | ||
| // call sites, not inside the generic method body where the endpoint delegate is | ||
| // defined. Walk constraint types to discover any validatable types reachable | ||
| // through type constraints. | ||
| if (typeSymbol is ITypeParameterSymbol typeParam) | ||
| { | ||
| // Add to visitedTypes BEFORE iterating constraints to prevent | ||
| // infinite recursion through circular constraints such as | ||
| // where T : class, IEnumerable<T> (SEC-001). | ||
| visitedTypes.Add(typeSymbol); | ||
| var foundValidatable = false; | ||
| foreach (var constraintType in typeParam.ConstraintTypes) | ||
| { | ||
| foundValidatable |= TryExtractValidatableType(constraintType, wellKnownTypes, ref validatableTypes, ref visitedTypes); | ||
| } | ||
| return foundValidatable; | ||
| } |
There was a problem hiding this comment.
This PR fixes a critical bug that would cause compilation errors when type parameters leak into generated code. However, there are no tests covering the scenarios described in the PR description and issue #65418:
- Generic endpoint extension
MapCommand<TRequest>()with constrained type parameter - CRTP pattern
RequestBase<TSelf> where TSelf : RequestBase<TSelf> - Circular constraints
where T : class, IEnumerable<T> - Edge cases like
List<T>,T[],Dictionary<string, T>,Nullable<T>
The existing test files in src/Validation/test/Microsoft.Extensions.Validation.GeneratorTests/ show comprehensive test coverage for other features (records, polymorphism, IValidatableObject, etc.), but there's no test file for generic type parameters.
Without tests, there's a risk that:
- Future changes could break this fix
- The fix may not actually work in all the scenarios it's intended to handle
- Edge cases may not be covered
Please add tests that verify:
- Type parameters in endpoint handler signatures are discovered through constraints
- CRTP patterns don't emit typeof(TSelf)
- Circular constraints don't cause stack overflow
- Properties with type parameter types are correctly skipped in both record and class code paths
There was a problem hiding this comment.
Added tests in e20d0e4 — covers CRTP class and record patterns with runtime endpoint verification for Required and Range attributes.
Verifies that types using the Curiously Recurring Template Pattern (CommandBase<TSelf> where TSelf : CommandBase<TSelf>) are correctly discovered and validated through the inheritance hierarchy. Covers both class and record CRTP patterns with runtime endpoint verification for Required and Range validation attributes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dotnet-policy-service agree |
|
CI failures are pre-existing infrastructure flakes, unrelated to this change:
Happy to re-run if needed. |
|
Commenter does not have sufficient privileges for PR 65419 in repo dotnet/aspnetcore |
1 similar comment
|
Commenter does not have sufficient privileges for PR 65419 in repo dotnet/aspnetcore |
Fixes #65418
Summary
The
ValidationsGeneratorsilently skips generic type parameters (ITypeParameterSymbol) and can emit invalidtypeof(T)expressions when type parameters leak intoValidatableProperty.Type.Changes
Single file:
src/Validation/gen/Parsers/ValidationsGenerator.TypesParser.csITypeParameterSymbolbranch inTryExtractValidatableType— recognizes type parameters and walks theirConstraintTypesto discover concrete validatable types. Adds tovisitedTypesbefore iterating constraints to prevent infinite recursion through circular constraints (e.g.,where T : class, IEnumerable<T>).ContainsTypeParameterguards — two guards (record property path + regular property path) that skip properties whose type contains an unresolved type parameter anywhere in the type tree. Without these, the emitter would generatetypeof(TSelf)for CRTP patterns likeRequestBase<TSelf>.ContainsTypeParameterhelper — recursively checksITypeParameterSymbol,IArrayTypeSymbol, andINamedTypeSymbol.TypeArgumentsto catchT,T[],List<T>,Dictionary<string, T>,Nullable<T>, etc.Why all four blocks are one fix
ITypeParameterSymbolbranch alone (without guards) causes a regression: constraint walking discovers types whose properties contain the type parameter, leading totypeof(TSelf)in emitted code.Test plan
CommandBase<TSelf> where TSelf : CommandBase<TSelf>with concrete derived types validates correctly (class + record paths)[Required]) and derived type properties ([Range]) discovered through CRTP inheritance