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
mavasani
merged 13 commits into
dotnet:master
from
paulomorgado:features/UsePropertyInsteadOfCountMethodWhenAvailable
Sep 12, 2019
Merged
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
8a97b16
add analyzer/fixer for usages of Enumerable.Count() where a property …
paulomorgado 65b2d79
fixed message
paulomorgado a179baa
CA1829: Use property instead of Enumerable.Count{).
paulomorgado 3a6e735
workaround for ImmutableArray<T>
paulomorgado e9891de
workaround for ImmutableArray<T>
paulomorgado c9835b7
fixed for conditional invocations
paulomorgado 02b30d5
Added GetVisibleTypeByMetadataName extensions to Compilation.
paulomorgado 623ca8c
added property names to diagnostic title.
paulomorgado aa4f8da
update documentation
paulomorgado 4900723
changed resolution of ICollection.Count from LINQ to foreach.
paulomorgado 344979d
Merge remote-tracking branch 'upstream/master' into features/UsePrope…
paulomorgado b7a5648
updated documentation
paulomorgado 8e28777
Merge remote-tracking branch 'upstream/master' into features/UsePrope…
paulomorgado File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
221 changes: 112 additions & 109 deletions
221
src/Microsoft.CodeAnalysis.FxCopAnalyzers/Microsoft.CodeAnalysis.FxCopAnalyzers.md
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
48 changes: 48 additions & 0 deletions
48
....Analyzers/CSharp/Performance/CSharpUsePropertyInsteadOfCountMethodWhenAvailable.Fixer.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
// 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: Use property instead of <see cref="System.Linq.Enumerable.Count{TSource}(System.Collections.Generic.IEnumerable{TSource})"/>, when available. | ||
/// </summary> | ||
[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; | ||
} | ||
} | ||
} |
54 changes: 54 additions & 0 deletions
54
...etCore.Analyzers/CSharp/Performance/CSharpUsePropertyInsteadOfCountMethodWhenAvailable.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// 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 | ||
{ | ||
[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) | ||
{ | ||
var method = invocationOperation.TargetMethod; | ||
|
||
if (invocationOperation.Arguments.Length == 1 && | ||
method.Name.Equals(nameof(Enumerable.Count), StringComparison.Ordinal) && | ||
this.Context.IsEnumerableType(method.ContainingSymbol) && | ||
((INamedTypeSymbol)(method.Parameters[0].Type)).TypeArguments[0] is ITypeSymbol methodSourceItemType) | ||
{ | ||
return invocationOperation.Arguments[0].Value is IConversionOperation convertionOperation | ||
? convertionOperation.Operand.Type | ||
: invocationOperation.Arguments[0].Value.Type; | ||
} | ||
|
||
return null; | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
115 changes: 115 additions & 0 deletions
115
....NetCore.Analyzers/Core/Performance/UsePropertyInsteadOfCountMethodWhenAvailable.Fixer.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
// 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 | ||
|
||
paulomorgado marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
/// <summary> | ||
/// CA1829: Use property instead of <see cref="Enumerable.Count{TSource}(System.Collections.Generic.IEnumerable{TSource})"/>, when available. | ||
/// Implements the <see cref="Microsoft.CodeAnalysis.Diagnostics.DiagnosticAnalyzer" /> | ||
/// </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); | ||
|
||
private class UsePropertyInsteadOfCountMethodWhenAvailableCodeAction : CodeAction | ||
paulomorgado marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
private readonly Document document; | ||
paulomorgado marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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; | ||
} | ||
|
||
public override string Title { get; } = MicrosoftNetCoreAnalyzersResources.UsePropertyInsteadOfCountMethodWhenAvailableTitle; | ||
|
||
public override string EquivalenceKey { get; } = MicrosoftNetCoreAnalyzersResources.UsePropertyInsteadOfCountMethodWhenAvailableTitle; | ||
|
||
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(); | ||
} | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
There was a problem hiding this comment.
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:
IOperation GetReceiverForExtensionMethodInvocation(IInvocationOperation)
.GetEnumerableCountInvocationTargetType
overrides can be moved to the base type which uses the above helper to just find the IOperation on which to operate.