Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New rule S2198: Silly mathematical comparisons should not be made #6695

Merged
merged 28 commits into from Feb 10, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e76d32a
Update RSPEC for S2198
gregory-paidis-sonarsource Feb 1, 2023
93391b2
Remove a useless auto-generated json
gregory-paidis-sonarsource Feb 1, 2023
ececfa2
Add some testcases
gregory-paidis-sonarsource Feb 1, 2023
46f3d70
Remove a useless using
gregory-paidis-sonarsource Feb 1, 2023
5bdd084
Initial implementation of the main logic
gregory-paidis-sonarsource Feb 1, 2023
fee626d
Initial working implementation of S2198
gregory-paidis-sonarsource Feb 2, 2023
36214a2
Refactor implementation
gregory-paidis-sonarsource Feb 2, 2023
2983cc7
Move some code around
gregory-paidis-sonarsource Feb 2, 2023
3374a35
Add a lot of testcases
gregory-paidis-sonarsource Feb 3, 2023
3564d52
Cut branches: only implement absolute inequality for float
gregory-paidis-sonarsource Feb 3, 2023
86696dc
Rewrite SillyMathematicalComparison, only out-of-range checks for flo…
gregory-paidis-sonarsource Feb 6, 2023
99f3b27
Apply PR review changes
gregory-paidis-sonarsource Feb 7, 2023
ee45a5c
Enrichen testcases, add long and ulong to the checked types
gregory-paidis-sonarsource Feb 7, 2023
ba6589f
Add 'is' pattern testcases, refactored a .ToString to a .ToDisplayString
gregory-paidis-sonarsource Feb 7, 2023
d463864
Simple rename
gregory-paidis-sonarsource Feb 7, 2023
4f0720a
Implement a constant-in-both-sides version of the rule.
gregory-paidis-sonarsource Feb 7, 2023
0070a96
Apply PR Review changes, some constant-check fixes and more testcases
gregory-paidis-sonarsource Feb 8, 2023
e3b6f24
Add enum, char and reference type to the testcases
gregory-paidis-sonarsource Feb 8, 2023
a84d441
Simplify the rule, only care about out-of-range mathematical checks
gregory-paidis-sonarsource Feb 8, 2023
4856e96
Add char out-of-range detection
gregory-paidis-sonarsource Feb 8, 2023
51d7b9c
Added some true negatives for const1 {{operator}} const2
gregory-paidis-sonarsource Feb 8, 2023
f79b42a
Bump coverage by adding a case I forgot about
gregory-paidis-sonarsource Feb 8, 2023
083ae47
Apply PR Review changes
gregory-paidis-sonarsource Feb 9, 2023
0980aa4
Add an is-pattern edgecase in the testcases
gregory-paidis-sonarsource Feb 9, 2023
16c517c
Fix a logical condition, refactor TryGetRange
gregory-paidis-sonarsource Feb 9, 2023
a0fd1ce
Minor naming refactor
gregory-paidis-sonarsource Feb 9, 2023
339ac49
Minor refactor for OutOfRange check
gregory-paidis-sonarsource Feb 10, 2023
a8f25d7
Annotate a testcase with an issue
gregory-paidis-sonarsource Feb 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions analyzers/rspec/cs/S2198_c#.html
@@ -0,0 +1,18 @@
<p>Certain mathematical comparisons will always return the same value, and should simply not be made.</p>
<p>This comparison will always return <code>false</code>:</p>
<ul>
<li> comparing a <code>float</code> with a <code>double</code> constant that’s outside the <code>float</code> range </li>
</ul>
<p>These will always return <code>true</code>:</p>
<ul>
<li> comparing <code>aByte &lt;= Byte.MaxValue</code> and <code>aByte &gt;= Byte.MinValue</code> </li>
<li> comparing <code>anInt &lt;= int.MaxValue</code> and <code>anInt &gt;= int.MinValue</code> </li>
<li> comparing <code>aLong &lt;= long.MaxValue</code> and <code>aLong &gt;= long.MinValue</code> </li>
</ul>
<h2>Noncompliant Code Example</h2>
<pre>
float f = 42.0f;
const double d = float.MaxValue + 1;
if (f &lt;= d) { } // Noncompliant
</pre>

17 changes: 17 additions & 0 deletions analyzers/rspec/cs/S2198_c#.json
@@ -0,0 +1,17 @@
{
"title": "Silly mathematical comparisons should not be made",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "15min"
},
"tags": [
"suspicious"
],
"defaultSeverity": "Critical",
"ruleSpecification": "RSPEC-2198",
"sqKey": "S2198",
"scope": "Main",
"quickfix": "unknown"
}
1 change: 1 addition & 0 deletions analyzers/rspec/cs/Sonar_way_profile.json
Expand Up @@ -66,6 +66,7 @@
"S2184",
"S2187",
"S2190",
"S2198",
"S2201",
"S2219",
"S2222",
Expand Down
@@ -0,0 +1,108 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2023 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using SonarAnalyzer.Helpers;

