What language is this for?
C# (.NET 6.0)
Which rule?
csharpsquid:S1066
Why do I believe it’s a false-positive?
Whenever SonarQube detects an if statement alone in another if statements, it flags csharpsquid:S1066 to indicate to the developers that the two if statements can (and should) be merged into one.
However, C# has a very interesting type resolving property that allows to declare variables within ifs:
dynamic anything = 1; // Assume that this variable could be anything of any type
if(anything is int n)
Console.WriteLine(n); // Here, n is declared and is an int containing value 1
Console.WriteLine(n); // Error CS0165: Here, n is still declared but is marked as unassigned, so an error is thrown.
This is also possible with ‘out’ variables, although it behaves differently:
string something = "1"; // Assume that this variable could also contain "hello"
if(int.TryParse(something, out int n))
Console.WriteLine(n); // Here, n is declared and is an int containing value 1
Console.WriteLine(n); // Here, n is still declared and will be at value 1 if int.TryParse returned true, or default(int) if it returned false
Now here’s the glitch: for some reason, if you combine both possibilities into one statement, the compiler rejects it:
dynamic anything = "2024-04-29"; // Assume that this variable could be anything of any type
if (anything is string && DateTime.TryParseExact(anything, "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.None, out DateTime dt))
return dt; // Error: CS0165 - Use of unassigned local variable 'dt'
The workaround is to use two nested statements, which get csharpsquid:S1066 flagged even though we CANNOT merge both statements here:
dynamic anything = "2024-04-29"; // Assume that this variable could be anything of any type
if (anything is string)
if (DateTime.TryParseExact(anything, "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.None, out DateTime dt))
return dt; // Works, but csharpsquid:S1066 is flagged
Therefore, csharpsquid:S1066 shouldn’t be flagged here.
Potential counter-arguments
-
“But you should simply not use
dynamicvariables. C# is a typed language for a reason.”
I agree! But I do not have the luxury to choose here. We get those values from our database, from which a column can return basically anything (and I don’t have the luxury of changing that either), so we need to be able to detect its type and treat it accordingly. -
“Then separate the conditions are different functions”
The goal of SonarQube is to help us make our code more readable, maintainable, reliable, etc.
Separating each and every condition into a function that basically has only one, unique, unrepeated line does NOT make anything more reasable, maintainable, nor reliable.
The full function I written that led me to reporting this here is 10 lines long (3 of which being blank) and has a complexity of 5. If we start splitting those into multiple functions then the code is just gonna be all over the place and figuring out its logic from an outside perspective will make any developer wonder why would anyone split those into multiple single-referenced functions.
I am open to better ways of formatting the code though. Maybe there’s a better workaround than nested if statements that I couldn’t find!
Product
SonarQube Community Edition v10.4.0.87286 (I’m sorry, I cannot test it with newer versions at the moment, I however doubt this has been fixed yet in newer released since it’s very obscure and I found no similar reports)
How can we reproduce the problem? Give us a self-contained snippet of code (formatted text, no screenshots)
DateTime? ThisWorks() {
dynamic anything = "2024-04-29"; // Obviously, this value is calculated by other means but for this example I'm making it fixed
if (anything is string)
if(DateTime.TryParseExact(anything, "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.None, out DateTime dt))
return dt; // Works, but csharpsquid:S1066 is flagged
return null;
}
DateTime? ThisWontWork() {
dynamic anything = "2024-04-29";
if (anything is string && DateTime.TryParseExact(anything, "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.None, out DateTime dt))
return dt; // Error: CS0165 - Use of unassigned local variable 'dt'
return null;
}
I have also reported this FP in a GitHub issue.
EDIT: Confirmed to be a false positive: Fix S1066 FP: Combination of `dynamic` and `out` should not raise · Issue #9221 · SonarSource/sonar-dotnet · GitHub