S4158 FP: Collection filled in a local function

[rent-a-developer]

Description

A S4158 violation is incorrectly reported when elements to a collection are added in a local function.

Repro steps

public static class Program
{
    public static void Main()
    {
        var list = new List<String>();

        void AddElement()
        {
            list.Add("Item1");
        }

        AddElement();

        for (int i = 0; i < list.Count; i++)
        {
            var element = list[i]; // SonarLint reports violation of rule S4158 here.
            Console.Out.WriteLine(element);
        }
    }
}

Expected behavior

Since the collection is clearly not empty in the code sample, SonarLint should not report a violation of rule S4158.

Actual behavior

SonarLint reports a violation of rule S4158.

Known workarounds

None.

Related information

• C#/VB.NET Plugins version: SonarLint for Visual Studio 2019, Version 4.35.0.32570
• Visual Studio version: Visual Studio 2019, Version 16.9.0
• MSBuild / dotnet version: MSBuild 16.9.0.11203 / .NET Framework 4.6.2
• SonarScanner for .NET version (if used)
• Operating System: Windows 10 x64

[pavel-mikula-sonarsource]

Hi @rent-a-developer ,

Thank you for reporting this, I can confirm it as False Positive, as our engine doesn't support local functions for this rule (yet).

[Keke71]

Hi, Not sure if I should post it here, or create a new issue, as my false positive is not related to local functions. However, it looks quite similar. The following code (a little bit simplified from my original code) also reports a false positive S4158, although the collections are definitely not empty:

Environment:

• Visual Studio Enterprise 17.3.0
• SonarAnalyzer.CSharp 8.46.0.54807
• .NET Framework 4.8

• Windows 10 x64

  public void FalsePositive()
  {
      var data = new[] { 0, 0, 1, 1, 1, 1, 1, 0, 0, 0, -1, -1, -1 };
      var rising = new List<List<int>>();
      var falling = new List<List<int>>();
      List<int> currentBin = null;
      var currentDirection = 0;
      for (var i = 0; i < data.Length; i++)
      {
          if (data[i] == 0)
          {
              if (currentBin != null)
              {
                  if (currentDirection == 1)
                  {
                      rising.Add(currentBin);
                  }
                  else
                  {
                      falling.Add(currentBin);
                  }
                  currentBin = null;
              }
  
              continue;
          }
  
          currentBin ??= new List<int>();
          currentBin.Add(i);
          currentDirection = data[i] > 0 ? 1 : -1;
      }
  
      for (var i = 0; i < rising.Count; i++)
      {
          // SonarAnalyzer reports S4158 here:
          Console.WriteLine($"{rising[i].Count}, {falling[i].Count}");
      }
  }

[pavel-mikula-sonarsource]

Hi @Keke71,

Thank you for reporting it, this is a good place to gather this evidence. This rule will get migrated to the new engine under MMF-2402 (non-public link). After that, we should be able to add support for local functions.

[Keke71]

Hi @pavel-mikula-sonarsource,

Thanks for your quick answer. However, please note that there are no local functions involved in my example. Maybe it is related though and will be fixed together with the local functions problem.

Best regards

[pavel-mikula-sonarsource]

@Keke71 indeed, I'm sorry, I've missread the "not" in "not related".

I've taken a closer look on the flow, it's quite complicated in terms of tracking values. There's a chance it will be fixed by the migration I mentioned (MMF-2402).

Please feel free to submit a new issue, as yours is related to tracking state in for loops rather than local function.

[Keke71]

I created a new issue here

[pavel-mikula-sonarsource]

We cannot fix the original issue yet, as the new SE doesn't support local functions yet.

At the same time, the rule is migrated to the new SE, so the actual local function support is the only thing needed to fix this.

[andrei-epure-sonarsource]

@pavel-mikula-sonarsource , according to MMF-2228, we should support local functions.

Is this still relevant, or was it fixed?

[pavel-mikula-sonarsource]

Yes, this is still relevant. MMF-2228 analyzes local function in a stand-alone mode. This issue requires us to do a light version of cross-procedure analysis because the outer method has to analyze the inner local function on the way. It's the size of an MMF and should be tackled before our actual cross-procedure capabilities - the solution for this will be unrolling the content of the local function.