Count > 0 normalized to Any should also work on ICollection #35381
Conversation
| @@ -70,7 +70,7 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression) | |||
| }, | |||
| Right: ConstantExpression { Value: 0 } | |||
| } | |||
| when (member.DeclaringType.GetGenericTypeDefinition().GetInterfaces().Any( | |||
| when (member.DeclaringType.GetGenericTypeDefinition() == typeof(ICollection<>) || member.DeclaringType.GetGenericTypeDefinition().GetInterfaces().Any( | |||
There was a problem hiding this comment.
Can invoke GetGenericTypeDefinition() only once:
when member.DeclaringType.GetGenericTypeDefinition() is Type genericTypeDefinition
&& (genericTypeDefinition == typeof(ICollection<>)
|| genericTypeDefinition.GetInterfaces()
.Any(x => x.IsGenericType && x.GetGenericTypeDefinition() == typeof(ICollection<>)))There was a problem hiding this comment.
Indeed we can. Just fixed that now
There was a problem hiding this comment.
Looks great, thanks @ChrisJollyAU. See suggestion below, once that's fixed and the CI works again I'll merge.
|
@Roj Just thinking on this, I might have a look around at some stage to see if there is any other |
|
@ChrisJollyAU good idea, I wouldn't be surprised if we made this mistake in other places... |
|
Just seen some others do it like this Shall I fix this one to be like that? |
|
@ChrisJollyAU I've disabled auto-merge here, so we can maybe push some more related fixes to this PR.
I guess it depends on the specific case - not every usage of GetInterfaces() is necessarily wrong... BTW if this is a common thing, we may want to simply extract out a GetInterfacesInclusive() extension method which does this. We already have GetBaseTypesAndInterfacesInclusive() on SharedTypeExtensions, we could add GetInterfacesInclusive() there... |
|
Just quick search for Of those under In the end it looks like theres only the ones from |
|
Yeah, that one looks legit, though quite contrived - that seems to strip away up-casts from an interface to a base interface, so the change would also strip away no-op "up-casts" from the interface to itself. The compiler most probably never generates such a useless node, but we could include it for completeness. |
|
changing it to On this quick look, it doesn't appear that there are any that would need to be changed |
|
OK, thanks @ChrisJollyAU! |
When normalizing
.Count > 0or.Count != 0to.Anyit wasn't picking up theICollectioninterface if the current declaring type was already anICollectionFixes #35365