-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Analyzer] Prefer .Length/Count/IsEmpty over Any() #75933
Comments
Tagging subscribers to this area: @dotnet/area-system-collections Issue DetailsIf (I really thought we had this analyzer, but I can't find any record of it now.)
|
Severity: Info |
I'd like to do this one. |
Excellent. Thanks! |
I just found this Analyzer. Should I rather extend that one's functionality or create a new Analyzer only for |
If extending that analyzer results in cleaner code overall, then I think that's probably fine, e.g. if you'd otherwise be duplicating the majority of that analyzer's code and the new analysis requires just a few additional lines. But regardless of whether it's augmenting an existing analyzer or adding a new one, it should have its own new diagnostic ID; which component raises that ID is an implementation detail. |
Isn't there some way to make the runtime performance of Default interface implementation plus inlining attribute? Some sort of "stronger" inlining attribute? Enhancing source generators? |
I don't believe |
I guess this is more about what you're familiar with then. To me,
Is there a common library or other platform that uses a
Yeah I was hoping for something to make compile it down to the same runtime cost no matter which syntax is used. I guess that would have to be in the compiler proper at this point and probably not considered to be a big enough gain for the complexity there. A fixer like this pushes the complexity cost out by scattering it as code churn across the vastly greater quantity of codebases. |
Would it work to try using |
Could |
In the same vein, can't |
For new API requests, please open new issues. Thanks. |
I get that a code analyzer is a lot easier to implement than then improving the runtime performance of CA1827 plus this new rule will result in these instruction to programmers:
obviously in the background the rules are actually:
I general I have been using |
@stephentoub, my comment wasn't exactly a request, but rather exactly what @Applesauce314 is talking about. The proposed analyzer would be confusing considering CA1827 and the fact that different, but similar in theme, types would have different best practices: In my opinion this analyzer shouldn't exist if a common, objective and performant API is possible. |
I agree that discouraging Any() in favour of Length and Count hurts readability and hides real user intent. It is also reasonable that there are implementations of collection interfaces where computing Count is more expensive than just checking if a list is empty. E.g. think of some kind of linked list implementation. So hiding real attempt to get performance gains in limitted cases is not a good solution. On the other hand checking if a collection is empty or not is a very common scenario that should have very low overhead in performance terms. So if there is no easy way to teach the compiler/runtime to optimize the call away, I think having a dedicated IsEmpty property is the best solution overall and would also solve the issue arround confusing and seemingly conflicting rules. To this end I have created an API proposal to get the discussion started: #79769 |
@aKzenT, I'm with Stephen Toub on this one:
Moreover, the Length or Count property gives a clear hint that this information is directly accessible. As a counterexample, an
Any such implementation that I have seen exposes a Wouldn't a general |
This wouldn't be even an issue in an ideal world, right? The performance of The general approach of using analyzers to work around usability issues in the frameworks seems wrong to me. |
Impossible? No, but we're then marching down a path of adding one for every collection type, which isn't viable, and in the limit actually is impossible since we don't control the implementations of nor can we reference most collection implementations in the world.
Why? Please share the implementation of |
The alternatives appear to be worse:
You're absolutely right. I should've written public bool Any() => Count!=0; to the most common collection types appears to be a simple solution, which appears to have only upsides:
In my view, an analyzer should be only considered if there's not easy way to make the framework do "the right thing" otherwise. Otherwise we should consider a "replace all your code with hand-optimized assembly" analyzer 😉 |
Regardless of this discusson, that's asking for perf problems as your consumers assume Count is cheap. And if you're your only consumer and you don't like the suggestion offered by this diagnostic id, you can turn off the suggestion.
It does not have only upside. For example, how do you propose handling Besides that, every single collection needs a new Any() method? That's not upside. And why stop there? What about the rest of LINQ? Add each of those hundreds of methods to every collection?
You've cited usability as a benefit here. I continue to disagree with that notion. I understand some devs think that
It's efficient if the collection exposes it. The vast majority of collection types will not, nor will any new methods be added to the core collections in past releases, in netstandard2.0, etc. And now anyone reading code needs to know the exact type of the variable in question and know whether
No one is being asked to do anything. The tool is making a suggestion. Not a warning, not an error, a suggestion. If you don't like the suggestion, you can ignore it, disable it. The suggestion isn't even made in command-line tooling and builds. (And if everyone who defines a collection needs to also add an |
Right, but in practice there are cases like
Here I wasn't talking about the specifics of Personally, I find No solution will work perfectly all the time, but shouldn't we try to remove sharp corners and focus on the pit of success?
Not every single collection. Commonly used collections should have a performant Ideally, all of LINQ would be performant. There are ways to achieve this (ask Joe Duffy or check https://github.com/NetFabric/LinqBenchmarks). |
If
.Any()
is used on an expression strongly-type as anICollection<T>
or an implementing type with aCount
property (or aT[]
with aLength
property), the code should be using.Count != 0
/.Length != 0
instead (or using!IsEmpty
if it has anIsEmpty
property).(I really thought we had this analyzer, but I can't find any record of it now.)
The text was updated successfully, but these errors were encountered: