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

Improve S109 messaging to align with the implementation #5251

Open
andrei-epure-sonarsource opened this issue Jan 10, 2022 · 0 comments
Open
Labels
Area: C# C# rules related issues. Type: Messages Change rule primary and/or secondary messages

Comments

@andrei-epure-sonarsource
Copy link
Contributor

andrei-epure-sonarsource commented Jan 10, 2022

Currently S109 only tells users to move magic numbers into well named variables or constants.

However, the rule implementation also ignores any construct with magic numbers which has the result stored in a variable or constant, because literalExpression.FirstAncestorOrSelf<VariableDeclarationSyntax>() != null will go up the tree hierarchy until reaching a variable declaration somewhere in the ancestor list.

For example:

var fooResult = Bar(42); // is currently considered compliant
var fooResult = Bar(42, Bar(110, Bar(233, 23454, Bar(999)))); // compliant ?!

I am not sure if this was intended as the test cases are very simplistic and did not include such examples. Removing this check would make the rule even more noisy than it is, although it surely has lots of false negatives as well.

Having this said, if we want to keep the existing behavior of the rule, it would be more useful (and honest) to tell users to use well-named variables when employing magic numbers in some constructs.

For example:

   Foo(new IntPtr(123456)); // Noncompliant {{Assign this object creation with magic number '123456' to a well-named variable/constant, and use that instead.}}
//     ^^^^^^^^^^^^^^^^^^

HOW:

  • the rule needs to be re-written using a CSharpSyntaxWalker that would collect information in each class member and avoid raising duplicate issues
  • we could decide to stop at one level of the parent hierarchy, like below:
new Bar(100, Foo(200, 300)); // Noncompliant {{Assign this object creation with magic number '100' to a well-named variable/constant, and use that instead.}}
// Noncompliant@-1 {{Assign this invocation using magic numbers '200', '300' to a well-named variable/constant, and use that instead.}}

Like this, we would ensure there cannot be abuse of this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Type: Messages Change rule primary and/or secondary messages
Projects
None yet
Development

No branches or pull requests

2 participants