S109 - magic numbers on attributes

We are using a lot of numbers in data transfer object attributes, so in each file we have to suppress this warning.
Currently C# does not support using variables in attributes, so there’s no way to avoid magic numbers from being used in attributes:

        [Properties(ModelMap: "key", Order: 10)]
        public int? PrimaryKey { get; set; }

hi @donatasj87

in this case, it looks like it would be easier to disable the rule for your project

I don’t want to disable it for all project, because it is quite useful rule in other parts of the project, so I end up disabling it on each file. Would be nice if sonar could detect it and ignore automatically.

1 Like

No, but it can still use constants

    [MyCustom(AttributeUsage.MAGIC)]
    public class AttributeUsage
    {
        private const int MAGIC = 10;
        [MyCustom(MAGIC)]
        public int? Foo { get; set; }

    }

    class MyCustomAttribute : Attribute
    {
        public int ParamValue { get; set; }
        public MyCustomAttribute(int paramValue)
        {
            this.ParamValue = paramValue;
        }
    }

Correct. It is useful for static and reusable values, but in some cases each properties have different values, like ordering for example, so in this case creating constant for each one would be a waste of keystrokes.

hi @donatasj87

We’ve had an internal talk. We agree that in your case this rule is creating a lot of noise. However, for now we don’t plan to change it to detect all exceptions that it would have to handle.

  • the rule is not part of SonarWay (we know it can generate quite some noise, so by default it’s turned off)
  • it would require quite some effort to tweak the rule to make it less noisy, and we don’t plan to invest that effort any time soon

Thank you for the feedback! We’ll keep it in mind for the future.

is there a way to disable this rule in some files?

@Daniel-Gabriel Please have a look at section “Ignore Issues” in the documentation.

1 Like

This rule makes a lot of noise because its doing silly things like flagging literal usage in an attribute as a magic number when the property/attribute arg is clearly titled “ORDER”. This kind of thing has been around since XML serialization, then in WCF, then protobuf/gRPC. The rule should be able to recognize being used in an attribute with a property name of “Order” as something not to flag.

The suggestions are to use a bunch of constants like this…

public const string N_1 = "1";
public const string N_2= "2";
public const string N_3= "3";
public const string N_4 = "4";
// etc... 

, disable the rule and get no magic number handling for the rest of the codebase, or manually ignore this for each and every individual report. None of these are really satisfactory ways to address the problem.

EDIT: Its also inconsistent. Flagging the use of 5 but not 1
image

Hi @StingyJack

Thank you for your valuable feedback.
As @Andrei_Epure already mentioned in this answer, we know that it can generate quite some noise and that is why it is not part of SonarWay.
It would require a lot of effort to make the rule less noisy and unfortunately we still do not plan to invest that effort in the near future.
Numbers -1, 0 and 1 are not considered magic numbers as in most cases it is clear why those numbers are used, and if we would consider those numbers as magic numbers too, it would create even more noise.

Its only creating noise because the rule is not programmed correctly and you are not interested in having it exclude attributes (I cant really imagine that would take so long. If the rule is public code and you can point me to it and accept PR’s I will probably be motivated enough to fix it and PR it.)

Otherwise it looks like you have abandoned the rule without marking it as deprecated, leaving a trap for those expecting to get basic magic number checking (“dont use magic numbers” one of the first basic code smells most of us learned about)

Hello @StingyJack - the rule is implemented here.
To avoid that you invest time in implementing something that we will refuse, I suggest your first add your new test cases in TestCases/MagicNumberShouldNotBeUsed.cs, open a PR, we can discuss on the specific test cases and agree what should be the change and then you can help with the implementation.

After a discussion with the team we agreed that attributes should not be excluded altogether. However for well-known APIs like DataMember Order (or any attribute with the “Order” parameter), it makes sense to exclude it to limit the noise, by examining the Order = NNN inside the attribute.

The Rule implementation is currently lightweight and we would like to stay it that way, that is the reason why only cases where the name of the parameter is explicitly mentioned should be covered and not all other cases.

Hi @StingyJack

Thank you for the time spent on this topic. We’ve discussed internally and opened S109 - reduce false positives · Issue #4737 · SonarSource/sonar-dotnet · GitHub to reduce the noise covered by this rule.