Skip to content

Commit

Permalink
Improve S1244: Add message to use "IsNaN" instead of "== double.NaN" (#…
Browse files Browse the repository at this point in the history 
…6664)
  • Loading branch information
@antonioaversa
antonioaversa authored Feb 1, 2023
1 parent 5ac0db2 commit bbd3433
Show file tree
Hide file tree
Showing 6 changed files with 298 additions and 246 deletions.
203 changes: 103 additions & 100 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/EqualityOnFloatingPoint.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,116 +18,119 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Rules.CSharp
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class EqualityOnFloatingPoint : SonarDiagnosticAnalyzer
{
internal const string DiagnosticId = "S1244";
private const string MessageFormat = "Do not check floating point {0} with exact values, use a range instead.";
namespace SonarAnalyzer.Rules.CSharp;

private static readonly DiagnosticDescriptor rule =
DescriptorFactory.Create(DiagnosticId, MessageFormat);
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class EqualityOnFloatingPoint : SonarDiagnosticAnalyzer
{
private const string DiagnosticId = "S1244";
private const string MessageFormat = "Do not check floating point {0} with exact values, use {1} instead.";

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(rule);
private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
private static readonly Dictionary<string, string> SpecialMembers = new()
{
{ nameof(double.NaN), nameof(double.IsNaN) },
};

private static readonly ISet<string> EqualityOperators = new HashSet<string> { "op_Equality", "op_Inequality" };
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);

protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterNodeAction(
CheckEquality,
SyntaxKind.EqualsExpression,
SyntaxKind.NotEqualsExpression);

context.RegisterNodeAction(
CheckLogicalExpression,
SyntaxKind.LogicalAndExpression,
SyntaxKind.LogicalOrExpression);
}
protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterNodeAction(
CheckEquality,
SyntaxKind.EqualsExpression,
SyntaxKind.NotEqualsExpression);

context.RegisterNodeAction(
CheckLogicalExpression,
SyntaxKind.LogicalAndExpression,
SyntaxKind.LogicalOrExpression);
}

private static void CheckLogicalExpression(SonarSyntaxNodeReportingContext context)
private static void CheckLogicalExpression(SonarSyntaxNodeReportingContext context)
{
var binaryExpression = (BinaryExpressionSyntax)context.Node;

if (TryGetBinaryExpression(binaryExpression.Left) is { } left
&& TryGetBinaryExpression(binaryExpression.Right) is { } right
&& CSharpEquivalenceChecker.AreEquivalent(right.Right, left.Right)
&& CSharpEquivalenceChecker.AreEquivalent(right.Left, left.Left)
&& IsIndirectEquality(context.SemanticModel, binaryExpression, left, right) is var isEquality
&& IsIndirectInequality(context.SemanticModel, binaryExpression, left, right) is var isInequality
&& (isEquality || isInequality))
{
var binaryExpression = (BinaryExpressionSyntax)context.Node;
var left = TryGetBinaryExpression(binaryExpression.Left);
var right = TryGetBinaryExpression(binaryExpression.Right);

if (right == null || left == null)
{
return;
}

var eqRight = CSharpEquivalenceChecker.AreEquivalent(right.Right, left.Right);
var eqLeft = CSharpEquivalenceChecker.AreEquivalent(right.Left, left.Left);
if (!eqRight || !eqLeft)
{
return;
}

var isEquality = IsIndirectEquality(context.SemanticModel, binaryExpression, left, right);

if (isEquality || IsIndirectInequality(context.SemanticModel, binaryExpression, left, right))
{
var messageEqualityPart = GetMessageEqualityPart(isEquality);

context.ReportIssue(Diagnostic.Create(rule, binaryExpression.GetLocation(), messageEqualityPart));
}
context.ReportIssue(Diagnostic.Create(Rule, binaryExpression.GetLocation(), MessageEqualityPart(isEquality), "a range"));
}
}

private static string GetMessageEqualityPart(bool isEquality) =>
isEquality ? "equality" : "inequality";
private static string MessageEqualityPart(bool isEquality) =>
isEquality ? "equality" : "inequality";

