Change CodeAnalysis rules from 'Info' to 'Warning' after fixing all instances of the violations

[elachlan]

As a follow up to #5656 each code analysis rule that is marked as Action="Info" should be evaluated and migrated to Action="Warning" once all instances of the violations are fixed or marked with ignore.

To make the process as painless as possible. Each rule should be implemented in its own PR.

Before the merges:

• Convert CodeAnalysis.ruleset to .globalconfig #7192

List of rules to enable after resolving occurrences:

• CA1050 - CA1050: Declare types in namespaces #7195 CA1050: Declare types in namespaces #7227
• CA1070 - Set CA1070 to Warning #7231
• CA1200 - CA1200: Avoid using cref tags with a prefix #7194
• CA1507 - Set CA1507 to Warning and fix instances of errors #7176
• CA1802 - CA1802 Fixes and Severity to Warning #7177
• CA1805 - Disable CA1805 #7251
• CA1810 - CA1810 Initialize reference type static fields inline #7179
• CA1823 - CA1823 Avoid unused private fields #7180
• CA1825 - CA1825 Avoid zero-length array allocations #7181
• CA1827 - CA1827 Count() is used where Any() could be used instead to improve p… #7207
• CA1829 - CA1829 Use Length/Count property instead of Count() when available #7182
• CA1834 - CA1834 Consider using 'StringBuilder.Append(char)' when applicable #7183
• CA1835 - CA1835 Prefer the 'Memory'-based overloads for 'ReadAsync' and 'Write… #7184
• CA1836 - CA1836 Prefer IsEmpty over Count #7185
• CA1837 - Cannot use Environment.ProcessId because it isn't available in older versions of .NET. When the team decides to no longer support versions below .NET 5 then this can be enabled.
• CA1838 - CA1838 Avoid 'StringBuilder' parameters for P/Invokes #7186
• CA2007
• CA2008
• CA2016 - CA2016 Forward the 'CancellationToken' parameter to methods that take… #7188
• CA2208 - CA2208 Instantiate argument exceptions correctly #7187
• CA2241 - CA2241 Provide correct arguments to formatting methods #7190
• CA2249 - not totally possible due to required string.contains method signatures not being available in older .NET versions.
• CA3075
• CA3076
• CA3077
• CA5350 - CA5350 Do Not Use Weak Cryptographic Algorithms #7233
• CA5384 - CA5384 Asymmetric encryption algorithm DSA is weak. Switch to an RSA … #7234
• SA0001
• SA1002 - SA1002 The spacing around a semicolon is incorrect #7199
• SA1004 - SA1004 Documentation line should begin with a space #7204
• SA1005 - SA1005 Single line comment should begin with a space #7259
• SA1006 - SA1006 A C# preprocessor-type keyword is preceded by space. #7232
• SA1008
• SA1009
• SA1010 - SA1010 Opening square brackets should not be preceded by a space #7205
• SA1011
• SA1012
• SA1013
• SA1014- SA1014 #7235
• SA1015- SA1015 Closing generic bracket should not be followed by a space #7236
• SA1020- SA1020 Increment symbol '++' should not be preceded by a space #7237
• SA1021
• SA1023 - SA1023 Dereference symbol '*' should not be preceded by a space. #7238
• SA1024 - SA1024 Colon should be followed by a space #7266
• SA1025
• SA1100
• SA1102 - SA1102 Query clause should follow previous clause #7261
• SA1106
• SA1107
• SA1110 - Added IDE analyzers checks and enabled certain checks #8336
• SA1111 - Added IDE analyzers checks and enabled certain checks #8336
• SA1114
• SA1116
• SA1117
• SA1120
• SA1122 - Disable SA1122 (Use string.Empty for empty strings) #7239
• SA1123
• SA1125 - SA1125 use shorthand for Nullable types #7201
• SA1127 - SA1127 Generic type constraints should be on their own line #7267
• SA1128
• SA1130
• SA1131
• SA1132 - SA1132 Do not combine fields #7240
• SA1133 - SA1133 Each attribute should be placed in its own set of square brackets #7262
• SA1134 - SA1134 Each attribute should be placed on its own line of code #7263
• SA1135 - SA1135 A using directive is not qualified #7264
• SA1136 - SA1136 place each enum value on its own line #7203
• SA1137
• SA1201
• SA1202
• SA1203
• SA1204
• SA1208
• SA1209
• SA1210
• SA1211
• SA1214
• SA1216 - SA1216 Using static directives should be placed at the correct location #7265
• SA1217
• SA1300
• SA1302 - SA1302 Interface names should begin with I #7268
• SA1303
• SA1304
• SA1306
• SA1308
• SA1311
• SA1312
• SA1313
• SA1316
• SA1401
• SA1402
• SA1403
• SA1404
• SA1405
• SA1407
• SA1408 - SA1408 Conditional expressions should declare precedence #7269
• SA1413
• SA1414
• SA1500
• SA1501
• SA1502
• SA1503
• SA1504 - SA1504 All accessors should be single-line or multi-line #7270
• SA1505 - SA1505 An opening brace should not be followed by a blank line #7272
• SA1506 - SA1506 Element documentation headers should not be followed by blank … #7271
• SA1507
• SA1508 - SA1508 A closing brace should not be preceded by a blank line #7273
• SA1509 - SA1509 Opening braces should not be preceded by blank line #7274
• SA1510 - Enable SA1510 to Warning #7275
• SA1512
• SA1513
• SA1514
• SA1515
• SA1516
• SA1518 - SA1518 - The line endings at the end of a file do not match the setti… #7571
• SA1519 - SA1519 Braces should not be omitted from multi-line child statement #7276
• SA1520 - SA1520 consistent use of opening and closing braces on if statements #7198
• SA1601
• SA1602
• SA1604
• SA1605
• SA1606
• SA1608
• SA1610
• SA1611
• SA1612
• SA1614
• SA1616
• SA1618
• SA1619
• SA1622
• SA1623
• SA1624
• SA1626- SA1626 single-line comment which begins with three forward slashes in… #7197
• SA1627
• SA1629
• SA1633 - Superseded by IDE0073
• SA1642
• SA1643 - SA1643: Finalizer appropriate summary text #7196
• SA1649

