Skip to content
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() #6236

Merged
merged 22 commits into from Jan 30, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -88,10 +88,9 @@ public static void AnalyzeInvocationForIgnoredReturnValue(OperationAnalysisConte
return;
}

INamedTypeSymbol? type = invocation.GetReceiverType(context.Compilation, beforeConversion: false, context.CancellationToken);

// If we're not in one of the known immutable types, quit
if (type is not null && type.GetBaseTypesAndThis().Any(immutableTypeSymbols.Contains))
if (invocation.GetReceiverType(context.Compilation, beforeConversion: false, context.CancellationToken) is INamedTypeSymbol type
&& type.GetBaseTypesAndThis().Any(immutableTypeSymbols.Contains))
{
context.ReportDiagnostic(invocation.CreateDiagnostic(DoNotIgnoreReturnValueDiagnosticRule, type.Name, methodName));
}
Expand Down
@@ -0,0 +1,97 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System.Composition;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.NetCore.Analyzers.Performance;
using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory;

namespace Microsoft.NetCore.CSharp.Analyzers.Performance
{
[ExportCodeFixProvider(LanguageNames.CSharp), Shared]
public sealed class CSharpPreferLengthCountIsEmptyOverAnyFixer : PreferLengthCountIsEmptyOverAnyFixer
{
protected override SyntaxNode? ReplaceAnyWithIsEmpty(SyntaxNode root, SyntaxNode node)
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
{
if (node is not InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax memberAccess } invocation)
{
return null;
}

var expression = memberAccess.Expression;
if (invocation.ArgumentList.Arguments.Count > 0)
{
expression = invocation.ArgumentList.Arguments[0].Expression;
}

var newMemberAccess = MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
expression,
IdentifierName(PreferLengthCountIsEmptyOverAnyAnalyzer.IsEmptyText)
);
if (invocation.Parent.IsKind(SyntaxKind.LogicalNotExpression))
{
return root.ReplaceNode(invocation.Parent, newMemberAccess.WithTriviaFrom(invocation.Parent));
}

var negatedExpression = PrefixUnaryExpression(
SyntaxKind.LogicalNotExpression,
newMemberAccess
);

return root.ReplaceNode(invocation, negatedExpression.WithTriviaFrom(invocation));
}

protected override SyntaxNode? ReplaceAnyWithLength(SyntaxNode root, SyntaxNode node)
{
return ReplaceAnyWithPropertyCheck(root, node, PreferLengthCountIsEmptyOverAnyAnalyzer.LengthText);
}

protected override SyntaxNode? ReplaceAnyWithCount(SyntaxNode root, SyntaxNode node)
{
return ReplaceAnyWithPropertyCheck(root, node, PreferLengthCountIsEmptyOverAnyAnalyzer.CountText);
}

private static SyntaxNode? ReplaceAnyWithPropertyCheck(SyntaxNode root, SyntaxNode node, string propertyName)
{
if (node is not InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax memberAccess } invocation)
{
return null;
}

var expression = memberAccess.Expression;
if (invocation.ArgumentList.Arguments.Count > 0)
{
// .Any() used like a normal static method and not like an extension method.
expression = invocation.ArgumentList.Arguments[0].Expression;
}

static BinaryExpressionSyntax GetBinaryExpression(ExpressionSyntax expression, string member, SyntaxKind expressionKind)
{
return BinaryExpression(
expressionKind,
MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
expression,
IdentifierName(member)
),
LiteralExpression(
SyntaxKind.NumericLiteralExpression,
Literal(0)
)
);
}

if (invocation.Parent.IsKind(SyntaxKind.LogicalNotExpression))
{
var binaryExpression = GetBinaryExpression(expression, propertyName, SyntaxKind.EqualsExpression);

return root.ReplaceNode(invocation.Parent, binaryExpression.WithTriviaFrom(invocation.Parent));
}

return root.ReplaceNode(invocation, GetBinaryExpression(expression, propertyName, SyntaxKind.NotEqualsExpression).WithTriviaFrom(invocation));
}
}
}
3 changes: 3 additions & 0 deletions src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
Expand Up @@ -12,6 +12,9 @@ CA1856 | Performance | Error | ConstantExpectedAnalyzer, [Documentation](https:/
CA1857 | Performance | Warning | ConstantExpectedAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1857)
CA1858 | Performance | Info | UseStartsWithInsteadOfIndexOfComparisonWithZero, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1858)
CA1859 | Performance | Info | UseConcreteTypeAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1859)
CA1860 | Performance | Info | PreferLengthCountIsEmptyOverAnyAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1860)
CA1861 | Performance | Info | PreferLengthCountIsEmptyOverAnyAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1861)
CA1862 | Performance | Info | PreferLengthCountIsEmptyOverAnyAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1862)

### Removed Rules

Expand Down
Expand Up @@ -91,7 +91,7 @@ public override void Initialize(AnalysisContext context)
return;
}