private static void CheckEquality(SonarSyntaxNodeReportingContext context)
private static void CheckEquality(SonarSyntaxNodeReportingContext context)
{
var equals = (BinaryExpressionSyntax)context.Node;
if (context.SemanticModel.GetSymbolInfo(equals).Symbol is IMethodSymbol { ContainingType: { } container } method
&& IsFloatingPointType(container)
&& (method.IsOperatorEquals() || method.IsOperatorNotEquals()))
{
var equals = (BinaryExpressionSyntax)context.Node;

if (context.SemanticModel.GetSymbolInfo(equals).Symbol is IMethodSymbol { ContainingType: { } container, Name: { } equalitySymbolName }
&& IsFloatingPointNumberType(container)
&& EqualityOperators.Contains(equalitySymbolName))
{
var messageEqualityPart = GetMessageEqualityPart(equals.IsKind(SyntaxKind.EqualsExpression));

context.ReportIssue(Diagnostic.Create(rule, equals.OperatorToken.GetLocation(), messageEqualityPart));
}
var messageEqualityPart = MessageEqualityPart(equals.IsKind(SyntaxKind.EqualsExpression));
var proposed = ProposedMessageForMemberAccess(context, equals.Right)
?? ProposedMessageForMemberAccess(context, equals.Left)
?? ProposedMessageForIdentifier(context, equals.Right)
?? ProposedMessageForIdentifier(context, equals.Left)
?? "a range";
context.ReportIssue(Diagnostic.Create(Rule, equals.OperatorToken.GetLocation(), messageEqualityPart, proposed));
}

// Returns true for the floating point types that suffer from equivalence problems. All .NET floating point types have this problem except `decimal.`
// - Reason for excluding `decimal`: the documentation for the `decimal.Equals()` method does not have a "Precision in Comparisons" section as the other .NET floating point types.
// - Power-2-based types like `double` implement `IFloatingPointIeee754`, but power-10-based `decimal` implements `IFloatingPoint`.
// - `IFloatingPointIeee754` defines `Epsilon` which indicates problems with equivalence checking.
private static bool IsFloatingPointNumberType(ITypeSymbol type) =>
type.IsAny(KnownType.FloatingPointNumbers)
|| (type.Is(KnownType.System_Numerics_IEqualityOperators_TSelf_TOther_TResult) // The operator originates from a virtual static member
&& type is INamedTypeSymbol { TypeArguments: { } typeArguments } // Arguments of TSelf, TOther, TResult
&& typeArguments.Any(IsFloatingPointNumberType))
|| (type is ITypeParameterSymbol { ConstraintTypes: { } constraintTypes } // constraints of TSelf or of TSelf, TOther, TResult from IEqualityOperators
&& constraintTypes.Any(constraint => constraint.DerivesOrImplements(KnownType.System_Numerics_IFloatingPointIeee754_TSelf)));

private static BinaryExpressionSyntax TryGetBinaryExpression(ExpressionSyntax expression) =>
expression.RemoveParentheses() as BinaryExpressionSyntax;

private static bool IsIndirectInequality(SemanticModel semanticModel, BinaryExpressionSyntax binaryExpression, BinaryExpressionSyntax left, BinaryExpressionSyntax right) =>
binaryExpression.IsKind(SyntaxKind.LogicalOrExpression)
&& HasAppropriateOperatorsForInequality(left, right)
&& HasFloatingType(semanticModel, right.Left, right.Right);

private static bool IsIndirectEquality(SemanticModel semanticModel, BinaryExpressionSyntax binaryExpression, BinaryExpressionSyntax left, BinaryExpressionSyntax right) =>
binaryExpression.IsKind(SyntaxKind.LogicalAndExpression)
&& HasAppropriateOperatorsForEquality(left, right)
&& HasFloatingType(semanticModel, right.Left, right.Right);

private static bool HasFloatingType(SemanticModel semanticModel, ExpressionSyntax left, ExpressionSyntax right) =>
IsExpressionFloatingType(semanticModel, right) || IsExpressionFloatingType(semanticModel, left);

private static bool IsExpressionFloatingType(SemanticModel semanticModel, ExpressionSyntax expression) =>
IsFloatingPointNumberType(semanticModel.GetTypeInfo(expression).Type);

private static bool HasAppropriateOperatorsForEquality(BinaryExpressionSyntax left, BinaryExpressionSyntax right) =>
(left.OperatorToken.Kind() is SyntaxKind.GreaterThanEqualsToken && right.OperatorToken.Kind() is SyntaxKind.LessThanEqualsToken)
|| (left.OperatorToken.Kind() is SyntaxKind.LessThanEqualsToken && right.OperatorToken.Kind() is SyntaxKind.GreaterThanEqualsToken);

private static bool HasAppropriateOperatorsForInequality(BinaryExpressionSyntax left, BinaryExpressionSyntax right) =>
(left.OperatorToken.Kind() is SyntaxKind.GreaterThanToken && right.OperatorToken.Kind() is SyntaxKind.LessThanToken)
|| (left.OperatorToken.Kind() is SyntaxKind.LessThanToken && right.OperatorToken.Kind() is SyntaxKind.GreaterThanToken);
}

