Add source generator to emit public Program class definition #58199

captainsafia · 2024-10-02T04:20:52Z

With the introduction of top-level statements in .NET 6, there is not longer an explicit Program class defined in user's ASP.NET Core applications. Instead, we rely on the Program class generated by the compiler, which has a default visibility of internal.

This introduces an annoying hurdle for users who want to write integration tests on top of our WebApplicationFactory which requires a public entrypoint as a generic type argument.

To work around this, we document that users can either:

Add an IVT from their application code to their test code
Add a public partial class Program {} declaration

The first approach runs the risk of the user having to expose truly internal types to their test code. The second approach is hard to discover.

To resolve this issue, we introduce a new source generator to the shared framework that will emit the public partial class Program {} declaration to applications that:

Use top-level statements
Don't declare the Program class as public in some other fashion
Don't use the old-style Program.Main declarations

captainsafia · 2024-10-02T04:21:51Z

src/Framework/App.Ref/src/Microsoft.AspNetCore.App.Ref.csproj

@@ -73,6 +73,10 @@ This package is an internal implementation of the .NET Core SDK and is not meant
      Private="false"
      ReferenceOutputAssembly="false" />

+    <ProjectReference Include="..\..\AspNetCoreAnalyzers\src\SourceGenerators\Microsoft.AspNetCore.App.SourceGenerators.csproj"


@wtgodbe Can you sanity check my packaging changes here? I think it's correct given what we had to do in .NEt 8 for the RequestDelegateGenerator but let me know if I missed something.

captainsafia · 2024-10-02T04:22:14Z

src/Framework/AspNetCoreAnalyzers/src/SourceGenerators/PublicTopLevelProgramGenerator.cs