[elachlan]

CA1837: https://docs.microsoft.com/en-au/dotnet/fundamentals/code-analysis/quality-rules/ca1837

We can't use this one because it isn't available in older versions of .NET. When the team decides to no longer support versions below .NET 5 then this can be enabled.

[elachlan]

CA2241: https://docs.microsoft.com/en-au/dotnet/fundamentals/code-analysis/quality-rules/CA2241
This has lots of matches in msbuild\src\Build.UnitTests\FileUtilitiesRegex_Tests.cs

Example:

string winDirectory = string.Format("", _directoryStart);
string unixDirectory = string.Format("", _altDirectoryStart);

I don't know if this is on purpose. This would just result in an empty string afaik.

[elachlan]

ca2249: https://docs.microsoft.com/en-au/dotnet/fundamentals/code-analysis/quality-rules/ca2249

This one also has issues because of the missing overloads on string.Contains. So not all usages can be converted.

[elachlan]

@Forgind I can't manage to get StyleCop Analyzers to run anymore. Are they working for you?

[Forgind]

CA2241: https://docs.microsoft.com/en-au/dotnet/fundamentals/code-analysis/quality-rules/CA2241 This has lots of matches in msbuild\src\Build.UnitTests\FileUtilitiesRegex_Tests.cs

Example:

string winDirectory = string.Format("", _directoryStart);
string unixDirectory = string.Format("", _altDirectoryStart);

I don't know if this is on purpose. This would just result in an empty string afaik.

The cases I saw looked like they should resolve to an empty string, so I think it's fine to replace those with string.Empty and skip the format part.

[Forgind]

ca2249: https://docs.microsoft.com/en-au/dotnet/fundamentals/code-analysis/quality-rules/ca2249

This one also has issues because of the missing overloads on string.Contains. So not all usages can be converted.

Does this also complain about cases like:

string s = "foobar";
int i = s.IndexOf("b");
if (i > 0) {
// Uses i
}

? Also, what do you mean by missing overloads? Like string.Contains requires a string, and we sometimes use a char? If that's the case, I'd leave the ones where we use a char; it's more efficient memory-wise to not allocate a string for no reason—though I guess it's probably short enough that the runtime wouldn't actually have to allocate anyway.

[Forgind]

@Forgind I can't manage to get StyleCop Analyzers to run anymore. Are they working for you?

Do you mean locally or in PRs? I just tested a local build, and the analyzer seemed to work fine. If PR builds aren't catching it anymore, it might be throttled, but I don't know how to change that.

[elachlan]

