CA1819 should not be in the Peformance category

[DmitryMak]

Analyzer

Diagnostic ID: CA1819

Describe the improvement

This is based on a discussion here: https://github.com/dotnet/docs/issues/28424#issuecomment-1877767195

@dceuinton The core idea of this rule is to discourage exposing mutable or cloned data structures through properties. It is a good API design principle to keep properties lightweight in terms of execution performance for it's getter/setter AND return immutable data to justify it being a property of the object. However, as you have mentioned, there are obviously corner cases where you still want to expose mutable data through properties, especially for internal consumers such as in tests. For such cases, it is best to return the data type that best suits your need - it could be an array or a Collection type, as you prefer. If you choose an array, you will need to add a source suppression for each such property, which can be avoided by using the Collection, but honestly either should be fine given that you have already made an explicit design decision for the API to return mutable data.

Based on this response it seems like there should be 2 rules: 1) Performance. Warning about properties that allocate array copies (for tamper proofing the object). Basically the clone part of this response. 2) Design or Usage or Maintainability. To discourage exposing mutable array through properties.

CA1819 is currently in the Performance category. I think this is misleading because it is triggered even if the property doesn't clone the array. So there is no Performance impact. Should the rule be moved to Usage or Maintainability (or even Design) category instead?

Describe suggestions on how to achieve the rule

CA1819 should not be in the Performance category. Maybe a new performance rule can be created that will detect cloning?

[mavasani]

Moving to dotnet\runtime for triaging the requested change in rule category.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Analyzer **Diagnostic ID**: [CA1819](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1819) ### Describe the improvement This is based on a discussion here: https://github.com/dotnet/docs/issues/28424#issuecomment-1877767195 > > @dceuinton The core idea of this rule is to discourage exposing mutable or cloned data structures through properties. It is a good API design principle to keep properties lightweight in terms of execution performance for it's getter/setter AND return immutable data to justify it being a property of the object. However, as you have mentioned, there are obviously corner cases where you still want to expose mutable data through properties, especially for internal consumers such as in tests. For such cases, it is best to return the data type that best suits your need - it could be an array or a Collection type, as you prefer. If you choose an array, you will need to add a source suppression for each such property, which can be avoided by using the Collection, but honestly either should be fine given that you have already made an explicit design decision for the API to return mutable data. > > > Based on this response it seems like there should be 2 rules: > > 1) Performance. Warning about properties that allocate array copies (for tamper proofing the object). Basically the clone part of this response. > > 2) Design or Usage or Maintainability. To discourage exposing mutable array through properties. > > > > CA1819 is currently in the Performance category. I think this is misleading because it is triggered even if the property doesn't clone the array. So there is no Performance impact. Should the rule be moved to Usage or Maintainability (or even Design) category instead? ### Describe suggestions on how to achieve the rule CA1819 should not be in the Performance category. Maybe a new performance rule can be created that will detect cloning?

Author:	DmitryMak
Assignees:	-
Labels:	`area-System.Runtime`, `untriaged`, `needs-area-label`
Milestone:	-

[rampaa]

CA1819 being under the performance category is quite misleading. And it's not clear at all why a Collection<T> is more preferable than a T[] if we do intend to expose a mutable collection. Quite frankly the rule in its current state is more harmful than it's helpful.