var receiverType = invocation.GetReceiverType(operationContext.Compilation, beforeConversion: true, cancellationToken: operationContext.CancellationToken);
var receiverType = (INamedTypeSymbol?)invocation.GetReceiverType(operationContext.Compilation, beforeConversion: true, cancellationToken: operationContext.CancellationToken);
if (receiverType != null &&
receiverType.DerivesFromOrImplementsAnyConstructionOf(immutableCollectionType))
{
Expand Down
Expand Up @@ -1998,4 +1998,40 @@
<data name="UseConcreteTypeForParameterMessage" xml:space="preserve">
<value>Change type of parameter '{0}' from '{1}' to '{2}' for improved performance</value>
</data>
<data name="PreferLengthOverAnyCodeFixTitle" xml:space="preserve">
<value>Use 'Length' check instead of 'Any()'</value>
</data>
<data name="PreferLengthOverAnyTitle" xml:space="preserve">
<value>Prefer 'Length' check over 'Any()'</value>
</data>
<data name="PreferLengthOverAnyMessage" xml:space="preserve">
<value>Prefer comparing 'Length' to 0 rather than using 'Any()', both for clarity and for performance</value>
</data>
<data name="PreferLengthOverAnyDescription" xml:space="preserve">
<value>Prefer comparing 'Length' to 0 rather than calling 'Any()'. The intent is clearer and the 'Any()' call could be slower, especially if it uses the 'IEnumerable.Any()' extension method.</value>
</data>
<data name="PreferCountOverAnyCodeFixTitle" xml:space="preserve">
<value>Use 'Count' check instead of 'Any()'</value>
</data>
<data name="PreferCountOverAnyTitle" xml:space="preserve">
<value>Prefer 'Count' property check over 'Any()'</value>
</data>
<data name="PreferCountOverAnyMessage" xml:space="preserve">
<value>Prefer comparing 'Count' to 0 rather than using 'Any()', both for clarity and for performance</value>
</data>
<data name="PreferCountOverAnyDescription" xml:space="preserve">
<value>Prefer comparing 'Count' to 0 rather than calling 'Any()'. The intent is clearer and the 'Any()' call could be slower, especially if it uses the 'IEnumerable.Any()' extension method.</value>
</data>
<data name="PreferIsEmptyOverAnyCodeFixTitle" xml:space="preserve">
<value>Use 'IsEmpty' check instead of 'Any()'</value>
</data>
<data name="PreferIsEmptyOverAnyTitle" xml:space="preserve">
<value>Prefer 'IsEmpty' check over 'Any()'</value>
</data>
<data name="PreferIsEmptyOverAnyMessage" xml:space="preserve">
<value>Prefer an 'IsEmpty' check rather than using 'Any()', both for clarity and for performance</value>
</data>
<data name="PreferIsEmptyOverAnyDescription" xml:space="preserve">
<value>Prefer checking 'IsEmpty' rather than calling 'Any()'. The intent is clearer and the 'Any()' call could be slower, especially if it uses the 'IEnumerable.Any()' extension method.</value>
</data>
</root>
@@ -0,0 +1,50 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Immutable;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;

namespace Microsoft.NetCore.Analyzers.Performance
{
public abstract class PreferLengthCountIsEmptyOverAnyFixer : CodeFixProvider
{
public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(
PreferLengthCountIsEmptyOverAnyAnalyzer.LengthId,
PreferLengthCountIsEmptyOverAnyAnalyzer.CountId,
PreferLengthCountIsEmptyOverAnyAnalyzer.IsEmptyId
);

public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
var node = root.FindNode(context.Span, getInnermostNodeForTie: true);

foreach (var diagnostic in context.Diagnostics)
{
var (newRoot, codeFixTitle) = diagnostic.Id switch
{
PreferLengthCountIsEmptyOverAnyAnalyzer.IsEmptyId => (ReplaceAnyWithIsEmpty(root, node), MicrosoftNetCoreAnalyzersResources.PreferIsEmptyOverAnyCodeFixTitle),
PreferLengthCountIsEmptyOverAnyAnalyzer.LengthId => (ReplaceAnyWithLength(root, node), MicrosoftNetCoreAnalyzersResources.PreferLengthOverAnyCodeFixTitle),
PreferLengthCountIsEmptyOverAnyAnalyzer.CountId => (ReplaceAnyWithCount(root, node), MicrosoftNetCoreAnalyzersResources.PreferCountOverAnyCodeFixTitle),
_ => throw new NotSupportedException()
};
if (newRoot is null)
{
continue;
}

var codeAction = CodeAction.Create(codeFixTitle, _ => Task.FromResult(context.Document.WithSyntaxRoot(newRoot)), codeFixTitle);
context.RegisterCodeFix(codeAction, diagnostic);
}
}

protected abstract SyntaxNode? ReplaceAnyWithIsEmpty(SyntaxNode root, SyntaxNode node);
protected abstract SyntaxNode? ReplaceAnyWithLength(SyntaxNode root, SyntaxNode node);
protected abstract SyntaxNode? ReplaceAnyWithCount(SyntaxNode root, SyntaxNode node);

public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
}
}
@@ -0,0 +1,155 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections;
using System.Collections.Immutable;
using System.Linq;
using Analyzer.Utilities;
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
using Analyzer.Utilities.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using static Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources;

