dotnet / roslyn-analyzers

MIT License
1.6k stars 468 forks source link

Add analyzer for returning private array/list #3841

Open goyzhang opened 4 years ago

goyzhang commented 4 years ago

Code like

        class Foo
        {
            private List<byte> _deviceIDList = new List<byte>();
            public List<byte> GetDeviceList()
            {
                return _deviceIDList;
            }
        }

is not safe since caller can modify internal list and foreach can introduce multi-threading exceptions since it passes the internal reference to a public interface. This is actually similar to CA1819, which is a backing field & get method behind the scene. Possible to add an analyzer to detect this?

Evangelink commented 4 years ago

@mavasani Any opinion on this suggestion? I am not a big fan of the rule anyways because:

1/ Most collections have a similar problem (multi-threading + write-access)

2/ Having a getter doing a clone or collection copy is IMO a wrong practice. I expect a property getter/setter to be a fast access but doing a collection copy can lead to a slow (to very slow) access.

mavasani commented 4 years ago

Yes, I am unsure about this one too, this is likely to be an extremely noisy rule.

goyzhang commented 4 years ago

Could it just be a warning with some restrictions on the return type? @mavasani talking about solving this issue, for example, if you return an IEnumerable then it's a safe design and no warning pops up. Of course this also applies to a return type that is a defensive copy, a valuetype or ImmutableCollection (records in C#9?), etc. But yea... I can see the definition of the warning triggers might be unclear and noisy.