Fix S2583 FP: Should not raise on double condition (#8475)

SonarSource · Dec 20, 2023 · da53571 · da53571
1 parent 8fcf7fa
commit da53571
Show file tree

Hide file tree

Showing 4 changed files with 129 additions and 6 deletions.
diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/TypeHelper.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/TypeHelper.cs
@@ -18,6 +18,9 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
+using Microsoft.CodeAnalysis;
+using static Google.Protobuf.WellKnownTypes.Field;
+
 namespace SonarAnalyzer.Helpers;
 
 internal static class TypeHelper
@@ -106,6 +109,12 @@ public static bool IsAny(this ITypeSymbol typeSymbol, ImmutableArray<KnownType>
         return false;
     }
 
+    public static bool IsNullableOfAny(this ITypeSymbol type, ImmutableArray<KnownType> argumentTypes) =>
+        NullableTypeArgument(type).IsAny(argumentTypes);
+
+    public static bool IsNullableOf(this ITypeSymbol type, KnownType typeArgument) =>
+        NullableTypeArgument(type).Is(typeArgument);
+
     public static bool IsType(this IParameterSymbol parameter, KnownType type) =>
         parameter != null && parameter.Type.Is(type);
 
@@ -121,10 +130,7 @@ public static bool IsAny(this ITypeSymbol typeSymbol, ImmutableArray<KnownType>
     #endregion TypeName
 
     public static bool IsNullableBoolean(this ITypeSymbol type) =>
-        type is INamedTypeSymbol namedType
-        && namedType.OriginalDefinition.Is(KnownType.System_Nullable_T)
-        && namedType.TypeArguments.Length == 1
-        && namedType.TypeArguments[0].Is(KnownType.System_Boolean);
+        type.IsNullableOf(KnownType.System_Boolean);
 
     public static bool Implements(this ITypeSymbol typeSymbol, KnownType type) =>
         typeSymbol is { }
@@ -205,4 +211,9 @@ symbol switch
             ITypeSymbol x => x,
             _ => null,
         };
+
+    private static ITypeSymbol NullableTypeArgument(ITypeSymbol type) =>
+        type is INamedTypeSymbol namedType && namedType.OriginalDefinition.Is(KnownType.System_Nullable_T)
+            ? namedType.TypeArguments[0]
+            : null;
 }
diff --git a/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Binary.cs b/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Binary.cs
@@ -97,16 +97,24 @@ private static ProgramState LearnBranchingNumberConstraint(ProgramState state, I
         var kind = falseBranch ? Opposite(binary.OperatorKind) : binary.OperatorKind;
         var leftNumber = state[binary.LeftOperand]?.Constraint<NumberConstraint>();
         var rightNumber = state[binary.RightOperand]?.Constraint<NumberConstraint>();
-        if (rightNumber is not null && !binary.LeftOperand.ConstantValue.HasValue && binary.LeftOperand.TrackedSymbol(state) is { } leftSymbol)
+        if (rightNumber is not null && OperandSymbol(binary.LeftOperand) is { } leftSymbol)
         {
             state = LearnBranching(leftSymbol, leftNumber, kind, rightNumber);
         }
-        if (leftNumber is not null && !binary.RightOperand.ConstantValue.HasValue && binary.RightOperand.TrackedSymbol(state) is { } rightSymbol)
+        if (leftNumber is not null && OperandSymbol(binary.RightOperand) is { } rightSymbol)
         {
             state = LearnBranching(rightSymbol, rightNumber, Flip(kind), leftNumber);
         }
         return state;
 
+        ISymbol OperandSymbol(IOperation operand) =>
+            !operand.ConstantValue.HasValue
+            && operand.TrackedSymbol(state) is { } symbol
+            && symbol.GetSymbolType() is INamedTypeSymbol type
+            && (type.IsAny(KnownType.IntegralNumbersIncludingNative) || type.IsNullableOfAny(KnownType.IntegralNumbersIncludingNative))
+                ? symbol
+                : null;
+
         ProgramState LearnBranching(ISymbol symbol, NumberConstraint existingNumber, BinaryOperatorKind kind, NumberConstraint comparedNumber) =>
             !(falseBranch && symbol.GetSymbolType().IsNullableValueType())  // Don't learn opposite for "nullable > 0", because it could also be <null>.
             && RelationalNumberConstraint(existingNumber, kind, comparedNumber, isLoopCondition, visitCount) is { } newConstraint

diff --git a/...sts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Binary.cs b/...sts/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Binary.cs
@@ -777,4 +777,48 @@ empty switch
                 _ => null
             };
     }
+
+    [DataTestMethod]
+    [DataRow("3.14")]   // Double
+    [DataRow("3.14M")]  // Decimal
+    [DataRow("3.14F")]  // Float
+    public void Binary_Relation_FloatingPoint_DoesNotLearnNumberConstraint(string value)
+    {
+        var code = $$"""
+            var value = {{value}};
+            if (value > 0)
+            {
+                Tag("If", value);
+            }
+            else
+            {
+                Tag("Else", value);
+            }
+            """;
+        var validator = SETestContext.CreateCS(code).Validator;
+        validator.TagValue("If").Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
+        validator.TagValue("Else").Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
+    }
+
+    [DataTestMethod]
+    [DataRow("3.14")]   // Double
+    [DataRow("3.14M")]  // Decimal
+    [DataRow("3.14F")]  // Float
+    public void Binary_Equals_FloatingPoint_DoesNotLearnNumberConstraint(string value)
+    {
+        var code = $$"""
+            var value = {{value}};
+            if (value == 0)
+            {
+                Tag("If", value);
+            }
+            else
+            {
+                Tag("Else", value);
+            }
+            """;
+        var validator = SETestContext.CreateCS(code).Validator;
+        validator.TagValue("If").Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
+        validator.TagValue("Else").Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
+    }
 }
diff --git a/...SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs b/...SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs
@@ -3602,3 +3602,63 @@ public void TestMethod()
 
     private void Callback() { }
 }
+
+// https://github.com/SonarSource/sonar-dotnet/issues/8470
+public class Repro_8470
+{
+    public string WithDouble()
+    {
+        double t = 0.5;
+        if (t <= 0)
+        {
+            return "a";
+        }
+        if (t >= 1) // Compliant, we don't track floating point numbers
+        {
+            return "b";
+        }
+        return "c";
+    }
+
+    public string WithDoubleSwappedOperands()
+    {
+        double t = 0.5;
+        if (0 >= t)
+        {
+            return "a";
+        }
+        if (1 <= t) // Compliant, we don't track floating point numbers
+        {
+            return "b";
+        }
+        return "c";
+    }
+
+    public string WithDecimal()
+    {
+        decimal t = 0.5M;
+        if (t <= 0)
+        {
+            return "a";
+        }
+        if (t >= 1) // Compliant, we don't track floating point numbers
+        {
+            return "b";
+        }
+        return "c";
+    }
+
+    public string WithFloat()
+    {
+        float t = 0.5F;
+        if (t <= 0)
+        {
+            return "a";
+        }
+        if (t >= 1) // Compliant, we don't track floating point numbers
+        {
+            return "b";
+        }
+        return "c";
+    }
+}