Skip to content

Commit 0a74dff

Browse files
captainsafiacaptainsafia
authored
Fix BindAsync and misc bugs in RDG (#49693)
* Fix BindAsync and misc bugs in RDG * Address feedback from review * Update requiredness checks for BindAsync params * Fix requirendess checks for struct types
1 parent 5917847 commit 0a74dff

14 files changed

+844
-268
lines changed

‎src/Http/Http.Extensions/gen/DiagnosticDescriptors.cs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -48,62 +48,62 @@ internal static class DiagnosticDescriptors
4848
new LocalizableResourceString(nameof(Resources.InvalidAsParametersAbstractType_Title), Resources.ResourceManager, typeof(Resources)),
4949
new LocalizableResourceString(nameof(Resources.InvalidAsParametersAbstractType_Message), Resources.ResourceManager, typeof(Resources)),
5050
"Usage",
51-
DiagnosticSeverity.Error,
51+
DiagnosticSeverity.Warning,
5252
isEnabledByDefault: true);
5353

5454
public static DiagnosticDescriptor InvalidAsParametersSignature { get; } = new(
5555
"RDG006",
5656
new LocalizableResourceString(nameof(Resources.InvalidAsParametersSignature_Title), Resources.ResourceManager, typeof(Resources)),
5757
new LocalizableResourceString(nameof(Resources.InvalidAsParametersSignature_Message), Resources.ResourceManager, typeof(Resources)),
5858
"Usage",
59-
DiagnosticSeverity.Error,
59+
DiagnosticSeverity.Warning,
6060
isEnabledByDefault: true);
6161

6262
public static DiagnosticDescriptor InvalidAsParametersNoConstructorFound { get; } = new(
6363
"RDG007",
6464
new LocalizableResourceString(nameof(Resources.InvalidAsParametersNoConstructorFound_Title), Resources.ResourceManager, typeof(Resources)),
6565
new LocalizableResourceString(nameof(Resources.InvalidAsParametersNoConstructorFound_Message), Resources.ResourceManager, typeof(Resources)),
6666
"Usage",
67-
DiagnosticSeverity.Error,
67+
DiagnosticSeverity.Warning,
6868
isEnabledByDefault: true);
6969

7070
public static DiagnosticDescriptor InvalidAsParametersSingleConstructorOnly { get; } = new(
7171
"RDG008",
7272
new LocalizableResourceString(nameof(Resources.InvalidAsParametersSingleConstructorOnly_Title), Resources.ResourceManager, typeof(Resources)),
7373
new LocalizableResourceString(nameof(Resources.InvalidAsParametersSingleConstructorOnly_Message), Resources.ResourceManager, typeof(Resources)),
7474
"Usage",
75-
DiagnosticSeverity.Error,
75+
DiagnosticSeverity.Warning,
7676
isEnabledByDefault: true);
7777

7878
public static DiagnosticDescriptor InvalidAsParametersNested { get; } = new(
7979
"RDG009",
8080
new LocalizableResourceString(nameof(Resources.InvalidAsParametersNested_Title), Resources.ResourceManager, typeof(Resources)),
8181
new LocalizableResourceString(nameof(Resources.InvalidAsParametersNested_Message), Resources.ResourceManager, typeof(Resources)),
8282
"Usage",
83-
DiagnosticSeverity.Error,
83+
DiagnosticSeverity.Warning,
8484
isEnabledByDefault: true);
8585

8686
public static DiagnosticDescriptor InvalidAsParametersNullable { get; } = new(
8787
"RDG010",
8888
new LocalizableResourceString(nameof(Resources.InvalidAsParametersNullable_Title), Resources.ResourceManager, typeof(Resources)),
8989
new LocalizableResourceString(nameof(Resources.InvalidAsParametersNullable_Message), Resources.ResourceManager, typeof(Resources)),
9090
"Usage",
91-
DiagnosticSeverity.Error,
91+
DiagnosticSeverity.Warning,
9292
isEnabledByDefault: true);
9393

