Roslyn-analyzers: CA2227:CollectionPropertiesShouldBeReadOnly is raised on immutable collection properties

[airbreather]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.6.2

Diagnostic ID

CA2227:CollectionPropertiesShouldBeReadOnly

Repro steps

Create a new project with the following:

Sample.cs

using System.Collections.Immutable;

public sealed class Sample
{
    public string Name { get; set; }

    // Line in question:
    public ImmutableArray<byte> ImmutableData { get; set; }
}

ConsoleApp0.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netcoreapp2.1</TargetFramework>
    <LangVersion>7.2</LangVersion>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.6.2" />
  </ItemGroup>

</Project>

Expected behavior

I feel like members with types from System.Collections.Immutable, such as the one on the "line in question", should not raise CA2227:CollectionPropertiesShouldBeReadOnly.

I agree with raising it for System.Collections.ObjectModel.ReadOnlyCollection<T>, but the contract of System.Collections.Immutable types is a promise that once you have an instance of an immutable collection, nobody should be able to edit its contents.

Actual behavior

The "line in question" raises CA2227:CollectionPropertiesShouldBeReadOnly.

[mavasani]

@airbreather Note that the violation is being fired for the property itself ImmutableData, not the contents of the immutable data it stores, i.e. a public client can still assign a new immutable array to c.ImmutableData = ..., which is undesirable. The recommendation is to remove the setter for the property or make it non-public. Closing this as by design.. Let us know if you still have any confusion.

[airbreather]

Note that the violation is being fired for the property itself ImmutableData, not the contents of the immutable data it stores, i.e. a public client can still assign a new immutable array to c.ImmutableData = ..., which is undesirable.

@mavasani What makes c.ImmutableData = ... more undesirable than c.Name = ...?

[mavasani]

The rule description and comments in the example clarifies the purpose of this rule, which is to avoid exposing _writable collections_. Feel to suppress or disable this rule in your ruleset if you feel it is fine to expose writable collections for your source code.

[airbreather]

Sorry, I'm not trying to be obstinate, but your responses to this issue surprise me a lot; I can live with having a difference of opinion, and I don't mind suppressing warnings, but I want to make sure that I've at least communicated the point that I'm trying to make before I walk away, since it seems unlikely that I'm going to be the last guy to think that immutable collections deserve special treatment here (even if just in the documentation). Sorry in advance for it being so long (which admittedly probably won't help with "make sure that I've at least communicated the point").