private static string ProposedMessageForMemberAccess(SonarSyntaxNodeReportingContext context, ExpressionSyntax expression) =>
expression is MemberAccessExpressionSyntax memberAccess
&& SpecialMembers.TryGetValue(memberAccess.GetName(), out var proposedMethod)
&& context.SemanticModel.GetTypeInfo(memberAccess).ConvertedType is { } type
&& IsFloatingPointType(type)
? $"'{type.ToMinimalDisplayString(context.SemanticModel, memberAccess.SpanStart)}.{proposedMethod}()'"
: null;

private static string ProposedMessageForIdentifier(SonarSyntaxNodeReportingContext context, ExpressionSyntax expression) =>
expression is IdentifierNameSyntax identifier
&& SpecialMembers.TryGetValue(identifier.GetName(), out var proposedMethod)
&& context.SemanticModel.GetSymbolInfo(identifier).Symbol is { ContainingType: { } type }
&& IsFloatingPointType(type)
? $"'{proposedMethod}()'"
: null;

// Returns true for the floating point types that suffer from equivalence problems. All .NET floating point types have this problem except `decimal.`
// - Reason for excluding `decimal`: the documentation for the `decimal.Equals()` method does not have a "Precision in Comparisons" section as the other .NET floating point types.
// - Power-2-based types like `double` implement `IFloatingPointIeee754`, but power-10-based `decimal` implements `IFloatingPoint`.
// - `IFloatingPointIeee754` defines `Epsilon` which indicates problems with equivalence checking.
private static bool IsFloatingPointType(ITypeSymbol type) =>
type.IsAny(KnownType.FloatingPointNumbers)
|| (type.Is(KnownType.System_Numerics_IEqualityOperators_TSelf_TOther_TResult) // The operator originates from a virtual static member
&& type is INamedTypeSymbol { TypeArguments: { } typeArguments } // Arguments of TSelf, TOther, TResult
&& typeArguments.Any(IsFloatingPointType))
|| (type is ITypeParameterSymbol { ConstraintTypes: { } constraintTypes } // constraints of TSelf or of TSelf, TOther, TResult from IEqualityOperators
&& constraintTypes.Any(x => x.DerivesOrImplements(KnownType.System_Numerics_IFloatingPointIeee754_TSelf)));

private static BinaryExpressionSyntax TryGetBinaryExpression(ExpressionSyntax expression) =>
expression.RemoveParentheses() as BinaryExpressionSyntax;

private static bool IsIndirectInequality(SemanticModel semanticModel, BinaryExpressionSyntax binaryExpression, BinaryExpressionSyntax left, BinaryExpressionSyntax right) =>
binaryExpression.IsKind(SyntaxKind.LogicalOrExpression)
&& IsOperatorPair(left, right, SyntaxKind.GreaterThanToken, SyntaxKind.LessThanToken)
&& HasFloatingType(semanticModel, right);