9494
public static DiagnosticDescriptor TypeParametersNotSupported { get; } = new(
9595
"RDG011",
9696
new LocalizableResourceString(nameof(Resources.TypeParametersNotSupported_Title), Resources.ResourceManager, typeof(Resources)),
9797
new LocalizableResourceString(nameof(Resources.TypeParametersNotSupported_Message), Resources.ResourceManager, typeof(Resources)),
9898
"Usage",
99-
DiagnosticSeverity.Error,
99+
DiagnosticSeverity.Warning,
100100
isEnabledByDefault: true);
101101

102102
public static DiagnosticDescriptor InaccessibleTypesNotSupported { get; } = new(
103103
"RDG012",
104104
new LocalizableResourceString(nameof(Resources.InaccessibleTypesNotSupported_Title), Resources.ResourceManager, typeof(Resources)),
105105
new LocalizableResourceString(nameof(Resources.InaccessibleTypesNotSupported_Message), Resources.ResourceManager, typeof(Resources)),
106106
"Usage",
107-
DiagnosticSeverity.Error,
107+
DiagnosticSeverity.Warning,
108108
isEnabledByDefault: true);
109109
}

‎src/Http/Http.Extensions/gen/RequestDelegateGenerator.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
9494
codeWriter.WriteLine(@"Debug.Assert(options.EndpointBuilder != null, ""EndpointBuilder not found."");");
9595
codeWriter.WriteLine(@"Debug.Assert(options.EndpointBuilder.ApplicationServices != null, ""ApplicationServices not found."");");
9696
codeWriter.WriteLine(@"Debug.Assert(options.EndpointBuilder.FilterFactories != null, ""FilterFactories not found."");");
97-
codeWriter.WriteLine($"var handler = Cast(del, {endpoint.EmitHandlerDelegateType(considerOptionality: true)} => throw null!);");
97+
codeWriter.WriteLine($"var handler = Cast(del, {endpoint.EmitHandlerDelegateType()} => throw null!);");
9898
codeWriter.WriteLine("EndpointFilterDelegate? filteredInvocation = null;");
9999
codeWriter.WriteLine("var serviceProvider = options.ServiceProvider ?? options.EndpointBuilder.ApplicationServices;");
100100
endpoint.EmitLoggingPreamble(codeWriter);

‎src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EmitterExtensions.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,13 @@ public static bool IsSerializableJsonResponse(this EndpointResponse endpointResp
3838
public static string EmitArgument(this EndpointParameter endpointParameter) => endpointParameter.Source switch
3939
{
4040
EndpointParameterSource.JsonBody or EndpointParameterSource.Route or EndpointParameterSource.RouteOrQuery or EndpointParameterSource.JsonBodyOrService or EndpointParameterSource.FormBody => endpointParameter.IsOptional ? endpointParameter.EmitHandlerArgument() : $"{endpointParameter.EmitHandlerArgument()}!",
41+
// When a BindAsync parameter is required, make sure that we are using `.Value` to access
42+
// the underlying value for a nullable value type instead of using the non-nullable reference type modifier.
43+
EndpointParameterSource.BindAsync => endpointParameter.IsOptional ?
44+
endpointParameter.EmitHandlerArgument() :
45+
endpointParameter.Type.IsValueType && endpointParameter.GetBindAsyncReturnType().IsNullableOfT()
46+
? $"{endpointParameter.EmitHandlerArgument()}.HasValue ? {endpointParameter.EmitHandlerArgument()}.Value : default"
47+
: $"{endpointParameter.EmitHandlerArgument()}",
4148
EndpointParameterSource.Unknown => throw new NotImplementedException("Unreachable!"),
4249
_ => endpointParameter.EmitHandlerArgument()
4350
};

‎src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/EndpointParameterEmitter.cs

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ internal static void EmitBindAsyncPreparation(this EndpointParameter endpointPar
270270
var bindMethodReceiverType = receiverType?.UnwrapTypeSymbol(unwrapNullable: true);
271271
var bindMethodReceiverTypeString = bindMethodReceiverType?.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);
272272

273-
var unwrappedType = endpointParameter.Type.UnwrapTypeSymbol(unwrapNullable: true);
273+
var unwrappedType = endpointParameter.UnwrapParameterType();
274274
var unwrappedTypeString = unwrappedType?.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);
275275

