Skip to content

Commit

Permalink
Improve messages for object creation
Browse files Browse the repository at this point in the history
  • Loading branch information
@andrei-epure-sonarsource
andrei-epure-sonarsource committed Jan 7, 2022
1 parent 2f7205f commit 7286d7b
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 26 deletions.
30 changes: 23 additions & 7 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/MagicNumberShouldNotBeUsed.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
using Microsoft.CodeAnalysis.Diagnostics;
using SonarAnalyzer.Common;
using SonarAnalyzer.Helpers;
using StyleCop.Analyzers.Lightup;

namespace SonarAnalyzer.Rules.CSharp
{
Expand All @@ -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<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down
46 changes: 27 additions & 19 deletions analyzers/tests/SonarAnalyzer.UnitTest/TestCases/MagicNumberShouldNotBeUsed.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -45,14 +54,11 @@ public ValidUseCases(string s, FooBar foo)
int i4 = 42;
var list = new List<string>(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++)
Expand All @@ -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()
Expand All @@ -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
Expand All @@ -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<int> 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
Expand All @@ -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) { }
}
}

Expand Down

0 comments on commit 7286d7b

Please sign in to comment.