Exclude Enumerable.Count<TSource>(IEnumerable<TSource>)

[paulomorgado]

Solves dotnet/roslyn#37959

[sharwell]

Can you clarify the exact analyzer change here?

[sharwell]

📝 I would prefer to keep all of these

[paulomorgado]

The change is to exclude Enumerable.Count<TSource>(IEnumerable<TSource>) from the analysis and fix.

Only this method. The overload with a predicate and all other applicable methods from the other extensions are still being targeted.

The difference between the C# and VB extension method invocation operations required to split the analyzer into language specific versions.

[mavasani]

The change is to exclude Enumerable.Count(IEnumerable) from the analysis and fix.

@paulomorgado I think there is some miscommunication here. I agree with @sharwell that we should not remove Enumerable.Count<TSource>(IEnumerable<TSource>) from analysis and fix. That is possibly the most commonly used overload of the Count API and hence most useful API to flag in CA1827. My suggestion was to do either of the following:

1. Keep CA1827 as-is given the fact that the allocation issue with Any() has been fixed in .netcore 3.0 and majority of the users will not worry about that extra allocation given the benefit of going from O(n) to O(1) operation.
2. Change CA1827 to ensure there is no overlap with CA1829 (Use Count/Length instead of Count()) for all overloads of Count():
   1. Check if the receiver of Count() invocation implements ICollection or ICollection<T>. If yes, flag CA1829.
   2. If not, then execute the current logic for CA1827 to check if the result of the invocation is being compared with 0 or 1, and if so flag CA1827.

I don't think we should completely exclude Enumerable.Count<TSource>(IEnumerable<TSource>) from the analysis and fix.

[paulomorgado]

I'm getting confused over all this.

My understanding of @stephentoub's comments around all this is a run time issue.

With this change, if you can for sure know at compile time that Count() is being invoked on an array or a type implementing ICollection or ICollection<T>, this change will not flag it and it will be flagged by CA1829. My proposal is also to add Any() to it.

If you cannot determine at compile time that Count() is not being invoked on an array or a type implementing ICollection or ICollection<T>, you might be offering to make the program worst than it was.

For example, if someone uses Enumerable.AsEnumerable() on a List<T> thinking it's protecting it against tampering (it isn't) it will no longer be an implementation of ICollection or ICollection<T> at compile time, but it will at run time. In fact, it's the whole purpose of Enumerable.AsEnumerable().

The benchmarks on dotnet/corefx#40377 show that the new version (as you said, @mavasani, only for .NET 5.0) is equal or better on allocations but it performs worst in 6 out of 9 use cases. And that's a comparison between new and old Any(), not between Any() and Count().

Is my understanding correct, @stephentoub?

On a side note, ImmutableArrayExtensions does a better job than this at compile time for ImmutableArray<T> and the same could/should be done for ICollection or ICollection<T>.

[mavasani]

If you cannot determine at compile time that Count() is not being invoked on an array or a type implementing ICollection or ICollection, you might be offering to make the program worst than it was.

Worse in the sense of additional allocation, but you lose out on a a diagnostic that can improve your code go from O(n) operation to O(1). As @sharwell mentioned, the latter is much more valuable for majority of programmers. Additionally, the allocation concern in Any() is addressed in newer versions of netcore after stephen's fix.

1. The current state of your analyzer prior to this PR is acceptable for majority of customers, given Sam's point here Fix CA1827 (DoNotUseCountWhenAnyCanBeUsed) violations in Roslyn.sln roslyn#37959 (comment) and corefx fix Use Count in Enumerable.Any if available corefx#40377 - I think this is @sharwell's preferred state.
2. If you make the implementation more tighter by bailing out in CA1827 for cases where we know at compile time that CA1829 will fire, it makes it even more closer to acceptable - less noise from seeing both CA1827 and CA1829 on same code + lesser cases that accepting CA1827 leads to additional allocation - this is my preferred state, though I am fine with 1. as well.
3. If this PR goes in the current form where Count() overload without a predicate is not flagged at all, but only predicated versions are flagged, usefulness of CA1827 goes down drastically - both @sharwell and I have reservations about this path forward.

[paulomorgado]

3. If this PR goes in the current form where Count() overload without a predicate is not flagged at all, but only predicated versions are flagged, usefulness of CA1827 goes down drastically - both @sharwell and I have reservations about this path forward.

This should already be flagged by CA1829.

What I'm going to add to CA1829, is the same for Any(). Kind of the opposite of CA1927 - "Dont use Any()! Use Length/Count!"

I don't have metrics to know if Count(predicate)/Any(predicate) are used more than Count()/Any(), but I still think there's usefulness left in CA1827.

This is, at this point, mostly a conceptual/decision making discussion as, in one analyzer or the other, the code is all done. But I think it's a discussion worth having.

[mavasani]

This should already be flagged by CA1829.

void M(List<T> list, IEnumerable<T> enumerable)
{
   if (list.Count() != 0) // This should only flag CA1829 - use property named Count
   { }
   if (enumerable.Count() != 0) // This should flag CA1827 - use Any()
   { }
}

This PR removes the CA1827 from the 2nd case above, which according to me makes this rule confusing and lose much of its value. I agree with Sam that we should not keep altering the rule based on API implementation details (which are likely to change again in future), when the drawback is just an additional allocation on certain code paths and that too has been fixed now.

This is, at this point, mostly a conceptual/decision making discussion as, in one analyzer or the other, the code is all done. But I think it's a discussion worth having.

Yes, that is correct. I am going to mark this PR as blocked until we conclude that the change in this PR is desirable.

[mavasani]

As per the review comments

[stephentoub]

and that too has been fixed now

On .NET Core post-3.0. it will not be "fixed" on .NET Framework, nor backported to NET Core 3.0.

[mavasani]

On .NET Core post-3.0. it will not be "fixed" on .NET Framework, nor backported to NET Core 3.0.

I think that is totally acceptable.

[sharwell]

I'm getting confused over all this.

My understanding of @stephentoub's comments around all this is a run time issue.

Manish and I disagree with Stephen's qualified objection to certain cases of this rule based on runtime behavior. We don't disagree with the arguments themselves (it's easy to prove they are correct in certain constructed scenarios), but rather disagree on whether those arguments are strong enough to impact the way this rule would be used in the majority of real-world applications. Even if this rule only ran on .NET Framework and no changes were ever made to the runtime behavior, we would want the rule to keep its original form.

[udlose]

I'm getting confused over all this.

My understanding of @stephentoub's comments around all this is a run time issue.

@paulomorgado - can you please provide a link to @stephentoub 's comments that you are referring to?

[paulomorgado]

Sorry @udlose. It was a private talk.