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
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
e76d32a
Update RSPEC for S2198
gregory-paidis-sonarsource 93391b2
Remove a useless auto-generated json
gregory-paidis-sonarsource ececfa2
Add some testcases
gregory-paidis-sonarsource 46f3d70
Remove a useless using
gregory-paidis-sonarsource 5bdd084
Initial implementation of the main logic
gregory-paidis-sonarsource fee626d
Initial working implementation of S2198
gregory-paidis-sonarsource 36214a2
Refactor implementation
gregory-paidis-sonarsource 2983cc7
Move some code around
gregory-paidis-sonarsource 3374a35
Add a lot of testcases
gregory-paidis-sonarsource 3564d52
Cut branches: only implement absolute inequality for float
gregory-paidis-sonarsource 86696dc
Rewrite SillyMathematicalComparison, only out-of-range checks for flo…
gregory-paidis-sonarsource 99f3b27
Apply PR review changes
gregory-paidis-sonarsource ee45a5c
Enrichen testcases, add long and ulong to the checked types
gregory-paidis-sonarsource ba6589f
Add 'is' pattern testcases, refactored a .ToString to a .ToDisplayString
gregory-paidis-sonarsource d463864
Simple rename
gregory-paidis-sonarsource 4f0720a
Implement a constant-in-both-sides version of the rule.
gregory-paidis-sonarsource 0070a96
Apply PR Review changes, some constant-check fixes and more testcases
gregory-paidis-sonarsource e3b6f24
Add enum, char and reference type to the testcases
gregory-paidis-sonarsource a84d441
Simplify the rule, only care about out-of-range mathematical checks
gregory-paidis-sonarsource 4856e96
Add char out-of-range detection
gregory-paidis-sonarsource 51d7b9c
Added some true negatives for const1 {{operator}} const2
gregory-paidis-sonarsource f79b42a
Bump coverage by adding a case I forgot about
gregory-paidis-sonarsource 083ae47
Apply PR Review changes
gregory-paidis-sonarsource 0980aa4
Add an is-pattern edgecase in the testcases
gregory-paidis-sonarsource 16c517c
Fix a logical condition, refactor TryGetRange
gregory-paidis-sonarsource a0fd1ce
Minor naming refactor
gregory-paidis-sonarsource 339ac49
Minor refactor for OutOfRange check
gregory-paidis-sonarsource a8f25d7
Annotate a testcase with an issue
gregory-paidis-sonarsource File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 <= Byte.MaxValue</code> and <code>aByte >= Byte.MinValue</code> </li> | ||
<li> comparing <code>anInt <= int.MaxValue</code> and <code>anInt >= int.MinValue</code> </li> | ||
<li> comparing <code>aLong <= long.MaxValue</code> and <code>aLong >= long.MinValue</code> </li> | ||
</ul> | ||
<h2>Noncompliant Code Example</h2> | ||
<pre> | ||
float f = 42.0f; | ||
const double d = float.MaxValue + 1; | ||
if (f <= d) { } // Noncompliant | ||
</pre> | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,7 @@ | |
"S2184", | ||
"S2187", | ||
"S2190", | ||
"S2198", | ||
"S2201", | ||
"S2219", | ||
"S2222", | ||
|
89 changes: 89 additions & 0 deletions
89
analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
/* | ||
* 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. | ||
*/ | ||
|
||
namespace SonarAnalyzer.Rules.CSharp | ||
{ | ||
[DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
public sealed class SillyMathematicalComparison : SonarDiagnosticAnalyzer | ||
{ | ||
private const string DiagnosticId = "S2198"; | ||
private const string MathComparisonMessage = "Comparison to this constant is useless; the constant is outside the range of type '{0}'"; | ||
|
||
private static readonly DiagnosticDescriptor MathComparisonRule = DescriptorFactory.Create(DiagnosticId, MathComparisonMessage); | ||
|
||
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(MathComparisonRule); | ||
|
||
protected override void Initialize(SonarAnalysisContext context) => | ||
context.RegisterNodeAction( | ||
CheckComparisonOutOfRange, | ||
CSharpFacade.Instance.SyntaxKind.ComparisonKinds); | ||
|
||
private static void CheckComparisonOutOfRange(SonarSyntaxNodeReportingContext context) | ||
{ | ||
if (TryGetConstantValue(context.SemanticModel, (BinaryExpressionSyntax)context.Node, out var constant, out var other) | ||
&& context.SemanticModel.GetTypeInfo(other).Type is { } typeSymbolOfOther | ||
&& TryGetRange(typeSymbolOfOther) is { } range | ||
&& (constant < range.Min || constant > range.Max)) | ||
{ | ||
var typeName = typeSymbolOfOther.ToMinimalDisplayString(context.SemanticModel, other.GetLocation().SourceSpan.Start); | ||
context.ReportIssue(Diagnostic.Create(MathComparisonRule, other.Parent.GetLocation(), typeName)); | ||
} | ||
} | ||
|
||
private static bool TryGetConstantValue(SemanticModel model, BinaryExpressionSyntax binary, out double constant, out SyntaxNode other) | ||
{ | ||
var optionalLeft = model.GetConstantValue(binary.Left); | ||
var optionalRight = model.GetConstantValue(binary.Right); | ||
|
||
if (optionalLeft.HasValue ^ optionalRight.HasValue) | ||
{ | ||
if (optionalLeft.HasValue && TryConvertToDouble(optionalLeft.Value, out constant)) | ||
{ | ||
other = binary.Right; | ||
return true; | ||
} | ||
else if (optionalRight.HasValue && TryConvertToDouble(optionalRight.Value, out constant)) | ||
{ | ||
other = binary.Left; | ||
return true; | ||
} | ||
} | ||
constant = default; | ||
other = default; | ||
return false; | ||
} | ||
|
||
// 'char' needs to roundtrip {{char -> int -> double}}, can't go {{char -> double}} | ||
private static bool TryConvertToDouble(object constant, out double typedConstant) => | ||
ConversionHelper.TryConvertWith(constant is char ? Convert.ToInt32(constant) : constant, Convert.ToDouble, out typedConstant); | ||
|
||
private static ValuesRange? TryGetRange(ITypeSymbol typeSymbol) => | ||
typeSymbol switch | ||
{ | ||
_ when typeSymbol.Is(KnownType.System_Char) => new(char.MinValue, char.MaxValue), | ||
_ when typeSymbol.Is(KnownType.System_Single) => new(float.MinValue, float.MaxValue), | ||
_ when typeSymbol.Is(KnownType.System_Int64) => new(long.MinValue, long.MaxValue), | ||
_ when typeSymbol.Is(KnownType.System_UInt64) => new(ulong.MinValue, ulong.MaxValue), | ||
_ => null, | ||
}; | ||
|
||
private readonly record struct ValuesRange(double Min, double Max); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
38 changes: 38 additions & 0 deletions
38
analyzers/tests/SonarAnalyzer.UnitTest/Rules/SillyMathematicalComparison.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/* | ||
* 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(); | ||
|
||
[TestMethod] | ||
public void SillyMathematicalComparison_CSharp9() => | ||
builderCS.AddPaths("SillyMathematicalComparison.CSharp9.cs").WithOptions(ParseOptionsHelper.FromCSharp9).Verify(); | ||
} | ||
} |
25 changes: 25 additions & 0 deletions
25
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SillyMathematicalComparison.CSharp9.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
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 IsPatterns() | ||
{ | ||
_ = 33 is 55; // Compliant | ||
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_ = 33 is < 55; // Compliant | ||
_ = 33 is <= 55; // Compliant | ||
_ = 33 is > 55; // Compliant | ||
_ = 33 is >= 55; // Compliant | ||
_ = 33 is not 55; // Compliant | ||
|
||
const double d = double.MaxValue; | ||
float x = 42; | ||
_ = x is d; // Error [CS0266] - cannot implicitly convert type 'double' to 'float' | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Could be made a method on the
ValueRange
type