Analyzer: Prefer .Length/Count/IsEmpty over Any()

[CollinAlpert]

This PR adds an analyzer which flags usages of an .Any() call which could be replaced with a Count, Length or IsEmpty check.

My VB tests are currently failing due to an indentation issue. Maybe someone more versed in VB than me could take a look and tell me what I am missing?

Fixes dotnet/runtime#75933

[CollinAlpert]

I ran the ./build.sh script and also dotnet msbuild -t:pack -v:m (both succeed), however the AnalyzerReleases.Unshipped.md file is not being populated. Thus, the CI build fails. Am I missing something?

[buyaa-n]

I ran the ./build.sh script and also dotnet msbuild -t:pack -v:m (both succeed), however the AnalyzerReleases.Unshipped.md file is not being populated. Thus, the CI build fails. Am I missing something?

Ah, that is not added with dotnet msbuild -t:pack -v:m, it should be added by the code fixer of the RS2000 analyzer, if you are using VS it looks like this, I think you can add that row manually too

[CollinAlpert]

@buyaa-n Thanks for the hint! I am using Rider, so I ran dotnet format analyzers --diagnostics RS2000.

[CollinAlpert]

Looks like only my VB tests are failing now. Although I am still clueless as to where that indentation issue is coming from.

[buyaa-n]

Looks like only my VB tests are failing now. Although I am still clueless as to where that indentation issue is coming from.

Can't you repro it locally? I can, somehow fixer produced source has an extra spacing before End Function, though it might not caused by your code:

    Context: Iterative code fix application
content of '/0/Test0.vb' did not match. Diff shown with expected as baseline:
 
 Imports System.Collections.Immutable
 Imports System.Linq
 
 Public Class Tests
     
 Public Function HasContents(array As ImmutableArray(Of Integer)) As Boolean
     Return Not array.IsEmpty
-End Function
+    End Function
 End Class

Looks like VB fixer issue, @mavasani @jmarolf is this a known issue? Is there any work around except adjusting the spacing?

[Youssef1313]

Something looks smelly here. In VB, you don't check the method name and return type at all.
In C#, you don't check the parameter name.

In general, I think this should be a symbol comparison.

NOTE: My suggestions below addresses this.

[buyaa-n]

In general, I think this should be a symbol comparison.

In case we want to restrict to a specific symbol like Enumerable.Any()

[buyaa-n]

Turns out we want to flag for any type, not only IEnumerable.Any()

[Youssef1313]

@mavasani Is this logic correct? The logic in GetReceiverType seems more expensive (involves semantic model calls).

for now, I think this should use GetReceiverType, and if this logic is correct and more efficient, then GetReceiverType should be updated as a follow up. What do you think?

[Youssef1313]

Suggested change

	context.RegisterOperationAction(OnInvocationAnalysis, OperationKind.Invocation);
	context.RegisterCompilationStartAction(context =>
	{
	var anyMethod = WellKnownTypeProvider.GetOrCreate(context.Compilation).GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemLinqEnumerable)
	?.GetMembers(AnyText).OfType<IMethodSymbol>().FirstOrDefault(m => m.IsExtensionMethod && m.Parameters.Length == 1);
	if (anyMethod is not null)
	{
	context.RegisterOperationAction(context => OnInvocationAnalysis(context, anyMethod), OperationKind.Invocation);
	}
	});

[CollinAlpert]

The problem with this approach is that we miss things like https://learn.microsoft.com/en-us/dotnet/api/system.linq.immutablearrayextensions.any?view=net-7.0#system-linq-immutablearrayextensions-any-1(system-collections-immutable-immutablearray((-0)))

[Youssef1313]

@CollinAlpert I see. Then consider not adding anything to compilation start, but check the method correctly.