namespace Microsoft.NetCore.Analyzers.Performance
{
/// <summary>
/// Prefer using 'IsEmpty' or comparing 'Count' / 'Length' property to 0 rather than using 'Any()', both for clarity and for performance.
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
public sealed class PreferLengthCountIsEmptyOverAnyAnalyzer : DiagnosticAnalyzer
{
private const string AnyText = nameof(Enumerable.Any);

internal const string IsEmptyText = nameof(ImmutableArray<dynamic>.IsEmpty);
internal const string LengthText = nameof(Array.Length);
internal const string CountText = nameof(ICollection.Count);

internal const string IsEmptyId = "CA1860";
internal const string LengthId = "CA1861";
internal const string CountId = "CA1862";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagnostics quite similar, do not see need to handle them separately, probably better to be one diagnostic?
CC @stephentoub @mavasani

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolving this comment as:

  1. Other similar analyzers using different ids
  2. Personally in case it was not IEnumerable.Any() that does count/length check in the implementation I would prefer to use Any() over Count or Length check, therefore it seems one might prefer to suppress one of them over the other.
  3. With different diagnostics title and description message are cleaner

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unresolving this as if we restrict to Enumerable.Any() only there will be no need to handle these warnings separately.

@CollinAlpert lets use only one ID for each DiagnosticDecriptors


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

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

internal static readonly DiagnosticDescriptor CountDescriptor = DiagnosticDescriptorHelper.Create(
CountId,
CreateLocalizableResourceString(nameof(PreferCountOverAnyTitle)),
CreateLocalizableResourceString(nameof(PreferCountOverAnyMessage)),
DiagnosticCategory.Performance,
RuleLevel.IdeSuggestion,
CreateLocalizableResourceString(nameof(PreferCountOverAnyDescription)),
isPortedFxCopRule: false,
isDataflowRule: false
);

buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(
LengthDescriptor,
CountDescriptor,
IsEmptyDescriptor
);

public override void Initialize(AnalysisContext context)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
context.EnableConcurrentExecution();
context.RegisterOperationAction(OnInvocationAnalysis, OperationKind.Invocation);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private static void OnInvocationAnalysis(OperationAnalysisContext context)
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
{
var invocation = (IInvocationOperation)context.Operation;
var originalMethod = invocation.TargetMethod.OriginalDefinition;
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
if (originalMethod.MethodKind == MethodKind.ReducedExtension)
{
originalMethod = originalMethod.ReducedFrom;
}

if (IsEligibleAnyMethod(originalMethod))
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
{
var type = invocation.GetReceiverType(context.Compilation, beforeConversion: true, context.CancellationToken);
if (type is null)
{
return;
}

if (HasEligibleIsEmptyProperty(type))
{
context.ReportDiagnostic(invocation.CreateDiagnostic(IsEmptyDescriptor));

return;
}

if (HasEligibleLengthProperty(type))
{
context.ReportDiagnostic(invocation.CreateDiagnostic(LengthDescriptor));

return;
}

if (HasEligibleCountProperty(type))
{
context.ReportDiagnostic(invocation.CreateDiagnostic(CountDescriptor));

return;
}
}
}

private static bool IsEligibleAnyMethod(IMethodSymbol method)
{
return method is
{
Name: AnyText,
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
ReturnType.SpecialType: SpecialType.System_Boolean,
IsExtensionMethod: true,
Parameters: [_]
};
}

private static bool HasEligibleIsEmptyProperty(ITypeSymbol typeSymbol)
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
{
return typeSymbol.GetMembers(IsEmptyText)
.OfType<IPropertySymbol>()
.Any(property => property.Type.SpecialType == SpecialType.System_Boolean);
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
}

private static bool HasEligibleLengthProperty(ITypeSymbol typeSymbol)
{
if (typeSymbol is IArrayTypeSymbol)
{
return true;
}

return typeSymbol.GetMembers(LengthText)
.OfType<IPropertySymbol>()
.Any(property => property.Type.SpecialType is SpecialType.System_Int32 or SpecialType.System_UInt32);
}

private static bool HasEligibleCountProperty(ITypeSymbol typeSymbol)
{
return typeSymbol.GetMembers(CountText)
.OfType<IPropertySymbol>()
.Any(property => property.Type.SpecialType is SpecialType.System_Int32 or SpecialType.System_UInt32);
}
}
}