Skip to content

Commit

Permalink
FIX S2252 FP: Rule raises issue in case of non integer value in for l…
Browse files Browse the repository at this point in the history
…oop. (#5523)
  • Loading branch information
csaba-sagi-sonarsource authored Apr 1, 2022
1 parent 0af0058 commit 13cdb8f
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<SyntaxKind> ConditionsToCheck = new HashSet<SyntaxKind>
{
Expand Down Expand Up @@ -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,
Expand All @@ -120,72 +120,63 @@ private static bool IsLogicalNot(ExpressionSyntax expression, out PrefixUnaryExp
&& prefixUnaryExpression.OperatorToken.IsKind(SyntaxKind.ExclamationToken);
}

/// <summary>
/// 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.
/// </summary>
/// <param name="variableNameToIntegerValue">A dictionary mapping variable names to the integer value they were initialized with if it exists</param>
/// <param name="expression">The expression for which we want to retrieve the integer value</param>
/// <param name="intValue">The output parameter that will hold the integer value if it is found</param>
/// <returns>true if an integer value was found for the expression, false otherwise</returns>
private static bool GetIntValue(IDictionary<string, int> variableNameToIntegerValue, ExpressionSyntax expression, out int intValue)
private static bool DecimalValue(IDictionary<string, decimal> 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;
}

/// <summary>
/// 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:
/// <code>
/// for (var i = 0;;) {}
/// </code>
/// </summary>
private static IDictionary<string, int> GetVariableDeclarationMapping(VariableDeclarationSyntax variableDeclarationSyntax)
private static IDictionary<string, decimal> VariableDeclarationMapping(VariableDeclarationSyntax variableDeclarationSyntax)
{
var loopInitializerValues = new Dictionary<string, int>();
var loopInitializerValues = new Dictionary<string, decimal>();
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);
}
}
}
return loopInitializerValues;
}

/// <summary>
/// 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:
/// <code>
/// int i;
/// for (i = 0;;) {}
/// </code>
/// </summary>
private static IDictionary<string, int> GetLoopInitializerMapping(IEnumerable<ExpressionSyntax> initializers)
private static IDictionary<string, decimal> LoopInitializerMapping(IEnumerable<ExpressionSyntax> initializers)
{
var loopInitializerValues = new Dictionary<string, int>();
var loopInitializerValues = new Dictionary<string, decimal>();
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);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>(SemanticModel semanticModel, SyntaxNode expression, Func<object, T> converter, Func<int, T, T> multiplierCalculator, out T value)
where T : struct
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ public class ForLoopConditionAlwaysFalseTest
{
[TestMethod]
public void ForLoopConditionAlwaysFalse() =>
OldVerifier.VerifyAnalyzer(@"TestCases\ForLoopConditionAlwaysFalse.cs", new ForLoopConditionAlwaysFalse());
new VerifierBuilder<ForLoopConditionAlwaysFalse>().AddPaths("ForLoopConditionAlwaysFalse.cs").Verify();

#if NET

[TestMethod]
public void ForLoopConditionAlwaysFalse_CSharp9() =>
new VerifierBuilder<ForLoopConditionAlwaysFalse>().AddPaths("ForLoopConditionAlwaysFalse.CSharp9.cs").WithOptions(ParseOptionsHelper.FromCSharp9).Verify();

#endif

}
}
Original file line number Diff line number Diff line change
@@ -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
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}

0 comments on commit 13cdb8f

Please sign in to comment.