No Copy In Property Getter

ID

csharp.no_copy_in_property_getter

Severity

high

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Efficiency

Language

CSharp

Tags

api-design, collection, efficiency, performance

Description

Reports a property getter whose returned value is a fresh copy of a collection or array, built by ToList(), ToArray(), an array Clone(), or the copy constructor of a mutable collection such as new List<T>(other). Callers treat a property read as field-cheap; a copy per read is neither cheap nor shared.

Rationale

A property reads like a field, so callers use it freely — inside a loop, inside a condition, twice on the same line. When the getter copies, each of those reads allocates and walks the whole collection. A loop that iterates Items.Count times and touches Items in its body becomes quadratic, and nothing at the call site hints at it.

The correctness side is worse than the cost. Two reads of the same property return two different objects, so reference equality between them is false, and a caller who adds to what the property returned mutates a throwaway copy — the change silently disappears. Both bugs are invisible at the call site, because nothing in x.Items suggests that a new list was just built.

Only the copying shapes above are reported, and only when the copy is part of what the getter returns — so a copy cached in a lazily initialised backing field is left alone, while a copy handed straight to a wrapper (lines.ToList().AsReadOnly()) is not. The same call in an ordinary method is fine — a method call does not promise to be cheap.

Two guards keep the report on genuine copies. Clone() counts only when the receiver is an array, so a user type implementing ICloneable is not mistaken for one. And a constructor counts only when its single argument is itself a collection or array: new List<T>(10) reserves capacity for an empty list, and a wrapper such as new ReadOnlyCollection<T>(lines) copies nothing at all.

public class Order
{
    private readonly List<string> lines = new List<string>();
    private readonly int[] codes = new int[4];
    private List<string> cached;

    public List<string> Lines => lines.ToList();          // FLAW — a copy on every read

    public List<string> Copy => new List<string>(lines);   // FLAW — the copy constructor

    public int[] Codes
    {
        get { return (int[])codes.Clone(); }               // FLAW — a copy on every read
    }

    public IReadOnlyList<string> ReadOnlyLines => lines;   // OK — a view, not a copy

    public List<string> Sized => new List<string>(16);     // OK — a capacity, not a copy

    public List<string> Cached
    {
        get
        {
            if (cached == null) cached = lines.ToList();    // OK — copied once, not per read
            return cached;
        }
    }

    public List<string> Snapshot() => lines.ToList();       // OK — a method advertises the cost
}

Remediation

Expose a view instead of a copy: type the property as IReadOnlyList<T> or IReadOnlyCollection<T>, or wrap the backing collection once with AsReadOnly() and hand out that wrapper. If callers genuinely need an independent copy, move the copy into a method whose name says so — Snapshot(), ToLineArray() — so the allocation is visible where it is paid for. If the copy is only there to keep the backing store private, a read-only interface achieves the same protection at no per-read cost.

References