Fix S2583: Rule should consider Nullable<bool> values

[Evangelink]

The following code generates a false positive:

bool? x = null;
if (x == true)
{
}
else if (x == false) // FP
{
}

[Evangelink]

The problem is that we set a true or false constraint on SV_x when a constraint is set on x == true, however there's a third option that the x == true is false due to x being null, and == being the lifted operator.

[tamasvajk]

@Evangelink, @valhristov This is one of the bigger bugs that I left behind. I started fixing it in https://github.com/tamasvajk/sonar-csharp/tree/feature/152, but I rarely find the time to work on it.

As I see, this problem is not just about Nullable<bool>, but nullables altogether.

x = GetNullableInt();
if (x > 5)
{
}
else if (x <= 5) // seemingly always true, but if x is null, then > and <= returns false
{
}

I think one way to solve this issue is to

1. introduce a NullableSymbolicValue, and
2. in operations (binary and unary symbolic values) check if any of the operands is NullableSymbolicValue, and if so, then the operation is a lifted op, and should behave accordingly.

Branch implements point 1. Point 2 is a bit tedious, and before I got there I realized that this solution leads a bit too far, namely (implicit) type changes and casts should be handled in some form:

Currently, for int? i = (int?)5; the data flow engine assigns the same SV to 5, (int?)5 and i. But from now on we'd need not null SV_1 for 5, and not null NullableSV_2 for the other two. There are multiple forms of this problem:

• int? i = 5;, the same as above.
• int? i = null;, where SV_null needs to be differentiated from NullableSV_1 with null constraint.

So this note is just to let you know that the branch is there, and could be continued, but before that some decision needs to be made. (And I guess, this should not come from a contributor.)
Also, I would be interested how the JS team handles type changes. I'm guessing (I vaguely remember) they are assigning "type" constraints to symbolic values. Maybe that's the way to go here too, but introducing a dedicated SV type for Nullable<T> seemed more straight forward for a statically typed language.