Maybe I'm misunderstanding the problems that this rule is intended to help with, but I actually really like CA2227 otherwise, because in every situation I've seen it raised (outside of these, of course), making the property read-only leads to a slightly-to-significantly better design (IMO), for a few reasons that I care about (and perhaps more that don't immediately come to mind):

• there's just one way to modify what someInstance.TheCollection does, and that's by going through the collection instance itself (which, in combination with CA1002:DoNotExposeGenericLists, gives designers a way to be able to inject custom "on-change" logic without either a source or binary break to consumers)
• changes to the collection will be seen by all consumers that hold a reference to that collection (which kinda matters when it's System.Collections.ObjectModel.ObservableCollection<T>, since without the "read-only property" rule, you'd have to watch for two different kinds of changes instead of one in order to be completely correct)

Immutable collections, on the other hand, are designed such that there's no way to "modify" them; rather, all "changes" need to be made by overwriting the pointer to point to a different one, and the implementations go to incredible lengths to ensure that this is done as efficiently as reasonably possible, given the individual constraints of each one. This, IMO, justifies special-casing those collection types, specifically in this rule, because the same items I identified above are solved at the type system level instead of through soft, ignorable warnings on otherwise working code:

• as above, there's still just one way to modify what someInstance.TheCollection does, it's just the other way around (via overwriting what the property points to).
• every consumer will still see all changes to the collection, because by design, there can't be any (just changes to the property itself)

To give a bigger example below, I understand and agree with flagging this rule on RosterWithMutableList if I had allowed its property to be settable, but the benefit of flagging it on RosterWithImmutableList escapes me. That's a more-or-less idiomatic use of this collection type:

using System.Collections.Immutable;
using System.Collections.ObjectModel;

public sealed class RosterWithMutableList
{
    public string RosterName { get; set; }

    public Collection<string> Names { get; } = new Collection<string>();
}

public sealed class RosterWithImmutableList
{
    public string RosterName { get; set; }

    public ImmutableList<string> Names { get; set; } = ImmutableList<string>.Empty;
}

static class Driver
{
    static void Main()
    {
        var withMutableList = new RosterWithMutableList
        {
            RosterName = "Mutable List",
        };

        withMutableList.Names.Add("Mike");
        withMutableList.Names.Add("Charlie");

        var withImmutableList = new RosterWithImmutableList
        {
            RosterName = "Immutable List",
        };

        withImmutableList.Names = withImmutableList.Names.Add("Mike");
        withImmutableList.Names = withImmutableList.Names.Add("Charlie");
    }
}

---

The rule description and comments in the example clarifies the purpose of this rule, which is to avoid exposing _writable collections_.

I saw that page too, and to me, that page seems to be written only mutable collections in mind, because when they concede that it is occasionally desirable and necessary to replace an entire collection, they identify alternatives to overwriting the collection variable that simply don't make sense here:

From the rule description

A read-only property stops the collection from being replaced, but still allows the individual members to be set. If replacing the collection is a goal, the preferred design pattern is to include a method to remove all the elements from the collection, and a method to repopulate the collection.

From the example code

// If the intent is to replace an entire collection,
// implement and/or use the Clear() and AddRange() methods instead.
collection.SomeStrings.Clear();
collection.SomeStrings.AddRange(newCollection);

There's really no way for the caller to Clear() / AddRange() an ImmutableArray<T> like it recommends, so the only way to make any changes to such a collection is by replacing the instance; following that recommendation to dance around this rule, using something like the following snippet seems like frankly a pointless ritual, given the design of ImmutableArray<T>:

using System.Collections.Generic;
using System.Collections.Immutable;

public sealed class Sample
{
    public string Name { get; set; }

    public ImmutableArray<byte> ImmutableData { get; private set; } = ImmutableArray<byte>.Empty;

    public void ReplaceImmutableData(IEnumerable<byte> newValues)
    {
        this.ImmutableData = this.ImmutableData.Clear();
        this.ImmutableData = this.ImmutableData.AddRange(newValues);
    }
}

Specifically, this dance doesn't really add any value above and beyond having a public property setter, since the containing class can still change the implementation of the property setter to do the exact same pre- and/or post-logic that a "replace the contents" method would do.

To show that I understand (hopefully) how this is supposed to work, I strongly agree with and approve of applying the following pattern to resolve CA2227 in the absence of immutable collections:

using System.Collections.Generic;
using System.Collections.ObjectModel;

public sealed class Sample
{
    private readonly List<byte> data = new List<byte>();

    public Sample()
    {
        this.ReadOnlyData = new ReadOnlyCollection<byte>(this.data);
    }

    public string Name { get; set; }

    public ReadOnlyCollection<byte> ReadOnlyData { get; }

    public void ReplaceReadOnlyData(IEnumerable<byte> newValues)
    {
        this.data.Clear();
        this.data.AddRange(newValues);
    }
}

---

The reason I included public string Name { get; set; } is because ignoring differences in what APIs are available, string is not meaningfully different than an ImmutableArray<char>. Both are essentially no more than length-tagged pointers to the start of a contiguous block of virtual memory containing char values, the contents of which cannot be modified, and so the only way to alter the data pointed to by some variable of either type is by overwriting the variable.
*without resorting to unsafe / System.Runtime.CompilerServices.Unsafe code*

Given that, at a high level and for the purposes of this rule, I can't come up with a convincing argument for why these two types should be treated differently (other than the trivial fact that string doesn't implement System.Collections.ICollection, which doesn't really seem like it makes a huge difference to the philosophy of this rule).

Feel to suppress or disable this rule in your ruleset if you feel it is fine to expose writable collections for your source code.

As I opened this comment with, I really like CA2227, I just think it makes no sense on immutable collections. If I disagreed with the rule in general, then I wouldn't bother either opening an issue over it or writing what's turned into a whole book to defend why I think this change would make it better :grin:.

Suppressing in each instance is, of course, a possibility, and I was planning to start doing that anyway, but I wanted to bring this up more broadly, since it sounded like the team simply hadn't considered the interaction between this otherwise really good rule and collections from System.Collections.Immutable, especially since the documentation doesn't provide any guidance for what to do with these special cases that I imagine may not have even existed when this rule was originally created.

[mavasani]

@airbreather Ah thanks for the detailed explanation. I did misinterpret your request, and this does seem like something we can special case for this rule.