-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Fix ValidationsGenerator ITypeParameterSymbol handling #65419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
fd4cedc
e20d0e4
b2b0dce
9e6a22a
bab9c95
fdf8bf6
a58595c
8508a90
85e971f
a8d2674
1556630
9be2650
e95aa0c
333f003
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,25 @@ internal static bool TryExtractValidatableType(ITypeSymbol incomingTypeSymbol, W | |
| return false; | ||
| } | ||
|
|
||
| // 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>. | ||
| visitedTypes.Add(typeSymbol); | ||
| var foundValidatable = false; | ||
| foreach (var constraintType in typeParam.ConstraintTypes) | ||
| { | ||
| foundValidatable |= TryExtractValidatableType(constraintType, wellKnownTypes, validatableTypes, visitedTypes); | ||
| } | ||
| return foundValidatable; | ||
| } | ||
|
ANcpLua marked this conversation as resolved.
|
||
|
|
||
| // Skip types that are not accessible from generated code | ||
| if (typeSymbol.DeclaredAccessibility is not Accessibility.Public) | ||
| { | ||
|
|
@@ -209,6 +228,16 @@ private static ImmutableArray<ValidatableProperty> ExtractValidatableMembers(ITy | |
| validatableTypes, | ||
| visitedTypes); | ||
|
|
||
| // Skip properties whose type is a type parameter (e.g., TSelf | ||
| // from CRTP pattern RequestBase<TSelf>). The emitter would | ||
| // generate typeof(TSelf) which is not valid C#. Concrete | ||
| // validatable types reachable through constraints are already | ||
| // discovered by TryExtractValidatableType above. | ||
| if (ContainsTypeParameter(correspondingProperty.Type)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // Record primary-constructor parameters can carry [Display]/[DisplayName] too. | ||
| // Prefer the parameter's attribute over the property's. | ||
| var (paramLiteral, paramHasResource) = parameter.GetDisplayInfo(displayAttributeSymbol, displayNameAttributeSymbol); | ||
|
|
@@ -276,6 +305,14 @@ private static ImmutableArray<ValidatableProperty> ExtractValidatableMembers(ITy | |
| continue; | ||
| } | ||
|
|
||
| // Skip properties whose type is a type parameter (e.g., TSelf | ||
| // from CRTP pattern RequestBase<TSelf>). The emitter would | ||
| // generate typeof(TSelf) which is not valid C#. | ||
| if (ContainsTypeParameter(member.Type)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| var (memberLiteral, memberHasResource) = member.GetDisplayInfo(displayAttributeSymbol, displayNameAttributeSymbol); | ||
|
|
||
| members.Add(new ValidatableProperty( | ||
|
|
@@ -310,4 +347,40 @@ internal static bool HasIValidatableObjectInterface(ITypeSymbol typeSymbol, Well | |
| var validatableObjectSymbol = wellKnownTypes.Get(WellKnownTypeData.WellKnownType.System_ComponentModel_DataAnnotations_IValidatableObject); | ||
| return typeSymbol.ImplementsInterface(validatableObjectSymbol); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns true if the given type symbol contains an unresolved type parameter | ||
| /// anywhere in its type tree. This catches not only bare <c>T</c> but also | ||
| /// constructed types like <c>List<T></c>, <c>T[]</c>, <c>T?</c>, and | ||
| /// <c>Dictionary<string, T></c> — all of which would produce invalid | ||
| /// <c>typeof(...)</c> expressions in the emitted code. | ||
| /// </summary> | ||
| private static bool ContainsTypeParameter(ITypeSymbol type) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this code covered by tests?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes — as of 333f003: |
||
| { | ||
| // Bare type parameter: T, TSelf, TSelf? | ||
| if (type is ITypeParameterSymbol) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Array: T[], T[,], List<T>[] | ||
| if (type is IArrayTypeSymbol arrayType) | ||
| { | ||
| return ContainsTypeParameter(arrayType.ElementType); | ||
| } | ||
|
|
||
| // Constructed generic: List<T>, Dictionary<string, T>, Nullable<T>, Func<T, bool> | ||
| if (type is INamedTypeSymbol { IsGenericType: true } namedType) | ||
| { | ||
| foreach (var typeArg in namedType.TypeArguments) | ||
| { | ||
| if (ContainsTypeParameter(typeArg)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks reasonable, but I'm not sure of a good use case for this. Why would you make it generic and constrained instead of having the parameter as the type you need right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern comes from reusable endpoint mappers in vertical-slice / CQRS-style minimal APIs — one generic helper like
MapValidated<T>(...)registers many request DTOs, so the concrete type only flows through the type parameter. But the stronger motivation is shape-independent: today the generator emitstypeof(TSelf)intoValidatableInfoResolver.g.cswhenever a discovered model uses the CRTP pattern (e.g.class CreateOrderCommand : RequestBase<CreateOrderCommand>used directly as a handler parameter) — invalid C#, so the user's build breaks with errors inside generated code they can't edit. A source generator emitting uncompilable code is a correctness bug regardless of how common the trigger is; the constraint-walking half then makes discovery actually work for the generic-mapper shape instead of silently skipping it.