Skip to content
bagley2014 edited this page Oct 29, 2021 · 3 revisions

Avoid ToReadOnlyCollection in constructors

The ReadOnlyCollection<T> returned by Libronix.Utility.EnumerableUtility.ToReadOnlyCollection does not guarantee that a new read-only collection is created; it may just return a wrapper around an existing List<T>.

The following pattern in a constructor is dangerous because it may save a reference to a List<T> that a caller could modify:

public class Data
{
    public Data(IEnumerable<T> values)
    {
        Values = values.ToReadOnlyCollection();
    }

    public ReadOnlyCollection<T> Values { get; }
}

Suggested Fixes

  • Use .ToList().AsReadOnly() to force a new, private List to be created.
  • Alternatively, change the argument to a ReadOnlyCollection or IReadOnlyList in situations where avoiding copying the collection unnecessarily is worth the risk that the caller passes a wrapper around a collection it later mutates.

Limitations

Constructions such as m_field = arg.Where(pred).Select(x => new { }).ToReadOnlyCollection(); will be flagged as violations of this rule, even though they clearly generate a private List instance. Use .ToList().AsReadOnly() in this situation to suppress the warning.

Clone this wiki locally