namespace SonarAnalyzer.Rules.CSharp
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class SillyMathematicalComparison : SonarDiagnosticAnalyzer
{
private const string DiagnosticId = "S2198";
private const string MessageFormat = "Comparison to integral constant is useless; the constant is outside the range of type '{0}'";
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(
CheckBinary,
SyntaxKind.GreaterThanExpression,
SyntaxKind.GreaterThanOrEqualExpression,
SyntaxKind.LessThanExpression,
SyntaxKind.LessThanOrEqualExpression);
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

private static void CheckBinary(SonarSyntaxNodeReportingContext context)
{
var binary = (BinaryExpressionSyntax)context.Node;
var typeIdentifier = string.Empty;
if (ShouldRaise(context.SemanticModel, binary.Left, binary.Right, ref typeIdentifier))
{
context.ReportIssue(Diagnostic.Create(Rule, binary.GetLocation(), typeIdentifier));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roslyn provides an API for naming things in the shortest possible way:

var typeName = context.SemanticModel.GetSymbolInfo(other).Symbol.ToMinimalDisplayString(model, other.GetLocation().SourceSpan.Start)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it, but for some reason it gives me the name of the entire node, for example ulong variableName.

I ended up using this, which is also slightly more succinct:

// returns ulong, instead of 'ulong variableName'
var typeName = context.SemanticModel.GetTypeInfo(second).Type.ToString(); 

It also works if you declare the type as System.UInt64, giving back the simplest form of the type (the primitive ulong).

Note: Included the ulong and long types, since they are not checked by the CS0652 compiler warning.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably called it on an ILocalSymbol which represents a local variable. You need it for the ITypeSymbol of the type of the expression. Use semanticModel.GetTypeInfo(second) and call ToDisplayString on it.

}

if (ShouldRaise(context.SemanticModel, binary.Right, binary.Left, ref typeIdentifier))
{
context.ReportIssue(Diagnostic.Create(Rule, binary.GetLocation(), typeIdentifier));
}
}

private static bool ShouldRaise(SemanticModel model, SyntaxNode expectedConstant, SyntaxNode other, ref string typeIdentifier) =>
TryGetConstant(model, expectedConstant, out var constant)
&& model.GetSymbolInfo(other).Symbol.GetSymbolType() is { } symbol
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
&& TryGetRange(symbol, out typeIdentifier, out var min, out var max)
&& (constant < min || constant > max);

private static bool TryGetConstant(SemanticModel model, SyntaxNode expectedConstant, out double constant)
{
if (FindConstant(model, expectedConstant, Convert.ToDouble) is { } constValue)
{
constant = constValue;
return true;
}

constant = default;
return false;
}
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

private static T? FindConstant<T>(SemanticModel semanticModel, SyntaxNode node, Func<object, T> converter) where T : struct
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
=> semanticModel.GetSymbolInfo(node).Symbol is var symbol
&& !IsFieldOrPropertyOutsideSystemNamespace(symbol)
&& !IsEnum(symbol)
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
&& CSharpFacade.Instance.FindConstantValue(semanticModel, node) is { } value
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
&& ConversionHelper.TryConvertWith(value, converter, out T typedValue)
? typedValue
: null;

private static bool TryGetRange(ITypeSymbol typeSymbol, out string typeIdentifier, out double min, out double max)
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
if (typeSymbol.Is(KnownType.System_Single))
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
typeIdentifier = "float";
min = float.MinValue;
max = float.MaxValue;
return true;
}

typeIdentifier = default;
min = default;
max = default;
return false;
}

private static bool IsFieldOrPropertyOutsideSystemNamespace(ISymbol symbol) =>
symbol is { Kind: SymbolKind.Field or SymbolKind.Property }
&& symbol.ContainingNamespace.Name != nameof(System);

private static bool IsEnum(ISymbol symbol) =>
symbol.GetSymbolType() is INamedTypeSymbol { EnumUnderlyingType: { } };
}
}
Expand Up @@ -2122,7 +2122,7 @@ internal static class RuleTypeMappingCS
// ["S2195"],
// ["S2196"],
["S2197"] = "CODE_SMELL",
// ["S2198"],
["S2198"] = "CODE_SMELL",
// ["S2199"],
// ["S2200"],
["S2201"] = "BUG",
Expand Down
@@ -0,0 +1,34 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2023 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using CS = SonarAnalyzer.Rules.CSharp;

namespace SonarAnalyzer.UnitTest.Rules
{
[TestClass]
public class SillyMathematicalComparisonTest
{
private readonly VerifierBuilder builderCS = new VerifierBuilder<CS.SillyMathematicalComparison>();

[TestMethod]
public void SillyMathematicalComparison_CS() =>
builderCS.AddPaths("SillyMathematicalComparison.cs").Verify();
}
}
@@ -0,0 +1,24 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace SonarAnalyzer.UnitTest.TestCases
{
internal class SillyMathematicalComparison
{
public void Floats()
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
float f = 42;

const double veryBig = double.MaxValue;
const double verySmall = double.MinValue;

_ = f < veryBig; // Noncompliant
_ = veryBig > f; // Noncompliant
_ = f > verySmall; // Noncompliant
_ = verySmall < f; // Noncompliant
}
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
}
}