Cleanup ValidationsGenerator for better incrementality#66373
Cleanup ValidationsGenerator for better incrementality#66373Youssef1313 wants to merge 8 commits intomainfrom
Conversation
github-actions
bot
added
the
area-infrastructure
There was a problem hiding this comment.
Pull request overview
This PR refactors the Microsoft.Extensions.Validation source generator pipeline to improve incrementality by avoiding keeping Roslyn symbols in the incremental pipeline, and removes dead validation-attribute modeling code.
Changes:
- Replace stored Roslyn symbols in generator models with fully-qualified type name strings (FQNs) to improve incremental caching behavior.
- Simplify endpoint/type parsing to return only the data needed for emission (validatable types/properties).
- Remove unused
ValidationAttributemodel and related extraction logic.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Validation/gen/ValidationsGenerator.cs | Updates the incremental pipeline to extract validatable types from endpoints directly. |
| src/Validation/gen/Parsers/ValidationsGenerator.TypesParser.cs | Switches parsing to store FQNs, removes attribute-model extraction, and adjusts helper signatures. |
| src/Validation/gen/Parsers/ValidationsGenerator.EndpointsParser.cs | Returns validatable types from the transform step (instead of Roslyn operations). |
| src/Validation/gen/Parsers/ValidationsGenerator.AttributeParser.cs | Updates syntax-kind checks for attribute-based discovery. |
| src/Validation/gen/Parsers/ValidationsGenerator.AddValidation.cs | Updates invocation detection to use SyntaxKind checks. |
| src/Validation/gen/Models/ValidationAttribute.cs | Deletes the unused ValidationAttribute generator model. |
| src/Validation/gen/Models/ValidatableTypeComparer.cs | Updates equality to compare by type FQN (string) instead of ITypeSymbol. |
| src/Validation/gen/Models/ValidatableType.cs | Changes model to store TypeFQN instead of ITypeSymbol. |
| src/Validation/gen/Models/ValidatableProperty.cs | Changes model to store containing/property type FQNs and removes attribute payload. |
| src/Validation/gen/Emitters/ValidationsGenerator.Emitter.cs | Emits typeof(...) checks using stored FQNs instead of Roslyn symbols. |
| src/Http/Http.Extensions/gen/.../InvocationOperationExtensions.cs | Adds an InvocationExpressionSyntax overload and simplifies member-access pattern matching. |
Youssef1313
force-pushed
the
dev/ygerges/cleanup-validations-gen
branch
from
ac20142 to
5178e70
Compare
Youssef1313
force-pushed
the
dev/ygerges/cleanup-validations-gen
branch
from
5178e70 to
6134541
Compare
| ITypeSymbol ContainingType, | ||
| ITypeSymbol Type, | ||
| string ContainingTypeFQN, | ||
| string TypeFQN, |
There was a problem hiding this comment.
These changes seem to be just shifting around of operations. I am not sure that replacing ITypeSymbol in ValidatableProperty metadata with the type's string FullyQualifiedName helps with incrementality (because ITypeSymbol already implements IEquatable) and it does not reduce the number of operations (because the name is resolved once per emit, just in a different location).
There was a problem hiding this comment.
@oroztocil It should help with incrementality. See discussion in dotnet/roslyn-analyzers#6352 for why it's bad to include ITypeSymbol in generator pipeline (even if it does implement IEquatable)
| if (syntaxNode is InvocationExpressionSyntax | ||
| && syntaxNode.TryGetMapMethodName(out var method) | ||
| if (syntaxNode.IsKind(SyntaxKind.InvocationExpression) | ||
| && ((InvocationExpressionSyntax)syntaxNode).TryGetMapMethodName(out var method) |
There was a problem hiding this comment.
Although I'd agree that from intuition syntaxNode.IsKind(SyntaxKind.InvocationExpression) feels like it should be faster than syntaxNode is InvocationExpressionSyntax, I am not a Roslyn expert and would be interested in empirical backing for it.
There was a problem hiding this comment.
There is an analyzer for that (but only used by roslyn repo itself AFAIK):
There was a problem hiding this comment.
The analyzer I linked is actually different. It doesn't consider type checking.
There was a problem hiding this comment.
@333fred Was there something about the perf of IsKind + cast vs doing a type check?
This PR cleans up ValidationsGenerator.
ValidationAttributecalculations were dead code. The properties of it were never used.