CA1860 should be reported for derived types as well (analyzer fix with performance improvements) #50397
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #50398
Changes
PreferLengthCountIsEmptyOverAnyAnalyzer
so thatCA1860 is reported not only if the type itself has
IsEmpty
/Count
/Length
property, but also if that type is derived from a type with one of those properties, e.g. a custom collection type. I also tried to simplify code and remove some code duplication.Performance improvements
I'm also suggesting some performance improvements that I included in this PR:
Currently the analyzer checks if an invocation is inside an expression tree before checking that it's actually
Enumerable.Any
call. I moved that check, so that it's not performed for each and every invocation expression, but only forAny
calls.Currently the analyzer checks if the type of an expression in
Any
call isIEnumerable<T>
orIEnumerable
. This check seems to be redundant because the analyzer makes a symbol check to make sure that it's exactlyEnumerable.Any
call, so the type of an expression should automatically beIEnumerable<T>
and the check can be removed.Currently the analyzer makes
ITypeSymbol.GetMembers
andEnumerable.OfType
calls for each of the three property checks while checking if the type hasIsEmpty
,Count
orLength
property. Each call apparently allocates anImmutableArray
with all type members, and also an enumerator returned byOfType
. I'm changing this to callITypeSymbol.GetMembers(name)
method which will return a singletonImmutableArray.Empty
instance if there's no such property on the type, and will allocate a single element array if the property is found.Currently when reporting an error the analyzer calls
ImmutableDictionary.CreateBuilder
and then callsToImmutable
on builder, this allocates both builder and dictionary. As it needs to add just one element to dictionary, it can be added to an empty instance thus avoiding builder allocation.I moved the check for
Count
property before the check forLength
property. I believe it's more likely that a type hasCount
property because all collection types haveCount
property while only arrays haveLength
. This should save a check in most cases.