Almost 45000 warnings in coreclr Pri-1 test build

[BruceForstall]

Previously there were ~17 warnings. Now there are ~45000 warnings.

This is due to #41947.

E.g., current warnings:

https://dev.azure.com/dnceng/public/_build/results?buildId=813140&view=logs&j=db768a9e-3325-5ce3-2d52-896c40bee68a&t=59112ad9-fe58-56ee-6943-84df6f0e3f8e

This noise makes it incredibly difficult to find actual problems (e.g., it slows down the AzDO UI).

Possibly unrelated: a spot check shows that the test build seems to gotten much slower as well.

Moving this conversation from the above issue. From @trylek:

I asked about that on the internal infra e-mail alias and my understanding is that this is due to some more stringent checks in new Roslyn. Which is an interesting question, perhaps @jaredpar may have thoughts on what is the preferable way forward - either booking the time to clean the tests up to conform with the latest compiler version, or treating them as "set in stone" (which may be more appropriate e.g. for various regression tests) and maybe devising a new way to mark newly added tests as (potentially) conforming to a more stringent warning policy employed by the newest C# compiler.

@trylek @jaredpar @ViktorHofer @dotnet/runtime-infrastructure @jkotas

[jkotas]

I do not see value in fixing vast majority of these warnings.

Properly fixing number of these warnings (e.g. deleting unused fields or changing readonly to const) would reduce test coverage and/or break the tests in number of situations.

The cosmetic warnings like "Declare types in namespaces" make sense for production libraries, but they do not make sense for simple test programs. Namespaces just add clutter to simple test programs.

[BruceForstall]

As @trylek suggested, we need to suppress all these new warnings in the existing tests, either globally for existing tests only, and let new tests only see them and deal with them, or globally for all tests current and future..

[jaredpar]

I asked about that on the internal infra e-mail alias and my understanding is that this is due to some more stringent checks in new Roslyn.

This is not strictly the case that they come from the Roslyn compiler. Notice that the new warnings pretty much all have the CA prefix. That means they are coming from the .NET SDK analyzers. If they were coming from the compiler they would have the CS prefix. Possible I missed a few CS here but scrolling through seemed to be mostly CA.

@terrajobst FYI

The cosmetic warnings like "Declare types in namespaces" make sense for production libraries, but they do not make sense for simple test programs. Namespaces just add clutter to simple test programs.

Agree. These are mostly meant as warnings for production code. Fixing them doesn't have a lot of value here. I think the best approach is to use an .editorconfig file in the test directory that suppresses these warnings. Or possibly we should use a Directory.Build.props file in this directory to disable the new analysis. This should be straight forward, just have to set $(AnalysisLevel) to 0 (I believe that is the right disable)

@jmarolf, @mavasani should be able to provide more details on how to disable.

[mavasani]

Majority of the CA warnings should be disabled or IDE suggestions by default in the SDK analyzers. For example, "Declare types in namespaces" is just an IDE refactoring/suggestion and does not run on build by default (I verified it with latest .NET5 RC2 SDK daily build). Unless you are explicitly turning these CA rules into build warnings somehow, these should not be running on the build.

[mavasani]

I just took a quick glance at all the 45k+ warnings, and none of them are enabled as build warnings by default. You can see the default severities/enabled by default state for CA rules in .NET SDK analyzers over here: https://github.com/dotnet/roslyn-analyzers/blob/master/src/NetAnalyzers/Core/AnalyzerReleases.Shipped.md

[jaredpar]

Curious: is there a reason we don't use /warnaserror in this leg? Doing so would've caught this diagonstic regression at the point the new SDK was inserted rather than causing this follow up issue here.

The estimate of ~17 existing warnings here seems like it would be fairly easy to tackle.

[mavasani]

Yep, seems you are explicitly turning these rules as warnings in your repo, for example CA1050: Declare types in namespaces is escalated to a build warning in your repo ruleset here:

runtime/eng/CodeAnalysis.ruleset

Line 41 in dcbac85

<Rule Id="CA1050" Action="Warning" /> <!-- Declare types in namespaces -->

[mavasani]

One thing that is likely is you previously enforced this ruleset only for production code, but something else is now passing this ruleset for test code? Tagging @stephentoub as well.

[mavasani]

The ruleset is set as CodeAnalysisRuleset in the props file here:

runtime/eng/Analyzers.props

Line 3 in 23919a4

<CodeAnalysisRuleset>$(MSBuildThisFileDirectory)CodeAnalysis.ruleset</CodeAnalysisRuleset>

and imported in the root Directory.Build.props file below:

runtime/Directory.Build.props

Line 115 in ad434fc

<Import Project="$(RepositoryEngineeringDir)Analyzers.props" />

You should probably move the import to specific product directories or condition it to only get imported for product code.

[jaredpar]

@mavasani do you have any idea why this just started showing up? Basically did compilers / analyzers change any of their logic here (I don't believe so)?

[mavasani]

do you have any idea why this just started showing up? Basically did compilers / analyzers change any of their logic here (I don't believe so)?

I am not sure if something changed in the way the ruleset/imports got adjusted recently, but the way it is set up in the repo right now, it is by design to see these CA warnings in build. I know that @stephentoub promoted many CA rules in this ruleset as build warnings for the runtime repo, but I was under the assumption that it is only for production code. I also agree with @jaredpar that CI should be running with /warnaserror.

[jmarolf]

I agree with @mavasani. This seems to be the intentional design for the runtime repo. You have config files that are overriding the defaults and are asking that these warning be shown. If this is not what the runtime team wants then you should stop overriding the default behavior.

[stephentoub]

I'm back from leave today and will take a look.

Just from this thread, my guess as to what happened is that, previously the code analysis ruleset was configured for all managed code in the repo, but then we only set EnableAnalyzers to true within src/libraries and then only for src projects:

runtime/src/libraries/Directory.Build.props

Line 231 in 46b430a

<EnableAnalyzers Condition="'$(EnableAnalyzers)' == '' and '$(IsSourceProject)' == 'true' and '$(MSBuildProjectExtension)' != '.ilproj'">true</EnableAnalyzers>

When the SDK was updated, it likely flipped this to be true everywhere, erroneously.

The intent was that rule set should only apply to src projects; it does not make sense for ref or tests (some of the rules could be enabled for those in time, but there's little ROI for that).

As for /warnaserror, it's already set for all of src/libraries:

runtime/src/libraries/Directory.Build.props

Line 229 in 5f56a50

<TreatWarningsAsErrors>true</TreatWarningsAsErrors>

as well as for managed code in src/intstallers:

runtime/src/installer/managed/CommonManaged.props

Line 8 in 46b430a

<TreatWarningsAsErrors>true</TreatWarningsAsErrors>

and sporadically elsewhere. I agree it'd be good to pay down minimal warnings debt to get to 0 warnings and flip this to be on for all managed code in the whole repo. That should be separate from fixing the application of the analyzers, though.