Locally, I am probably doing it wrong somehow. It has been a long while since I worked on this. Do I use visual studio 2019 release , 2022 release, or 2022 preview?

[Forgind]

I don't think it matters. What are you doing to run them? I went to my msbuild repo and ran build.cmd. msbuild MSBuild.sln also worked, though it didn't upgrade the warning to an error.

[elachlan]

ca2249: https://docs.microsoft.com/en-au/dotnet/fundamentals/code-analysis/quality-rules/ca2249
This one also has issues because of the missing overloads on string.Contains. So not all usages can be converted.

Does this also complain about cases like:

string s = "foobar";
int i = s.IndexOf("b");
if (i > 0) {
// Uses i
}

? Also, what do you mean by missing overloads? Like string.Contains requires a string, and we sometimes use a char? If that's the case, I'd leave the ones where we use a char; it's more efficient memory-wise to not allocate a string for no reason—though I guess it's probably short enough that the runtime wouldn't actually have to allocate anyway.

yes, there is a char overload and there are functions which take different comparators than StringComparison.Ordinal. But those are only added in .NET 5.

[elachlan]

CA2241: https://docs.microsoft.com/en-au/dotnet/fundamentals/code-analysis/quality-rules/CA2241 This has lots of matches in msbuild\src\Build.UnitTests\FileUtilitiesRegex_Tests.cs
Example:

string winDirectory = string.Format("", _directoryStart);
string unixDirectory = string.Format("", _altDirectoryStart);

I don't know if this is on purpose. This would just result in an empty string afaik.

The cases I saw looked like they should resolve to an empty string, so I think it's fine to replace those with string.Empty and skip the format part.

Awesome. I will submit a PR with the fixes.

[elachlan]

@rainersigwald some of these PRs are about to increase churn pretty heavily. How would you like me to handle larger change sets like those?

For now I will avoid any analyzers with over 1-2k warnings.

[elachlan]

@Forgind I just want to summarise where I am up to on this. Basically we ran into an issue where some of the analysers fail the build because of code pulled in from nuget packages e.g :

• CA2208 Instantiate argument exceptions correctly #7187
• SA1505 An opening brace should not be followed by a blank line #7272
• SA1127 Generic type constraints should be on their own line #7267

This is because of how analyzers work with .editorconfigs and .globalconfigs. The main difference between the two formats is that .editorconfigs apply to a directory where as .globalconfigs apply to a project. So with the switch to .globalconfigs it now means all code is analyzed not just the code in the project directory.

This was all explained in dotnet/roslyn#55992. Where many people gave input on the system. This culminated in this pull request to demonstrate a solution:

• Change NuGet cache to be in repository root #7310

There is a catch in that solution, in that the added NuGet.config creates a folder in the project directory as a cache for all the nuget packages used by the solution. This means the solution avoids using the global cache. This resulted in an extra 2GB being used on my machine and extra downloading of packages when a first did a project restore. The advantage of this method is that it specifically excludes the nuget package code files from being analyzed.

The other solution is that we move back to using the .editorconfig and avoid the .globalconfig so that the analyzers are applied to the whole solution directory and excludes any files in the project that are included from outside that directory (code files from nuget packages). I like this option because it keeps it simple. But it means the .editorconfig becomes massive.

There may be another method around this but it wasn't suggested from the other people. Could you please discuss with the team and then get back to me? I will then make the required changes.

[elachlan]

@sharwell do you have any thoughts of the configuration of analyzers based on the above issues?

[Forgind]

I'd like to wait to see what sharwell suggests. Personally, both solutions sound problematic in their own ways, and although I'd personally be fine with wasting a couple gbs, I suspect at least one person on my team will be strongly opposed. On the other hand, it would be frustrating to have a massive .editorconfig.

One other option I think we should at least consider is only promoting analysis messages to warnings if we can do some without a warning coming from anything we depend on. That's a stupid requirement, but it may be the most pragmatic. We have a lot of issues we want to tackle for 17.2 with major customer impact, and you've probably noticed we're pretty far behind even with just reviewing and merging what's already open.

[elachlan]

I'd like to wait to see what sharwell suggests. Personally, both solutions sound problematic in their own ways, and although I'd personally be fine with wasting a couple gbs, I suspect at least one person on my team will be strongly opposed. On the other hand, it would be frustrating to have a massive .editorconfig.

