Add infrastructure for RequestDelegateGenerator

[captainsafia]

Part of #45524.

This PR adds a new project to encapsulate the Minimal APIs generator:

• The generator is off by default and enabled via the EnableRequestDelegateGenerator build property
• The generator is shipped as part of the Microsoft.AspNetCore.App ref pack
• The generator is bare-bones as is and only works for emitting endpoints that take no parameters and return a string
• The generator includes integration and unit tests scaffold

The goal of this PR is to get a barebones steel-thread working so we can validate a generator can be built, tested, shipped, and run from the SDK.

[captainsafia]

Instead of disabling the generator by suppressing the emission of endpoints as in what was removed in 2d8a509, it's better to disable it by removing it from the Analyzers item group in MSBuild. This will require a two-step change where we make a modification in the SDK as well to remove reference to the analyzer if EnableRazorSourceGenerator is not true.

[wtgodbe]

LGTM other than the 2 comments

[eerhardt]

I took a real quick skim over the high-level stuff. This looks really good.

[eerhardt]

For my learning - what are these files for? And why do we need both net7.0 and net8.0 in them?

[captainsafia]

I'm not 100% sure of the reasons but I ran this delta past @wtgodbe since it was automatically introduced when I ran the GenerateProjectList task after adding the new source generator project to the repo.

From my understanding, it's a requirement of the package validation SDK we use for validating changes across versions.

As to why we have both versions, I'm thinking we might be able to get away with just having net8 in the main branch but I'll let @wtgodbe share.

For now, I just settled with whatever the build produced here 😄

[JamesNK]

I think net7.0 can be removed now that the source is targeting .NET 8.

[captainsafia]

I think net7.0 can be removed now that the source is targeting .NET 8.

Roger that. It probably makes sense to do this in a separate PR. I assume there's no harm to having net7.0 here since we've had it in the main branch for a while.

We might also want to add removing the older framework target as part of our updating branding instructions.

[mitchdenny]

Notice that these files keep changing because the order of the namespaces emitted into the file keep changing. Anyone know the reason for this non-determinism?

[wtgodbe]

@ViktorHofer and @smasher164 own the Package Validation SDK - any idea on why there's a net7.0 & and a net8.0 section in the generated file, or why there may nondeterminism in the ordering of the file? It's been somewhat noisy lately

[ViktorHofer]

@smasher164 can you please take a look? We might have caused a non-deterministic behavior by ordering the compatibility differences in ApiCompat. Or maybe not and something else is wrong.

[eerhardt]

Why do we need this? I thought this wasn't even used anymore.

[davidfowl]

So we can test with reference assemblies, not runtime assemblies.

[captainsafia]

Yep, you can view that code here.

This also means that we can run tests with changes in the repo incorporated. Say we add a new API in the framework that affects the output of the generator, this infrastructure allows us to reference those changes even if they aren't shipped yet.

[eerhardt]

We should consider using the new file class feature for classes that we don't expect user code to need to interact with. That way code inside the user's project can't see these "implementation details" of our source generation.

See https://learn.microsoft.com/En-Us/dotnet/csharp/language-reference/proposals/csharp-11.0/file-local-types.

[davidfowl]

This will disappear anyways. @captainsafia I think on the issue for the source generator, we should output the current generated code and what the north start will look like.

[captainsafia]

I think we would benefit the most from this in the SourceGeneratedRouteEndpointDataSource type that is defined here but since the plan is to remove it eventually, I think the benefit is marginally and we can keep it as a nested private type.

[eerhardt]

Just something to keep in mind when writing a source generator - any internal code here will be visible to the user's project. And people will definitely start using it in ways we don't expect. On the runtime team, we treat the internal code more or less like public API. If we change it in the future, it would be considered a breaking change. Thus the reason we made the language feature.

[captainsafia]

Just something to keep in mind when writing a source generator - any internal code here will be visible to the user's project.

Yeah, we tend to exploit that aspect of the generator for our purposes here, we want user code to be able to see the strongly-typed MapAction overloads so those overloads can be chosen and the user can opt-in to our code-generated logic.

I've added a sample of what the generator currently emits and some explanation of why it works and how it might evolve in the future in the issue here: #45524

[eerhardt]

Yeah, we tend to exploit that aspect of the generator for our purposes here, we want user code to be able to see the strongly-typed MapAction overloads so those overloads can be chosen and the user can opt-in to our code-generated logic.

Right - for the code we want the user to be able to call, we should use internal. But for the implementation details, we don't want it visible to the user.

In your sample code, the SourceKey class is an example of this. This is an implementation detail we don't want visible to the user. So we should use file class SourceKey there.

[davidfowl]

This is an implementation detail we don't want visible to the user. So we should use file class SourceKey there.

we're actually going to expose SourceKey or something like it at runtime publicly. Though, not in this PR.

[Youssef1313]

Not a detailed review, but I'm concerned about proper incrementality here.

[Youssef1313]