276276
var resolveParameterInfo = endpointParameter.IsProperty
@@ -280,36 +280,33 @@ internal static void EmitBindAsyncPreparation(this EndpointParameter endpointPar
280280
switch (endpointParameter.BindMethod)
281281
{
282282
case BindabilityMethod.IBindableFromHttpContext:
283-
codeWriter.WriteLine($"var {endpointParameter.EmitTempArgument()} = await BindAsync<{unwrappedTypeString}>(httpContext, {resolveParameterInfo});");
283+
codeWriter.WriteLine($"var {endpointParameter.EmitHandlerArgument()} = await BindAsync<{unwrappedTypeString}>(httpContext, {resolveParameterInfo});");
284284
break;
285285
case BindabilityMethod.BindAsyncWithParameter:
286-
codeWriter.WriteLine($"var {endpointParameter.EmitTempArgument()} = await {bindMethodReceiverTypeString}.BindAsync(httpContext, {resolveParameterInfo});");
286+
codeWriter.WriteLine($"var {endpointParameter.EmitHandlerArgument()} = await {bindMethodReceiverTypeString}.BindAsync(httpContext, {resolveParameterInfo});");
287287
break;
288288
case BindabilityMethod.BindAsync:
289-
codeWriter.WriteLine($"var {endpointParameter.EmitTempArgument()} = await {bindMethodReceiverTypeString}.BindAsync(httpContext);");
289+
codeWriter.WriteLine($"var {endpointParameter.EmitHandlerArgument()} = await {bindMethodReceiverTypeString}.BindAsync(httpContext);");
290290
break;
291291
default:
292292
throw new NotImplementedException($"Unreachable! Unexpected {nameof(BindabilityMethod)}: {endpointParameter.BindMethod}");
293293
}
294294

295-
// TODO: Generate more compact code if the type is a reference type and/or the BindAsync return nullability matches the handler parameter type.
296-
if (endpointParameter.IsOptional)
297-
{
298-
codeWriter.WriteLine($"var {endpointParameter.EmitHandlerArgument()} = ({unwrappedTypeString}?){endpointParameter.EmitTempArgument()};");
299-
}
300-
else
295+
if (!endpointParameter.IsOptional)
301296
{
302-
codeWriter.WriteLine($"{unwrappedTypeString} {endpointParameter.EmitHandlerArgument()};");
303-
codeWriter.WriteLine($"if ((object?){endpointParameter.EmitTempArgument()} == null)");
297+
// Non-nullable value types can never be null so we can avoid emitting the requiredness check.
298+
if (endpointParameter.Type.IsValueType && !endpointParameter.GetBindAsyncReturnType().IsNullableOfT())
299+
{
300+
return;
301+
}
302+
codeWriter.WriteLine(endpointParameter.Type.IsValueType && endpointParameter.GetBindAsyncReturnType().IsNullableOfT()
303+
? $"if (!{endpointParameter.EmitHandlerArgument()}.HasValue)"
304+
: $"if ({endpointParameter.EmitHandlerArgument()} == null)");
304305
codeWriter.StartBlock();
305306
codeWriter.WriteLine($@"logOrThrowExceptionHelper.RequiredParameterNotProvided({SymbolDisplay.FormatLiteral(endpointParameter.Type.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat), true)}, {SymbolDisplay.FormatLiteral(endpointParameter.SymbolName, true)}, {SymbolDisplay.FormatLiteral(endpointParameter.ToMessageString(), true)});");
306307
codeWriter.WriteLine("wasParamCheckFailure = true;");
307308
codeWriter.WriteLine($"{endpointParameter.EmitHandlerArgument()} = default!;");
308309
codeWriter.EndBlock();
309-
codeWriter.WriteLine("else");
310-
codeWriter.StartBlock();
311-
codeWriter.WriteLine($"{endpointParameter.EmitHandlerArgument()} = ({unwrappedTypeString}){endpointParameter.EmitTempArgument()};");
312-
codeWriter.EndBlock();
313310
}
314311
}
315312

