Clean up generation of NetAnalyzers GenerateAnalyzerRulesMissingDocumentationFile stuff

[baronfel]

Is your feature request related to a problem? Please describe.

With #50167 added to the repo the build now creates a new file that's constantly being generated on many different PRs from the GenerateAnalyzerConfigAndDocumentationFiles Target.

This file needs some other generation strategy - the Target itself isn't incremental (in the MSBuild sense), it gets constantly regenerated on every build (which the SDK team does frequently to run tests, test new layouts, etc), and generally makes me a very sad panda.

Describe the solution you'd like

If this file is required, it should be

• generated to a location that doesn't show as an untracked file to everyone working on the SDK
• the generation of the should ideally be incremental
• there should be some sort of guidance/check that can flag this file for follow-up by the net analyzers owners, etc.
• not generate noise in the day-to-day of the SDK repo

[Youssef1313]

The file is expected to be checked in source control https://github.com/dotnet/sdk/blob/main/src/Microsoft.CodeAnalysis.NetAnalyzers/src/RulesMissingDocumentation.md

Whenever the content changes "suddenly" between builds, the diff will be "removal" of entries that got documented.

the build now creates a new file

It should be modifies (the file already exists), not creates. If it's being created in a directory other than src/Microsoft.CodeAnalysis.NetAnalyzers/src/RulesMissingDocumentation.md, then the generation and the file checked in source control should be aligned.

sdk/src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers.Package.csproj

Lines 68 to 70 in 27c4bbf

	<AnalyzerDocumentationFileDir>$(NetAnalyzersRootDir)src</AnalyzerDocumentationFileDir>
	<AnalyzerSarifFileDir>$(NetAnalyzersRootDir)src</AnalyzerSarifFileDir>
	<AnalyzerConfigurationFileDir>$(NetAnalyzersRootDir)docs</AnalyzerConfigurationFileDir>

[baronfel]

@Youssef1313 regardless of the generation/diffs/etc - there should be a gate that prevents merging a PR if there are changes to this file. I'd suggest a unit test of some kind. If there are changes to this file introduced by a PR, dependency flow, etc - the need to react to that change should be scoped to the PR that causes the change, not become a cost paid by every other contributor.

[Youssef1313]

@baronfel There are two different kind of changes to RulesMissingDocumentation.md:

1. Adding a new rule
   • This happens when a PR adds a new analyzer, and this is validated in CI (or at least I'm pretty confident it was validated when the code was in roslyn-analyzers repo)
2. Removing a rule
   • This happens when a PR on dotnet/docs is merged and becomes live, causing the rule to become documented and is no longer missing documentation.

Then, there are two modes of running the GenerateDocumentationAndConfigFiles tool:

1. "validation only" mode, this runs in CI
   • This mode tolerates any "extra" entries in RulesMissingDocumentation.md. It's done this way to prevent a sudden break of CI builds when PRs are merged to dotnet/docs.
   • This mode doesn't tolerate missing entries in RulesMissingDocumentation.md. Any missing entry means that it's a PR adding a new analyzer but didn't update RulesMissingDocumentation.md
2. "generation" mode
   • This mode doesn't run in CI. Back when it was in roslyn-analyzers, I believe it was run only on "pack", not on "build". So the experience was:
     • If user is making a simple bug fix, building and testing it only, without packing. They are fine.
     • If user is doing pack for any reason, they can potentially get changes to this file, which was acceptable. At the end of the day, the content of this file is dependent on what stuff are documented in dotnet/docs repo, so we cannot avoid the fact that its content can suddenly change.

The "validation only" mode was introduced in dotnet/roslyn-analyzers#4295, because previously CI would have always been green regardless of the contents of those generated files.

[Youssef1313]

If there are changes to this file introduced by a PR, dependency flow, etc - the need to react to that change should be scoped to the PR that causes the change

To make it more clear answering this part, the changes to RulesMissingDocumentation.md that are not validated in CI are not scoped to a PR in dotnet/sdk (or formerly roslyn-analyzers). It's a reaction on a documentation merge in dotnet/docs.

[baronfel]

It's a reaction on a documentation merge in dotnet/docs.

That, frankly, sounds horrible for build correctness?

[baronfel]

I would strongly push for putting this generation/check into a different, periodic workflow that then triggers a PR with the changed file. We have prior art for this with the manpage generation: https://github.com/dotnet/sdk/blob/main/.github/workflows/update-man-pages.yml

This

• keeps aligned with an external source
• doesn't impact local builds in any way