-
Notifications
You must be signed in to change notification settings - Fork 485
Analyzer: Prefer .Length/Count/IsEmpty over Any() #6236
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
Changes from 14 commits
846ed66
39ddb18
3502244
f466a9e
0d6689e
061c15d
bae2acb
2df3b4d
ca68478
92675a6
1e36aac
639af3d
cc86232
bc89cba
e825483
36d65f0
f99b71f
aa5b25e
9a0cb96
0dcf2a6
5ed99ad
9aac134
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) | ||
{ | ||
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)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -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"; | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolving this comment as:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am unresolving this as if we restrict to @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); | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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))) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CollinAlpert That suggestion to use |
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
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); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} |
Uh oh!
There was an error while loading. Please reload this page.