Fix S109 FP: named arguments, constructor calls, single-value attributes

[andrei-epure-sonarsource]

As an output of this community post: https://community.sonarsource.com/t/s109-magic-numbers-on-attributes/15784/16

And verifying the implementation of the same rule in our Java analyzer: https://github.com/SonarSource/sonar-java/blob/7.1.0.26670/java-checks/src/main/java/org/sonar/java/checks/MagicNumberCheck.java

We want to ignore magic numbers when used in:

• attributes (still to discuss whether we should consider just well-known values - like Order)
• in equality-checks or assignments to Count , Size, Length, Order for single-digit integer values
• enum values

Also consider (maybe have another internal talk) to tolerate:

• numbers passed to constructors
• numbers passed to methods with named parameters

[StingyJack]

We want to ignore magic numbers

We want to ignore literal numbers when used in places where the meaning of them is obvious. Literal numbers where the meaning is obscured or hidden or not obvious is what makes them "magic"

This would mean things like using a literal number in a new DateTime(int year, int month, int day) (or one of the other overloads) does not trigger it, because those are not magic numbers. Also things like Timespan.FromSeconds(double value) shouldn't trigger it because the value is obvious. An attribute parameter named "Order" followed by a literal would be obvious.

Magic numbers are magic in any programming language. That Java check seems equally problematic. I'd define a list of identifiers that can hold a literal, something like

private readonly List<string> _allowedLiterals = new List<string>{ "year", "month", "day", "hour", "minute", "second", "tick", "order", etc};

Then when visiting the usage of each literal number usage in the analyzer see if its being used for a parameter that has a name matching a record in the allowed list or if it matches the plural of an allowed list item, or if its the only parameter and the function name is From + one of the allowed literals or its plural.

