-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Update RDG to use interceptors feature #48817
Changes from 4 commits
7b491a9
0fb3ca6
20133a7
7ac7fd7
4997fc0
0ce39d2
8574786
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,8 @@ internal static class RequestDelegateGeneratorSources | |
#nullable enable | ||
"""; | ||
|
||
public static string GeneratedCodeAttribute => $@"[System.CodeDom.Compiler.GeneratedCodeAttribute(""{typeof(RequestDelegateGeneratorSources).Assembly.FullName}"", ""{typeof(RequestDelegateGeneratorSources).Assembly.GetName().Version}"")]"; | ||
public static string GeneratedCodeConstructor => $@"System.CodeDom.Compiler.GeneratedCodeAttribute(""{typeof(RequestDelegateGeneratorSources).Assembly.FullName}"", ""{typeof(RequestDelegateGeneratorSources).Assembly.GetName().Version}"")"; | ||
public static string GeneratedCodeAttribute => $"[{GeneratedCodeConstructor}]"; | ||
|
||
public static string ContentTypeConstantsType => $$""" | ||
{{GeneratedCodeAttribute}} | ||
|
@@ -436,25 +437,19 @@ public override bool IsDefined(Type attributeType, bool inherit) | |
} | ||
"""; | ||
|
||
public static string GetGeneratedRouteBuilderExtensionsSource(string genericThunks, string thunks, string endpoints, string helperMethods, string helperTypes) => $$""" | ||
public static string GetGeneratedRouteBuilderExtensionsSource(string endpoints, string helperMethods, string helperTypes) => $$""" | ||
{{SourceHeader}} | ||
|
||
namespace Microsoft.AspNetCore.Builder | ||
namespace System.Runtime.CompilerServices | ||
{ | ||
{{GeneratedCodeAttribute}} | ||
internal sealed class SourceKey | ||
[AttributeUsage(AttributeTargets.Method, AllowMultiple = true)] | ||
file sealed class InterceptsLocationAttribute : Attribute | ||
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. No public API is included as part of the interceptors feature in preview so we have to define this type ourselves. We use |
||
{ | ||
public string Path { get; init; } | ||
public int Line { get; init; } | ||
|
||
public SourceKey(string path, int line) | ||
public InterceptsLocationAttribute(string filePath, int line, int column) | ||
{ | ||
Path = path; | ||
Line = line; | ||
} | ||
} | ||
|
||
{{GetEndpoints(endpoints)}} | ||
} | ||
|
||
namespace Microsoft.AspNetCore.Http.Generated | ||
|
@@ -468,6 +463,7 @@ namespace Microsoft.AspNetCore.Http.Generated | |
using System.Globalization; | ||
using System.Linq; | ||
using System.Reflection; | ||
using System.Runtime.CompilerServices; | ||
using System.Text.Json; | ||
using System.Text.Json.Serialization.Metadata; | ||
using System.Threading.Tasks; | ||
|
@@ -490,8 +486,29 @@ namespace Microsoft.AspNetCore.Http.Generated | |
{{GeneratedCodeAttribute}} | ||
file static class GeneratedRouteBuilderExtensionsCore | ||
{ | ||
{{GetGenericThunks(genericThunks)}} | ||
{{GetThunks(thunks)}} | ||
private static readonly string[] GetVerb = new[] { Microsoft.AspNetCore.Http.HttpMethods.Get }; | ||
private static readonly string[] PostVerb = new[] { Microsoft.AspNetCore.Http.HttpMethods.Post }; | ||
private static readonly string[] PutVerb = new[] { Microsoft.AspNetCore.Http.HttpMethods.Put }; | ||
private static readonly string[] DeleteVerb = new[] { Microsoft.AspNetCore.Http.HttpMethods.Delete }; | ||
private static readonly string[] PatchVerb = new[] { Microsoft.AspNetCore.Http.HttpMethods.Patch }; | ||
|
||
{{endpoints}} | ||
|
||
internal static RouteHandlerBuilder MapCore( | ||
this IEndpointRouteBuilder routes, | ||
string pattern, | ||
Delegate handler, | ||
IEnumerable<string>? httpMethods, | ||
MetadataPopulator populateMetadata, | ||
RequestDelegateFactoryFunc createRequestDelegate) | ||
{ | ||
return RouteHandlerServices.Map(routes, pattern, handler, httpMethods, populateMetadata, createRequestDelegate); | ||
} | ||
|
||
private static T Cast<T>(Delegate d, T _) where T : Delegate | ||
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. Now that we are no longer using strongly-typed delegate types in the overload we need to get a concrete type from the 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. I think it would be good to add a comment here as well to fully explain this since it's kind of obscure (just to help future maintainers). |
||
{ | ||
return (T)d; | ||
} | ||
|
||
private static EndpointFilterDelegate BuildFilterDelegate(EndpointFilterDelegate filteredInvocation, EndpointBuilder builder, MethodInfo mi) | ||
{ | ||
|
@@ -554,63 +571,4 @@ private static bool ShouldUseWith(this JsonTypeInfo jsonTypeInfo, [NotNullWhen(f | |
{{LogOrThrowExceptionHelperClass}} | ||
} | ||
"""; | ||
private static string GetGenericThunks(string genericThunks) => genericThunks != string.Empty ? $$""" | ||
private static class GenericThunks<T> | ||
{ | ||
public static readonly Dictionary<(string, int), (MetadataPopulator, RequestDelegateFactoryFunc)> map = new() | ||
{ | ||
{{genericThunks}} | ||
}; | ||
} | ||
|
||
internal static RouteHandlerBuilder MapCore<T>( | ||
this IEndpointRouteBuilder routes, | ||
string pattern, | ||
Delegate handler, | ||
IEnumerable<string> httpMethods, | ||
string filePath, | ||
int lineNumber) | ||
{ | ||
var (populateMetadata, createRequestDelegate) = GenericThunks<T>.map[(filePath, lineNumber)]; | ||
return RouteHandlerServices.Map(routes, pattern, handler, httpMethods, populateMetadata, createRequestDelegate); | ||
} | ||
""" : string.Empty; | ||
|
||
private static string GetThunks(string thunks) => thunks != string.Empty ? $$""" | ||
private static readonly Dictionary<(string, int), (MetadataPopulator, RequestDelegateFactoryFunc)> map = new() | ||
{ | ||
{{thunks}} | ||
}; | ||
|
||
internal static RouteHandlerBuilder MapCore( | ||
this IEndpointRouteBuilder routes, | ||
string pattern, | ||
Delegate handler, | ||
IEnumerable<string>? httpMethods, | ||
string filePath, | ||
int lineNumber) | ||
{ | ||
var (populateMetadata, createRequestDelegate) = map[(filePath, lineNumber)]; | ||
return RouteHandlerServices.Map(routes, pattern, handler, httpMethods, populateMetadata, createRequestDelegate); | ||
} | ||
""" : string.Empty; | ||
|
||
private static string GetEndpoints(string endpoints) => endpoints != string.Empty ? $$""" | ||
// This class needs to be internal so that the compiled application | ||
// has access to the strongly-typed endpoint definitions that are | ||
// generated by the compiler so that they will be favored by | ||
// overload resolution and opt the runtime in to the code generated | ||
// implementation produced here. | ||
{{GeneratedCodeAttribute}} | ||
internal static class GenerateRouteBuilderEndpoints | ||
{ | ||
private static readonly string[] GetVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Get }; | ||
private static readonly string[] PostVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Post }; | ||
private static readonly string[] PutVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Put }; | ||
private static readonly string[] DeleteVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Delete }; | ||
private static readonly string[] PatchVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Patch }; | ||
|
||
{{endpoints}} | ||
} | ||
""" : string.Empty; | ||
} |
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. This 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. I was able to integrate validations for the To spare us the MSBuild-foo, it should be sufficient to add E2E validation for the suppressor in the SDK repo. The setup in the branch above can also be used to validate the changeset locally. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,74 @@ | ||||||
// Licensed to the .NET Foundation under one or more agreements. | ||||||
// The .NET Foundation licenses this file to you under the MIT license. | ||||||
|
||||||
using System.Collections.Immutable; | ||||||
using System.Linq; | ||||||
using Microsoft.AspNetCore.App.Analyzers.Infrastructure; | ||||||
using Microsoft.AspNetCore.Http.RequestDelegateGenerator.StaticRouteHandlerModel; | ||||||
using Microsoft.CodeAnalysis; | ||||||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||||||
using Microsoft.CodeAnalysis.Diagnostics; | ||||||
using Microsoft.CodeAnalysis.Operations; | ||||||
|
||||||
/* | ||||||
* This class contains the logic for suppressing diagnostics that are | ||||||
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. (nit) why isn't this comment in a 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. I believe that generally in the repo we use code comments for dev-focused comments and Even though it doesn't matter for this class since it doesn't ship in a library (and docs won't surface on the docs.microsoft.com site anyways), I kept the pattern. |
||||||
* emitted by the linker analyzers when encountering the framework-provided | ||||||
* `Map` invocations. Pending the completion of https://github.com/dotnet/roslyn/issues/68669, | ||||||
* this workaround is necessary to apply these suppressions for `Map` invocations that the RDG | ||||||
* is able to generate code at compile time for that the analyzer is not able to resolve. | ||||||
*/ | ||||||
|
||||||
namespace Microsoft.AspNetCore.Http.RequestDelegateGenerator; | ||||||
|
||||||
[DiagnosticAnalyzer(LanguageNames.CSharp)] | ||||||
public sealed class RequestDelegateGeneratorSuppressor : DiagnosticSuppressor | ||||||
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. More of a question because it's the first time I've seen these in action. Does a DiagnosticSuppressor need to be registered explicitly anywhere, or does its mere reference in the dependency graph kick it into action? 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. Another thought do we need to write a test here to verify that this suppressor doesn't supress things that it shouldn't? 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.
It doesn't need to be registered. Since it is annotated with the
Copying my response from another thread: So, I was able to get this working with the automated tests in https://github.com/dotnet/aspnetcore/blob/safia/interceptors-rdg-save/src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTestBase.cs#L96-L100. One of the reasons I didn't pursue this testing strategy in the actual PR is because it's a bit of a pain to do a lookup for the linker analyzer assembly and then upload it to Helix, especially because the test projects themselves don't take a dependency on the linker analyzer. The current proposal is to add tests for this E2E behavior in the SDK, where all the components already exist, so I'll make a point of adding a test to validate the true negative scenario there. But TL;DR, the results I saw with the test infra locally gives me confidence about diagnostics being emitted for true negatives like Lines 66 to 67 in 4870eea
|
||||||
{ | ||||||
private static readonly SuppressionDescriptor SuppressRUCDiagnostic = new( | ||||||
id: "RDGS001", | ||||||
suppressedDiagnosticId: "IL2026", | ||||||
justification: "The target method has been intercepted by a statically generated variant."); | ||||||
|
||||||
private static readonly SuppressionDescriptor SuppressRDCDiagnostic = new( | ||||||
id: "RDGS002", | ||||||
suppressedDiagnosticId: "IL3050", | ||||||
justification: "The target method has been intercepted by a statically generated variant."); | ||||||
|
||||||
public override void ReportSuppressions(SuppressionAnalysisContext context) | ||||||
{ | ||||||
foreach (var diagnostic in context.ReportedDiagnostics) | ||||||
{ | ||||||
if (diagnostic.Id != SuppressRDCDiagnostic.SuppressedDiagnosticId && diagnostic.Id != SuppressRUCDiagnostic.SuppressedDiagnosticId) | ||||||
{ | ||||||
continue; | ||||||
} | ||||||
|
||||||
var location = diagnostic.AdditionalLocations.Count > 0 | ||||||
? diagnostic.AdditionalLocations[0] | ||||||
: diagnostic.Location; | ||||||
|
||||||
if (location.SourceTree is not { } sourceTree | ||||||
|| sourceTree.GetRoot().FindNode(location.SourceSpan) is not InvocationExpressionSyntax node | ||||||
|| !node.TryGetMapMethodName(out var method) | ||||||
|| !InvocationOperationExtensions.KnownMethods.Contains(method)) | ||||||
{ | ||||||
continue; | ||||||
} | ||||||
|
||||||
var semanticModel = context.GetSemanticModel(sourceTree); | ||||||
var operation = semanticModel.GetOperation(node, context.CancellationToken); | ||||||
var wellKnownTypes = WellKnownTypes.GetOrCreate(semanticModel.Compilation); | ||||||
if (operation.IsValidOperation(wellKnownTypes, out var invocationOperation)) | ||||||
{ | ||||||
var endpoint = new Endpoint(invocationOperation, wellKnownTypes, semanticModel); | ||||||
if (endpoint.Diagnostics.Count == 0) | ||||||
{ | ||||||
var targetSuppression = diagnostic.Id == SuppressRUCDiagnostic.SuppressedDiagnosticId | ||||||
? SuppressRUCDiagnostic | ||||||
: SuppressRDCDiagnostic; | ||||||
context.ReportSuppression(Suppression.Create(targetSuppression, diagnostic)); | ||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions => ImmutableArray.Create(SuppressRUCDiagnostic, SuppressRDCDiagnostic); | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ public EndpointParameter(Endpoint endpoint, IParameterSymbol parameter, WellKnow | |
{ | ||
Ordinal = parameter.Ordinal; | ||
IsOptional = parameter.IsOptional(); | ||
HasDefaultValue = parameter.HasExplicitDefaultValue; | ||
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. We add tracking for whether or not a method has a default parameter so that we can pass it in the concrete delegate type to support default parameter values in lambdas. |
||
DefaultValue = parameter.GetDefaultValueString(); | ||
ProcessEndpointParameterSource(endpoint, parameter, parameter.GetAttributes(), wellKnownTypes); | ||
} | ||
|
@@ -32,6 +33,7 @@ private EndpointParameter(Endpoint endpoint, IPropertySymbol property, IParamete | |
Ordinal = parameter?.Ordinal ?? 0; | ||
IsProperty = true; | ||
IsOptional = property.IsOptional() || parameter?.IsOptional() == true; | ||
HasDefaultValue = parameter?.HasExplicitDefaultValue ?? false; | ||
DefaultValue = parameter?.GetDefaultValueString() ?? "null"; | ||
// Coalesce attributes on the property and attributes on the matching parameter | ||
var attributeBuilder = ImmutableArray.CreateBuilder<AttributeData>(); | ||
|
@@ -251,6 +253,7 @@ private static bool ImplementsIEndpointParameterMetadataProvider(ITypeSymbol typ | |
public bool IsOptional { get; set; } | ||
public bool IsArray { get; set; } | ||
public string DefaultValue { get; set; } = "null"; | ||
public bool HasDefaultValue { get; set; } | ||
[MemberNotNullWhen(true, nameof(PropertyAsParameterInfoConstruction))] | ||
public bool IsProperty { get; set; } | ||
public EndpointParameterSource Source { get; set; } | ||
|
@@ -610,17 +613,17 @@ obj is EndpointParameter other && | |
other.SymbolName == SymbolName && | ||
other.Ordinal == Ordinal && | ||
other.IsOptional == IsOptional && | ||
SymbolEqualityComparer.Default.Equals(other.Type, Type); | ||
SymbolEqualityComparer.IncludeNullability.Equals(other.Type, Type); | ||
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. Prior to the interceptors feature, we couldn't generate separate overloads for endpoints that had the same parameter type but differing nullability (see #46622). Interceptors don't have the same problem so we can generate code for endpoints that use the same type but differing nullability. |
||
|
||
public bool SignatureEquals(object obj) => | ||
obj is EndpointParameter other && | ||
SymbolEqualityComparer.Default.Equals(other.Type, Type); | ||
SymbolEqualityComparer.IncludeNullability.Equals(other.Type, Type); | ||
|
||
public override int GetHashCode() | ||
{ | ||
var hashCode = new HashCode(); | ||
hashCode.Add(SymbolName); | ||
hashCode.Add(Type, SymbolEqualityComparer.Default); | ||
hashCode.Add(Type, SymbolEqualityComparer.IncludeNullability); | ||
return hashCode.ToHashCode(); | ||
} | ||
} |
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.
Was this just missed in #49245?
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.
Yes 😅
The other PR has already flown into the installer without any major issues so this hopefully doesn't cause any headaches....