‎src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ private EndpointParameter(Endpoint endpoint, ITypeSymbol typeSymbol, string para
6161
ElementType = elementType;
6262
IsEndpointMetadataProvider = ImplementsIEndpointMetadataProvider(typeSymbol, wellKnownTypes);
6363
IsEndpointParameterMetadataProvider = ImplementsIEndpointParameterMetadataProvider(typeSymbol, wellKnownTypes);
64-
endpoint.EmitterContext.HasEndpointParameterMetadataProvider = IsEndpointParameterMetadataProvider;
64+
endpoint.EmitterContext.HasEndpointParameterMetadataProvider |= IsEndpointParameterMetadataProvider;
6565
endpoint.EmitterContext.HasEndpointMetadataProvider |= IsEndpointMetadataProvider;
6666
}
6767

‎src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Model/EndpointParameterExtensions.cs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
using Microsoft.AspNetCore.Analyzers.RouteEmbeddedLanguage.Infrastructure;
4+
using Microsoft.CodeAnalysis;
5+
6+
namespace Microsoft.AspNetCore.Http.RequestDelegateGenerator.StaticRouteHandlerModel;
7+
8+
internal static class EndpointParameterExtensions
9+
{
10+
public static ITypeSymbol UnwrapParameterType(this EndpointParameter parameter)
11+
{
12+
var handlerParameterType = parameter.Type;
13+
var bindAsyncReturnType = parameter.GetBindAsyncReturnType();
14+
15+
// If we can't resolve a return type from the `BindAsync` method, then just use the handler return type.
16+
if (bindAsyncReturnType is null)
17+
{
18+
return parameter.Source == EndpointParameterSource.BindAsync ? handlerParameterType.UnwrapTypeSymbol(unwrapNullable: true) : handlerParameterType;
19+
}
20+
// If the parameter defined in the route handler and the return type of the BindAsync method are the same,
21+
// then we can use the handler parameter type as the return type of the BindAsync method.
22+
if (SymbolEqualityComparer.IncludeNullability.Equals(handlerParameterType, bindAsyncReturnType))
23+
{
24+
return handlerParameterType;
25+
}
26+
// If the route handler parameter type is a value type, then we use it as the return type of the BindAsync method.
27+
// Even if the BindAsync method returns a type with mismatched nullability, we need to be able to correctly handle
28+
// mapping nullable value types to non-nullable value types, as in the following example.
29+
// public struct NullableStruct
30+
// {
31+
// public static ValueTask<NullableStruct?> BindAsync(HttpContext context, ParameterInfo parameter) {}
32+
// }
33+
// app.MapPost("/", (NullableStruct foo) => { })
34+
if (handlerParameterType.IsValueType)
35+
{
36+
return handlerParameterType;
37+
}
38+
39+
return bindAsyncReturnType;
40+
}
41+
42+
public static ITypeSymbol? GetBindAsyncReturnType(this EndpointParameter parameter)
43+
=> ((INamedTypeSymbol?)parameter.BindableMethodSymbol?.ReturnType)?.TypeArguments[0];
44+
45+
public static bool IsNullableOfT(this ITypeSymbol? typeSymbol)
46+
=> typeSymbol?.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T;
47+
}

‎src/Http/Http.Extensions/gen/StaticRouteHandlerModel/StaticRouteHandlerModel.Emitter.cs

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,15 @@
55
using System.Collections.Generic;
66
using System.Linq;
77
using System.Text;
8+
using Microsoft.AspNetCore.Analyzers.RouteEmbeddedLanguage.Infrastructure;
89
using Microsoft.AspNetCore.Http.RequestDelegateGenerator.StaticRouteHandlerModel.Emitters;
910
using Microsoft.CodeAnalysis;
1011

1112
namespace Microsoft.AspNetCore.Http.RequestDelegateGenerator.StaticRouteHandlerModel;
1213

