Fix S2583 FP/FN: Conditionally executed code should be reachable

[gabenovotny]

Description

Block of code with an if statement at the end that allows control to flow when a collection count is equal to zero is being flagged as "unreachable code" for rule S2583

Repro steps

// This is of type "MergePublicationCollection" from the Microsoft.SqlServer.Rmo nuget package
var mergePublications = replicationServer.ReplicationDatabases[databaseName].MergePublications;
// This is of type "MergePublication" from the Microsoft.SqlServer.Rmo nuget package
var mergePublication = mergePublications[publication];

mergePublication?.Remove(true);

mergePublications.Refresh();

if (mergePublications.Count == 0)
{
     DisablePublishing(replicationServer, databaseName);
}

Expected behavior

The above collection could be empty, so it is likely that the block within the above if statement would be executed.

Actual behavior

S2583 rule is flagged as a warning in the pipeline running the analysis.

Known workarounds

We could suppress the warning in code or in our project configuration, but we want the warning for other instances of this issue. This appears to be a false-positive.

Related information

• C#/VB.NET Plugins version - 7.3.0.77872
• Visual Studio version - 17.4.1
• MSBuild / dotnet version - using Azure Pipeline task - VSBuild@1
• Windows Server 2019

[zsolt-kolbay-sonarsource]

Hi @gabenovotny. Thank you for reporting this issue. Confirmed as FP.
There are 2 reasons why this is happening:

1. Our rule engine assumes that the indexer will throw an exception if the collection does not contain the key. Therefore it thinks by the time it reaches the mergePublications.Count == 0 condition that the collection has at least one element. This is not the case with the class behind mergePublications, it simply returns null when an element is not found. I created a reproducer for this. We'll check out how much effort it will take to fix this.
2. We do not track elements deleting themselves from a collection (I assume mergePublication?.Remove(true); is doing exactly that before the call to Refresh()). A general solution for that would be very difficult to implement, so it's unlikely we will add a fix for this part of the problem. Maybe we can create a list of collection types that should be ignored and add ReplicationBaseCollection to it?