+    {
+        var internalGeneratedProgramClass = context.CompilationProvider.Select((compilation, cancellationToken) =>
+        {
+            var program = compilation.GetTypeByMetadataName("Program");


cc: @amcasey for Roslyn review

martincostello · 2024-10-02T06:19:03Z

Just curious, will there be any analyzer or anything that could point out this was added and that any manually added Program declarations can be removed?

I have a tonne of these I could delete with this built-in (example), so anything that could point it out rather than me needing to go find them all would be useful 😅

gfoidl · 2024-10-02T09:15:33Z

A different idea / approach:

Update the compiler so that it can emit a public class Program that is configurable via MsBuild-property.
So the default remains the same, when the property is set the public class is emitted.
Thus the change here boils down to updating the template csproj.

Or is the process via compiler, sdk, etc. too tedious that this approach is cheaper?

captainsafia · 2024-10-02T15:44:00Z

Just curious, will there be any analyzer or anything that could point out this was added and that any manually added Program declarations can be removed?

Oh, good idea! We can definitely author an analyzer + codefix that discovers the public partial class Program { } and recommends removing them if you're targeting net10 and above.

Update the compiler so that it can emit a public class Program that is configurable via MsBuild-property.

There was a proposal to change the compiler's behavior to emit a public Program class by default for top-level statements but it was rejected because:

At the moment, this ASP.NET Core integration testing scenario seems to be the only one.
More importantly, there were questions about what the visibility/accessibility of types defined in the generated Program class should be.

With those in mind, it was a bit more straightforward for ASP.NET to control its own destiny here and author a source generator.

amcasey · 2024-10-02T20:22:44Z

(Haven't read the code yet)

We also don't want to do this if they've explicitly declared it internal, right?

internal partial class Program {}

src/Framework/AspNetCoreAnalyzers/src/SourceGenerators/PublicTopLevelProgramGenerator.cs

amcasey · 2024-10-02T20:32:59Z

I'm pretty sure the compilation knows its own entrypoint, but I'm not sure that's exposed anywhere.

Edit: this might be a useful reference.

captainsafia · 2024-10-03T00:25:42Z

We also don't want to do this if they've explicitly declared it internal, right?

Yep, this is another good case to cover in tests. Practically, I don't know how frequently users are doing this since the visibility of the Program class is already internal. ~~I suppose it's also possible for somebody to declare private partial class Program { }.~~ It's not you can't declare private or protected access in top-level classes.

Edit: this might be a useful reference.

Good reference! I'll incorporate some of the checks here into our code.

Tornhoof · 2024-10-03T07:59:00Z

I kinda second @gfoidl's question, isn't there a better way to do this, which technically should just be some switch somewhere during project generation or similar, run only once. Instead a source generator which is evaluated more often.

captainsafia · 2024-10-03T16:12:48Z

I kinda second @gfoidl's question, isn't there a better way to do this, which technically should just be some switch somewhere during project generation or similar, run only once. Instead a source generator which is evaluated more often.

I started off with this strategy but reached a limitation because there's not a ton of ability to introspect what the Program class that is already defined looks like. We'd end up generating the public partial class Program { } declaration in more situations that is acceptable which might result in surprising behavior.

Also,

some switch somewhere during

I'm assuming you're referring to a switch that the user would set. I'm keen to have this behavior be on-by-default since that moves the needle for this experience a lot more than users having to switch from a declaration in their source code to another switch in their app code.

amcasey

The checks that Program is the generated one seem sufficient. I'm ignorant about whether this is an efficient way to write a source generator.

amcasey · 2024-10-03T16:26:38Z

src/Framework/AspNetCoreAnalyzers/src/SourceGenerators/PublicTopLevelProgramGenerator.cs

+            }
+            // If the discovered `Program` type is an interface, struct or generic type then its not
+            // generated and has been defined in source, so we can skip it
+            if (program.TypeKind == TypeKind.Struct || program.TypeKind == TypeKind.Interface || program.IsGenericType)


Out of curiosity, why not program.TypeKind != TypeKind.Class?

That's....a good idea 😅 I'll update as needed.

src/Framework/AspNetCoreAnalyzers/src/SourceGenerators/PublicTopLevelProgramGenerator.cs

Tornhoof · 2024-10-03T16:54:26Z

I'm assuming you're referring to a switch that the user would set.

Yeah, something like the implicit usings setting or something similar, could then even be enabled automatically during project creation, but I understand that's a lot more effort than a src gen. Using MSBuild properties to decide if a srcgen should be executed is also not as straight forward as one might think. I admit, that I don't really see any quick win here.

amcasey

The selection logic looks correct to me.

...rk/AspNetCoreAnalyzers/src/SourceGenerators/Microsoft.AspNetCore.App.SourceGenerators.csproj

amcasey · 2024-10-08T16:59:57Z

src/Framework/AspNetCoreAnalyzers/src/SourceGenerators/PublicTopLevelProgramGenerator.cs

+    {
+        var internalGeneratedProgramClass = context.CompilationProvider
+            // Get the entry point associated with the compilation, this maps to the Main method definition
+            .Select((compilation, cancellationToken) => compilation.GetEntryPoint(cancellationToken))


Iterated selection is very tidy, but it seems like it might be expensive. If this is run repeatedly, I wonder if it would be more efficient to have a single Select with a bigger lambda.

According to the guidance on writing incremental generators it's better to split things out to multiple Selects so that there's an opportunity to cache more frequently in between steps.

Surely that's only true for things that change independently? Retrieving and casting the containing type in the first select doesn't seem like something that could affect caching.

Yes, I suppose the first two selects could be collapsed into one given the things they evaluate are closely related but I don't think there's anything actively harmful about splitting up the transformation.

src/Framework/AspNetCoreAnalyzers/src/SourceGenerators/PublicTopLevelProgramGenerator.cs

captainsafia · 2024-10-09T00:48:31Z

Merging this now to validate the E2E both perf and functionality wise once it is inserted into the alpha SDK. I'll follow up with building the analyzer after that.

* Add source generator to emit public Program class definition * Add more checks and test cases * Use GetEntrypoint API and transformations for better caching * Address feedback

Add source generator to emit public Program class definition

7163cb0

captainsafia requested review from wtgodbe and a team as code owners October 2, 2024 04:20

dotnet-issue-labeler bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 2, 2024

captainsafia added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Oct 2, 2024

captainsafia commented Oct 2, 2024

View reviewed changes

amcasey reviewed Oct 2, 2024

View reviewed changes

Add more checks and test cases

707bd27

amcasey reviewed Oct 3, 2024

View reviewed changes

Use GetEntrypoint API and transformations for better caching

3bc89cd

captainsafia force-pushed the safia/public-program-gen branch from 1e09f50 to 3bc89cd Compare October 5, 2024 04:23

amcasey approved these changes Oct 8, 2024

View reviewed changes

Address feedback

7477d54

captainsafia merged commit 15f0a89 into main Oct 9, 2024
27 checks passed

captainsafia deleted the safia/public-program-gen branch October 9, 2024 00:48

dotnet-policy-service bot added this to the 10.0-preview1 milestone Oct 9, 2024

This was referenced Oct 17, 2024

Provide analyzer for removing unneeded public partial class Program #58482

Merged

Provide information diagnostic when public partial class Program { } declaration is found in code #58488

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source generator to emit public Program class definition #58199

Add source generator to emit public Program class definition #58199

captainsafia commented Oct 2, 2024

captainsafia Oct 2, 2024

captainsafia Oct 2, 2024

martincostello commented Oct 2, 2024

gfoidl commented Oct 2, 2024

captainsafia commented Oct 2, 2024

amcasey commented Oct 2, 2024

amcasey commented Oct 2, 2024 •

edited

Loading

captainsafia commented Oct 3, 2024 •

edited

Loading

Tornhoof commented Oct 3, 2024

captainsafia commented Oct 3, 2024

amcasey left a comment

amcasey Oct 3, 2024

captainsafia Oct 3, 2024

Tornhoof commented Oct 3, 2024 •

edited

Loading

amcasey left a comment

amcasey Oct 8, 2024

captainsafia Oct 8, 2024

amcasey Oct 8, 2024

captainsafia Oct 8, 2024

captainsafia commented Oct 9, 2024

Add source generator to emit public Program class definition #58199

Add source generator to emit public Program class definition #58199

Conversation

captainsafia commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martincostello commented Oct 2, 2024

gfoidl commented Oct 2, 2024

captainsafia commented Oct 2, 2024

amcasey commented Oct 2, 2024

amcasey commented Oct 2, 2024 • edited Loading

captainsafia commented Oct 3, 2024 • edited Loading

Tornhoof commented Oct 3, 2024

captainsafia commented Oct 3, 2024

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tornhoof commented Oct 3, 2024 • edited Loading

amcasey 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

captainsafia commented Oct 9, 2024

amcasey commented Oct 2, 2024 •

edited

Loading

captainsafia commented Oct 3, 2024 •

edited

Loading

Tornhoof commented Oct 3, 2024 •

edited

Loading