Convert CodeAnalysis.ruleset to .globalconfig

[elachlan]

Follow up to #7174 to CodeAnalysis.ruleset to .globalconfig

This will need to be looked at once the other PRs in the set are merged. It will need to be updated to enable each of the rules.

[Therzok]

Nice, thank you for the quick turnaround!

[sharwell]

It's more efficient to convert .ruleset to .globalconfig (this is the as-designed direct replacement for ruleset files). Here are some examples:

https://github.com/dotnet/roslyn/tree/main/eng/config/globalconfigs
https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-files#global-analyzerconfig

[sharwell]

📝 This should never be set to a severity below warning

Suggested change

	# XML comments
	dotnet_diagnostic.SA0001.severity = suggestion
	# XML comments
	dotnet_diagnostic.SA0001.severity = warning

[elachlan]

Getting this error when enabled.

error SA0001: XML comment analysis is disabled due to project configuration [msbuild\src\Xunit.Net
Core.Extensions\Xunit.NetCore.Extensions.csproj]

[sharwell]

Yes, this warning is alerting the author to a build configuration error (the compiler has been instructed to treat /// comments the same as // comments, and will allow all sorts of syntax errors through without any warnings and analyzers will not be able to help). XML documentations files are an essential build output for all projects due to current limitations in the compiler. You can avoid problems related to this configuration by following the steps described for SA0001:

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA0001.md#suppression-via-the-project-file

[elachlan]

Thanks, I will address that in another PR.

[elachlan]

I tried adding this to Directory.Build.props, it didn't work.

<!--Generate XML Documentation file on build-->
<PropertyGroup>
  <GenerateDocumentationFile>true</GenerateDocumentationFile>
</PropertyGroup>

I also tried adding the same to each individual project in the solution, this also didn't work. I still received:
CSC : warning SA0001: XML comment analysis is disabled due to project configuration

[sharwell]

📝 GitHub won't let me remove the "request changes" status while the pull request is marked as a draft. The second round of comments are suggestions; there are no more critical matters that would warrant a Request Changes state from me.

[Forgind]

I'm guessing the "suggestions" are places where there are quite a few instances in MSBuild that would need changing, and that can be done in a separate PR. (Without knowing what they are, they might even be things we don't really want.)

[Forgind]

📝 GitHub won't let me remove the "request changes" status while the pull request is marked as a draft. The second round of comments are suggestions; there are no more critical matters that would warrant a Request Changes state from me.

I can make it go away by requesting a review from you 🙂

[elachlan]

@Forgind I made the changes and we aren't getting build failures. I am pretty sure the stylecop analyzer isn't running and I don't know why.

[elachlan]

OK!!! I think that fixes the analyzers!

Basically the way it was setup, it was using the built in analyzer and not using the nuget package. So I switched it to use a GlobalPackageReference and set EnableNetAnalyzers.

I then added the rules which were returning warnings which were breaking the build and set them to suggestion.

We will address the fixes for those in separate PRs.

I suggest we merge this and rebase all the existing PRs for the analyzer fixes.

Requested change was addressed

[ladipro]

I've left a couple of nits. I don't think I'm qualified to sign off but if @sharwell is fine with the changes I'll approve.

[elachlan]

I think I have sorted it so that the analyzers in the deprecated folder don't run. I have added a Directory.Build.props file to the deprecated folder to define a variable indicating that those projects are deprecated. This makes it a lot easier to exclude them using conditions.

Next is making the so that we disable SA1614 on test projects. Should I do this via a globalsupression file, or should we use a separate .globalconfig for test projects?

[Forgind]

I commented on things I think I know about, but I also don't know too much about this, so we can wait for sharwell.

[Forgind]

LGTM! Will merge this upon sharwell's approval.

[rainersigwald]

Hi @elachlan! It looks like you were busy this new year! Unfortunately, I don't think we're going to be able to quickly review and merge 27 PRs. Since it sounds like this should go in first, should we focus on it, then reactivate the other fixes + rule-strengthening changes a bit more gradually over time? The simplifying changes @sharwell just proposed sound very nice to me.

[elachlan]

Hi @elachlan! It looks like you were busy this new year! Unfortunately, I don't think we're going to be able to quickly review and merge 27 PRs. Since it sounds like this should go in first, should we focus on it, then reactivate the other fixes + rule-strengthening changes a bit more gradually over time? The simplifying changes @sharwell just proposed sound very nice to me.

@rainersigwald yes, this should get a merge first. Then each of the others. For the others I have purposely picked the ones with less changes to make it less of a burden.

This is probably more important because the analyzers were previously not working properly. Meaning new occurrences could slip through.

[elachlan]

Anyone feel free to jump in and sort the last bits. I am afk for the next few days.

[Forgind]

Thanks @elachlan!