Update RDG to use interceptors feature #48817

captainsafia · 2023-06-14T22:22:20Z

captainsafia · 2023-07-05T17:06:49Z

src/Framework/AspNetCoreAnalyzers/src/Analyzers/Microsoft.AspNetCore.App.Analyzers.csproj

@@ -8,6 +8,7 @@
    <Nullable>Enable</Nullable>
    <RootNamespace>Microsoft.AspNetCore.Analyzers</RootNamespace>
    <SuppressNullableAttributesImport>true</SuppressNullableAttributesImport>
+    <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>


This change, and stylistic modifications in the analyzers, are a result of new diagnostics emitted from the updated compiler version.

captainsafia · 2023-07-05T17:07:26Z

...AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/FrameworkParametersCompletionProviderTests.cs

 using Microsoft.CodeAnalysis.Completion;

 namespace Microsoft.AspNetCore.Analyzers.RouteEmbeddedLanguage;

+[QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/49126")]


A downstream change in Roslyn makes these tests unreliable. A PR with a fix is out but I am quarantining for now so that we can move forward with this change.

captainsafia · 2023-07-05T17:11:00Z

src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs

+            return RouteHandlerServices.Map(routes, pattern, handler, httpMethods, populateMetadata, createRequestDelegate);
+        }
+
+        private static T Cast<T>(Delegate d, T _) where T : Delegate


Now that we are no longer using strongly-typed delegate types in the overload we need to get a concrete type from the Delegate parameter so that we can use it in the generated code. This Cast method allows us to capture the concrete type and to capture default parameters passed to the lambda/method group (see here).

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).

captainsafia · 2023-07-05T17:11:58Z