The code should be language-agnostic (no C#/VB checks), and parameter name shouldn't be checked. Just check method name, whether it's extension method, whether it has a single parameter, and whether its return type is boolean.

[CollinAlpert]

I remember having problems with eliminating language checks, as I believe Roslyn considers Any to be an extension method in C# and not an extension method in VB.

[Youssef1313]

@CollinAlpert That suggestion to use ReducedFrom if MethodKind is ReducedExtension should probably fix the problems you were seeing.

[Youssef1313]

Suggested change

	private static bool IsEligibleAnyMethod(IMethodSymbol method)
	{
	return method.Name == AnyText
	&& method.ReturnType.SpecialType == SpecialType.System_Boolean
	&& method.Language == LanguageNames.CSharp && method.Parameters.Length == 1
	|| (method.Language == LanguageNames.VisualBasic && (method.Parameters.Length == 0 || method.Parameters.Length == 1 && method.Parameters[0].Name == SourceParameterName));
	}

[Youssef1313]

Given that we need to catch any Any method. The check should be:

return method is
    {
        Name: AnyText,
        ReturnType.SpecialType: SpecialType.System_Boolean,
        IsExtensionMethod: true, 
        Parameters: [_]
    };

On the callsite, the passed method should be:

var targetMethod = invocation.TargetMethod;
if (targetMethod.MethodKind == MethodKind.ReducedExtension)
{
    targetMethod = targetMethod.ReducedFrom;
}

// pass targetMethod to IsEligibleAnyMethod at this point.

[Youssef1313]

Suggested change

	public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(
	LengthDescriptor,
	CountDescriptor,
	IsEmptyDescriptor
	);

[Youssef1313]

Looks like VB fixer issue, @mavasani @jmarolf is this a known issue? Is there any work around except adjusting the spacing?

@buyaa-n To me it looks like this is a test authoring issue. The input source code isn't formatted correctly.

[Youssef1313]

Suggested change

	if (invocation.Parent is PrefixUnaryExpressionSyntax prefixExpression && prefixExpression.IsKind(SyntaxKind.LogicalNotExpression))
	if (invocation.Parent.IsKind(SyntaxKind.LogicalNotExpression))

[jmarolf]

Thanks so much for all the great work here. I would like us to

1. Verify that the Any extension method we get is the one defined in the framework by checking the namespace that it is defined in.
2. Verify that the type inherits from either IEnumerable or IEnumerable<T>

With those additional checks in place I would be happy to merge this.

[CollinAlpert]

Thanks for the feedback! I have incorporated the changes and now check that the Any method is System.Linq.Enumerable.Any() and that the corresponding type implements IEnumerable or IEnumerable<T>.

[jmarolf]

Looks good! thanks for all the work here.

[CollinAlpert]

Happy to be able to contribute!

[buyaa-n]

@CollinAlpert the different ids not suggested in the API review and I do not see need of using 3 ids, the id ranges are limited, we better reuse same id for all diagnostics. Sorry for requesting it now, in case you are busy, I will try to contribute to the PR with the change

[CollinAlpert]

No worries, I can change it. However then we need a new message which encapsulates all three properties. Do you have a suggestion?

[buyaa-n]

No worries, I can change it.

Thank you!

However then we need a new message which encapsulates all three properties.

I think you can create each diagnostic with existing different messages, no? Though not sure which title/description will be on doc

        internal const string RuleId = "CA1860";
        internal static readonly DiagnosticDescriptor IsEmptyDescriptor = DiagnosticDescriptorHelper.Create(
            RuleId,
            CreateLocalizableResourceString(nameof(PreferIsEmptyOverAnyTitle)),
            CreateLocalizableResourceString(nameof(PreferIsEmptyOverAnyMessage)),
            DiagnosticCategory.Performance,
            RuleLevel.IdeSuggestion,
            CreateLocalizableResourceString(nameof(PreferIsEmptyOverAnyDescription)),
            isPortedFxCopRule: false,
            isDataflowRule: false
        );

        internal static readonly DiagnosticDescriptor LengthDescriptor = DiagnosticDescriptorHelper.Create(
            RuleId,
            CreateLocalizableResourceString(nameof(PreferLengthOverAnyTitle)),
            CreateLocalizableResourceString(nameof(PreferLengthOverAnyMessage)),
            DiagnosticCategory.Performance,
            RuleLevel.IdeSuggestion,
            CreateLocalizableResourceString(nameof(PreferLengthOverAnyDescription)),
            isPortedFxCopRule: false,
            isDataflowRule: false
        );
       ...

[buyaa-n]

Though not sure which title/description will be on doc

The generated docs just using the 1st diagnostic description and title, for me that is OK as integrated title and/or description doesn't look good to me. For example: I prefer Prefer 'Length' check instead of 'Any()' over Prefer 'Length/ Count/IsEmpty' check instead of 'Any()'. Is that OK for you @jmarolf @mavasani?

For distinguishing the diagnostics in fixer It seems better to use the properties as checking the DiagnosticDescriptiors somehow not triggering the fixer:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseCountProperly.cs

Line 377 in 9b5d130

properties: propertiesBuilder.ToImmutable(),

[jmarolf]

Diagnostic Ids are most important when thinking about how a developer is going to suppress this analyzer. Because I cannot think of a good reason to suppress some of these cases and not others, I agree with @buyaa-n that we should just have once id.

For documentation I think we should have the same Title for all the diagnostics with different descriptions depending on the situation.

[buyaa-n]

The generated docs just using the 1st diagnostic description and title, for me that is OK as integrated title and/or description doesn't look good to me.

Hm, it seems the other title/descriptions are not used/visible anywhere else.

For documentation I think we should have the same Title for all the diagnostics with different descriptions depending on the situation.

Right, it seems we should use one title, is the different descriptions will be used anywhere?

How is this title sounds? Avoid using 'Enumerable.Any()' extension method

And a description could be something like: Prefer using 'IsEmpty', 'Count' or 'Length' properties whichever available, rather than calling ‘Any()'. The intent is clearer and is more performant than 'Enumerable.Any()' extension method.

[buyaa-n]

LGTM thanks!

[cdrfenix]

The main discussion thread its closed, but this is a bad change, at least its a warning we can disable, but this is favoring performance over legibility.

[stephentoub]

The main discussion thread its closed, but this is a bad change, at least its a warning we can disable, but this is favoring performance over legibility.

It's not even a warning, it defaults to info / suggestion. If you're seeing it as a warning, it's because you're doing something to bump up its severity.

(And legibility is in the eye of the beholder; personally, I find explicit use of the properties more readable/understandable.)