Add support for response writing to RequestDelegateGenerator #46291

captainsafia · 2023-01-27T16:29:07Z

Addresses #46276 and resolves #45524.

Share WellKnownTypes infrastructure between analyzers and generator
Emit diagnostics from RequestDelegateGenerator for unsupported syntax
Add support for writing more response types to generator
Dedupe strongly-typed endpoints generated from the same delegate type
Use RequestDelegateGenerator in MinimalSample
Fix nullability warnings in generated source when referencing RequestDelegateFactoryOptions.EndpointBuilder

JamesNK · 2023-01-31T00:46:37Z

src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/WellKnownTypeData.cs

+
+internal static class WellKnownTypeData
+{
+    public enum WellKnownType


Does this need to be nested?

Not necessarily. It was largely a stylistic choice to put the associated data in the same class. To minimize the code delta, I ended up using an alias in the existing analyzers/codefixers but it felt nicer to capture these types in class instead of having them on the top-level namespace.

Wouldn't leaving this unnested minimize the code delta by removing the using WellKnownType = WellKnownTypeData.WellKnownType;?

JamesNK · 2023-01-31T00:49:37Z

...est/RequestDelegateGenerator/Baselines/Multiple_MapAction_NoParam_StringReturn.generated.txt

+
+    file static class GeneratedRouteBuilderExtensionsCore
+    {
+


super nit: Is there a way to not start the type with a blank line? My OCD is triggered 😄

Hm, I think we might be able to remove the newline in the interpolated string {GetGenericThunks()}{GetThunks()} and change the strings emitted from those methods to get around it.

In the long-term, we might be able to take advantage of the SyntaxTree.NormalizeWhitespace feature to correct the indentation for the outputted source. I wasn't able to use it successfully here because it has a really hard time formatting the Dictionary of source key to tuples of funcs pairing. As we evolve the code, we might be able to get rid of this (e.g. once the interceptors feature lands) and we can do all the formatting at once instead of doing the bookkeeping for the indents here.

A related issue on the Roslyn side is: dotnet/roslyn#43821

JamesNK · 2023-01-31T00:54:01Z

src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.Generators.csproj

  <ItemGroup>
    <Compile Include="$(SharedSourceRoot)IsExternalInit.cs" LinkBase="Shared" />
+    <Compile Include="$(SharedSourceRoot)RoslynUtils\BoundedCacheWithFactory.cs" LinkBase="Shared" />
+    <Compile Include="$(SharedSourceRoot)RoslynUtils\CodeWriter.cs" LinkBase="Shared" />
+    <Compile Include="$(SharedSourceRoot)RoslynUtils\WellKnownTypes.cs" LinkBase="Shared" />


I think sharing code is ok for now, but doing this results in two caches. I think we should investigate whether we can have a shared library project output to the analyzers directory that generators, analyzers, and code fixers can reference.

For route pattern parsing, there is a lot more code, and we don't want to duplicate parsing and caching route patterns twice (once in analyzer assembly, once in generators assembly)

I think sharing code is ok for now, but doing this results in two caches. I think we should investigate whether we can have a shared library project output to the analyzers directory that generators, analyzers, and code fixers can reference.

This is an interesting proposal. @chsienki do you happen to know if there is a pattern for sharing a symbol cache across analyzers/generators in the same compilation process?

For route pattern parsing, there is a lot more code, and we don't want to duplicate parsing and caching route patterns twice (once in analyzer assembly, once in generators assembly)

Roger that! I captured that in #46342 so we can start discussing what that might look like.

eerhardt · 2023-01-31T16:50:31Z

src/Http/samples/MinimalSample/MinimalSample.csproj

@@ -3,6 +3,8 @@
  <PropertyGroup>
    <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
    <Nullable>enable</Nullable>
+    <EnableRequestDelegateGenerator>true</EnableRequestDelegateGenerator>
+    <EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles>


Do we need this all the time? VS allows you to see the generate files in the Solution Explorer.

I don't use Visual Studio so I'm not able to take advantage of any shortcuts there.

Do we need this all the time?

I found it helpful to have on while debugging as it makes it easier to examine the generated code in another format. There's no con to having it so I figured it was helpful.

I don't use Visual Studio

eerhardt · 2023-01-31T16:57:38Z

src/Shared/RoslynUtils/CodeWriter.cs

+
+internal class CodeWriter
+{
+    private readonly StringBuilder _codeBuilder = new();


This allocation seems unnecessary since this class also takes the StringBuilder stringBuilder in its only ctor.

eerhardt · 2023-01-31T16:58:12Z

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

-                }
-""";
+        var code = new CodeWriter(new StringBuilder());
+        code.Indent(5);


Why the magic 5 here?

When @captainsafia was getting @brunolins16 up to speed yesterday we were discussing the CodeWriter in general. My feedback was that using the CodeWriter rather than simple string concatenation/interpolation actually made it harder to reason over the generated code. I'd rather see us drop the CodeWriter (at the expense of indentation) and then either do some code formatting at the end before handing it back to Roslyn or just leave it a little messy. We can also do a Format Document on the generated code after build if we need to read it.

The Regex source generator uses IndentedTextWriter. Seems to work really well there. You just writer.Indent++ and -- when you are done.

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs#L30

eerhardt · 2023-01-31T16:59:40Z

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

+
+    private static string EmitResponseWritingCall(this Endpoint endpoint)
+    {
+        var code = new CodeWriter(new StringBuilder());


Allocating these intermediate strings seems unnecessary. Why not just pass the existing CodeWriter into this method? It would also help with the WriteNoIndent method calls. You wouldn't need those if you were just writing to the "outer" CodeWriter.

eerhardt · 2023-01-31T17:03:02Z

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

+            if (endpoint.Response.IsAwaitable)
+            {
+                code.WriteLine("var result = await handler();");
+                code.WriteLine(endpoint.EmitResponseWritingCall());
+            }
+            else
+            {
+                code.WriteLine("var result = handler();");
+                code.WriteLine("return GeneratedRouteBuilderExtensionsCore.ExecuteObjectResult(result, httpContext);");
+            }


EmitResponseWritingCall handles IsAwaitable(). So I think this can be refactored to just:

Suggested change

if (endpoint.Response.IsAwaitable)

{

code.WriteLine("var result = await handler();");

code.WriteLine(endpoint.EmitResponseWritingCall());

}

else

{

code.WriteLine("var result = handler();");

code.WriteLine("return GeneratedRouteBuilderExtensionsCore.ExecuteObjectResult(result, httpContext);");

}

code.WriteLine(endpoint.Response.IsAwaitable

? "var result = await handler();"

: "var result = handler();");

code.WriteLine(endpoint.EmitResponseWritingCall());

src/Http/Http.Extensions/gen/GeneratorSteps.cs

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

halter73 · 2023-01-30T21:16:39Z

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

+            else
+            {
+                code.WriteLine("var result = handler();");
+                code.WriteLine("return GeneratedRouteBuilderExtensionsCore.ExecuteObjectResult(result, httpContext);");


Nit: we should be able special-case string and IResult in the generated RequestHandler, because unlike RequestHandlerFiltered, this is only called for non-filtered handlers, right?

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

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

src/Framework/AspNetCoreAnalyzers/src/Analyzers/Http/HeaderDictionaryAddAnalyzer.cs

src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/WellKnownTypeData.cs

captainsafia · 2023-02-02T02:59:01Z

🆙 📅

Removed CodeWriter in favor of string literals with indentation in code sources
Added incrementality tests to validate behavior if Endpoint's comparable implementation
Added and consume Microsoft.Bcl.HashCode API as a dependency
Tweaked smaller pieces of feedback

halter73

I've got a draft PR based on this branch open at #46407. Now I just want to minimize my merge conflicts! 😆 Looks good though!

halter73 · 2023-02-02T18:47:36Z

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

+                {
+                    if (options == null || options.EndpointBuilder == null)
+                    {
+                        return new RequestDelegateMetadataResult { EndpointMetadata = ReadOnlyCollection<object>.Empty };


I don't think this branch is possible given the only caller to this callback is RouteEndpointDataSource, so there will always be an EndpointBuilder.

If this branch were possible, we'd probably want to add the SourceKey here anyway. But since I think it's not, it's probably better to add an assert or throw instead.

src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.Generators.csproj

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

captainsafia · 2023-02-03T01:40:29Z

Merging to unblock forthcoming PRs. Happy to address any other feedback in a followup PR.

captainsafia force-pushed the safia/rdg-responses branch 3 times, most recently from 0571c04 to 26c3f11 Compare January 27, 2023 17:58

build-analysis bot mentioned this pull request Jan 27, 2023

Request failed "500 Internal Server Error" dotnet/arcade#11737

Open

5 tasks

mkArtakMSFT added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jan 27, 2023

captainsafia force-pushed the safia/rdg-responses branch 2 times, most recently from cbbaef0 to 544c00a Compare January 30, 2023 17:44

Add support for response writing to RequestDelegateGenerator

8c5b980

captainsafia force-pushed the safia/rdg-responses branch from 544c00a to 8c5b980 Compare January 30, 2023 18:17

captainsafia mentioned this pull request Jan 30, 2023

Add support for validating nullability warnings in RequestDelegateGenerator #46331

Closed

captainsafia marked this pull request as ready for review January 30, 2023 18:42

captainsafia requested review from Tratcher, BrennanConroy and halter73 as code owners January 30, 2023 18:42

captainsafia requested review from mitchdenny and eerhardt January 30, 2023 18:42

captainsafia linked an issue Jan 31, 2023 that may be closed by this pull request

Support response writing in RequestDelegateGenerator #46276

Closed

4 tasks

JamesNK reviewed Jan 31, 2023

View reviewed changes

eerhardt reviewed Jan 31, 2023

View reviewed changes

halter73 reviewed Jan 31, 2023

View reviewed changes

React to feedback and add incrementality tests

4da1b3a

Route patterns don't factor into incrementality check

b83329e

halter73 approved these changes Feb 2, 2023

View reviewed changes

Address feedback from review

6bdabaa

captainsafia merged commit b138bde into main Feb 3, 2023

captainsafia deleted the safia/rdg-responses branch February 3, 2023 01:40

ghost added this to the 8.0-preview2 milestone Feb 3, 2023

halter73 mentioned this pull request Feb 4, 2023

Using minimal APIs source generator results in null dereference warnings (CS8602) #46446

Closed

Add support for response writing to RequestDelegateGenerator #46291

Add support for response writing to RequestDelegateGenerator #46291

Uh oh!

Conversation

captainsafia commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

captainsafia commented Feb 2, 2023

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

captainsafia commented Feb 3, 2023

Uh oh!

Uh oh!

captainsafia commented Jan 27, 2023 •

edited

Loading