Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,30 +10,42 @@ namespace Microsoft.AspNetCore.SourceGenerators;
[Generator]
public class PublicProgramSourceGenerator : IIncrementalGenerator
{
private const string Program = "Program";
private const string PublicPartialProgramClassSource = """
// <auto-generated />
#pragma warning disable CS1591
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right here. AFAIK, analyzers shouldn't trigger on generated code.

In the past, I've noticed that the // <auto-generated /> marker on the file isn't sufficient. We might also want to emit [GeneratedCode] here to mark this type as generated.

You can see how we do it for the OpenAPI XML generator and the minimal API source generator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an analyzer, it's a compile warning. There were discussions in dotnet/roslyn to special case that specific one for generated code, but I'm not sure if anything was done yet, or if any decision was taken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Projects that have <GenerateDocumentationFile>true</GenerateDocumentationFile> in the .csproj will have the CS1591 compiler warning when the current generated code is output preventing building the solution

CS1591 looks as follows in this case
Missing XML comment for publicly visible type or member 'Program'

This #pragma should suppress this warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something important to note is that the project in the issue only has the CS1591 warning preventing the build due to them also having
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
That being said we still don't want this warning to flag for the generated code when the below option is stated in the project
<GenerateDocumentationFile>true</GenerateDocumentationFile>

public partial class Program { }
#pragma warning restore CS1591
""";

public void Initialize(IncrementalGeneratorInitializationContext context)
{
var internalGeneratedProgramClass = context.CompilationProvider.Select(static (compilation, cancellationToken) =>
{
// If there is a user-declared Program in a non-global namespace, skip generation
if (compilation.GetSymbolsWithName(Program, SymbolFilter.Type, cancellationToken)
.OfType<INamedTypeSymbol>()
.Any(symbol => !symbol.ContainingNamespace.IsGlobalNamespace))
{
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like whether or not there is a class named Program in non-global-namespace is irrelevant? What's the use case that this change helps with?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I have done this is due to what Martin stated in the original issue. He had a Program class that was in a user-declared namespace (with relevant xml doc comment) but the code was still getting generated and throwing the CS1591 compiler warning.

This is extra puzzling, as the Program.cs file already has code to manually expose the class:

namespace WebApi
{
    /// <summary>
    /// Expose the Program class for use with <c>WebApplicationFactory</c>
    /// </summary>
    public partial class Program
    {
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He had a Program class that was in a user-declared namespace (with relevant xml doc comment) but the code was still getting generated and throwing the CS1591 compiler warning.

I don't think we got confirmation that this was indeed what was happening.

@martincostello Would you be able to verify if this source generator was triggering by settign <EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles> and seeing if you see a source file emitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@captainsafia I have this sln cloned in the issue, I also needed to add
<CompilerGeneratedFilesOutputPath>...</CompilerGeneratedFilesOutputPath>
This has produced the below file so it is generating
...\Microsoft.AspNetCore.App.SourceGenerators\Microsoft.AspNetCore.SourceGenerators.PublicProgramSourceGenerator\PublicTopLevelProgram.Generated.g.cs

It is also confirmed by the tests that are in the latest changes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If you still need me to try this out I can take a look tomorrow)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benhopkinstech Thanks for verifying this, Ben! @martincostello I think we're good to go here.


// Get the entry point associated with the compilation, this maps to the Main method definition
// Get the containing symbol of the entry point, this maps to the Program class
compilation.GetEntryPoint(cancellationToken)?.ContainingSymbol is INamedTypeSymbol
{
// If the discovered `Program` type is not a class then its not
// generated and has been defined in source, so we can skip it
// If the program class is already public, we don't need to generate anything.
DeclaredAccessibility: not Accessibility.Public,
TypeKind: TypeKind.Class,
// If there are multiple partial declarations, then do nothing since we don't want
// to trample on visibility explicitly set by the user
DeclaringSyntaxReferences: { Length: 1 } declaringSyntaxReferences
} &&
// If the `Program` class is already declared in user code, we don't need to generate anything.
declaringSyntaxReferences.Single().GetSyntax(cancellationToken) is not ClassDeclarationSyntax
);
return compilation.GetEntryPoint(cancellationToken)?.ContainingSymbol is INamedTypeSymbol
{
// If the program class is already public, we don't need to generate anything
DeclaredAccessibility: not Accessibility.Public,
// If the discovered `Program` type is not a class then its not
// generated and has been defined in source, so we can skip it
TypeKind: TypeKind.Class,
// If there are multiple partial declarations, then do nothing since we don't want
// to trample on visibility explicitly set by the user
DeclaringSyntaxReferences: { Length: 1 } declaringSyntaxReferences
} &&
// If the `Program` class is already declared in user code, we don't need to generate anything
declaringSyntaxReferences.Single().GetSyntax(cancellationToken) is not ClassDeclarationSyntax;
});

context.RegisterSourceOutput(internalGeneratedProgramClass, (context, result) =>
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,25 @@ public async Task GeneratesSource_ProgramWithTopLevelStatements()

var expected = """
// <auto-generated />
#pragma warning disable CS1591
public partial class Program { }
#pragma warning restore CS1591
""";

await VerifyCS.VerifyAsync(source, "PublicTopLevelProgram.Generated.g.cs", expected);
}

[Fact]
public async Task DoesNotGeneratesSource_IfProgramIsAlreadyPublic()
[Theory]
[InlineData("public partial class Program { }")]
[InlineData("""
namespace Foo
{
public partial class Program { }
}
""")]
public async Task DoesNotGeneratesSource_IfProgramIsAlreadyPublic(string declaration)
Copy link
Member

@Youssef1313 Youssef1313 Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this case should generate. The containing type of the entrypoint is not public in this case.

Because of the use of top-level statements, the compiler will synthesize a Program class in the global namespace. So, Foo.Program becomes irrelevant.

@captainsafia should be able to confirm though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem a bit of a workaround and the analyzer ASP0027 doesn't pick this up for removal when it is within a user-defined namespace.

I don't think it is correct the user defining a namespace in this way and putting the Program within it but it has obviously worked for them prior to the auto generation logic.

The logic in PublicTopLevelProgramGenerator.cs prior to my change only fails on the DoesNotGeneratesSource_IfProgramIsAlreadyPublic and DoesNotGeneratesSource_IfProgramDeclaresExplicitInternalAccess namespace version of the tests, the rest of the namespace tests I have added passed successfully.

If we can agree on what this should be doing in this minimal API scenario where the user has defined a namespace I can adjust the logic and tests accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the generator should still run in that case, as the user-defined Program class is completely unrelated to the entrypoint. It's just as any regular type. Maybe the analyzer should be relaxed in this case, if it's currently producing a diagnostic. @captainsafia What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analyzer is not producing a diagnostic in this case, so that is probably correct if the generator should be running in this case as per your comments

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the generator is running in this case and the analyzer is not producing a diagnostic, then I'm not following on what the issue is

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #pragma warning disable change is good to me. But the change on whether or not the generation happens is questionable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #pragma warning disable change is good to me. But the change on whether or not the generation happens is questionable.

I agree with this as well. I want to get more clarity on the exact repro (see #60727 (comment)) before we make the other changes.

The analyzer and the source generator are two separate entities. There is a chance that the analyzer is not emitting a diagnostic for a given syntax structure but the source generator is emitting source code regardless.

Some additional tests in https://github.com/dotnet/aspnetcore/blob/1f82fa558dd7afb0ac94a43c7b80f40499bad98c/src/Framework/AspNetCoreAnalyzers/test/SourceGenerators/PublicTopLevelProgramGeneratorTests.cs should help us verify this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@captainsafia I have pushed a commit today that reverts any changes to logic, only supressing the compiler warning and including additional tests around namespaces

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benhopkinstech Thanks! This PR looks good to me now.

@Youssef1313 Are there any other compiler warnings we should consider pragma disabling here while we're at it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@captainsafia Not that I know of.

{
var source = """
var source = $$"""
using Microsoft.AspNetCore.Builder;

var app = WebApplication.Create();
Expand All @@ -40,16 +49,23 @@ public async Task DoesNotGeneratesSource_IfProgramIsAlreadyPublic()

app.Run();

public partial class Program { }
{{declaration}}
""";

await VerifyCS.VerifyAsync(source);
}

[Fact]
public async Task DoesNotGeneratesSource_IfProgramDeclaresExplicitInternalAccess()
[Theory]
[InlineData("internal partial class Program { }")]
[InlineData("""
namespace Foo
{
internal partial class Program { }
}
""")]
public async Task DoesNotGeneratesSource_IfProgramDeclaresExplicitInternalAccess(string declaration)
{
var source = """
var source = $$"""
using Microsoft.AspNetCore.Builder;

var app = WebApplication.Create();
Expand All @@ -58,7 +74,7 @@ public async Task DoesNotGeneratesSource_IfProgramDeclaresExplicitInternalAccess

app.Run();

internal partial class Program { }
{{declaration}}
""";

await VerifyCS.VerifyAsync(source);
Expand Down Expand Up @@ -86,6 +102,31 @@ public static void Main()
await VerifyCS.VerifyAsync(source);
}

[Fact]
public async Task DoesNotGeneratorSource_ExplicitPublicProgramClassInNamespace()
{
var source = """
using Microsoft.AspNetCore.Builder;

namespace Foo
{
public class Program
{
public static void Main()
{
var app = WebApplication.Create();

app.MapGet("/", () => "Hello, World!");

app.Run();
}
}
}
""";

await VerifyCS.VerifyAsync(source);
}

[Fact]
public async Task DoesNotGeneratorSource_ExplicitInternalProgramClass()
{
Expand All @@ -108,6 +149,31 @@ public static void Main()
await VerifyCS.VerifyAsync(source);
}

[Fact]
public async Task DoesNotGeneratorSource_ExplicitInternalProgramClassInNamespace()
{
var source = """
using Microsoft.AspNetCore.Builder;

namespace Foo
{
internal class Program
{
public static void Main()
{
var app = WebApplication.Create();

app.MapGet("/", () => "Hello, World!");

app.Run();
}
}
}
""";

await VerifyCS.VerifyAsync(source);
}

[Theory]
[InlineData("interface")]
[InlineData("struct")]
Expand All @@ -127,6 +193,33 @@ public static void Main(string[] args)
app.Run();
}
}
""";

await VerifyCS.VerifyAsync(source);
}

[Theory]
[InlineData("interface")]
[InlineData("struct")]
public async Task DoesNotGeneratorSource_ExplicitInternalProgramTypeInNamespace(string type)
{
var source = $$"""
using Microsoft.AspNetCore.Builder;

namespace Foo
{
internal {{type}} Program
{
public static void Main(string[] args)
{
var app = WebApplication.Create();

app.MapGet("/", () => "Hello, World!");

app.Run();
}
}
}
""";

await VerifyCS.VerifyAsync(source);
Expand Down
Loading