Fix S1066 FP: Combination of `dynamic` and `out` should not raise
Opened this issue · 2 comments
Description
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 feature that allows to declare variables within if
s:
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 gets 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
Repro steps
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;
}
Expected behavior
SonarQube detects that the two if
statements cannot be merged into a single one and doesn't flag it as an issue.
Actual behavior
SonarQube assumes the two statements can be merged and flags it as a S1066
issue.
Known workarounds
Flagging the issue as a false positive, or separating the conditions in different functions (which would make the code more complex and harder to understand in my specific case)
Related information
- C#/VB.NET Plugins version: 9.19.0.84025
- Visual Studio version: 2022
- MSBuild version: 17.7.4
- Dotnet version: .NET 6.0
- SonarScanner for .NET version: 6.2.0
- Operating System: Windows/Linux
I have also reported the issue on the Sonar Community forum.
Thanks for taking the time to read!
Hi @mgirolet-gl,
Thank you for reporting this case, it's a really interesting one. I confirm it as a False Positive.
The reason is, that using dynamic anywhere, like DoSomething(anything)
, doesn't just use dynamic argument. The whole statement becomes dynamic by nature, because the compiler doesn't really know what will really be invoked and what are the usual consequences.
So anything is string && DateTime.DoSomething-well-known
is in reality interpreted
as anything is string && There's-some-other-uknown-dynamic-statement-here,-have-fun
Using dynamic type anywhere in the statement should prevent the rule from raising, so also IsTrue(anything) && ...
should not raise
Hey there, thanks for your reply!
Thanks for the explanation, I hadn't thought of it this way.
I had (wrongly) assumed it was due to a misinterpretation of the is
and out
keywords being combined in the if
, but it indeed seems to be a quirk of how the compiler interprets dynamic
variables in conditions as you explained since there's no compiling error when the input type isn't dynamic
.
Looking forward to see it fixed.
Thanks for all your work, you guys are the best!