Fix S109 FP: named arguments, constructor calls, single-value attributes #5247
Conversation
analyzers/src/SonarAnalyzer.CSharp/Rules/MagicNumberShouldNotBeUsed.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs
Show resolved
Hide resolved
|
To avoid a rabbit-hole, I am splitting the discussion&implementation around:
This PR will only focus on reducing the noise. |
andrei-epure-sonarsource
force-pushed
the
andrei/4737
branch
from
7286d7b to
98fe791
Compare
andrei-epure-sonarsource
force-pushed
the
andrei/4737
branch
from
71a65c8 to
8df471f
Compare
andrei-epure-sonarsource
changed the title
andrei-epure-sonarsource
changed the title
| @@ -61,52 +61,49 @@ public sealed class MagicNumberShouldNotBeUsed : SonarDiagnosticAnalyzer | |||
|
|
|||
| private static bool IsExceptionToTheRule(LiteralExpressionSyntax literalExpression) => | |||
| NotConsideredAsMagicNumbers.Contains(literalExpression.Token.ValueText) | |||
| // It's ok to use magic numbers as part of a variable declaration | |||
There was a problem hiding this comment.
the comments were useless, removing them
| // Inside property we consider magic numbers as exceptions in the following cases: | ||
| // - A {get; set;} = MAGIC_NUMBER | ||
| // - A { get { return MAGIC_NUMBER; } } | ||
| private static bool IsInsideProperty(SyntaxNode node) |
There was a problem hiding this comment.
this was just reordered above, no functional change
|
|
||
| private static bool IsSingleDigitInToleratedComparisons(LiteralExpressionSyntax literalExpression) => | ||
| literalExpression.Parent is BinaryExpressionSyntax binaryExpression | ||
| && binaryExpression.IsAnyKind(SyntaxKind.EqualsExpression, SyntaxKind.LessThanExpression, SyntaxKind.LessThanOrEqualExpression) |
There was a problem hiding this comment.
this was fixed in bc1eebc
andrei-epure-sonarsource
changed the title
| // Inside property we consider magic numbers as exceptions in the following cases: | ||
| // - A {get; set;} = MAGIC_NUMBER | ||
| // - A { get { return MAGIC_NUMBER; } } | ||
| private static bool IsInsideProperty(SyntaxNode node) |
There was a problem hiding this comment.
this was just reordered from below, no functional change (see in overall changes this part didn't change)
|
|
||
| private static bool ToStringContainsAnyAcceptedNames(SyntaxNode syntaxNode) | ||
| { | ||
| var toString = syntaxNode.ToString().ToLower(); |
There was a problem hiding this comment.
My understanding is that we accept the methods Count, Size, Length, Order coming from particular namespaces (e.g., IEnumerable).
In this case, the full name needs to be checked otherwise if a developer creates their own method count which might do something different, it's going to be a false negative.
Also, I think the character case needs to be taken into account (no need to turn the method names to lower), as COUNT() and Count() are two different methods even if they are in the same class.
There was a problem hiding this comment.
In case the intention is to avoid using the semantic model, at least a strict comparison with equals should create fewer FNs.
There was a problem hiding this comment.
well, I'd say that in general if we want to compare anything that is Count to a single digit, it should be fine
if (Foo.BarCount < 3) {}
why would the rule ask the dev to put 3 in a magic constant?
There was a problem hiding this comment.
I think the risk would be something like if (CountOfMonteChristo == 9), or something like this, but I'd say it's fairly low
And I dropped the idea of Order - should I actually add it?
Will people do something like if (Foo.Order < 3) - I guess I should?
There was a problem hiding this comment.
(offline discussion - the initial idea was to avoid noise when someone is comparing the size of a collection with single-digit number to access the first elements - so I reduced it to this scenario
we will still have FPs because this is a simple AST-based rule so we won't have dataflow and we don't plan to
we will still have FPs , in general, with this rule - this is why it's not (and probably never will be) part of SonarWay)
mary-georgiou-sonarsource
assigned andrei-epure-sonarsource and unassigned mary-georgiou-sonarsource
|
Updated RSPEC (to be merged after this gets merged) SonarSource/rspec#694 |
| <li> a variable/field declaration </li> | ||
| <li> the single argument of an attribute </li> | ||
| <li> a named argument for a method or attribute </li> | ||
| <li> a constructor call </li> |
There was a problem hiding this comment.
I haven't included the single-digit comparison because it's hard to explain and isn't worth the effort TBH
| // We allow single-digit comparisons when checking the size of a collection, which is usually done to access the first elements. | ||
| private static bool IsComparingCollectionSize(BinaryExpressionSyntax binaryComparisonToLiteral) | ||
| { | ||
| var comparedToLiteral = binaryComparisonToLiteral.Left is LiteralExpressionSyntax ? binaryComparisonToLiteral.Right : binaryComparisonToLiteral.Left; |
There was a problem hiding this comment.
add a test with number on left
|
Kudos, SonarCloud Quality Gate passed!
|
|
Kudos, SonarCloud Quality Gate passed! |
andrei-epure-sonarsource
changed the title
|
Validation: for some projects it didn't reduce the noise, for others it significantly did. For example:
Validation done. |
Fixes #4737
For validation - doc with Peach numbers before this PR
Please review commit by commit. Most of the changed lines are in updated ITs - 8df471f