This will require a two-step change where we make a modification in the SDK as well to remove reference to the analyzer if EnableRazorSourceGenerator is not true.

Why? isn't it possible that you ship an MSBuild targets file in the source generator package as follows <Analyzer Remove="@(Analyzer)" Condition="'%(Analyzer.NugetPackageId)'=='PackageIdGoesHere' and 'EnableRazorSourceGenerator'!='true'"/>

[eerhardt]

isn't it possible that you ship an MSBuild targets file in the source generator package

There is no NuGet package here. This source generator ships in the ASP.NET ref pack.

[JamesNK]

Should we be adding GeneratedCodeAttribute to generated types and methods?

For example, from generating log source:

partial class Log
{
    [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "8.0.8.5613")]
    private static readonly global::System.Action<global::Microsoft.Extensions.Logging.ILogger, global::Microsoft.AspNetCore.Http.Endpoint, global::System.Exception?> __ExecutingEndpointCallback =
        global::Microsoft.Extensions.Logging.LoggerMessage.Define<global::Microsoft.AspNetCore.Http.Endpoint>(global::Microsoft.Extensions.Logging.LogLevel.Information, new global::Microsoft.Extensions.Logging.EventId(0, "ExecutingEndpoint"), "Executing endpoint '{EndpointName}'", new global::Microsoft.Extensions.Logging.LogDefineOptions() { SkipEnabledCheck = true }); 

    [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "8.0.8.5613")]
    public static partial void ExecutingEndpoint(global::Microsoft.Extensions.Logging.ILogger logger, global::Microsoft.AspNetCore.Http.Endpoint endpointName)
    {
        if (logger.IsEnabled(global::Microsoft.Extensions.Logging.LogLevel.Information))
        {
            __ExecutingEndpointCallback(logger, endpointName, null);
        }
    }
}

[JamesNK]

Also, generated source tends to use fully qualified type names with global:: prefix to avoid type conflicts.

[JamesNK]

Also, what is the usual suffix for source generators projects? We currently have Microsoft.AspNetCore.Http.SourceGeneration. The logging generator lives in Microsoft.Extensions.Logging.Generators. Is there a standard we should follow?

[captainsafia]

Also, what is the usual suffix for source generators projects? We currently have Microsoft.AspNetCore.Http.SourceGeneration. The logging generator lives in Microsoft.Extensions.Logging.Generators. Is there a standard we should follow?

The naming here was partially inspired by the fact that the JSON source generator is under the System.Text.Json.SourceGeneration namespace (ref). It does look like logging uses a different namespace (ref) and the regular expression generator as well with a singular instead of plural name scheme. (ref).

With all that in mind, I actually think that the plural "Generators" is the way to go to match with the naming for "Analyzers" and "CodeFixers".

Also, generated source tends to use fully qualified type names with global:: prefix to avoid type conflicts.

Good call!

Should we be adding GeneratedCodeAttribute to generated types and methods?

I discovered this thread that talks about this a little bit further. It seems like the auto-generated header provides decent coverage but the GeneratedCodeAttribute is helpful for scenarios where compiled binaries need to be audited for generated code.

[eerhardt]

Also, generated source tends to use fully qualified type names with global:: prefix to avoid type conflicts.

See the strategy employed in the Regex source generator and discussion in dotnet/runtime#66432 and dotnet/runtime#63512 for a way to not have to fully qualify everything with global:: prefix.

With this PR, we instead move all of the types into a separate namespace and parent class, which then enables a) having usings inside the namespace declaration so that we can avoid fully-qualifying everything

[Youssef1313]

Consider adding a reference to Microsoft.CodeAnalysis.Analyzers. It currently comes as a transitive package from Microsoft.CodeAnalysis.Common. The direct dependency will ensure dotnet/roslyn-analyzers#6229 doesn't happen in future (I'm not sure why it doesn't happen currently).

[captainsafia]

Where's everyone barometer at with merging this once the build is green? I've addressed the major points of feedback, and there are some outstanding comments:

• Minor (IMO) notes on how we reference Microsoft.CodeAnalysis.Analyzers in the package (Add infrastructure for RequestDelegateGenerator #45924 (comment))
• Notes on how we structure the code to avoid qualifying all references global:(Add infrastructure for RequestDelegateGenerator #45924 (comment)) and leveraging file class where it might be appropriate (Add infrastructure for RequestDelegateGenerator #45924 (comment)). This will depend on where we go with in-framework changes to support new APIs for RouteEndpointDataSource.

I want to make sure we can validate the steel thread with the generator landing in the SDK and supporting the EnableRequestDelegateGenerator property sooner rather than later.

[eerhardt]

Where's everyone barometer at with merging this once the build is green?

I'm good with merging it, as long as you have follow up issues tracking the "TODO" pieces. We should make progress where we can.

[captainsafia]

Where's everyone barometer at with merging this once the build is green?

I'm good with merging it, as long as you have follow up issues tracking the "TODO" pieces. We should make progress where we can.

TODOs tracked in #45524. Let me know if i missed anything.