Sonar-dotnet: Fix S2589 / S1854 FP: There is only one block for the `try` body in the CFG

[andrei-epure-sonarsource]

Description

See below. It's a tricky one because, to fix this, we should make a block for every statement inside the try block that can either continue or throw.

Repro steps

        public int Start()
        {
            const int x = -1;
            int exitCode = x; // Noncompliant FP - if Archive throws, it will be returned
            try
            {
                Archive(); // can throw here

                exitCode = 1;
            }
            catch (SystemException e)
            {

            }

            return exitCode;
        }

Expected behavior

No issue

Related information

• SonarC# Version 7.16

This was reported on the community forum

[andrei-epure-sonarsource]

Adding another repro for S2589 with same behavior from community

bool blip = false;
try
{
    var rnd = new System.Random();
    if (rnd.Next() % 100 > 50) throw new Exception();
    blip = true;
}
catch { /* Ignore */  }

if (blip) Console.WriteLine("blip");

[RonaldvanMeer]

Hello,

Are there any updates on this matter?

[andrei-epure-sonarsource]

@RonaldvanMeer the short answer is no (for the moment). Fixing it means a substantial rework of the Control Flow Graph shape, which can have many side-effects. At the moment, this is not our highest priority.

[pavel-mikula-sonarsource]

Another rexample from #3327 with the same root cause:
```C#
public string Main(string value)
{
bool created = false;
try
{
if (value == null)
{
value = "";
created = true; //FP here
}
return value;
}
catch (Exception exception)
{
Log(created);
throw;
}
}

private void Log(bool created)
{
}
```

[pavel-mikula-sonarsource]

Another reproducer is in #3386

[costin-zaharia-sonarsource]

Another reproducer from community forum: https://community.sonarsource.com/t/csharpsquid-s2583-false-positive/31948

[costin-zaharia-sonarsource]

Another FP related to the way try/finally is handled in our CFG and SE: https://peach.sonarsource.com/project/issues?id=lucene&issues=AWDkrc4hex0bVixdYtgH&open=AWDkrc4hex0bVixdYtgH

[andrei-epure-sonarsource]

@costin-zaharia-sonarsource I think the root cause is different for the one on peach.

        public override FieldsConsumer FieldsConsumer(SegmentWriteState state)
        {
            PostingsWriterBase docs = new Lucene41PostingsWriter(state);
            TermsIndexWriterBase indexWriter;
            bool success = false;
            try
            {
               indexWriter = new FixedGapTermsIndexWriter(state);
                success = true;
            }
            finally
            {
                if (!success)
                {
                    docs.Dispose();
                }
            }
            success = false;
            try
            {
                FieldsConsumer ret = new BlockTermsWriter(indexWriter, state, docs);
                success = true;
                return ret;
            }
            finally
           {
                if (!success)
                {
                    try
                    {
                        docs.Dispose(); // Noncompliant FP
                    }
                    finally
                    {
                       indexWriter.Dispose();
                    }
                }
            }
        }

This is not related to the fact that there is only one block inside the try, but maybe because finally does not have a branch to EXIT.

[costin-zaharia-sonarsource]

@andrei-epure-sonarsource, I've added a link since it's related to try/catch/finally and, if we decide to rework how this is implemented, we should take in consideration different aspects. The idea is to avoid the situation where we fix a localized issue and later figure out that it doesn't fit well in the global picture.

[costin-zaharia-sonarsource]

@andrei-epure-sonarsource: I've noticed now that this issue is focused on a solution and not the actual problem (FP). We might want to reword it a bit since there might be multiple solutions (e.g. using the CFG from Roslyn)

[SHEePYTaGGeRNeP]

This also happens with "is" checks.
It says that b is always true.

[TestMethod]
public void TestBooleanCHeck()
{
    Assert.IsFalse(StrangeStuff(false));
    Assert.IsTrue(StrangeStuff(true));
}

private static bool StrangeStuff(object objs)
{
    if (!(objs is bool b))
        return false;

    if (b)
        return true;
    return false;
}

[pavel-mikula-sonarsource]

@SHEePYTaGGeRNeP Your example is something different. It doesn't contain try/catch block and it was actually fixed in latest https://github.com/SonarSource/sonar-dotnet/releases/tag/8.16.0.25740