New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CA1827 (DoNotUseCountWhenAnyCanBeUsed) violations in Roslyn.sln #37959
Comments
You'll want to be cautious with this one. In both .NET Framework and .NET Core, It calls into question the validity of the added rule. |
Thanks @stephentoub. Tagging @paulomorgado who added this rule very recently. |
@stephentoub, I would say that By the way, |
As I noted, that might be something to do for .NET Core. It will not be changed in .NET Framework. So the rule is problematic on .NET Framework at the very least. There are also downsides to changing it, which is why it hasn't been done thus far, e.g. the extra interface checks add overhead, which is less impactful when the alternative is a potentially long iteration through every element, and more impactful when at most one element will be considered. |
@stephentoub, are your comments only related to |
Just the ones that don't take a predicate. |
So. this is still a valid analyzer/fixer for those that take the predicate, right? |
Yes |
Sorry, one more question: this also doesn't apply to |
Tagging @sharwell for his views. |
The analyzer attempts to avoid an O(n) amount of work during the switch from Count() to Any(). This is a more acute problem for general-purpose code than avoiding an O(1) allocation on a fast path. I would further argue that code sensitive enough to performance where the special-case behavior matters would be using more concrete types than |
And this will catch most of them using |
@paulomorgado - can we change the analyzer to do the following:
This should address @stephentoub's concern while also avoiding both the above diagnostics on certain code patterns, when clearly accepting the first diagnostic/fix is preferred. |
FYI, I did submit dotnet/corefx#40377, however a) this will be post-.NET Core 3.0, and b) it will not change .NET Framework. |
Given @stephentoub's fix and @sharwell's point about performance sensitive code using more concrete types then Going forward, we will involve the corefx team in the early design/triage phase for any new analyzers being added to Microsoft.NetCore.Analyzers and Microsoft.NetFramework.Analyzers |
It's up to you. I'm more conservative when it comes to code change recommendations, and so unless something is strictly better 99.9% of the time, I hesitate to suggest something that someone might blindly auto-fix. In this case, on .NET Framework and on .NET Core 3.0 and earlier, it is still the case that Any() will allocate in many situations where Count() won't, and it'll be impossible for the analyzer to always know the right answer, nor whether Count() will be O(N) or O(1), nor whether the cost of an allocation from Any() will be more or less impactful than iteration that might happen as part of Count(). It's fine if you want to keep the analyzer, but I suggest then that the associated text/description/docs make it clear that it's a trade-off. |
We can exclude from this analyzer/fixer the use cases covered by pull/2736 and add replacement of Performance wise, it should be the best compromise. As for users, it might be a bit more confusing, but they might learn from it. When the framework changes, we'll change the analyzers/fixers accordingly. |
Attempting to migrate Roslyn.sln to latest version (2.9.4) of FxCop analyzer packages leads to a bunch of CA1827 violations where the code is using
Count()
Linq invocation whenAny()
could be used. This CA rule was added in 2.9.4 analyzer package. A ruleset suppression would be added for this rule and this issue tracks fixing these violations and removing the ruleset entry.The text was updated successfully, but these errors were encountered: