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

gregory-paidis-sonarsource · 2023-02-01T09:16:29Z

Fixed #6658
RSPEC PR: SonarSource/rspec#1534
S2198

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyBitwiseOperation.cs

martin-strecker-sonarsource

@gregory-paidis-sonarsource you are probably better of deleting much of your changes so far. This is the most difficult thing to do as a developer (I'm also bad at this) but there is too much debris from previous attempts lurking around that is just a distraction. Start with my bullet points so we have something sensible to start with.

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyBitwiseOperation.cs

gregory-paidis-sonarsource · 2023-02-03T14:38:00Z

@gregory-paidis-sonarsource you are probably better of deleting much of your changes so far. This is the most difficult thing to do as a developer (I'm also bad at this) but there is too much debris from previous attempts lurking around that is just a distraction. Start with my bullet points so we have something sensible to start with.

@martin-strecker-sonarsource makes sense, it looks quite messy right now. Thank you greatly for all the feedback and the steps you suggested, I will implement this in that manner only for floats (baby steps) and then get back to you.

…ats for this step

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

martin-strecker-sonarsource

Some clean-up.

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

martin-strecker-sonarsource · 2023-02-07T08:03:04Z

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

+            var typeIdentifier = string.Empty;
+            if (ShouldRaise(context.SemanticModel, binary.Left, binary.Right, ref typeIdentifier))
+            {
+                context.ReportIssue(Diagnostic.Create(Rule, binary.GetLocation(), typeIdentifier));


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)

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.

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.

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SillyMathematicalComparison.cs

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SillyMathematicalComparison.cs

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

martin-strecker-sonarsource

I think we will scope the PR to what we have now. The checks at the boundary can be done later.

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SillyMathematicalComparison.CSharp9.cs

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SillyMathematicalComparison.cs

martin-strecker-sonarsource

Looks good. Let's do some clean-up.

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

martin-strecker-sonarsource · 2023-02-09T12:46:38Z

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

+               && TryGetRange(typeSymbolOfOther, out var min, out var max)
+               && (constant < min || constant > max))
+            {
+                var typeName = context.SemanticModel.GetTypeInfo(other).Type.ToMinimalDisplayString(context.SemanticModel, other.GetLocation().SourceSpan.Start);


You can use the location of the node here and in the report. Store the location as a local for re-use.

But i use two different locations. One if other.GetLocation(), the other is other.Parent.GetLocation().

ToMinimalDisplayString needs the position to be aware of the usings and namespace that are relevant to the position. You can post whatever location is close. other or other.Parent don't make a difference in this respect.

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SillyMathematicalComparison.CSharp9.cs

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

martin-strecker-sonarsource

LGTM. Two minor suggestions.

martin-strecker-sonarsource · 2023-02-10T10:07:25Z

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SillyMathematicalComparison.cs

+            const float number1 = 42;
+            const double number2 = double.MaxValue;
+
+            _ = number1 != number2; // Compliant, constant in both operands


Link to the issue about this phenomenon.

martin-strecker-sonarsource · 2023-02-10T10:10:10Z

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs

+            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))


NIT: Could be made a method on the ValueRange type

Suggested change

&& (constant < range.Min || constant > range.Max))

&& range.IsOutOfRange(constant)

sonarcloud · 2023-02-10T12:54:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-02-10T12:56:47Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

97.8% Coverage
0.0% Duplication

gregory-paidis-sonarsource added 8 commits February 1, 2023 10:13

Update RSPEC for S2198

e76d32a

Remove a useless auto-generated json

93391b2

Add some testcases

ececfa2

Remove a useless using

46f3d70

Initial implementation of the main logic

5bdd084

Initial working implementation of S2198

fee626d

Refactor implementation

36214a2

Move some code around

2983cc7

gregory-paidis-sonarsource commented Feb 3, 2023

View reviewed changes

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyBitwiseOperation.cs Outdated Show resolved Hide resolved

Add a lot of testcases

3374a35

gregory-paidis-sonarsource requested a review from martin-strecker-sonarsource February 3, 2023 10:08

gregory-paidis-sonarsource commented Feb 3, 2023

View reviewed changes

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyBitwiseOperation.cs Outdated Show resolved Hide resolved

martin-strecker-sonarsource requested changes Feb 3, 2023

View reviewed changes

Cut branches: only implement absolute inequality for float

3564d52

Rewrite SillyMathematicalComparison, only out-of-range checks for flo…

86696dc

…ats for this step

gregory-paidis-sonarsource commented Feb 6, 2023

View reviewed changes

analyzers/src/SonarAnalyzer.CSharp/Rules/SillyMathematicalComparison.cs Outdated Show resolved Hide resolved

gregory-paidis-sonarsource requested a review from martin-strecker-sonarsource February 6, 2023 12:59

martin-strecker-sonarsource requested changes Feb 7, 2023

View reviewed changes

gregory-paidis-sonarsource added 2 commits February 7, 2023 11:38

Apply PR review changes

99f3b27

Enrichen testcases, add long and ulong to the checked types

ee45a5c

gregory-paidis-sonarsource requested a review from martin-strecker-sonarsource February 7, 2023 12:52

martin-strecker-sonarsource requested changes Feb 7, 2023

View reviewed changes

gregory-paidis-sonarsource added 3 commits February 7, 2023 16:23

Add 'is' pattern testcases, refactored a .ToString to a .ToDisplayString

ba6589f

Simple rename

d463864

Implement a constant-in-both-sides version of the rule.

4f0720a

gregory-paidis-sonarsource requested a review from martin-strecker-sonarsource February 7, 2023 15:58

martin-strecker-sonarsource requested changes Feb 7, 2023

View reviewed changes

gregory-paidis-sonarsource added 2 commits February 8, 2023 11:03

Apply PR Review changes, some constant-check fixes and more testcases

0070a96

Add enum, char and reference type to the testcases

e3b6f24

gregory-paidis-sonarsource added 3 commits February 8, 2023 15:13

Simplify the rule, only care about out-of-range mathematical checks

a84d441

Add char out-of-range detection

4856e96

Added some true negatives for const1 {{operator}} const2

51d7b9c

gregory-paidis-sonarsource requested a review from martin-strecker-sonarsource February 8, 2023 14:50

Bump coverage by adding a case I forgot about

f79b42a

martin-strecker-sonarsource requested changes Feb 9, 2023

View reviewed changes

gregory-paidis-sonarsource added 3 commits February 9, 2023 15:47

Apply PR Review changes

083ae47

Add an is-pattern edgecase in the testcases

0980aa4

Fix a logical condition, refactor TryGetRange

16c517c

gregory-paidis-sonarsource requested a review from martin-strecker-sonarsource February 9, 2023 17:29

Minor naming refactor

a0fd1ce

martin-strecker-sonarsource approved these changes Feb 10, 2023

View reviewed changes

gregory-paidis-sonarsource added 2 commits February 10, 2023 13:30

Minor refactor for OutOfRange check

339ac49

Annotate a testcase with an issue

a8f25d7

gregory-paidis-sonarsource merged commit 833e003 into master Feb 10, 2023

gregory-paidis-sonarsource deleted the greg/implement-S2198 branch February 10, 2023 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gregory-paidis-sonarsource commented Feb 1, 2023 •

edited by martin-strecker-sonarsource

martin-strecker-sonarsource left a comment

gregory-paidis-sonarsource commented Feb 3, 2023

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource Feb 7, 2023

gregory-paidis-sonarsource Feb 7, 2023

martin-strecker-sonarsource Feb 7, 2023

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource Feb 9, 2023

gregory-paidis-sonarsource Feb 9, 2023

martin-strecker-sonarsource Feb 10, 2023

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource Feb 10, 2023

martin-strecker-sonarsource Feb 10, 2023

sonarcloud bot commented Feb 10, 2023

sonarcloud bot commented Feb 10, 2023

	&& (constant < range.Min \|\| constant > range.Max))
	&& range.IsOutOfRange(constant)

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

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

Conversation

gregory-paidis-sonarsource commented Feb 1, 2023 • edited by martin-strecker-sonarsource

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

gregory-paidis-sonarsource commented Feb 3, 2023

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 10, 2023

sonarcloud bot commented Feb 10, 2023

gregory-paidis-sonarsource commented Feb 1, 2023 •

edited by martin-strecker-sonarsource