diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MagicNumberShouldNotBeUsed.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MagicNumberShouldNotBeUsed.cs index 220bbeaf3d7..d94f283d817 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MagicNumberShouldNotBeUsed.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MagicNumberShouldNotBeUsed.cs @@ -28,6 +28,7 @@ using Microsoft.CodeAnalysis.Diagnostics; using SonarAnalyzer.Common; using SonarAnalyzer.Helpers; +using StyleCop.Analyzers.Lightup; namespace SonarAnalyzer.Rules.CSharp { @@ -36,7 +37,7 @@ namespace SonarAnalyzer.Rules.CSharp public sealed class MagicNumberShouldNotBeUsed : SonarDiagnosticAnalyzer { private const string DiagnosticId = "S109"; - private const string MessageFormat = "Assign this magic number '{0}' to a well-named (variable|constant), and use the (variable|constant) instead."; + private const string MessageFormat = "Assign this {0} '{1}' to a well-named variable/constant, and use that instead."; private static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager); public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); @@ -53,8 +54,19 @@ protected override void Initialize(SonarAnalysisContext context) => if (!IsExceptionToTheRule(literalExpression)) { - c.ReportIssue(Diagnostic.Create(Rule, literalExpression.GetLocation(), - literalExpression.Token.ValueText)); + Location location; + string message; + if (GrandGrandParentIsObjectCreation(literalExpression)) + { + location = literalExpression.Parent.Parent.Parent.GetLocation(); + message = "object creation with magic number"; + } + else + { + location = literalExpression.GetLocation(); + message = "magic number"; + } + c.ReportIssue(Diagnostic.Create(Rule, location, message, literalExpression.Token.ValueText)); } }, SyntaxKind.NumericLiteralExpression); @@ -89,13 +101,17 @@ literalExpression.Parent is BinaryExpressionSyntax binaryExpression && ToStringContainsAnyAcceptedNames(binaryExpression); private static bool IsToleratedArgument(LiteralExpressionSyntax literalExpression) => - IsToleratedMethodArgument(literalExpression) - || IsSingleOrNamedAttributeArgument(literalExpression); + IsToleratedMethodArgument(literalExpression) || IsSingleOrNamedAttributeArgument(literalExpression); + + private static bool GrandGrandParentIsObjectCreation(LiteralExpressionSyntax literalExpression) => + literalExpression.Parent is ArgumentSyntax argument && IsObjectCreation(argument.Parent.Parent); + + private static bool IsObjectCreation(SyntaxNode node) => + ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(node) || node is ObjectCreationExpressionSyntax; // Named argument or constructor argument. private static bool IsToleratedMethodArgument(LiteralExpressionSyntax literalExpression) => - literalExpression.Parent is ArgumentSyntax arg - && (arg.NameColon is not null || arg.Parent.Parent is ObjectCreationExpressionSyntax); + literalExpression.Parent is ArgumentSyntax arg && arg.NameColon is not null; private static bool IsSingleOrNamedAttributeArgument(LiteralExpressionSyntax literalExpression) => literalExpression.Parent is AttributeArgumentSyntax arg diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs index 417d389d93e..85f2d0dcdb9 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs @@ -35,6 +35,15 @@ public static double FooProp get { return 2; } } + public static double FooProp2 + { + get + { + var someName = Math.Sqrt(4096); // Compliant, stored in a variable + return someName; + } + } + public ValidUseCases(int x, int y) { } public ValidUseCases(string s, FooBar foo) @@ -45,14 +54,11 @@ public ValidUseCases(string s, FooBar foo) int i4 = 42; var list = new List(42); // Compliant, set to variable var bigInteger = new IntPtr(1612342); // compliant, set to variable - var g = new Guid(0xA, 0xB, 0xC, new Byte[] { 0, 1, 2, 3, 4, 5, 6, 7 }); // Compliant, set in a variable + var g = new Guid(0xA, 0xB, 0xC, new Byte[] { 0, 1, 2, 3, 4, 5, 6, 7 }); // Compliant, set to a variable + var valid = new ValidUseCases(100, 300); - Foo(new IntPtr(123456)); // Compliant, class constructor - var byteArrayName = new Byte[] { 0, 1, 2, 3, 4, 5, 6, 7 }; - Foo(new Guid(0xA, 0xB, 0xC, byteArrayName)); // Compliant, magic numbers set directly as constructor params - - long[] values1 = { 30L, 40L }; // Compliant, array initialisation - int[] values2 = new int[] { 100, 200 }; // Compliant, array initialisation + long[] values1 = { 30L, 40L }; // Compliant, set to variable + int[] values2 = new int[] { 100, 200 }; // Compliant, set to variable Console.WriteLine(value: 12); // Compliant, named argument for (int i = 0; i < 0; i++) @@ -73,9 +79,6 @@ public ValidUseCases(string s, FooBar foo) const int VAL = 15; Console.Write("test"); - - // we tolerate magic constants sent to constructors - new ValidUseCases(100, 300); } public override int GetHashCode() @@ -100,8 +103,6 @@ public void Foo3() { } [Foo(42)] // Compliant, attribute with only one argument public static int Bar(int value) => 0; - public static void Foo(IntPtr x) { } - public static void Foo(Guid x) { } } public enum MyEnum @@ -115,27 +116,33 @@ public class WrongUseCases { public static double FooProp { - get { return Math.Sqrt(4); } // Noncompliant + get { return Math.Sqrt(4); } // Noncompliant {{Assign this magic number '4' to a well-named variable/constant, and use that instead.}} +// ^ } public WrongUseCases(int x, int y) { } public WrongUseCases(List list, string s, FooBar foo) { - Console.WriteLine(12); // Noncompliant + Console.WriteLine(12); // Noncompliant {{Assign this magic number '12' to a well-named variable/constant, and use that instead.}} - for (int i = 10; i < 50; i++) // Noncompliant + for (int i = 10; i < 50; i++) // Noncompliant {{Assign this magic number '50' to a well-named variable/constant, and use that instead.}} { } var array = new string[10]; - array[5] = "test"; // Noncompliant - Foo(new int[] { 100 }); // Noncompliant, array with magic numbers should have a decent name + array[5] = "test"; // Noncompliant {{Assign this magic number '5' to a well-named variable/constant, and use that instead.}} + Foo(new int[] { 100 }); // Noncompliant + new WrongUseCases(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 magic number '200' to a well-named variable/constant, and use that instead.}} + // Noncompliant@-2 {{Assign this magic number '300' to a well-named variable/constant, and use that instead.}} - new WrongUseCases(100, Foo(200, 300)); // Noncompliant {{Assign this magic number '200' to a well-named (variable|constant), and use the (variable|constant) instead.}} - // Noncompliant@-1 {{Assign this magic number '300' to a well-named (variable|constant), and use the (variable|constant) instead.}} + Foo(new IntPtr(123456)); // Noncompliant {{Assign this object creation with magic number '123456' to a well-named variable/constant, and use that instead.}} +// ^^^^^^^^^^^^^^^^^^ + Foo(new int[] { 100, 200 }); // Noncompliant {{Assign this magic number '100' to a well-named variable/constant, and use that instead.}} + // Noncompliant@-1 {{Assign this magic number '200' to a well-named variable/constant, and use that instead.}} var result = list.Count == 99; result = list.Count < 400; // Noncompliant @@ -155,6 +162,7 @@ public int GetValue() public static void Foo(int[] array) { } public static int Foo(int x, int y) => 1; + public static void Foo(IntPtr x) { } } }