search

dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.22k stars 1.35k forks source link

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

Open elachlan opened 2 years ago

elachlan commented 2 years ago

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:

List of rules to enable after resolving occurrences:

elachlan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

Forgind commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 :

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 https://github.com/dotnet/roslyn/issues/55992. Where many people gave input on the system. This culminated in this pull request to demonstrate a solution:

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 commented 2 years ago

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

Forgind commented 2 years ago

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 commented 2 years ago

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 https://github.com/dotnet/msbuild/issues/7174#issuecomment-1020018628 and let us know your thoughts?

Nirmal4G commented 2 years ago

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

sharwell commented 2 years ago

@sharwell are you able to look at https://github.com/dotnet/msbuild/issues/7174#issuecomment-1020018628 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 commented 2 years ago

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

sharwell commented 2 years ago

There are a few ways to resolve:

rainersigwald commented 2 years ago
  • 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 commented 2 years ago

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 commented 1 year ago

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

vlada-shubina commented 1 year ago

SA1010 - https://github.com/dotnet/msbuild/pull/7205

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

ignore me, it is SA1110.

vlada-shubina commented 1 year ago

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 commented 1 year ago

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.