3

SonarQube identifies a bug in my code based on rule csharpsquid:S2114 and I cannot see how this applies to the code. The code in question concatenates the values IEnumerable from a dictionary Dictionary<string,object> attributes one time after another like so:

var valuesTwice = attributes.Values.Concat(attributes.Values).ToArray();

The rule states:

Collections should not be passed as arguments to their own methods

Passing a collection as an argument to the collection's own method is either an error - some other argument was intended - or simply nonsensical code.

Further, because some methods require that the argument remain unmodified during the execution, passing a collection to itself can result in an unexpected behavior.

However, as I understand it, this is not actually a method on the list attributes.Values itself, but an extension method, so the code in question could be written (less elegantly, in my opinion) like the variant added below (the original included for comparison):

var valuesTwice = attributes.Values.Concat(attributes.Values).ToArray();
var valuesTwice = Enumerable.Concat(attributes.Values, attributes.Values).ToArray();

From reading the documentation page on Concat, I don't see how the statement can have any unintended effects on the attributes Dictionary, which is what I understand the rule is to guard against. It is not like the Concat modifies the Values structure in the Dictionary.

My only explanation is that SonarQube's matching rules confuse this extension method as a method on the actual collection itself. Is this Concat safe to do, and I can just ignore this application of the rule, or am I missing something?

As a bonus question: Is there an alternative (elegant) way to achieve the same effect, producing an array where the Values are repeated?

Update/Clarification: My problem with the SonarQube description is the lack of distinction between passing a collection to a method on the collection itself (which may have unintended side effects that I think the rule is about) and passing a collection as both arguments to an extension method, see stackoverflow.com/questions/100196/net-listt-concat-vs-addrange. My reasoning is that the Concat method does not alter the collection, but returns a new collection (or rather, enumerable) combining the two passed collections. I question if Enumerable.Concat is an appropriate match for the rule, where I would agree that AddRange method is: feeding a collection "with its own tail" by passing it to AddRange, I would have a problem with, since it modifies the collection.

MikkelRJ
  • 181
  • 8
  • 3
    This sounds like a bug in sonarqube – Ackdari May 04 '20 at 08:10
  • Doubling a list is a code smell. SonarCube's purpose is to detect such smell and block it. The best course of action is to remove the need for duplication in the first place. – Agent_L May 04 '20 at 08:43
  • @Agent_L In our use context, we have to have the list doubled. I don't think you can generalize that doubling a list is a code smell. For example, you might have a third party component that needs the data in this way. – MikkelRJ May 04 '20 at 12:07
  • 2
    There's nothing inherent to extension methods that renders them *unable* to modify their arguments. Are you suggesting that the analyzer hard code the semantics of *particular* extension methods that it knows are "safe"? – Damien_The_Unbeliever May 04 '20 at 12:59
  • @Damien_The_Unbeliever That sounds like an interesting avenue to help SonarQube give more accurate recommendations and avoid "false positives". – MikkelRJ May 04 '20 at 13:47
  • @MikkelRJ I'm challenging the context exactly. If you have a 3rd party component that smells it makes your code smelly. "x.Concat(x)" really looks like a mistake on the first glance. Sonar is just doing it's job. It's not a bug, it's a feature. This is not a false positive. I'd make an extension method `Duplicate()` or eventually `Enumerable.Repeat(attributes.values, 2).SelectMany` if you want to control amount . Because you are doing what no sane programmer would do out of their own volition, it deserves good documentation. – Agent_L May 04 '20 at 15:08
  • @Agent_L To be specific, the use case is passing arguments to an SQL query that needs them twice in that particular order. Whether this is sane or not, or whether you and I agree on what to consider a code smell, is not my question. Sonarqube considers this a *bug*, which typically is something that causes a program to "produce an incorrect or unexpected result, or to behave in unintended ways". This is what I question. – MikkelRJ May 05 '20 at 04:22
  • @MikkelRJ Ok, what I mean is that in 99% cases 'x.Concat(x)' is a typo where you've meant 'x.Concat(y)'. That constitutes "error - some other argument was intended". That's the reasoning of SQ. You've found this one place when this rule doesn't apply. But the cost of doing `var valuesTwice` once here is small compared to the cost of missing a typo in one of the 99 'x.Concat(y)' places. – Agent_L May 05 '20 at 11:02

1 Answers1

0

The reasoning behind the check in SonarQube can be found in their JIRA (and C# version, and JIC github PR). And it makes sense in quite a lot of scenarios.

As for bonus question, it is matter of preference and, maybe performance(if the code will be called a lot), but you can try something like:

var x = new[] {1,2};    
var times = 2;
x.SelectMany(i => Enumerable.Repeat(i, times)).ToList();

It will produce not the same output though, your code will return [1, 2, 1, 2] and this will - [1, 1, 2, 2].

You can also create your own extension method:

    public static IEnumerable<T> RepeatElements<T>(this IEnumerable<T> en, int times)
    {
        // check arguments
        var inner = en is ICollection<T> col ? col : en.ToList();
        foreach (var elem in inner)
        {
            for (int i = 0; i < times; i++)
            {
                yield return elem;
            }
        }
    }

Reorder cycles if return order of elements is important.

Guru Stron
  • 102,774
  • 10
  • 95
  • 132
  • Thank you much. I don't quite feel the question answered, though. My problem with the linked description is the lack of distinction between passing a collection to a method on the collection itself and passing a collection as both arguments to an extension method (see https://stackoverflow.com/questions/100196/net-listt-concat-vs-addrange). My reasoning is that the Concat method does not alter the collection, but returns a new collection (or rather, enumerable) combining the two passed collections. I am not sure the rule should apply both to AddRange and (the extension method) Concat. – MikkelRJ May 04 '20 at 12:36
  • I didn't comment on the "bonus", the alternatives: I like the succinct first alternative. In our specific use context, the ordering is important, produced by concatenating the values on the values. – MikkelRJ May 04 '20 at 12:43
  • 1
    " lack of distinction between passing a collection to a method on the collection itself and passing a collection as both arguments to an extension method" In this concrete case I agree that this check does not make much sense, but there are usecases where it does for extension methods: for example `Union` or `Intersect`. So it would not be correct to distinguish based on is method an extension or "instance" one – Guru Stron May 04 '20 at 12:59