Another facet not yet discussed was finding things like const int ONE = 1; and flagging those as magic, which would be probably harder or more tedious to derive all of the alphabetical representations of numbers and compare them with the assignment (so it didnt flag const int ONE_YEAR_IN_SECONDS = 31_557_600;

[Corniel]

I have some suggestions:

I would argue that magic numbers should be ignored when used in creating a static readonly:

class NoMagicNumber
{
    static readonly SomeClass ReadOnlyClass = new SomeClass(1, 2, 3); // Compliant
    static readonly SomeClass ReadOnlyClassChaine = new SomeClass().Add(3); // Complaint
    static readonly SomeClass ReadOnlyFactory = SomeClass.FromInt(1234); // Compliant
}

When extension methods are used to create value types:

class NoMagicNumber
{
    void ExtensionMethods()
    {
        /* struct */ Percentage percentage = 34.5.Percent(); // Compliant
        TimeSpan duration = 12.Seconds(); // Compliant
    }
}

When value types (as struct) are created:

class NoMagicNumber
{
   void ValueTypes()
   {
       var date = new DateOnly(2017, 06, 11); // Compliant
       var duration = TimeSpan.FromMilliseconds(12.4); // Compliant
       var later = DateTime.NowUtc.Add(new TimeSpan(4, 5, 2)); // Compliant
   } 
}

[Corniel]

Another case that should be excluded: GetHashcode()

class Hash
{
    public int Value1 { get; }
    public string Value2 { get; }

    public override int GetHashCode()
    {
        var hash1 = value?.GetHashCode() ?? 0;
        return Value1 + (hash2 * 1566083941); // Compliant
    }
    
    public static int GetHashCode(object obj)
    {
        var hash = 1;
        foreach (var prop in obj.GetType().GetProperties())
        {
            hash += prop.GetValue(obj).GetHashCode();
            hash *= 17; // Compliant
        }
        return hash;
    }
}

[StingyJack]

@andrei-epure-sonarsource the list in the OP doesnt meet the minimum need to reduce FP unless all of the items it mentions are included (attributes and parameters). Anything less than that scope and there probably isnt enough value to to enable the rule again.

[cotzo]

What about Protobuf code first serialization where you have to put [ProtoMember(number)] and we have to pollute all our model classes with pragma?

[andrei-epure-sonarsource]

Anything less than that scope and there probably isnt enough value to to enable the rule again.

@StingyJack - do you mean if we ignore all numbers inside attributes, the rule wouldn't be useful anymore?

What about Protobuf code first serialization where you have to put [ProtoMember(number)] and we have to pollute all our model classes with pragma?

@cotzo - if we'd ignore all attributes altogether, this issue would be solved. Especially if there's only one parameter OR if named parameters are used.

---

[Internal] We should look at projects on Peach and see what's noisy categories we can emerge from there.

[andrei-epure-sonarsource]

@Corniel - good points!

I would argue that magic numbers should be ignored when used in creating a static readonly:

This is doable at syntax level.

When extension methods are used to create value types:
When value types (as struct) are created:

I would avoid adding semantic calls into this rule because it would be quite costly. I would try to keep the improvements just at syntax level, if possible.

[Corniel]

@Corniel - good points!

I would argue that magic numbers should be ignored when used in creating a static readonly:

This is doable at syntax level.

When extension methods are used to create value types:
When value types (as struct) are created:

I would avoid adding semantic calls into this rule because it would be quite costly. I would try to keep the improvements just at syntax level, if possible.

I agree. 13.Percent() however should be detectable without semantic calls; any method call on a literal should be a candidate for exclusion, I suppose.

Also, a constructor only containing literals is a save guess, I think.

[StingyJack]

do you mean if we ignore all numbers inside attributes, the rule wouldn't be useful anymore?

@andrei-epure-sonarsource - i mean that all 5 bullet pointed items need to be implemented for this rule. There isnt much value for me in the first three,

[bcrudolp]

Hi Andrei,

My name is Benjamin, and my team is very interested in the resolution of this issue for a project we are working on right now. We wish to rid the codebase of its SonarLint warnings, but the attribute false positives really make that impossible or senseless for S109.

What is the likely timeline on this, if any?

Warm Regards,
Benjamin R.

[andrei-epure-sonarsource]

@bcrudolp I've started working on it, however I'm not sure what your usecase is.

[bcrudolp]

Hi Andrei,

We are concerned with the false positives induced by attribute values. We have many of those that are simply indices or allowable ranges for settings, etc.

Thanks!
Benjamin

[andrei-epure-sonarsource]

yes the attribute cases in handled in: #5247

[sharbhajan]

Hi
Accessing array elements using index are non-compliant?

byte[] bytes = new byte[10];
var time = DateTime.Now;
bytes[3] = (byte)time.Year;
bytes[2] = (byte)time.Month;
bytes[1] = (byte)time.Day;
bytes[0] = (byte)time.Hour;
bytes[5] = (byte)time.Minute;
bytes[4] = (byte)time.Second;
...

I feel that reporting array indexes as magic numbers is a false positive.

[StingyJack]

Accessing array elements using index are non-compliant?

Yeah, that code is a classic example of a magic number and why they aren't good to use.

[sharbhajan]

Array declarations seem false positives as well

var PngSignatures = new byte[] { 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A };
var DocSignatures = new byte[] { 0xD0, 0xCF, 0x11, 0xE0, 0xA1, 0xB1, 0x1A, 0xE1 };
var PdfSignatures = new byte[] { 0x25, 0x50, 0x44, 0x46 };

[Tim-Pohlmann]

@sharbhajan Can you explain why you think these are FP? From my point of view your code samples are the exact cases S109 is supposed to report about. But I had trouble with this rule myself and am very interested in hearing your perspective.

[sharbhajan]

@sharbhajan Can you explain why you think these are FP? From my point of view your code samples are the exact cases S109 is supposed to report about. But I had trouble with this rule myself and am very interested in hearing your perspective.

Array index:
Processing arrays in a loop is not an issue, loop index variable is already there. But many a times we need to get or set array elements, without a loop, using indexes which may not be in order. It may seems fair to declare constants for 2-3 indexes but doesn't feel logical for more elements. As an example, an array may contain 100 elements but there may requirement to get / set only indexes 5, 7, 10, 51, 52 at one place and 6, 8, 9, 50, 98, 99 at another place. There may be different requirements in different scenarios, particularly when processing CSVs. Do I need to declare constants for all such indexes?

Array declarations:
I hope the example given above (file signature byte arrays for verifying file types) should be convincing itself. It doesn't feel justifiable to declare all the values (0x89, 0x50 ...) as constants and then initialize the arrays using these constants.

[Tim-Pohlmann]

Both of these examples are very clear "magic numbers" in the sense that it is hard to understand from just looking at the code what these numbers mean and why they have been chosen.
That being said, the last example should not even raise S109 because it is part of a variable declaration (which is one of S109's exceptions).

I don't want to dismiss your concern, though and instead fully understand where you are coming from. So let me ask you this: When do you find S109 issues valuable? Can you give a prime example where you would say "I'm glad S109 is raised here"?

[Corniel]

@Tim-Pohlmann I would argue (again) that if those signatures where declared as static read-only, they are the solution for the magic numbers themselves. You could argue that byte[] most certainly does something like that in most all cases, but you could argue that by flagging the case of @sharbhajan you force the writer of that code, to define these signatures as static readonly so that in the places where they are used, you don't have the magic numbers (goal of the rule) and can understand that what's going on based on the descriptive name.

[sharbhajan]

It seems there is some confusion of context here. Indeed, S109 is a good, valuable rule and raised genuine issues in a legacy project that I inherited, analyzed and reviewed. In almost all other areas, I found it very helpful in code maintainability. My concerns are only for arrays indexes and declarations.

Array index:
The given example is from a code block that is used to generate sequential guids based on current date-time where elements need to be set with values of year, month, day, hour, minute, seconds and so on for indexes 3, 2, 1, 0, 5, 4, 7, 6, 9, 8, ...
Other references given in previous comment point to some CSV data processing where some selective elements (which are based on given context, type of file, data format etc.) need to get/set.
Sure, there are ways to refactor such code. But often there are cases when we need to access arrays using indexes and it doesn't feel logical to declare array indexes as constants. In my opinion array indexes may be an exception to this rule.

Array declarations:
There are two different code blocks, at one place an array is declared and then passed as parameter to another function and at other place array is directly passed as parameter:

var PngSignatures = new byte[] { 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A };
AddSignatures(PngSignatures);

//gif signatures
AddSignatures(new byte[] { 0x47, 0x49, 0x46, 0x38 });

S109 is raised for both code blocks, that is why I posted here.

[Tim-Pohlmann]

var PngSignatures = new byte[] { 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A };

does not raise S109 for me. If it does for you, please report it as an FP with a reproducer in our community forum.

Regarding the other cases: I'll raise awareness with my team to get a few different point of views.