CA1868: Option to limit to concrete types only

[skrysmanski]

Analyzer

Diagnostic ID: CA1868

Describe the improvement

The rule CA1868 currently creates a (dangerous) suggestion in our code. Reason: Some ICollection<T> implementations throw a NotSupportedException in Remove() - for example ReadOnlyCollection<T>.

We have basically this code:

public IList<T> MyList { get; set; }

public void RemoveItem(T myItem)
{
    if (this.MyList.Contains(myItem)
    {
        this.MyList.Remove(myItem);
    }
}

Now, the CA rule suggests that the developer removes the Contains() call - which will result in a guaranteed NotSupportedException if MyList is a ReadOnlyCollection (which was done most likely because of CA1002).

While, in my opinion, our code is... sub-optimal, it's unfortunately a public API which we can't change (easily).

So, at good compromise for us would be to restrict this rule - via an option - to only concrete types (e.g. List<T>).

Side note: The rule's name is currently "Unnecessary call to 'Contains' for sets" which apparently is no longer true because the rule now also checks for IList.

[mavasani]

Tagging @buyaa-n @CollinAlpert

Similar to https://github.com/dotnet/runtime/issues/97293. Moving to dotnet\runtime for triage

[ghost]

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

Issue Details

---

### Analyzer **Diagnostic ID**: [CA1868](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1868) ### Describe the improvement The rule CA1868 currently creates a (dangerous) suggestion in our code. Reason: Some `ICollection` implementations throw a `NotSupportedException` in `Remove()` - for example `ReadOnlyCollection`. We have basically this code: ```c# public IList MyList { get; set; } public void RemoveItem(T myItem) { if (this.MyList.Contains(myItem) { this.MyList.Remove(myItem); } } ``` Now, the CA rule suggests that the developer removes the `Contains()` call - which will result in a guaranteed `NotSupportedException` if `MyList` is a `ReadOnlyCollection` (which was done most likely because of [CA1002](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1002)). While, in my opinion, our code is... sub-optimal, it's unfortunately a public API which we can't change (easily). So, at good compromise for us would be to restrict this rule - via an option - to only concrete types (e.g. `List`). *Side note:* The rule's name is currently "Unnecessary call to 'Contains' for sets" which apparently is no longer true because the rule now also checks for `IList`.

Author:	skrysmanski
Assignees:	-
Labels:	`area-System.Collections`
Milestone:	-

[skrysmanski]

@mavasani Just out of curiosity: Why was this issue moved to the runtime repo?

[chrisoverzero]

Now, the CA rule suggests that the developer removes the Contains() call - which will result in a guaranteed NotSupportedException if MyList is a ReadOnlyCollection

The CA rule is correct in that Remove is meant never to throw based on a presence precondition, but to return true or false depending on whether the value was successfully removed. It should not be gated on a Contains call.

In order to avoid calling mutating operations on a read-only collection type, you should check the value of the IsReadOnly property. It's defined on ICollection<T> for exactly this reason.

The rule's name is currently "Unnecessary call to 'Contains' for sets" which apparently is no longer true because the rule now also checks for IList.

The name aside, the description of the rule states that it looks for ICollection<T>.Remove.

[eiriktsarpalis]

I think the concern of a collection being read-only is orthogonal to this analyzer -- the example would throw an exception regardless of whether its recommendation has been applied.

Side note: The rule's name is currently "Unnecessary call to 'Contains' for sets" which apparently is no longer true because the rule now also checks for IList.

Perhaps the title could be improved, however the documentation for the analyzer does state that it applies to ICollection<T> types as well.

[skrysmanski]

@chrisoverzero

The CA rule is correct in that Remove is meant never to throw based on a presence precondition, but to return true or false depending on whether the value was successfully removed.

Maybe I misunderstood you but that's exactly what ReadOnlyCollection.Remove() does - it throws an exception.

@eiriktsarpalis

I think the concern of a collection being read-only is orthogonal to this analyzer -- the example would throw an exception regardless of whether its recommendation has been applied.

The original code would not throw an exception if the value wasn't present in the list - whereas the "fixed" code throws an exception in this case.

[chrisoverzero]

@skrysmanski It does throw, but not based on a presence precondition.

[eiriktsarpalis]

The original code would not throw an exception if the value wasn't present in the list - whereas the "fixed" code throws an exception in this case.

But it still throws if it isn't. It cannot be claimed that the original implementation was correct and that the fixer broke it.