Suppress CS1591 warnings for PublicTopLevelProgram.Generated.g.cs #60727

benhopkinstech · 2025-03-03T22:36:15Z

Description

Suppress CS1591 compiler warnings for PublicTopLevelProgram.Generated.g.cs
Added some additional tests surrounding namespaces

benhopkinstech · 2025-03-03T22:37:51Z

@dotnet-policy-service agree

captainsafia · 2025-03-04T02:53:25Z

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

    private const string PublicPartialProgramClassSource = """
 // <auto-generated />
+#pragma warning disable CS1591


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.

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.

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

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>

captainsafia · 2025-03-04T02:57:15Z

@Youssef1313 Would appreciate your thoughts on this. It seems like there were some cases that the analyzer was false flagging earlier.

Youssef1313 · 2025-03-04T04:36:57Z

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

+            // 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;
+            }


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?

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 { } }

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?

@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

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

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

Youssef1313 · 2025-03-04T04:38:18Z

src/Framework/AspNetCoreAnalyzers/test/SourceGenerators/PublicTopLevelProgramGeneratorTests.cs

+namespace Foo
+{
+    public partial class Program { }
+}
+""")]
+    public async Task DoesNotGeneratesSource_IfProgramIsAlreadyPublic(string declaration)


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.

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.

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?

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

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

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

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.

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

@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?

@captainsafia Not that I know of.

benhopkinstech added 3 commits March 3, 2025 21:04

Added pragma to suppress CS1591 in generated code

f3fc721

Merge branch 'dotnet:main' into main

8d6d8ed

Fixed program declaration in explicit namespace

3575aa9

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

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 3, 2025

captainsafia reviewed Mar 4, 2025

View reviewed changes

Youssef1313 reviewed Mar 4, 2025

View reviewed changes

benhopkinstech marked this pull request as draft March 4, 2025 10:03

Accounted for comments, reverting additional logic

d992cb6

benhopkinstech changed the title ~~Fix some issues surrounding PublicTopLevelProgramGenerator.cs~~ Suppress CS1591 warnings for PublicTopLevelProgram.Generated.g.cs Mar 4, 2025

benhopkinstech marked this pull request as ready for review March 4, 2025 12:38

captainsafia approved these changes Mar 4, 2025

View reviewed changes

captainsafia merged commit 6ffab87 into dotnet:main Mar 4, 2025
27 checks passed

dotnet-policy-service bot added this to the 10.0-preview3 milestone Mar 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress CS1591 warnings for PublicTopLevelProgram.Generated.g.cs #60727

Suppress CS1591 warnings for PublicTopLevelProgram.Generated.g.cs #60727

benhopkinstech commented Mar 3, 2025 •

edited

Loading

benhopkinstech commented Mar 3, 2025

captainsafia Mar 4, 2025

Youssef1313 Mar 4, 2025

benhopkinstech Mar 4, 2025

benhopkinstech Mar 4, 2025

captainsafia commented Mar 4, 2025

Youssef1313 Mar 4, 2025

benhopkinstech Mar 4, 2025

captainsafia Mar 4, 2025

benhopkinstech Mar 4, 2025

martincostello Mar 4, 2025

captainsafia Mar 4, 2025

Youssef1313 Mar 4, 2025 •

edited

Loading

benhopkinstech Mar 4, 2025

Youssef1313 Mar 4, 2025

benhopkinstech Mar 4, 2025

Youssef1313 Mar 4, 2025

Youssef1313 Mar 4, 2025

captainsafia Mar 4, 2025

benhopkinstech Mar 4, 2025

captainsafia Mar 4, 2025

Youssef1313 Mar 4, 2025

Suppress CS1591 warnings for PublicTopLevelProgram.Generated.g.cs #60727

Suppress CS1591 warnings for PublicTopLevelProgram.Generated.g.cs #60727

Conversation

benhopkinstech commented Mar 3, 2025 • edited Loading

Description

benhopkinstech commented Mar 3, 2025

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 Mar 4, 2025

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

Youssef1313 Mar 4, 2025 • 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

benhopkinstech commented Mar 3, 2025 •

edited

Loading

Youssef1313 Mar 4, 2025 •

edited

Loading