Property getter returns a fresh copy of a collection or array
ID |
vbnet.performance.no_copy_in_property_getter |
Severity |
high |
Remediation Complexity |
medium |
Remediation Risk |
medium |
Remediation Effort |
low |
Resource |
Performance |
Language |
VB.NET |
Description
Reports a property whose Get accessor returns a newly built copy of a collection or an array:
ToList(), ToArray(), Clone(), or a collection constructed from another
(New List(Of T)(source), New Dictionary(Of K, V)(source)). Returning the backing field
itself, exposing it through AsReadOnly(), or returning a scalar derived from it are all
accepted, and the same expressions inside a method rather than a property are not reported -
a method call sets a different expectation about cost.
Rationale
Callers read a property as if it were a field. Nothing at the call site distinguishes
invoice.Lines from a field access, so the ordinary ways of using one are written without a
second thought - inside a loop condition, indexed repeatedly, iterated more than once - and
every one of those uses now allocates and copies the whole collection. For i = 0 To
invoice.Lines.Count - 1 … invoice.Lines(i) turns a linear walk into a quadratic one, with a
fresh copy per iteration, and nothing in the source hints at it. This is the kind of cost that
does not appear in a profile as one hot method, only as diffuse allocation.
The copy also changes semantics in a way that surprises callers twice over. Because each read
produces a distinct object, mutating what the property returned has no effect on the invoice -
invoice.Lines.Add(line) compiles, runs, and silently does nothing - and two reads are not
reference-equal, so invoice.Lines Is invoice.Lines is False. Callers that cache the
returned collection quietly get a snapshot that stops reflecting later changes.
The following code illustrates the pattern detected by this rule:
Private _amounts As Decimal() = New Decimal(9) {}
' FLAGGED: Property getter returns a fresh copy of a collection or array
Public ReadOnly Property Lines As List(Of InvoiceLine)
Get
Return _lines.ToList()
End Get
End Property
' FLAGGED: Property getter returns a fresh copy of a collection or array
Public ReadOnly Property Tags As String()
Get
Return _tags.ToArray()
End Get
End Property
' FLAGGED: Property getter returns a fresh copy of a collection or array
Public ReadOnly Property TagsCopy As List(Of String)
Get
Return New List(Of String)(_tags)
End Get
End Property
' FLAGGED: Property getter returns a fresh copy of a collection or array
Public ReadOnly Property TagIndex As Dictionary(Of String, Integer)
Get
Return New Dictionary(Of String, Integer)(_index)Remediation
Decide what the property is for and make it say so. If callers should be able to modify the
collection, return the field itself. If the point of the copy was to stop callers modifying it,
express that in the type rather than by copying: return IReadOnlyList(Of T) or
IReadOnlyCollection(Of T), or wrap the field once in a ReadOnlyCollection(Of T) via
AsReadOnly() and hold that wrapper in a field so the property allocates nothing. If a genuine
snapshot really is the intent, make it a method - ToLineList(), ToArray() - so the cost is
visible at the call site and callers stop treating it as a field.
' Before: every read allocates and copies, and callers' mutations are lost
Public ReadOnly Property Lines As List(Of InvoiceLine)
Get
Return _lines.ToList()
End Get
End Property
' After: a wrapper built once, no allocation per read, immutability in the type
Private ReadOnly _linesView As ReadOnlyCollection(Of InvoiceLine) = _lines.AsReadOnly()
Public ReadOnly Property Lines As IReadOnlyList(Of InvoiceLine)
Get
Return _linesView
End Get
End Property
' Or, when a snapshot is the point, make the cost visible
Public Function ToLineList() As List(Of InvoiceLine)
Return _lines.ToList()
End Function