1314
internal static class StaticRouteHandlerModelEmitter
1415
{
15-
public static string EmitHandlerDelegateType(this Endpoint endpoint, bool considerOptionality = false)
16+
public static string EmitHandlerDelegateType(this Endpoint endpoint)
1617
{
1718
// Emits a delegate type to use when casting the input that captures
1819
// default parameter values.
@@ -23,25 +24,20 @@ public static string EmitHandlerDelegateType(this Endpoint endpoint, bool consid
2324
{
2425
return endpoint.Response == null || (endpoint.Response.HasNoResponse && !endpoint.Response.IsAwaitable) ? "void ()" : $"{endpoint.Response.WrappedResponseType} ()";
2526
}
26-
var parameterTypeList = string.Join(", ", endpoint.Parameters.Select((p, i) => $"{getType(p, considerOptionality)} arg{i}{(p.HasDefaultValue ? $"= {p.DefaultValue}" : string.Empty)}"));
27+
var parameterTypeList = string.Join(", ", endpoint.Parameters.Select((p, i) => $"{EmitUnwrappedParameterType(p)} arg{i}{(p.HasDefaultValue ? $"= {p.DefaultValue}" : string.Empty)}"));
2728

2829
if (endpoint.Response == null || (endpoint.Response.HasNoResponse && !endpoint.Response.IsAwaitable))
2930
{
3031
return $"void ({parameterTypeList})";
3132
}
3233
return $"{endpoint.Response.WrappedResponseType} ({parameterTypeList})";
33-
34-
static string getType(EndpointParameter p, bool considerOptionality)
35-
{
36-
return considerOptionality
37-
? p.Type.ToDisplayString(p.IsOptional ? NullableFlowState.MaybeNull : NullableFlowState.NotNull, EmitterConstants.DisplayFormat)
38-
: p.Type.ToDisplayString(EmitterConstants.DisplayFormat);
39-
}
4034
}
4135

42-
public static string EmitSourceKey(this Endpoint endpoint)
36+
private static string EmitUnwrappedParameterType(EndpointParameter p)
4337
{
44-
return $@"(@""{endpoint.Location.File}"", {endpoint.Location.LineNumber})";
38+
var type = p.UnwrapParameterType();
39+
var isOptional = p.IsOptional || type.NullableAnnotation == NullableAnnotation.Annotated;
40+
return type.ToDisplayString(isOptional ? NullableFlowState.MaybeNull : NullableFlowState.NotNull, EmitterConstants.DisplayFormat);
4541
}
4642

4743
public static string EmitVerb(this Endpoint endpoint)
@@ -394,7 +390,7 @@ public static string EmitFilteredArgumentList(this Endpoint endpoint)
394390
// dealing with nullable types here. We could try to do fancy things to branch the logic here depending on
395391
// the nullability, but at the end of the day we are going to call GetArguments(...) - at runtime the nullability
396392
// suppression operator doesn't come into play - so its not worth worrying about.
397-
sb.Append($"ic.GetArgument<{endpoint.Parameters[i].Type.ToDisplayString(EmitterConstants.DisplayFormat)}>({i})!");
393+
sb.Append($"ic.GetArgument<{EmitUnwrappedParameterType(endpoint.Parameters[i])}>({i})!");
398394

399395
if (i < endpoint.Parameters.Length - 1)
400396
{
@@ -416,7 +412,7 @@ public static string EmitFilterInvocationContextTypeArgs(this Endpoint endpoint)
416412

417413
for (var i = 0; i < endpoint.Parameters.Length; i++)
418414
{
419-
sb.Append(endpoint.Parameters[i].Type.ToDisplayString(endpoint.Parameters[i].IsOptional ? NullableFlowState.MaybeNull : NullableFlowState.NotNull, EmitterConstants.DisplayFormat));
415+
sb.Append(EmitUnwrappedParameterType(endpoint.Parameters[i]));
420416

421417
if (i < endpoint.Parameters.Length - 1)
422418
{

0 commit comments

Comments
 (0)