From 13cdb8f334c3b06a5dfcf37f6a24b312f2e3ab99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=8Caba=20=C5=A0agi?= <75226367+csaba-sagi-sonarsource@users.noreply.github.com> Date: Fri, 1 Apr 2022 10:41:26 +0200 Subject: [PATCH] FIX S2252 FP: Rule raises issue in case of non integer value in for loop. (#5523) --- .../Rules/ForLoopConditionAlwaysFalse.cs | 55 ++++++++----------- .../Helpers/ExpressionNumericConverterBase.cs | 3 + .../Rules/ForLoopConditionAlwaysFalseTest.cs | 11 +++- .../ForLoopConditionAlwaysFalse.CSharp9.cs | 16 ++++++ .../TestCases/ForLoopConditionAlwaysFalse.cs | 14 ++++- 5 files changed, 63 insertions(+), 36 deletions(-) create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ForLoopConditionAlwaysFalse.CSharp9.cs diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/ForLoopConditionAlwaysFalse.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/ForLoopConditionAlwaysFalse.cs index 85b170c062a..0d0aa9720f0 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/ForLoopConditionAlwaysFalse.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/ForLoopConditionAlwaysFalse.cs @@ -36,7 +36,7 @@ public sealed class ForLoopConditionAlwaysFalse : SonarDiagnosticAnalyzer private const string DiagnosticId = "S2252"; private const string MessageFormat = "This loop will never execute."; - private static readonly CSharpExpressionNumericConverter ExpressionNumericConverter = new CSharpExpressionNumericConverter(); + private static readonly CSharpExpressionNumericConverter ExpressionNumericConverter = new(); private static readonly ISet ConditionsToCheck = new HashSet { @@ -82,23 +82,23 @@ private static bool IsConditionFalseAtInitialization(ForStatementSyntax forNode) return false; } - var loopVariableDeclarationMapping = GetVariableDeclarationMapping(forNode.Declaration); - var loopInitializerMapping = GetLoopInitializerMapping(forNode.Initializers); + var loopVariableDeclarationMapping = VariableDeclarationMapping(forNode.Declaration); + var loopInitializerMapping = LoopInitializerMapping(forNode.Initializers); - var variableNameToIntegerMapping = loopVariableDeclarationMapping + var variableNameToDecimalMapping = loopVariableDeclarationMapping .Union(loopInitializerMapping) .ToDictionary(d => d.Key, d => d.Value); var binaryCondition = (BinaryExpressionSyntax)condition; - if (GetIntValue(variableNameToIntegerMapping, binaryCondition.Left, out var leftValue) - && GetIntValue(variableNameToIntegerMapping, binaryCondition.Right, out var rightValue)) + if (DecimalValue(variableNameToDecimalMapping, binaryCondition.Left, out var leftValue) + && DecimalValue(variableNameToDecimalMapping, binaryCondition.Right, out var rightValue)) { return !ConditionIsTrue(condition.Kind(), leftValue, rightValue); } return false; } - private static bool ConditionIsTrue(SyntaxKind syntaxKind, int leftValue, int rightValue) => + private static bool ConditionIsTrue(SyntaxKind syntaxKind, decimal leftValue, decimal rightValue) => syntaxKind switch { SyntaxKind.GreaterThanExpression => leftValue > rightValue, @@ -120,45 +120,37 @@ private static bool IsLogicalNot(ExpressionSyntax expression, out PrefixUnaryExp && prefixUnaryExpression.OperatorToken.IsKind(SyntaxKind.ExclamationToken); } - /// - /// We try to retrieve the integer value of an expression. If the expression is an integer literal, we return its value, otherwise if - /// the expression is an identifier, we attempt to retrieve the integer value the variable was initialized with if it exists. - /// - /// A dictionary mapping variable names to the integer value they were initialized with if it exists - /// The expression for which we want to retrieve the integer value - /// The output parameter that will hold the integer value if it is found - /// true if an integer value was found for the expression, false otherwise - private static bool GetIntValue(IDictionary variableNameToIntegerValue, ExpressionSyntax expression, out int intValue) + private static bool DecimalValue(IDictionary variableNameToDecimalValue, ExpressionSyntax expression, out decimal parsedValue) { - if (ExpressionNumericConverter.TryGetConstantIntValue(expression, out intValue) + if (ExpressionNumericConverter.TryGetConstantDecimalValue(expression, out parsedValue) || (expression is SimpleNameSyntax simpleName - && variableNameToIntegerValue.TryGetValue(simpleName.Identifier.ValueText, out intValue))) + && variableNameToDecimalValue.TryGetValue(simpleName.Identifier.ValueText, out parsedValue))) { return true; } - intValue = default(int); + parsedValue = default; return false; } /// - /// Retrieves the mapping of variable names to their integer value from the variable declaration part of a for loop. + /// Retrieves the mapping of variable names to their decimal value from the variable declaration part of a for loop. /// This will find the mapping for such cases: /// /// for (var i = 0;;) {} /// /// - private static IDictionary GetVariableDeclarationMapping(VariableDeclarationSyntax variableDeclarationSyntax) + private static IDictionary VariableDeclarationMapping(VariableDeclarationSyntax variableDeclarationSyntax) { - var loopInitializerValues = new Dictionary(); + var loopInitializerValues = new Dictionary(); if (variableDeclarationSyntax != null) { foreach (var variableDeclaration in variableDeclarationSyntax.Variables) { - if (variableDeclaration.Initializer is EqualsValueClauseSyntax initializer - && ExpressionNumericConverter.TryGetConstantIntValue(initializer.Value, out var intValue)) + if (variableDeclaration.Initializer is { } initializer + && ExpressionNumericConverter.TryGetConstantDecimalValue(initializer.Value, out var decimalValue)) { - loopInitializerValues.Add(variableDeclaration.Identifier.ValueText, intValue); + loopInitializerValues.Add(variableDeclaration.Identifier.ValueText, decimalValue); } } } @@ -166,26 +158,25 @@ private static IDictionary GetVariableDeclarationMapping(VariableDe } /// - /// Retrieves the mapping of variable names to their integer value from the initializer part of a for loop. + /// Retrieves the mapping of variable names to their decimal value from the initializer part of a for loop. /// This will find the mapping for such cases: /// /// int i; /// for (i = 0;;) {} /// /// - private static IDictionary GetLoopInitializerMapping(IEnumerable initializers) + private static IDictionary LoopInitializerMapping(IEnumerable initializers) { - var loopInitializerValues = new Dictionary(); + var loopInitializerValues = new Dictionary(); if (initializers != null) { foreach (var initializer in initializers) { if (initializer.IsKind(SyntaxKind.SimpleAssignmentExpression) - && initializer is AssignmentExpressionSyntax assignment - && assignment.Left is SimpleNameSyntax simpleName - && ExpressionNumericConverter.TryGetConstantIntValue(assignment.Right, out var intValue)) + && initializer is AssignmentExpressionSyntax { Left: SimpleNameSyntax simpleName } assignment + && ExpressionNumericConverter.TryGetConstantDecimalValue(assignment.Right, out var decimalValue)) { - loopInitializerValues.Add(simpleName.Identifier.ValueText, intValue); + loopInitializerValues.Add(simpleName.Identifier.ValueText, decimalValue); } } } diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/ExpressionNumericConverterBase.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/ExpressionNumericConverterBase.cs index 4b7e17474de..1022850e836 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/ExpressionNumericConverterBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/ExpressionNumericConverterBase.cs @@ -42,6 +42,9 @@ public bool TryGetConstantIntValue(SyntaxNode expression, out int value) => public bool TryGetConstantDoubleValue(SyntaxNode expression, out double value) => TryGetConstantValue(null, expression, Convert.ToDouble, (multiplier, v) => multiplier * v, out value); + public bool TryGetConstantDecimalValue(SyntaxNode expression, out decimal value) => + TryGetConstantValue(null, expression, Convert.ToDecimal, (multiplier, v) => multiplier * v, out value); + private bool TryGetConstantValue(SemanticModel semanticModel, SyntaxNode expression, Func converter, Func multiplierCalculator, out T value) where T : struct { diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/ForLoopConditionAlwaysFalseTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/ForLoopConditionAlwaysFalseTest.cs index 05616cc8bf7..bf21a2de6d6 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/ForLoopConditionAlwaysFalseTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/ForLoopConditionAlwaysFalseTest.cs @@ -29,6 +29,15 @@ public class ForLoopConditionAlwaysFalseTest { [TestMethod] public void ForLoopConditionAlwaysFalse() => - OldVerifier.VerifyAnalyzer(@"TestCases\ForLoopConditionAlwaysFalse.cs", new ForLoopConditionAlwaysFalse()); + new VerifierBuilder().AddPaths("ForLoopConditionAlwaysFalse.cs").Verify(); + +#if NET + + [TestMethod] + public void ForLoopConditionAlwaysFalse_CSharp9() => + new VerifierBuilder().AddPaths("ForLoopConditionAlwaysFalse.CSharp9.cs").WithOptions(ParseOptionsHelper.FromCSharp9).Verify(); + +#endif + } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ForLoopConditionAlwaysFalse.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ForLoopConditionAlwaysFalse.CSharp9.cs new file mode 100644 index 00000000000..b360e3bc626 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ForLoopConditionAlwaysFalse.CSharp9.cs @@ -0,0 +1,16 @@ +using System; + +namespace Tests.Diagnostics +{ + class Program + { + void LoopTest(int x, int y) + { + for (nint i = 9; i >= 5;) { } // Compliant + for (nuint i = 9; i >= 5;) { } // Compliant + + for (nint a = 0, b = 1; b < 1;) { } // Noncompliant + for (nuint a = 0, b = 1; b < 1;) { } // Noncompliant + } + } +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ForLoopConditionAlwaysFalse.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ForLoopConditionAlwaysFalse.cs index b54232faf3e..c9531b8a287 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ForLoopConditionAlwaysFalse.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ForLoopConditionAlwaysFalse.cs @@ -56,9 +56,17 @@ void LoopTest(int x, int y) for (; z < 0; z++) { } // FN - we only check for initializers inside the loop statement // Reproducer for https://github.com/SonarSource/sonar-dotnet/issues/5428 - for (float n = 0.0F; n > -0.1F; n -= 0.005F) { } // Noncompliant FP - for (double n = 0.0; n > -0.1; n -= 0.005) { } // Noncompliant FP - for (var n = 0.0; n > -0.1; n -= 0.005) { } // Noncompliant FP + for (float n = 0.0F; n > -0.1F; n -= 0.005F) { } // Compliant + for (double n = 0.0; n > -0.1; n -= 0.005) { } // Compliant + for (var n = 0.0; n > -0.1; n -= 0.005) { } // Compliant + for (decimal n = 0.0M; n > -0.1M; n -= 0.005M) { } // Compliant + + for (float n = -0.16F; n == -0.23F;) { } // Noncompliant + for (double n = -0.42; n != -0.42;) { } // Noncompliant + for (var n = 0.0; n <= -0.1; n -= 0.005) { } // Noncompliant + for (decimal n = 0.0M; n <= -0.1M; n -= 0.005M) { } // Noncompliant + + for (var i = 9; i < 4 - 2;) { } // FN } } }