One other option I think we should at least consider is only promoting analysis messages to warnings if we can do some without a warning coming from anything we depend on. That's a stupid requirement, but it may be the most pragmatic. We have a lot of issues we want to tackle for 17.2 with major customer impact, and you've probably noticed we're pretty far behind even with just reviewing and merging what's already open.

We need to address this as it is the same issue we ran into for #7571, but its worse because code style can be different between the nuget package code and msbuild.

@sharwell are you able to look at #7174 (comment) and let us know your thoughts?

[Nirmal4G]

You should add file scoped namespaces to the list, is there a code for it?

[sharwell]

@sharwell are you able to look at #7174 (comment) and let us know your thoughts?

Style rules in .globalconfig will not apply to a correctly authored NuGet package. If you are running into problems with the global NuGet cache, there is a bug in one or more NuGet packages which needs to be corrected (or have a workaround). It's not clear which package(s) or rule(s) are involved in the problem you saw.

[rainersigwald]

@sharwell the package in question is microsoft.codeanalysis.collections, for instance in this check run: https://github.com/dotnet/msbuild/runs/6150398057.

[sharwell]

There are a few ways to resolve:

• A good solution for the NuGet package itself is to use the approach of IsExternalInit, where the NuGet package contains the source code, but it's copied to the obj directory by build logic and included in the compilation from there. The copy operation would know to rename files from *.cs (which is how they appear in Roslyn, since they aren't generated code in that repository) to *.g.cs so they are automatically treated as generated code by downstream builds.
• You can work around the issue by setting the generated_code property in .globalconfig for each of the impacted files
• You can work around the issue by creating a DiagnosticSuppressor that knows how to suppress style rules in the files in this package. This is how Roslyn modified VSTHRD200 behavior after a feature request to add logic to the rule itself was rejected (VSTHRD200 should not trigger on test methods microsoft/vs-threading#670).

[rainersigwald]

• You can work around the issue by setting the generated_code property in .globalconfig for each of the impacted files

My understanding is that this isn't possible because we don't know an absolute path for the NuGet packages folder. Is that not the case?

[sharwell]

My understanding is that this isn't possible because we don't know an absolute path for the NuGet packages folder. Is that not the case?

I'm not completely sure here. For central builds (with determinism enabled), the paths should be normalized to a /_/ prefix of some sort that's predictable. I'm not sure if there's something similar for local builds.

[vlada-shubina]

Code-style rules (IDE) are also useful.
They can be enabled using dotnet_analyzer_diagnostic.category-Style.severity = warning or individually.

[vlada-shubina]

SA1010 - #7205

in ObjectModelHelpers (shared file), this check is not flagged as warning. Probably worth checking why.

ignore me, it is SA1110.

[vlada-shubina]

Additional checks to enable once fixed:

# Cast is redundant
dotnet_diagnostic.IDE0004.severity = suggestion

# IDE0005: Remove unnecessary usings/imports
dotnet_diagnostic.IDE0005.severity = warning

# Use explicit type instead of 'var'
dotnet_diagnostic.IDE0008.severity = suggestion

# Populate switch
dotnet_diagnostic.IDE0010.severity = suggestion

# Null check can be simplified
dotnet_diagnostic.IDE0016.severity = suggestion

# Object initialization can be simplified
dotnet_diagnostic.IDE0017.severity = suggestion

# Variable declaration can be inlined
dotnet_diagnostic.IDE0018.severity = suggestion

# Use pattern matching
dotnet_diagnostic.IDE0019.severity = suggestion
dotnet_diagnostic.IDE0020.severity = suggestion

# Use expression body for constructor
dotnet_diagnostic.IDE0021.severity = suggestion

# Use expression body for method
dotnet_diagnostic.IDE0022.severity = suggestion

# Use expression body for conversion operator
dotnet_diagnostic.IDE0023.severity = suggestion

# Use block body for operator
dotnet_diagnostic.IDE0024.severity = suggestion

# Use expression body for property
dotnet_diagnostic.IDE0025.severity = suggestion

# Use expression body for indexer
dotnet_diagnostic.IDE0026.severity = suggestion

# Use expression body for accessor
dotnet_diagnostic.IDE0027.severity = suggestion

# Collection initialization can be simplified
dotnet_diagnostic.IDE0028.severity = suggestion

# Null check can be simplified
dotnet_diagnostic.IDE0031.severity = suggestion

# Use auto property
dotnet_diagnostic.IDE0032.severity = suggestion

# 'default' expression can be simplified
dotnet_diagnostic.IDE0034.severity = suggestion

# Member name can be simplified
dotnet_diagnostic.IDE0037.severity = suggestion

# Use local function
dotnet_diagnostic.IDE0039.severity = suggestion

# Null check can be simplified
dotnet_diagnostic.IDE0041.severity = suggestion

# Variable declaration can be deconstructed
dotnet_diagnostic.IDE0042.severity = suggestion

# Made field readonly
dotnet_diagnostic.IDE0044.severity = suggestion

# 'if' statement can be simplified
dotnet_diagnostic.IDE0045.severity = suggestion
dotnet_diagnostic.IDE0046.severity = suggestion

# Parentheses can be removed
dotnet_diagnostic.IDE0047.severity = suggestion

# Parentheses should be added for clarity
dotnet_diagnostic.IDE0048.severity = suggestion

# Member name can be simplified
dotnet_diagnostic.IDE0049.severity = suggestion

# Use compound assignment
dotnet_diagnostic.IDE0054.severity = suggestion

# Indexing can be simplified
dotnet_diagnostic.IDE0056.severity = suggestion

# Slice can be simplified
dotnet_diagnostic.IDE0057.severity = suggestion

# Expression value is never used
dotnet_diagnostic.IDE0058.severity = suggestion

# Unnecessary assignment of a value
dotnet_diagnostic.IDE0059.severity = suggestion

# Remove unused parameter
dotnet_diagnostic.IDE0060.severity = suggestion

# Use expression body for a local function
dotnet_diagnostic.IDE0061.severity = suggestion

# Local function can be made static
dotnet_diagnostic.IDE0062.severity = suggestion

# Using directives must be placed outside of a namespace declaration
dotnet_diagnostic.IDE0065.severity = suggestion

# Use 'switch' expression
dotnet_diagnostic.IDE0066.severity = suggestion

# 'GetHashCode' implementation can be simplified
dotnet_diagnostic.IDE0070.severity = suggestion

# Interpolation can be simplified
dotnet_diagnostic.IDE0071.severity = suggestion

# Populate switch
dotnet_diagnostic.IDE0072.severity = suggestion

# Use compound assignment
dotnet_diagnostic.IDE0074.severity = suggestion

# Conditional expression can be simplified
dotnet_diagnostic.IDE0075.severity = suggestion

# Use pattern matching
dotnet_diagnostic.IDE0078.severity = suggestion
dotnet_diagnostic.IDE0083.severity = suggestion

# 'typeof' can be converted to 'nameof'
dotnet_diagnostic.IDE0082.severity = suggestion

# 'new' expression can be simplified
dotnet_diagnostic.IDE0090.severity = suggestion

# Simplify LINQ expression
dotnet_diagnostic.IDE0120.severity = suggestion

# namespace does not match folder structure
dotnet_diagnostic.IDE0130.severity = suggestion

# Null check can be clarified
dotnet_diagnostic.IDE0150.severity = suggestion

# Convert to block scoped namespaces
dotnet_diagnostic.IDE0160.severity = suggestion

# Simplify property pattern
dotnet_diagnostic.IDE0170.severity = suggestion

# Use tuple to swap values
dotnet_diagnostic.IDE0180.severity = suggestion

# Use tuple to swap values
dotnet_diagnostic.IDE0180.severity = suggestion

# Lambda expression can be removed
dotnet_diagnostic.IDE0200.severity = suggestion

# Convert to top-level statements
dotnet_diagnostic.IDE0210.severity = suggestion

# 'foreach' statement implicitly converts
dotnet_diagnostic.IDE0220.severity = suggestion

# Use UTF-8 string literal
dotnet_diagnostic.IDE0230.severity = suggestion

# Nullable directives
dotnet_diagnostic.IDE0240.severity = suggestion
dotnet_diagnostic.IDE0241.severity = suggestion

# Struct can be made 'readonly'
dotnet_diagnostic.IDE0250.severity = suggestion

# Null check can be simplified
dotnet_diagnostic.IDE0270.severity = suggestion

# naming rule violation
dotnet_diagnostic.IDE1006.severity = suggestion

[stan-sz]

Regarding some of the rules, per @rainersigwald's comment in https://github.com/dotnet/msbuild/pull/7953/files/c517eaada858f113d7bbf6c06cdf6044b5accb15#r1051020281, it does make sense to leave them as warning for the inner-loop agility, while turn warnings to errors at CI builds.