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

Use property instead of count method when available #2736

Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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

Large diffs are not rendered by default.

Expand Up @@ -3861,6 +3861,21 @@
]
}
},
"CA1829": {
"id": "CA1829",
"shortDescription": "Use Length/Count property instead of Count() when available",
"fullDescription": "Enumerable.Count() potentially enumerates the sequence while a Length/Count property is a direct access.",
"defaultLevel": "warning",
"helpUri": "https://docs.microsoft.com/visualstudio/code-quality/ca1829",
"properties": {
"category": "Performance",
"isEnabledByDefault": true,
"typeName": "CSharpUsePropertyInsteadOfCountMethodWhenAvailableAnalyzer",
"languages": [
"C#"
]
}
},
"CA2010": {
"id": "CA2010",
"shortDescription": "Always consume the value returned by methods marked with PreserveSigAttribute",
Expand Down Expand Up @@ -3996,6 +4011,21 @@
]
}
},
"CA1829": {
"id": "CA1829",
"shortDescription": "Use Length/Count property instead of Count() when available",
"fullDescription": "Enumerable.Count() potentially enumerates the sequence while a Length/Count property is a direct access.",
"defaultLevel": "warning",
"helpUri": "https://docs.microsoft.com/visualstudio/code-quality/ca1829",
"properties": {
"category": "Performance",
"isEnabledByDefault": true,
"typeName": "BasicUsePropertyInsteadOfCountMethodWhenAvailableAnalyzer",
"languages": [
"Visual Basic"
]
}
},
"CA2010": {
"id": "CA2010",
"shortDescription": "Always consume the value returned by methods marked with PreserveSigAttribute",
Expand Down
@@ -0,0 +1,50 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

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

namespace Microsoft.NetCore.CSharp.Analyzers.Performance
{
/// <summary>
/// CA1829: C# implementation of use property instead of <see cref="System.Linq.Enumerable.Count{TSource}(System.Collections.Generic.IEnumerable{TSource})"/>, when available.
/// Implements the <see cref="CodeFixProvider" />
/// </summary>
/// <seealso cref="UsePropertyInsteadOfCountMethodWhenAvailableFixer"/>
[ExportCodeFixProvider(LanguageNames.CSharp), Shared]
public sealed class CSharpUsePropertyInsteadOfCountMethodWhenAvailableFixer : UsePropertyInsteadOfCountMethodWhenAvailableFixer
{
/// <summary>
/// Gets the expression from the specified <paramref name="invocationNode" /> where to replace the invocation of the
/// <see cref="System.Linq.Enumerable.Count{TSource}(System.Collections.Generic.IEnumerable{TSource})" /> method with a property invocation.
/// </summary>
/// <param name="invocationNode">The invocation node to get a fixer for.</param>
/// <param name="memberAccessNode">The member access node for the invocation node.</param>
/// <param name="nameNode">The name node for the invocation node.</param>
/// <returns><see langword="true" /> if a <paramref name="memberAccessNode" /> and <paramref name="nameNode" /> were found;
/// <see langword="false" /> otherwise.</returns>
protected override bool TryGetExpression(SyntaxNode invocationNode, out SyntaxNode memberAccessNode, out SyntaxNode nameNode)
{
if (invocationNode is InvocationExpressionSyntax invocationExpression)
{
switch (invocationExpression.Expression)
{
case MemberAccessExpressionSyntax memberAccessExpression:
memberAccessNode = invocationExpression.Expression;
nameNode = memberAccessExpression.Name;
return true;
case MemberBindingExpressionSyntax memberBindingExpression:
memberAccessNode = invocationExpression.Expression;
nameNode = memberBindingExpression.Name;
return true;
}
}

memberAccessNode = default;
nameNode = default;
return false;
}
}
}
@@ -0,0 +1,62 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.NetCore.Analyzers.Performance;

namespace Microsoft.NetCore.CSharp.Analyzers.Performance
{
/// <summary>
/// CA1829: C# implementation of use property instead of <see cref="Enumerable.Count{TSource}(System.Collections.Generic.IEnumerable{TSource})"/>, when available.
/// Implements the <see cref="UsePropertyInsteadOfCountMethodWhenAvailableAnalyzer" />
/// </summary>
/// <remarks>
/// Flags the use of <see cref="Enumerable.Count{TSource}(System.Collections.Generic.IEnumerable{TSource})"/> on types that are know to have a property with the same semantics:
/// <c>Length</c>, <c>Count</c>.
/// </remarks>
/// <seealso cref="UsePropertyInsteadOfCountMethodWhenAvailableAnalyzer"/>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class CSharpUsePropertyInsteadOfCountMethodWhenAvailableAnalyzer
: UsePropertyInsteadOfCountMethodWhenAvailableAnalyzer
{
/// <summary>
/// Creates the operation actions handler.
/// </summary>
/// <param name="context">The context.</param>
/// <returns>The operation actions handler.</returns>
protected override OperationActionsHandler CreateOperationActionsHandler(OperationActionsContext context)
=> new CSharpOperationActionsHandler(context);

/// <summary>
/// Handler for operaction actions for C#. This class cannot be inherited.
/// Implements the <see cref="Microsoft.NetCore.Analyzers.Performance.UsePropertyInsteadOfCountMethodWhenAvailableAnalyzer.OperationActionsHandler" />
/// </summary>
/// <seealso cref="Microsoft.NetCore.Analyzers.Performance.UsePropertyInsteadOfCountMethodWhenAvailableAnalyzer.OperationActionsHandler" />
private sealed class CSharpOperationActionsHandler : OperationActionsHandler
{
internal CSharpOperationActionsHandler(OperationActionsContext context)
: base(context)
{
}

protected override ITypeSymbol GetEnumerableCountInvocationTargetType(IInvocationOperation invocationOperation)
Copy link
Member

Choose a reason for hiding this comment

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

I presume the language specific code is needed due to dotnet/roslyn#23625 :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

Copy link
Member

@mavasani mavasani Sep 3, 2019

Choose a reason for hiding this comment

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

I think the logic here can be significantly simplified as follows:

  1. Base analyzer defines an abstract method IOperation GetReceiverForExtensionMethodInvocation(IInvocationOperation).
  2. All the logic that you have in GetEnumerableCountInvocationTargetType overrides can be moved to the base type which uses the above helper to just find the IOperation on which to operate.
  3. C# and VB specific implementations should ideally then be just a single method override with a trivial one line implementation to address the core difference from Difference in Operation tree shape between VB and C# for invocations to extension methods roslyn#23625

{
var method = invocationOperation.TargetMethod;

if (invocationOperation.Arguments.Length == 1 &&
method.Name.Equals(nameof(Enumerable.Count), StringComparison.Ordinal) &&
this.Context.IsEnumerableType(method.ContainingSymbol))
{
return invocationOperation.Arguments[0].Value is IConversionOperation convertionOperation
? convertionOperation.Operand.Type
: invocationOperation.Arguments[0].Value.Type;
}

return null;
}
}
}
}
Expand Up @@ -1134,6 +1134,15 @@
<data name="DoNotUseCountAsyncWhenAnyAsyncCanBeUsedTitle" xml:space="preserve">
<value>Do not use CountAsync() or LongCountAsync() when AnyAsync() can be used</value>
</data>
<data name="UsePropertyInsteadOfCountMethodWhenAvailableDescription" xml:space="preserve">
<value>Enumerable.Count() potentially enumerates the sequence while a Length/Count property is a direct access.</value>
</data>
<data name="UsePropertyInsteadOfCountMethodWhenAvailableMessage" xml:space="preserve">
<value>Use the "{0}" property instead of Enumerable.Count().</value>
</data>
<data name="UsePropertyInsteadOfCountMethodWhenAvailableTitle" xml:space="preserve">
<value>Use Length/Count property instead of Count() when available</value>
</data>
<data name="SetHttpOnlyForHttpCookie" xml:space="preserve">
<value>Set HttpOnly to true for HttpCookie</value>
</data>
Expand Down
@@ -0,0 +1,123 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

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

namespace Microsoft.NetCore.Analyzers.Performance
{
/// <summary>
/// CA1829: Use property instead of <see cref="Enumerable.Count{TSource}(System.Collections.Generic.IEnumerable{TSource})"/>, when available.
/// Implements the <see cref="CodeFixProvider" />
/// </summary>
public abstract class UsePropertyInsteadOfCountMethodWhenAvailableFixer : CodeFixProvider
{
/// <summary>
/// A list of diagnostic IDs that this provider can provider fixes for.
/// </summary>
/// <value>The fixable diagnostic ids.</value>
public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(UsePropertyInsteadOfCountMethodWhenAvailableAnalyzer.RuleId);

paulomorgado marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Gets an optional <see cref="FixAllProvider" /> that can fix all/multiple occurrences of diagnostics fixed by this code fix provider.
/// Return null if the provider doesn't support fix all/multiple occurrences.
/// Otherwise, you can return any of the well known fix all providers from <see cref="WellKnownFixAllProviders" /> or implement your own fix all provider.
/// </summary>
/// <returns>FixAllProvider.</returns>
public sealed override FixAllProvider GetFixAllProvider()
{
// See https://github.com/dotnet/roslyn/blob/master/docs/analyzers/FixAllProvider.md for more information on Fix All Providers
return WellKnownFixAllProviders.BatchFixer;
}

/// <summary>
/// Computes one or more fixes for the specified <see cref="CodeFixContext" />.
/// </summary>
/// <param name="context">A <see cref="CodeFixContext" /> containing context information about the diagnostics to fix.
/// The context must only contain diagnostics with a <see cref="Diagnostic.Id" /> included in the <see cref="CodeFixProvider.FixableDiagnosticIds" />
/// for the current provider.</param>
/// <returns>A <see cref="Task" /> that represents the asynchronous operation.</returns>
public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
var node = root.FindNode(context.Span);

if (node is object &&
context.Diagnostics[0].Properties.TryGetValue(UsePropertyInsteadOfCountMethodWhenAvailableAnalyzer.PropertyNameKey, out var propertyName) &&
paulomorgado marked this conversation as resolved.
Show resolved Hide resolved
propertyName is object &&
TryGetExpression(node, out var expressionNode, out var nameNode))
{
context.RegisterCodeFix(
new UsePropertyInsteadOfCountMethodWhenAvailableCodeAction(context.Document, node, expressionNode, nameNode, propertyName),
context.Diagnostics);
}
}

/// <summary>
/// Gets the expression from the specified <paramref name="invocationNode" /> where to replace the invocation of the
/// <see cref="Enumerable.Count{TSource}(System.Collections.Generic.IEnumerable{TSource})" /> method with a property invocation.
/// </summary>
/// <param name="invocationNode">The invocation node to get a fixer for.</param>
/// <param name="memberAccessNode">The member access node for the invocation node.</param>
/// <param name="nameNode">The name node for the invocation node.</param>
/// <returns><see langword="true"/> if a <paramref name="memberAccessNode" /> and <paramref name="nameNode"/> were found;
/// <see langword="false" /> otherwise.</returns>
protected abstract bool TryGetExpression(SyntaxNode invocationNode, out SyntaxNode memberAccessNode, out SyntaxNode nameNode);

/// <summary>
/// Implements the <see cref="CodeAction"/> for replacing the use of <see cref="System.Linq.Enumerable.Count{TSource}(System.Collections.Generic.IEnumerable{TSource})"/>
/// for the use of a property of the receiving type.
/// This class cannot be inherited.
/// </summary>
/// <seealso cref="Microsoft.CodeAnalysis.CodeActions.CodeAction" />
private sealed class UsePropertyInsteadOfCountMethodWhenAvailableCodeAction : CodeAction
{
private readonly Document _document;
private readonly SyntaxNode _invocationNode;
private readonly SyntaxNode _memberAccessNode;
private readonly SyntaxNode _nameNode;
private readonly string _propertyName;

public UsePropertyInsteadOfCountMethodWhenAvailableCodeAction(
Document document,
SyntaxNode invocationNode,
SyntaxNode memberAccessNode,
SyntaxNode nameNode,
string propertyName)
{
this._document = document;
this._invocationNode = invocationNode;
this._memberAccessNode = memberAccessNode;
this._nameNode = nameNode;
this._propertyName = propertyName;
}

/// <inheritdoc/>
public override string Title { get; } = MicrosoftNetCoreAnalyzersResources.UsePropertyInsteadOfCountMethodWhenAvailableTitle;

/// <inheritdoc/>
public override string EquivalenceKey { get; } = MicrosoftNetCoreAnalyzersResources.UsePropertyInsteadOfCountMethodWhenAvailableTitle;

/// <inheritdoc/>
protected sealed override async Task<Document> GetChangedDocumentAsync(CancellationToken cancellationToken)
{
var editor = await DocumentEditor.CreateAsync(this._document, cancellationToken).ConfigureAwait(false);
var generator = editor.Generator;
var replacementSyntax = generator.ReplaceNode(this._memberAccessNode, this._nameNode, generator.IdentifierName(_propertyName))
.WithAdditionalAnnotations(Formatter.Annotation)
.WithTriviaFrom(this._invocationNode);

editor.ReplaceNode(this._invocationNode, replacementSyntax);

return editor.GetChangedDocument();
}
}
}
}