private static bool IsIndirectEquality(SemanticModel semanticModel, BinaryExpressionSyntax binaryExpression, BinaryExpressionSyntax left, BinaryExpressionSyntax right) =>
binaryExpression.IsKind(SyntaxKind.LogicalAndExpression)
&& IsOperatorPair(left, right, SyntaxKind.GreaterThanEqualsToken, SyntaxKind.LessThanEqualsToken)
&& HasFloatingType(semanticModel, right);

private static bool HasFloatingType(SemanticModel semanticModel, BinaryExpressionSyntax binary) =>
IsExpressionFloatingType(semanticModel, binary.Right) || IsExpressionFloatingType(semanticModel, binary.Left);

private static bool IsExpressionFloatingType(SemanticModel semanticModel, ExpressionSyntax expression) =>
IsFloatingPointType(semanticModel.GetTypeInfo(expression).Type);

private static bool IsOperatorPair(BinaryExpressionSyntax left, BinaryExpressionSyntax right, SyntaxKind first, SyntaxKind second) =>
(left.OperatorToken.IsKind(first) && right.OperatorToken.IsKind(second))
|| (left.OperatorToken.IsKind(second) && right.OperatorToken.IsKind(first));
}
73 changes: 26 additions & 47 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/OperatorsShouldBeOverloadedConsistently.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,56 +102,35 @@ private static IEnumerable<string> FindMissingMethods(INamedTypeSymbol classSymb

private static IEnumerable<string> GetImplementedMethods(INamedTypeSymbol classSymbol)
{
var classMethods = classSymbol
.GetMembers()
.OfType<IMethodSymbol>()
.Where(m => !m.IsConstructor())
.ToList();

if (classMethods.Any(KnownMethods.IsOperatorBinaryPlus))
{
yield return MethodName.OperatorPlus;
}

if (classMethods.Any(KnownMethods.IsOperatorBinaryMinus))
{
yield return MethodName.OperatorMinus;
}

if (classMethods.Any(KnownMethods.IsOperatorBinaryMultiply))
{
yield return MethodName.OperatorMultiply;
}

if (classMethods.Any(KnownMethods.IsOperatorBinaryDivide))
{
yield return MethodName.OperatorDivide;
}

if (classMethods.Any(KnownMethods.IsOperatorBinaryModulus))
{
yield return MethodName.OperatorReminder;
}

if (classMethods.Any(KnownMethods.IsOperatorEquals))
foreach (var member in classSymbol.GetMembers().OfType<IMethodSymbol>().Where(x => !x.IsConstructor()))
{
yield return MethodName.OperatorEquals;
}

if (classMethods.Any(KnownMethods.IsOperatorNotEquals))
{
yield return MethodName.OperatorNotEquals;
}

if (classMethods.Any(KnownMethods.IsObjectEquals))
{
yield return MethodName.ObjectEquals;
if (ImplementedOperator(member) is { } name)
{
yield return name;
}
else if (KnownMethods.IsObjectEquals(member))
{
yield return MethodName.ObjectEquals;
}
else if (KnownMethods.IsObjectGetHashCode(member))
{
yield return MethodName.ObjectGetHashCode;
}
}
}

if (classMethods.Any(KnownMethods.IsObjectGetHashCode))
private static string ImplementedOperator(IMethodSymbol member) =>
member switch
{
yield return MethodName.ObjectGetHashCode;
}
}
{ MethodKind: not MethodKind.UserDefinedOperator } => null,
_ when KnownMethods.IsOperatorBinaryPlus(member) => MethodName.OperatorPlus,
_ when KnownMethods.IsOperatorBinaryMinus(member) => MethodName.OperatorMinus,
_ when KnownMethods.IsOperatorBinaryMultiply(member) => MethodName.OperatorMultiply,
_ when KnownMethods.IsOperatorBinaryDivide(member) => MethodName.OperatorDivide,
_ when KnownMethods.IsOperatorBinaryModulus(member) => MethodName.OperatorReminder,
_ when KnownMethods.IsOperatorEquals(member) => MethodName.OperatorEquals,
_ when KnownMethods.IsOperatorNotEquals(member) => MethodName.OperatorNotEquals,
_ => null
};
}
}
Loading

0 comments on commit bbd3433

Please sign in to comment.