Fix S3267 FP: Loops should be simplified with "LINQ" expressions

[paulomorgado]

Description

S3267 advises to use LINQ To Objects in loops, which is a sever performance penalty.

Related information

The compliant code:

foreach (var someValue in collection2.Select(x => x.Property).Where(y => y != null))
{
  result.Add(someValue);
}

has 2 delegate instantiations with invocations per item.

Whereas the non-compliant code has no delegate instantiation or invocation.

foreach (var element in collection2) // Noncompliant
{
  var someValue = element.Property;
  if (someValue != null)
  {
    result.Add(someValue);
  }
}

Besides that, because LINQ To Objects is being used, instead of the non heap allocating enumerator for List<T> being used, an heap allocating enumerator generated by the iterator inside LINQ To Objects.

Todo by @martin-strecker-sonarsource

• [ ] Add support for [PerformanceSensitiveAttribute] (The attribute must be named PerformanceSensitive but does not necessarily need to be from the Microsoft.CodeAnalysis.PerformanceSensitiveAnalyzers package). The AttributeTragets of PerformanceSensitiveAttribute should be respected.
• [ ] Update RSpec with a hint about the performance penalty and the option to opt out in the hot path via an imported or self-declared

  [AttributeUsage(AttributeTargets.Constructor | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = true, Inherited = false)]
  public sealed class PerformanceSensitiveAttribute(): Attribute;

[martin-strecker-sonarsource]

Hello @paulomorgado,

You are right about the additional allocations caused by Linq. The rule has the clean code attribute clear which is defined as

The code is self-explanatory, transparently communicating its functionality. It is written in a straightforward way that minimizes ambiguity, avoiding unnecessary clever or intricate solutions.

The rule description states, "Those functions can be handled with a clearer, more concise LINQ expression instead".

So, the rule is about readability, which, in the case of Linq, always comes at the price of allocations.

All code in the hot path needs to follow different rules than "normal" code. We, in our codebase, use the PerformanceSensitiveAnalyzers of the Roslyn analyzers and apply their [PerformanceSensitive] attribute to the hot path to indicate code, that is supposed to avoid Linq:

https://github.com/SonarSource/sonar-dotnet/blob/62007f79ad1105e09d6baea6843b1d1cc16d3a14/analyzers/src/SonarAnalyzer.Common/Helpers/HashCode.cs#L34-L51

We could add support for that attribute to the rule, so we would not raise if the attribute is present. Would you consider this a viable solution?

[paulomorgado]

Hello @martin-strecker-sonarsource,

Not giving any performance degradation under [PerformanceSensitive] is a good idea.

But there should be a performance notice warning on diagnostics the avise to introduce performance degradations.

And a way to disable all those diagnostics. For when all the code is performance sensitive.

[martin-strecker-sonarsource]

And a way to disable all those diagnostics.

There are a couple of exclusion mechanisms supported by our products. You can e.g. define your own profile and exclude the rule there. You can read more about exclusion options here:

• https://docs.sonarsource.com/sonarqube/latest/instance-administration/quality-profiles/#customizing-a-quality-profile
• https://docs.sonarsource.com/sonarqube/latest/project-administration/analysis-scope/
• https://docs.sonarsource.com/sonarqube/latest/analyzing-source-code/languages/csharp/#exclusions

[paulomorgado]

I think something like category severity configuration like this is would be great:

From Overview of .NET source code analysis - Code-style analysis - Enable on build:

[*.{cs,vb}]

# Default severity for analyzer diagnostics with category 'Style' (escalated to build warnings)
dotnet_analyzer_diagnostic.category-Style.severity = warning

# IDE0040: Accessibility modifiers required (disabled on build)
dotnet_diagnostic.IDE0040.severity = silent

[martin-strecker-sonarsource]

Our rules do have a category already so you can do this. The recommended way is to change the quality profile, though. This gives you more control and can be re-used across projects.

[paulomorgado]

Is that something I can put in a .editorconfig file?

[Kazpers]

Claiming this improves readability is a stretch. IMO the original foreach loop is far more readable than the convoluted LINQ statement.