Fix S4158 FP: Variable instance mismatch

[Corniel]

Description

public virtual object Clone()
{
    var clonedEntry = (Entry)MemberwiseClone();
    clonedEntry._id = Guid.NewGuid();
    clonedEntry._lines = new List<Line>();
   _lines.ForEach(line => clonedEntry.AddLine(line.Clone() as Line));
    return clonedEntry;
}

Will falsely raise. It seems to be different from earlier raised issue.

Workarround

public virtual object Clone()
{
    var clonedEntry = (Entry)MemberwiseClone();
    clonedEntry._id = Guid.NewGuid();
    clonedEntry._lines = new List<Line>();
    forach(var line in clonedEntry._lines )
   {
       clonedEntry.AddLine(line.Clone() as Line);
   }
    return clonedEntry;
}

Related information

• 7.9.1.7622
• Visual Studio 14.0

[christophe-zurn-sonarsource]

As we are currently working on CFG/Symbolic Execution, I investigated a bit more this issue to understand the underlying problem.
For future reference: symbolic execution currently only keeps track of fields in the current class being analyzed. However, symbolic values of objects (variables, fields, parameters, ...) are mapped to their symbols (https://docs.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.isymbol?view=roslyn-dotnet). This is what is causing the issue: when an instance of the current class being analyzed is created within a method, the fields of the newly created instance and the current one are aliased since they are represented by the same ISymbol object. Fixing this implies changing how symbolic values are stored and mapped inside the ProgramState.

Here are two reproducers to illustrate this:

    class A
    {
        public List<string> _lines;

        public void doSomething()
        {
            var obj1 = new A();
            obj1._lines = new List<string>(); // obj1._lines and this._lines are both represented by the 'A#List<string>_lines' symbol
            _lines.ForEach(line => obj1._lines.Add(line)); // FP
        }
    }

    class B
    {
        public List<string> _lines;

        public void doSomething()
        {
            var obj1 = new B();
            var obj2 = new B();
            obj1._lines = new List<string>();
            obj1._lines.Add("str");
            obj2._lines = new List<string>();
            obj1._lines.ForEach(line => obj2._lines.Add(line)); // FP
        }
    }

[Thieum]

Another false positive case for S4158 involving local method and event:

        public void RemoveErrorsTest()
        {
            // Arrange
            var module = new TestModule();

            var errorsToRemove = new List<int>();
            var RemovedErrors = new List<ApplicationModuleError>();

            void Module_ErrorsRemovingEnded(object sender, ApplicationModuleErrorsRemovedEventArgs e)
            {
                errorsToRemove.AddRange(e.ErrorsToRemove);
                RemovedErrors.AddRange(e.RemovedErrors);
            }

            module.ErrorsRemovingEnded += Module_ErrorsRemovingEnded;

            // Act
            module.RemoveErrors(TestErrors.TestError0);

            // Assert
            Assert.AreEqual((int)TestErrors.TestError0, errorsToRemove[0]); // <-- False positive here
            Assert.AreEqual(0, RemovedErrors.Count);
        }

[stebla27]

Had the same problem! When can I expect a fix?
The issue is very old (2018) and a little bit annoying!

var clone = MemberwiseClone() as MyClass;
clone.myList= new List<Item>();
myList.ForEach(item => clone.myList.Add(item.Clone() as Item));

[pavel-mikula-sonarsource]

Hi @stebla27,

We don't have any ETA to fix reported issues. Prerequisite for this to be fixed is reimplementing our Symbolic Execution Engine that will be done in MMF-2229 in 2021. That will directly fix or unblock all issues related to CFG and SE.