Add BindAsync support to RequestDelegateGenerator #47033
Conversation
# Conflicts: # src/Http/Http.Extensions/gen/RequestDelegateGenerator.cs # src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs # src/Http/Http.Extensions/test/RequestDelegateGenerator/Baselines/MapAction_ExplicitSource_SimpleReturn_Snapshot.generated.txt # src/Http/Http.Extensions/test/RequestDelegateGenerator/Baselines/MapAction_SingleComplexTypeParam_StringReturn.generated.txt # src/Http/Http.Extensions/test/RequestDelegateGenerator/Baselines/MapAction_SingleEnumParam_StringReturn.generated.txt # src/Http/Http.Extensions/test/RequestDelegateGenerator/Baselines/MapAction_SingleTimeOnlyParam_StringReturn.generated.txt # src/Http/Http.Extensions/test/RequestDelegateGenerator/Baselines/Multiple_MapAction_NoParam_StringReturn.generated.txt # src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateGeneratorTests.cs
mitchdenny
left a comment
mitchdenny
left a comment
There was a problem hiding this comment.
Looks good, although the analyzer seems to have now lost its support to warn about not implementing BindAsync as an alternative to TryParse ... seems intentional though - interested to understand the reasoning behind that.
| public class Customer | ||
| { | ||
| public async static Task<Customer> BindAsync(HttpContext context) | ||
| public async static ValueTask<Customer> BindAsync(HttpContext context) |
There was a problem hiding this comment.
I think the intention for this test was to fail due to the return type of BindAsync being Task<T> and not ValueTask<T>
| return parameterTypeSymbol; | ||
| } | ||
|
|
||
| static DiagnosticDescriptor SelectDescriptor(Parsability parsability, Bindability bindability) |
There was a problem hiding this comment.
Are you introducing a new analyzer here? I thought that we wanted to cover parsability and bindability detection in that analyzer?
| { | ||
| // Change this to true and run tests in development to regenerate baseline files. | ||
| // Then: cp artifacts/bin/Microsoft.AspNetCore.Http.Extensions.Tests/Debug/net8.0/RequestDelegateGenerator/Baselines/* src/Http/Http.Extensions/test/RequestDelegateGenerator/Baselines | ||
| public bool RegenerateBaselines => false; |
There was a problem hiding this comment.
Good addition. In the CompareLines(...) method we should probably add logic ignore lines with the code analysis attribute so that when folks follow the instruction above they then don't need to do a regex/replace on the attributes with the %%...%%.
There was a problem hiding this comment.
... I see you've taken care of this below :)
| using System.Text; | ||
|
|
||
| namespace Microsoft.AspNetCore.Analyzers.Infrastructure; | ||
| internal enum Parsability |
| SymbolEqualityComparer.Default.Equals(methodSymbol.Parameters[1].Type, wellKnownTypes.Get(WellKnownType.System_Reflection_ParameterInfo)) && | ||
| methodSymbol.ReturnType is INamedTypeSymbol returnType && | ||
| IsReturningValueTaskOfT(returnType, typeSymbol, wellKnownTypes); | ||
| IsReturningValueTaskOfTOrNullableT(returnType, typeSymbol, wellKnownTypes); |
ghost
added this to the
Fixes #46338