src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs

 {
    {{GeneratedCodeAttribute}}
-    internal sealed class SourceKey
+    [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
+    file sealed class InterceptsLocationAttribute : Attribute


No public API is included as part of the interceptors feature in preview so we have to define this type ourselves. We use AllowMultiple = true here so we can use the same interceptor for Map action calls that implement the same signature.

captainsafia · 2023-07-05T17:13:35Z

src/Http/Http.Extensions/gen/RequestDelegateGeneratorSuppressor.cs

This DiagnosticSuppressor is the recommended workaround for dotnet/roslyn#68669. It specifically suppresses the false-positive diagnostics that are emitted by the linker analyzer when the RDG wille emit an interceptor (we produce an endpoint model with no diagnostics).

captainsafia · 2023-07-05T17:18:29Z

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

@@ -23,6 +23,7 @@ public EndpointParameter(Endpoint endpoint, IParameterSymbol parameter, WellKnow
    {
        Ordinal = parameter.Ordinal;
        IsOptional = parameter.IsOptional();
+        HasDefaultValue = parameter.HasExplicitDefaultValue;


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.

captainsafia · 2023-07-05T17:32:24Z

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

@@ -610,17 +613,17 @@ private static string ConvertEndOfLineAndQuotationCharactersToEscapeForm(string
        other.SymbolName == SymbolName &&
        other.Ordinal == Ordinal &&
        other.IsOptional == IsOptional &&
-        SymbolEqualityComparer.Default.Equals(other.Type, Type);
+        SymbolEqualityComparer.IncludeNullability.Equals(other.Type, Type);


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.

captainsafia · 2023-07-05T17:36:32Z

src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTestBase.cs

@@ -167,11 +169,14 @@ internal Endpoint[] GetEndpointsFromCompilation(Compilation compilation, bool? e

        foreach (var endpoint in endpoints)
        {
-            var sourceKeyMetadata = endpoint.Metadata.FirstOrDefault(metadata => metadata.GetType() == sourceKeyType);
+            var sourceKeyMetadata = endpoint.Metadata.OfType<GeneratedCodeAttribute>().SingleOrDefault();


We emitted the SourceKey originally as a proactive measure to allow other possibly generated code (like the validation generator) to do look-ups on generated endpoints by the source location. Since we don't really need this for .NET 8 (we're not shipping other minimal API-related generators), I'm removing this type from the generated code and reusing the GeneratedCode attribute to serve as a marker for when the endpoint is statically generated.

I asked this in my review below, but I don't fully understand why we are doing this. Why do we need this marker?

It helps us determine at runtime if an endpoint was generated at compile-time or runtime. For example, for determining what generator version an endpoint in a library was generated with compared to the app code consuming it.

We typically populate all attributes that are on an endpoint into its metadata so this makes sense to me assuming something like this:

app.MapGet("/", [GeneratedCode] (string name) => ...);

At runtime, we can get resolve if the endpoint was generated:

endpoint.Metadata.OfType<GeneratedCodeAttribute>();

captainsafia · 2023-07-05T17:39:47Z

src/Http/Http.Extensions/gen/RequestDelegateGeneratorSuppressor.cs

I was able to integrate validations for the DiagnosticSuppressor in our existing test infrastructure in https://github.com/dotnet/aspnetcore/tree/safia/interceptors-rdg-save. The changes work locally but it is challenging to get them to work in a Helix environment since we have to upload a version of the linker analyzer assembly onto the Helix machine along with the tests.

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.

eng/SourceBuildPrebuiltBaseline.xml

...xtensions/test/RequestDelegateGenerator/Baselines/MapAction_BindAsync_Snapshot.generated.txt

eerhardt · 2023-07-05T23:33:17Z

...xtensions/test/RequestDelegateGenerator/Baselines/MapAction_BindAsync_Snapshot.generated.txt

+            {
+                Debug.Assert(options != null, "RequestDelegateFactoryOptions not found.");
+                Debug.Assert(options.EndpointBuilder != null, "EndpointBuilder not found.");
+                options.EndpointBuilder.Metadata.Add(new System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.AspNetCore.Http.RequestDelegateGenerator, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60", "42.42.42.42"));


Is this really necessary to add to the Metadata? What's its purpose?

eerhardt · 2023-07-05T23:34:45Z

...xtensions/test/RequestDelegateGenerator/Baselines/MapAction_BindAsync_Snapshot.generated.txt

-                        {
-                            httpContext.Response.StatusCode = 400;
-                        }
-


Why doesn't this just happen inline? It may make sense to throw a comment in the generated code...

The parameter binding logic that sets the wasParamCheckFailureis shared between the non-filter and filter-handler. However, the way we react to it is different depending on whether we are executing an endpoint with filters or not.

When no filters are registered, we return from the RequestDelegate immediately. When filters are registered, we don't return because we want the filters to run, but not the request delegate. This is easier to do if the check doesn't happen inline.

I think GitHub is messing up showing where I was referring to. When you switch to the "Files changed" tab, you can see where I was referring to:

Why is the Cast helper method necessary? Why can't we just cast inline without using this method?

Why is the Cast helper method necessary? Why can't we just cast inline without using this method?

Ah, that part is in reaction to a breaking change in the compiler. Method group references are now inferred as anonymous delegate types instead of Action/Func to match the conversion for lambdas. This is the workaround @jjonescz and I came up with given the new constraint.

Cast is more like Infer. Using generics to reference and unspeakable type (like an anonymous class).

eerhardt · 2023-07-05T23:36:38Z

...egateGenerator/Baselines/MapAction_JsonBodyOrService_HandlesBothJsonAndService.generated.txt

 {
    %GENERATEDCODEATTRIBUTE%
-    internal sealed class SourceKey
+    [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
+    file sealed class InterceptsLocationAttribute : Attribute


What happens if another source generator adds this same file sealed class InterceptsLocationAttribute to the project? For example - if I enable both RDG and the ConfigurationBinder source generator.

Since the type is file scoped, it'll be fine to have multiple definitions in each generator as each will produce a unique type definition (e.g. System.Runtime.CompilerServices.<GeneratedFileName_g>FBAC_InterceptsLocationAttribute).

Things will get interesting if generators aren't cautious about making them file private (as opposed to say internal) but we should fine here.

cc: @RikkiGibson At minimum, we probably want to document the requirement for InterceptsLocationAttribute definitions to be file private. Not sure if we can add a compiler check/analyzer as well.

...estDelegateGenerator/Baselines/MapAction_MultipleSpecialTypeParam_StringReturn.generated.txt

mitchdenny · 2023-07-07T01:25:24Z

src/Http/Http.Extensions/gen/RequestDelegateGeneratorSuppressor.cs

+namespace Microsoft.AspNetCore.Http.RequestDelegateGenerator;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class RequestDelegateGeneratorSuppressor : DiagnosticSuppressor


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?

Another thought do we need to write a test here to verify that this suppressor doesn't supress things that it shouldn't?

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?

It doesn't need to be registered. Since it is annotated with the DiagnosticAnalyzer attribute, the compiler infrastructure will pick it up automatically.

Another thought do we need to write a test here to verify that this suppressor doesn't supress things that it shouldn't?

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

aspnetcore/src/Http/Http.Extensions/test/RequestDelegateGenerator/CompileTimeCreationTests.AsParameters.cs

Lines 66 to 67 in 4870eea

public async Task BuildRequestDelegateEmitsDiagnosticForInvalidParameterListConstructor(

string parameterType,

mitchdenny

This is really nice! The size of the PR doesn't do justice to the level of effort that went into this (I guess that is a sign that the interceptors stuff really simplifies things!).

src/Http/samples/MinimalSample/MinimalSample.csproj

src/Identity/Core/src/Microsoft.AspNetCore.Identity.csproj

src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTestBase.cs

....Extensions/test/RequestDelegateGenerator/Baselines/VerifyAsParametersBaseline.generated.txt

eerhardt · 2023-07-07T15:53:39Z

.../RequestDelegateGenerator/Baselines/SupportsSameInterceptorsFromDifferentFiles.generated.txt

+        [InterceptsLocation(@"TestMapActions.cs", 25, 13)]
+        [InterceptsLocation(@"OtherTestMapActions.cs", 25, 13)]
+        internal static RouteHandlerBuilder MapGet_2513(


What happens if the calls come from different line and column numbers? Do we still merge them into 1 method?

If the calls have the same exact signature, then yes. I can add this to the test case.

eerhardt · 2023-07-07T15:56:13Z

src/Http/Http.Extensions/gen/RequestDelegateGeneratorSuppressor.cs

+using Microsoft.CodeAnalysis.Operations;
+
+/*
+ * This class contains the logic for suppressing diagnostics that are


(nit) why isn't this comment in a /// <summary> on the class?

I believe that generally in the repo we use code comments for dev-focused comments and /// <summary> tags for things that should surface in user facing docs.

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.

eerhardt

Looks good. Nice work, @captainsafia!

eerhardt · 2023-07-10T23:51:18Z

eng/Versions.props

@@ -245,7 +245,7 @@
    <Analyzer_MicrosoftCodeAnalysisCSharpWorkspacesVersion>3.3.1</Analyzer_MicrosoftCodeAnalysisCSharpWorkspacesVersion>
    <!-- Pin the version of the M.CA dependencies that we utilize with a cutom version property $(MicrosoftCodeAnalysisVersion_LatestVS) to avoid automatically
    consuming the newest version of the packages when using the $(MicrosoftCodeAnalysisCSharpVersion) properties in source-build. -->
-    <MicrosoftCodeAnalysisVersion_LatestVS>4.5.0</MicrosoftCodeAnalysisVersion_LatestVS>
+    <MicrosoftCodeAnalysisVersion_LatestVS>4.7.0-3.23314.3</MicrosoftCodeAnalysisVersion_LatestVS>


Was this just missed in #49245?

Yes 😅

The other PR has already flown into the installer without any major issues so this hopefully doesn't cause any headaches....

ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jun 14, 2023

captainsafia force-pushed the safia/interceptors-rdg branch 6 times, most recently from 2860bdc to d1412b1 Compare June 20, 2023 23:42

captainsafia force-pushed the safia/interceptors-rdg branch 8 times, most recently from c06f2e0 to 2adb5f6 Compare June 30, 2023 22:08

captainsafia force-pushed the safia/interceptors-rdg branch from 2adb5f6 to d45c8f7 Compare July 5, 2023 16:20

captainsafia added feature-rdg area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Jul 5, 2023

captainsafia commented Jul 5, 2023

View reviewed changes

Update RDG to use interceptors feature

7b491a9

captainsafia commented Jul 5, 2023

View reviewed changes

captainsafia force-pushed the safia/interceptors-rdg branch from d45c8f7 to 7b491a9 Compare July 5, 2023 17:37

captainsafia marked this pull request as ready for review July 5, 2023 17:37

captainsafia requested review from BrennanConroy, halter73, Tratcher, a team and wtgodbe as code owners July 5, 2023 17:37

captainsafia requested review from mitchdenny and eerhardt July 5, 2023 17:37

captainsafia commented Jul 5, 2023

View reviewed changes

eerhardt reviewed Jul 5, 2023

View reviewed changes

captainsafia added 3 commits July 6, 2023 09:42

Address feedback from review

0fb3ca6

Merge remote-tracking branch 'origin/main' into safia/interceptors-rdg

20133a7

Merge remote-tracking branch 'origin/main' into safia/interceptors-rdg

7ac7fd7

mitchdenny reviewed Jul 7, 2023

View reviewed changes

mitchdenny approved these changes Jul 7, 2023

View reviewed changes

eerhardt reviewed Jul 7, 2023

View reviewed changes

ejsmith mentioned this pull request Jul 7, 2023

3.0 tracking issue - rewriting source generation martinothamar/Mediator#98

Open

9 tasks

Add separator to method name and update same interceptor test

4997fc0

captainsafia force-pushed the safia/interceptors-rdg branch from 2d526b2 to 4997fc0 Compare July 7, 2023 19:17

captainsafia added 2 commits July 7, 2023 13:04

Use group index to distinguish interceptor methods

0ce39d2

Merge remote-tracking branch 'origin/main' into safia/interceptors-rdg

8574786

eerhardt approved these changes Jul 11, 2023

View reviewed changes

captainsafia merged commit 2196cfd into main Jul 11, 2023

captainsafia deleted the safia/interceptors-rdg branch July 11, 2023 16:14

ghost added this to the 8.0-preview7 milestone Jul 11, 2023

eerhardt mentioned this pull request Jul 12, 2023

Update binding generator to do case-insensitive config-key/property name matching, & make some formatting improvements dotnet/runtime#88338

Merged

layomia mentioned this pull request Jul 21, 2023

Use Roslyn interceptors feature in config binder generator dotnet/runtime#89322

Closed

This was referenced Jul 25, 2023

Address online only prebuilts dotnet/source-build#3557

Closed

Flow Microsoft.CodeAnalysis version to aspnetcore dotnet/installer#17067

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update RDG to use interceptors feature #48817

Update RDG to use interceptors feature #48817

captainsafia commented Jun 14, 2023 •

edited

Loading

captainsafia Jul 5, 2023

captainsafia Jul 5, 2023

captainsafia Jul 5, 2023

mitchdenny Jul 7, 2023 •

edited

Loading

captainsafia Jul 5, 2023

captainsafia Jul 5, 2023

captainsafia Jul 5, 2023

captainsafia Jul 5, 2023

captainsafia Jul 5, 2023

eerhardt Jul 6, 2023

captainsafia Jul 6, 2023

captainsafia Jul 5, 2023

eerhardt Jul 5, 2023

eerhardt Jul 5, 2023

captainsafia Jul 6, 2023

eerhardt Jul 7, 2023 •

edited

Loading

captainsafia Jul 7, 2023

davidfowl Jul 9, 2023

eerhardt Jul 5, 2023

captainsafia Jul 6, 2023

mitchdenny Jul 7, 2023 •

edited

Loading

mitchdenny Jul 7, 2023

captainsafia Jul 7, 2023

mitchdenny left a comment

eerhardt Jul 7, 2023

captainsafia Jul 7, 2023

eerhardt Jul 7, 2023

captainsafia Jul 7, 2023

eerhardt left a comment

eerhardt Jul 10, 2023

captainsafia Jul 11, 2023

	public async Task BuildRequestDelegateEmitsDiagnosticForInvalidParameterListConstructor(
	string parameterType,

Update RDG to use interceptors feature #48817

Update RDG to use interceptors feature #48817

Conversation

captainsafia commented Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainsafia commented Jun 14, 2023 •

edited

Loading

mitchdenny Jul 7, 2023 •

edited

Loading

eerhardt Jul 7, 2023 •

edited

Loading

mitchdenny Jul 7, 2023 •

edited

Loading