Add new global analyzer config file support

[jonathanou]

Justification

Rulesets have been deprecated by Microsoft since Visual Studio 2019 16.5, The general recommendation by Microsoft to enable/disable rules is to go through .editorconfig and/or .globalconfig (i.e. Global Analyzer Config).

In addition, if both a ruleset and .editorconfig/globalconfig specifies the same rule, the behavior is undefined per Microsoft's documentation:

The precedence rules for conflicting severity entries from a ruleset file and an EditorConfig or global AnalyzerConfig file is undefined.

As more projects need to customize rules, they will undoubtedly enable/disable through .editorconfig/.globalconfig, and we should switch the NI Analyzer rulesets to the new .editorconfig/globalconfig to prevent undefined behavior.

Implementation

• Convert NI Analyzer rulesets to equivalent .globalconfig files via Visual Studio as instructed by Microsoft.
• Add a new NI.CSharp.Analyzer.props file to include the new .globalconfig files into <GlobalAnalyzerConfigFiles>.
• Update the nuspec file to put props file and .globalconfig files into build directory.
• Update NI.CSharp.Analyzer.targets file to continue to use rulesets if less than .NET 6 (to maintain backwards compatibility).

Testing

Manually built the nupkg with the changes and tested it with an existing C# solution to confirm build is correct. Without the globalconfig files, the codebase had over 10,000 warnings that were disabled by the original ruleset. The warnings largely went away with the new globalconfig files, but I was surprised to see there were still 243 warnings left.

However, upon inspection it seems the warnings are all valid, as the original ruleset also did not disable those warnings. This behavior also explains past code reviews where I noticed certain warnings like not documenting public types not get raised. With the new globalconfig, those warnings all come up correctly; so it seems rulesets had bugs that the .globalconfig files "fixed".

[jryckman]

@jonathanou Which project(s) did you test this with? I'd like to make sure it works with the NXG stack (or what's left of it) if you haven't tested that.

@pakdev could you look into this more and give me your thoughts as well?

[jonathanou]

@jonathanou Which project(s) did you test this with? I'd like to make sure it works with the NXG stack (or what's left of it) if you haven't tested that.

@pakdev could you look into this more and give me your thoughts as well?

I tested with my team's C# solution, which is an ASP.NET Core web app with 22 projects (including tests). I did not test the NXG stack, I can do that.

[jonathanou]

@jonathanou Which project(s) did you test this with? I'd like to make sure it works with the NXG stack (or what's left of it) if you haven't tested that.
@pakdev could you look into this more and give me your thoughts as well?

I tested with my team's C# solution, which is an ASP.NET Core web app with 22 projects (including tests). I did not test the NXG stack, I can do that.

@jryckman I tested it with a subset of Everything.NetCore.sln (98 projects built) and I also saw new warnings pop up. Popular offenders were:

• CA1805 (https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1805)
• CA1829 (https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1829)
• CA1834 (https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1834)
• CA1846 (https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1846)
• CA1848 (https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1848)

These rules are not mentioned at all in NI.Ruleset, and with <IncludeAll Action="Warning" /> I assume that means any rules not mentioned in the ruleset should default to warning? The converted NI.CSharp.Analyzers.globalconfig has this in response to IncludeAll:

# Default severity for analyzer diagnostics - Requires **VS2019 16.5** or later
dotnet_analyzer_diagnostic.severity = warning

If this is the intended behavior, it's not clear to me why the rules mentioned above aren't being reported with NI.Ruleset (is it a bug?); but at least with .globalconfig, it is now correctly reporting them.

[pakdev]

These rules are not mentioned at all in NI.Ruleset, and with <IncludeAll Action="Warning" /> I assume that means any rules not mentioned in the ruleset should default to warning?...If this is the intended behavior, it's not clear to me why the rules mentioned above aren't being reported with NI.Ruleset (is it a bug?)

