[OptionsValidator] source generator: emit ValidateAsync() for IAsyncValidateOptions<T>#130263
[OptionsValidator] source generator: emit ValidateAsync() for IAsyncValidateOptions<T>#130263ViveliDuCh wants to merge 2 commits into
Conversation
|
Tagging subscribers to this area: @dotnet/area-extensions-options |
There was a problem hiding this comment.
Pull request overview
This PR extends the Microsoft.Extensions.Options [OptionsValidator] source generator to support async options validation by emitting a ValidateAsync(string?, TOptions, CancellationToken) method when the validator implements IAsyncValidateOptions<T>. It also adds unit tests that exercise async member validation, async self-validation (IAsyncValidatableObject), and the “manual ValidateAsync isn’t overwritten” guard.
Changes:
- Add optional symbol loading for
IAsyncValidateOptions<T>andIAsyncValidatableObjectand thread async-generation decisions through the parser/model. - Emit generated
ValidateAsync(...)methods that useValidator.TryValidateValueAsync(...)and async self-validation viaawait foreach. - Add new unit tests and test models validating async behavior (success/failure, shared synthesized validator, manual override).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/OptionsRuntimeTests.cs | Adds async validation tests and new option/validator types used by those tests. |
| src/libraries/Microsoft.Extensions.Options/gen/SymbolLoader.cs | Loads async validation symbols as optional inputs for downlevel-safe generation. |
| src/libraries/Microsoft.Extensions.Options/gen/SymbolHolder.cs | Stores optional async symbol references used by parser/emitter. |
| src/libraries/Microsoft.Extensions.Options/gen/Parser.cs | Propagates async generation requirements through member and synthesized-validator parsing. |
| src/libraries/Microsoft.Extensions.Options/gen/Model/ValidatedModel.cs | Tracks per-model async self-validation and async method emission flags. |
| src/libraries/Microsoft.Extensions.Options/gen/Model/ValidatedMember.cs | Tracks whether nested/synthesized validators emit async methods so the emitter can await conditionally. |
| src/libraries/Microsoft.Extensions.Options/gen/Emitter.cs | Emits generated ValidateAsync(...) methods and async member/transitive/enumeration/self-validation code. |
| // Decide, per model, whether we additionally emit a ValidateAsync method. We do so when the | ||
| // validator type explicitly implements IAsyncValidateOptions<T> for this specific model (and the | ||
| // async symbols are available). A multi-model validator therefore only gets async validation for | ||
| // the models it opted into. If the user already hand-wrote a matching ValidateAsync, we skip | ||
| // generation to avoid emitting a duplicate member. The async requirement is propagated to any | ||
| // synthesized child validators reached from this model. | ||
| bool generateAsync = ValidatorImplementsAsyncInterfaceFor(validatorType, modelType) | ||
| && !AlreadyImplementsValidateAsyncMethod(validatorType, modelType); |
| // Detects a user-supplied ValidateAsync(string, TModel, CancellationToken) overload so the generator doesn't emit | ||
| // a duplicate member for a validator that already provides its own asynchronous implementation. | ||
| private static bool AlreadyImplementsValidateAsyncMethod(INamespaceOrTypeSymbol validatorType, ISymbol modelType) | ||
| => validatorType | ||
| .GetMembers("ValidateAsync") | ||
| .Where(m => m.Kind == SymbolKind.Method) | ||
| .Select(m => (IMethodSymbol)m) | ||
| .Any(m => m.Parameters.Length == NumValidationMethodArgs + 1 | ||
| && m.Parameters[0].Type.SpecialType == SpecialType.System_String | ||
| && SymbolEqualityComparer.Default.Equals(m.Parameters[1].Type, modelType)); |
There was a problem hiding this comment.
I agree with this suggestion in principle, though I don't think type comparisons should be done using ToDisplayString.
Also, the return type should probably not be checked here: if a method with the right parameters, but different return type exists, you can't implement the method anyway (at least not through implicit implementation).
…hesized validator cache
| bool generateAsync = ValidatorImplementsAsyncInterfaceFor(validatorType, modelType) | ||
| && !AlreadyImplementsValidateAsyncMethod(validatorType, modelType); |
There was a problem hiding this comment.
I think this should produce a diagnostic, instead of silently doing nothing, to stay symmetric with the sync case. I don't see a reason to delay this to a followup PR, as the PR description suggests.
| { | ||
| if (vm.ValidationAttributes.Count > 0) | ||
| { | ||
| GenAsyncMemberValidation(vm, ref staticValidationAttributesDict, cleanListsBeforeUse); |
There was a problem hiding this comment.
If all validation attributes on a member are sync-only, would it be worthwhile to perform their validation synchronously?
| if (willAwait) | ||
| { | ||
| OutLn($"return builder is null ? global::Microsoft.Extensions.Options.ValidateOptionsResult.Success : builder.Build();"); | ||
| } | ||
| else | ||
| { | ||
| OutLn($"return global::System.Threading.Tasks.Task.FromResult(builder is null ? global::Microsoft.Extensions.Options.ValidateOptionsResult.Success : builder.Build());"); | ||
| } |
There was a problem hiding this comment.
Tiny nit: builder is null ? global::Microsoft.Extensions.Options.ValidateOptionsResult.Success : builder.Build() could be extracted to a local variable (either in the generator or in the generated code) and reused in both branches.
| OutOpenBrace(); | ||
|
|
||
| OutLn($"var count = 0;"); | ||
| OutLn($"foreach (var o in options.{vm.Name}{valueAccess})"); |
There was a problem hiding this comment.
Have you considered adding support for validating items of an IAsyncEnumerable<T>?
Though that would require also modifying DataAnnotationValidateOptions and so it's going to be out of scope for this PR.
| { | ||
| OutLn($"context.MemberName = \"ValidateAsync\";"); | ||
| OutLn($"context.DisplayName = string.IsNullOrEmpty(name) ? \"ValidateAsync\" : $\"{{name}}.ValidateAsync\";"); | ||
| OutLn($"await foreach (var asyncValidationResult in global::System.Threading.Tasks.TaskAsyncEnumerableExtensions.ConfigureAwait(((global::System.ComponentModel.DataAnnotations.IAsyncValidatableObject)options).ValidateAsync(context, cancellationToken), false))"); |
There was a problem hiding this comment.
Nit: why is ConfigureAwait() called as a static method for IAsyncEnumerable<T> here but as an extension method for Task elsewhere?
| // Detects a user-supplied ValidateAsync(string, TModel, CancellationToken) overload so the generator doesn't emit | ||
| // a duplicate member for a validator that already provides its own asynchronous implementation. | ||
| private static bool AlreadyImplementsValidateAsyncMethod(INamespaceOrTypeSymbol validatorType, ISymbol modelType) | ||
| => validatorType | ||
| .GetMembers("ValidateAsync") | ||
| .Where(m => m.Kind == SymbolKind.Method) | ||
| .Select(m => (IMethodSymbol)m) | ||
| .Any(m => m.Parameters.Length == NumValidationMethodArgs + 1 | ||
| && m.Parameters[0].Type.SpecialType == SpecialType.System_String | ||
| && SymbolEqualityComparer.Default.Equals(m.Parameters[1].Type, modelType)); |
There was a problem hiding this comment.
I agree with this suggestion in principle, though I don't think type comparisons should be done using ToDisplayString.
Also, the return type should probably not be checked here: if a method with the right parameters, but different return type exists, you can't implement the method anyway (at least not through implicit implementation).
| // a duplicate member for a validator that already provides its own asynchronous implementation. | ||
| private static bool AlreadyImplementsValidateAsyncMethod(INamespaceOrTypeSymbol validatorType, ISymbol modelType) | ||
| => validatorType | ||
| .GetMembers("ValidateAsync") |
There was a problem hiding this comment.
I think this (and the existing sync version) should also check for the existence of an explicit interface implementation.
| // the models it opted into. If the user already hand-wrote a matching ValidateAsync, we skip | ||
| // generation to avoid emitting a duplicate member. The async requirement is propagated to any | ||
| // synthesized child validators reached from this model. | ||
| bool generateAsync = ValidatorImplementsAsyncInterfaceFor(validatorType, modelType) |
There was a problem hiding this comment.
Suggestion (optional, better-to-have): IAsyncValidateOptions<T> is compiled into every TFM of the Microsoft.Extensions.Options package (down to netstandard2.0), but IAsyncValidatableObject / Validator.TryValidateValueAsync are net11+. So when the package is referenced on net8/netstandard, a validator implementing IAsyncValidateOptions<T> sets generateAsync = true here, yet the emitter skips ValidateAsync because the async symbols are absent, and the class then fails to compile with a bare CS0535. This is not a regression (the same CS0535 happens today without this change), but emitting a SYSLIB diagnostic here (e.g. "source-generated ValidateAsync requires .NET 11 or later") would be friendlier than letting it surface as CS0535. Fine as a follow-up.
|
|
||
| AsyncOptionsValidator validator = new(); | ||
|
|
||
| ValidateOptionsResult asyncResult = await validator.ValidateAsync("AsyncOptions", options, default); |
There was a problem hiding this comment.
Suggestion: consider adding a cancellation test. Every async test passes default here, so nothing verifies the generated ValidateAsync actually forwards cancellationToken into TryValidateValueAsync, the child ValidateAsync calls, and IAsyncValidatableObject.ValidateAsync. A future emitter change that dropped the token (e.g. emitting ValidateAsync(context) or ..., default) would still pass every current test. A model whose ValidateAsync observes the token, validated with a pre-canceled CancellationTokenSource and Assert.ThrowsAnyAsync<OperationCanceledException>, would close that gap deterministically.
| } | ||
| else | ||
| { | ||
| OutLn($"return global::System.Threading.Tasks.Task.FromResult(builder is null ? global::Microsoft.Extensions.Options.ValidateOptionsResult.Success : builder.Build());"); |
There was a problem hiding this comment.
Nit: this non-async Task.FromResult branch (willAwait == false) looks untested. It's reachable, e.g. a validator implementing IAsyncValidateOptions<T> for a model that only self-validates synchronously via IValidatableObject with no validation attributes and no async children. All current async test models set willAwait = true, so only the async path is covered. A small test over such a model would guard this wrapping.
|
|
||
| #if NET | ||
| [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))] | ||
| public async Task TestAsyncValidationSucceeds() |
There was a problem hiding this comment.
Nit: these are behavioral tests only. The async emission isn't captured by either golden-file baseline, Main.cs's Baselines/*.g.cs comparisons or SourceGenerationTests/Baselines/*/Validators.g.cs (unchanged, since no existing input opts into IAsyncValidateOptions<T>, which is the "zero baseline drift"). So the emitted async shape (async modifier elision, Task.FromResult, await foreach + ConfigureAwait, [EnumeratorCancellation]) isn't locked anywhere. Consistent with the rest of this generator, one async golden baseline would guard against silent emission drift.
|
Suggestion: consider handling async-only validators in this PR rather than deferring it. Today the generator discovers models only through One more reason to fold it in here: with P7 being the last preview, there isn't much runway left to land it as a separate follow-up and still get preview coverage. Addressing it in this PR keeps the async support complete for the release rather than shipping a gap we'd have to fill later. I prototyped it to gauge the cost and risk: tarekgh@b1d605c The change is small and localized:
One caveat worth emphasizing: this should be paired with the gate-mismatch fix I flagged earlier. Risk on .NET 11 itself looks low. Because the flag stays I also added two runtime tests that give good confidence here: one asserts an async-only validator generates |
Fixes #128882
Adds source-generator support for the async Options validation API. When
[OptionsValidator]is applied to a type that implementsIAsyncValidateOptions<T>, the generator now emits aValidateAsync()method alongside the existing synchronousValidate().Follow-up to:
System.ComponentModel.DataAnnotations(Validator.TryValidateValueAsync,IAsyncValidatableObject)IAsyncValidateOptions<T>, async startup validation)This is the source-generator piece that #128788 explicitly deferred to the #128882 tracking issue. It is purely additive: no public API surface changes (
gen/+tests/only), and pure-synchronous compilations produce byte-for-byte identical output (verified: zero baseline drift across the 60SourceGenerationTests).What's included
ValidateAsync(string?, TOptions, CancellationToken)emitted next toValidate(...)when the validator implementsIAsyncValidateOptions<T>for that model.Validator.TryValidateValueAsync(...)instead of the syncTryValidateValue.IAsyncValidatableObject, the generated method drives it viaawait foreachoverValidateAsync(...); when it implements onlyIValidatableObject, it falls back to the synchronousValidate(...).[ValidateObjectMembers]and[ValidateEnumeratedItems]children synthesized in an async context emit their ownValidateAsync, which the parent awaits.IAsyncValidateOptions<TThisModel>), so a multi-model validator only getsValidateAsyncfor the models that opted in.ValidateAsync(string, TModel, CancellationToken), generation is skipped (mirrors the syncAlreadyImplementsValidateMethodbehavior).Validateexactly as before.ValidateAsyncis not overwritten".Not in scope (intentionally deferred to keep the change minimal)
Task.WhenAllfan-out (issue point 3). The generated body is sequential and deterministic, so failure ordering is stable and assertable. Parallelism would require per-taskValidationContext/scratch-list allocation (undoing the allocate-nothing-on-success design) and would only benefit genuinely I/O-bound validators; deferred pending a benchmarked scenario.IAsyncValidateOptions<T>withoutIValidateOptions<T>). Model discovery is still driven byIValidateOptions<T>; async emission is layered on top. Supporting async-only changes theSYSLIB1204diagnostic semantics and is a natural follow-up.AlreadyImplementsValidateAsyncMethoddiagnostic. The duplicate case is handled correctly (generation is skipped) but silently; surfacing a SYSLIB warning to match the sync UX needs a new resx string plus the localized.xlffan-out and can follow separately.Implementation notes
IAsyncValidatableObjectandIAsyncValidateOptions`1are resolved as optional symbols (null ⇒ feature skipped), stored as nullable onSymbolHolder.isAsyncflag down the member/synthesized-validator recursion, and bubbles anemitsAsyncflag back up. Because synthesized child validators are memoized by model type only, a cache hit reports the cached child's real async capability, so the parent awaitsValidateAsynconly when the child actually emits it (otherwise it calls the child's syncValidate). New model facts:ValidatedModel.{GenerateAsyncValidateMethod, SelfValidatesAsync}andValidatedMember.{TransValidatorEmitsAsync, EnumerationValidatorEmitsAsync}.GenAsyncModelValidationMethodmirrors the sync emitter. It elides theasyncmodifier and returnsTask.FromResult(...)when the body would contain noawait(avoids CS1998). All other structure (lazy builder, reused scratch lists,#if NETtrim-safeValidationContextctor) is unchanged from the sync path.Note
This PR description was drafted with GitHub Copilot assistance.