No Collection Passed To Own Method

ID

csharp.no_collection_passed_to_own_method

Severity

high

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Reliability

Language

CSharp

Tags

collection, reliability, suspicious-argument

Description

Reports a collection or sequence operation that receives, as an argument, the collection it is called on. Set and sequence combinators degenerate when both operands are the same collection, so the result stops depending on the contents.

Rationale

items.Union(items) hands back items. items.Except(items) is always empty. set.ExceptWith(set) always clears the set, set.UnionWith(set) always does nothing, and items.SequenceEqual(items) is always true. None of these is a plausible intent, and none will fail loudly: the code compiles, runs, and quietly produces a constant where a real comparison or combination was expected. The usual cause is copy-paste or an autocompletion that picked the nearest name — the second operand was meant to be the other collection.

AddRange and Concat are excluded on purpose. Passing a collection to either genuinely means "append a second copy of myself", which is unusual but well defined and occasionally what the author wants.

Receiver and argument must both be name paths: an identifier, this, or a chain of member accesses over either, such as this._tags or _cache.Keys. A call or an indexer on either side is not reported, because its two evaluations need not give back the same collection. Two plain identifiers are compared by resolved symbol whenever the symbol table has both, so two same-named variables in different scopes are not confused with each other; longer paths are compared by their parsed tokens.

public class Merger
{
    private HashSet<string> _tags = new HashSet<string>();
    private Dictionary<string, string> _cache = new Dictionary<string, string>();

    public void Combine(List<string> left, List<string> right, HashSet<string> tags)
    {
        var all = left.Union(left);            // FLAW — the union of a list with itself
        var none = left.Except(left);          // FLAW — always empty
        tags.ExceptWith(tags);                 // FLAW — always clears the set

        var same = this._tags.SequenceEqual(this._tags);  // FLAW — always true
        var empty = _cache.Keys.Except(_cache.Keys);      // FLAW — always empty

        var merged = left.Union(right);        // OK — two different operands
        left.AddRange(left);                   // OK — doubling a list is a real operation
        var twice = left.Concat(left);         // OK — repeating a sequence is a real operation
    }
}

Remediation

Replace the argument with the collection that was meant to be the second operand. If the degenerate result really is what the code wants, write it directly and say so: an empty sequence instead of Except against itself, true instead of SequenceEqual against itself, Clear() instead of ExceptWith against itself. The direct form states the intent and cannot be mistaken for a bug.

References