It seems that <IncludeAll Action="Warning" /> doesn't work properly. I checked the first couple of rules that you mentioned (1805, 1829, and 1834), and their RunLevel is either IdeHidden_BulkConfigurable or IdeSuggestion - both values that yield a DiagnosticSeverity of Hidden or Info, respectively. Thus, it seems like their absence makes sense in light of that issue, and switching to editorconfig will mitigate the problem.

@pakdev could you look into this more and give me your thoughts as well?

Searching Teams, it seems like my only reservation with switching to editorconfig in the past was that it was unenforceable. This was true until VS 2019 16.3 (released shortly after my comment in September 2019). Since we should all be at least at that version (likely 2022+), I think this is a great change that we should embrace. Thanks for tackling this, @jonathanou!

[pakdev]

Do you know if this is something we're required to support?

[nick-beer]

This is a good question, but I think I would phrase it a little differently. I wonder if it makes sense for us to freeze our .NET Framework projects on the current version or earlier, and choose to support only .NET 6+ with subsequent changes to this analyzer package.

I'd really like to be taking things out of this package more than putting more and more into it - and maybe if we froze things for .NET Framework we could do that? For instance, we really should remove our dependencies on the microsoft analyzer packages that are supplied by the dotnet SDK - that's not needed (and causes problems) for modern .NET.

[jonathanou]

I wonder if it makes sense for us to freeze our .NET Framework projects on the current version or earlier, and choose to support only .NET 6+ with subsequent changes to this analyzer package.

I went in assuming we had to maintain compatibility for the .NET framework build, but if there's the option to stop supporting it I would definitely be in favor.

ASW's Everything.sln is still using 1.2.14 of this analyzer package, not even the current 2.0.11, so I think it may already be effectively frozen for all intents and purposes? I don't know if there are other .NET framework builds in the company that want to adopt the latest analyzer changes though.

Who can make the call for stopping support?

[jryckman]

Good question on support. I don't know. I think someone like @nick-beer or @gregr-ni could help drive that or point us in the right direction.

[gregr-ni]

I don't have a fundamental problem with saying that new versions don't support .NET framework or making "breaking" changes like dropping ruleset support in favor of configs. The things I'd think about are the ability to provide patches to the version that .NET framework projects are expected to freeze on and the workflow for projects that support both.

If framework projects are expected to freeze, that implies projects that support framework would continue to use rulesets while core projects would switch to configs. That is not exactly convenient for a project that supports both. Both would need to be maintained and stay in sync.

[nick-beer]

@gregr-ni - unfortunately, I think we're already in the place you describe - in ASW at least. We have a set of .ruleset files for .NET Framework and a set of .ruleset files for .NET 6+. Additionally, we haven't brought .NET Framework forward to the latest version of this package, so we already have issues with patches.

My thought is that this is ok - .NET Framework development is very stable at this point. We haven't made many changes to this package that are needed by the .NET Framework build, and we haven't changed the rulesets in some time. The problem with being frozen, and the task of keeping things in sync hasn't been significant.

One upside to this is that as we're able to improve things for .NET 6+, we also have the opportunity to improve .NET Framework code that's shared with .NET 6+ code. We'll be able to uptake new Microsoft analyzers with each .NET SDK release - and because the code is shared, both versions have the opportunity to improve.

I do think that if we want to freeze .NET Framework support, we should do that in a separate PR though - so we can focus on the task. I'm ok with letting this go forward as is.

[jryckman]

Let's keep iterating on it to make sure it works for all of our projects, but I agree that this is something that we've always wanted but it has been unable to be enforced through editorconfig

[jryckman]

@nick-beer @pakdev are there any other outstanding issues in this PR that you guys have?

[nick-beer]

@nick-beer @pakdev are there any other outstanding issues in this PR that you guys have?

Nope, nothing from me.

[pakdev]

@nick-beer @pakdev are there any other outstanding issues in this PR that you guys have?

Me neither

[pakdev]

@nick-beer @pakdev are there any other outstanding issues in this PR that you guys have?

Me neither

Sorry, didn't mean to close. I thought "Close with comment" meant close the issue - so dumb