Sonar-dotnet: Fix S2583 - FP when using null propagation followed by null coalesence

[rjgotten]

Description

S2583 - Change this condition so that it does not always evaluate to 'false'; some subsequent code is never executed. is raised incorrectly when guard conditions contain null propagation followed by null coalescence.

Repro steps

Run analysis on code which uses null propagation followed by null coalescence in a guard condition, similar to the following:

if (somePotentiallyNullinstance?.BooleanProperty ?? false)
{
}

Expected behavior

S2583 is not raised.

Actual behavior

S2538 is raised.

Known workarounds

Change source code to use != true.

Related information

SonarC# Version: 7.9 (build 7583)

[valhristov]

Thanks for the feedback @rjgotten! This is related to #1347, but is not caused by the same reason. The CFG that we generate for conditional access expression (x?.y) is not perfect and we get false positives in the Data Flow Analysis related rules. As with #1347 it is not easy to fix and we cannot promise a quick solution.

On the other hand (unrelated to our inability to fix this problem quickly) and this is completely subjective and you could ignore it, I think that the following is easier to read and understand:

if (foo != null &&
    foo.IsBar)
{
}

My reasoning is that when I read the example above I can do it step by step (e.g. when foo is not null and IsBar is true), while the following requires some backtracking and having to understand what's happening (e.g. when foo is null fallback to false, otherwise check what's IsBar), especially for longer expressions:

if (foo?.IsBar ?? false)
{
}

[rjgotten]

especially for longer expressions

Really? Because I'd claim the inverse of that.

Longer property chains evaluated through AND-composition, such as:

if (foo != null && foo.Bar != null && foo.Bar.Baz != null && foo.Bar.Baz.IsTrue)

give quite poor readability compared to shorthand conditional access such as:

if (foo?.Bar?.Baz?.IsTrue ?? false)

Personally I prefer an explicit != true over the ?? false fallback though:

if (foo?.Bar?.Baz?.IsTrue != true)

As for the reading order argument: "when foo is null skip the remainder of this expression" doesn't seem that big of a cognitive load. It seems harder to keep track of multiple conditional expressions being AND-composed, especially when there's a risk there for continuation of the evaluation of the property chain to end up being mixed with other conditional checks.

it is not easy to fix and we cannot promise a quick solution

That's a bit disappointing, but atleast we can work around this by creating awareness about this shortcoming in our development team, and simply marking these Sonar findings as false positives.

[andrei-epure-sonarsource]

fixed in #2497