-
Notifications
You must be signed in to change notification settings - Fork 486
Add analyzer/fixer to suggest using cached SearchValues instances #6898
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
Merged
Merged
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
4f18da4
Initial Analyzer implementation
MihaZupan 69f39fa
Code fixer
MihaZupan da368f2
Add support for string.IndexOfAny(char[])
MihaZupan 621aa7c
Catch simple cases of array element modification
MihaZupan 0dbfaf9
Use built-in helper instead of Linq
MihaZupan 942d49e
Also detect field assignments in ctor
MihaZupan 9a389dd
Move system namespace import to helper method
MihaZupan e1bd88d
Replace array creations wtih string literals
MihaZupan df31739
Add support for more property getter patterns
MihaZupan b480ad5
Simplify test helper
MihaZupan 0c40d49
Revert Utf8String support :(
MihaZupan 3db5ce9
Update tests
MihaZupan 4fbc7cc
msbuild /t:pack /v:m
MihaZupan 2eafccc
Fix editor renaming
MihaZupan a30f3e1
Exclude string uses on a conditional access
MihaZupan 153c682
Add test for array field with const char reference
MihaZupan c1e6dff
Add back Utf8String support
MihaZupan 79c2c8b
Update messages/descriptions
MihaZupan 37a6eab
Add support for field initialized from literal.ToCharArray
MihaZupan 21578a8
More tests for ToCharArray
MihaZupan 32fe939
Better handle member names that start with _
MihaZupan c8021ec
Avoid some duplication between Syntax and Operation analysis
MihaZupan 38ffb6d
Fix top-level statements and add logic to remove unused members
MihaZupan 644dd74
ImmutableHashSet, no OfType
MihaZupan ac26e6b
Remove some duplication
MihaZupan f6c1fab
Turn one analyzer test into code fixer tests
MihaZupan 1fe8dd8
Shorten analyzer title
MihaZupan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
133 changes: 133 additions & 0 deletions
133
...etAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Performance/CSharpUseSearchValues.Fixer.cs
This file contains hidden or 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,133 @@ | ||
// 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.Generic; | ||
using System.Diagnostics; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Analyzer.Utilities.Extensions; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CodeActions; | ||
using Microsoft.CodeAnalysis.CodeFixes; | ||
using Microsoft.CodeAnalysis.CSharp; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
using Microsoft.CodeAnalysis.Operations; | ||
using Microsoft.NetCore.Analyzers.Performance; | ||
|
||
namespace Microsoft.NetCore.CSharp.Analyzers.Performance | ||
{ | ||
/// <inheritdoc/> | ||
[ExportCodeFixProvider(LanguageNames.CSharp)] | ||
public sealed class CSharpUseSearchValuesFixer : UseSearchValuesFixer | ||
{ | ||
protected override async ValueTask<(SyntaxNode TypeDeclaration, INamedTypeSymbol? TypeSymbol, bool IsRealType)> GetTypeSymbolAsync(SemanticModel semanticModel, SyntaxNode node, CancellationToken cancellationToken) | ||
{ | ||
SyntaxNode? typeDeclarationOrCompilationUnit = node.FirstAncestorOrSelf<TypeDeclarationSyntax>(); | ||
|
||
typeDeclarationOrCompilationUnit ??= await node.SyntaxTree.GetRootAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
return typeDeclarationOrCompilationUnit is TypeDeclarationSyntax typeDeclaration | ||
? (typeDeclaration, semanticModel.GetDeclaredSymbol(typeDeclaration, cancellationToken), IsRealType: true) | ||
: (typeDeclarationOrCompilationUnit, semanticModel.GetDeclaredSymbol((CompilationUnitSyntax)typeDeclarationOrCompilationUnit, cancellationToken)?.ContainingType, IsRealType: false); | ||
} | ||
|
||
protected override SyntaxNode ReplaceSearchValuesFieldName(SyntaxNode node) | ||
{ | ||
if (node is FieldDeclarationSyntax fieldDeclaration && | ||
fieldDeclaration.Declaration is { } declaration && | ||
declaration.Variables is [var declarator]) | ||
{ | ||
var newDeclarator = declarator.ReplaceToken(declarator.Identifier, declarator.Identifier.WithAdditionalAnnotations(RenameAnnotation.Create())); | ||
return fieldDeclaration.WithDeclaration(declaration.WithVariables(new SeparatedSyntaxList<VariableDeclaratorSyntax>().Add(newDeclarator))); | ||
} | ||
|
||
return node; | ||
} | ||
|
||
protected override SyntaxNode GetDeclaratorInitializer(SyntaxNode syntax) | ||
{ | ||
if (syntax is VariableDeclaratorSyntax variableDeclarator) | ||
{ | ||
return variableDeclarator.Initializer!.Value; | ||
} | ||
|
||
if (syntax is PropertyDeclarationSyntax propertyDeclaration) | ||
{ | ||
return CSharpUseSearchValuesAnalyzer.TryGetPropertyGetterExpression(propertyDeclaration)!; | ||
} | ||
|
||
throw new InvalidOperationException($"Expected 'VariableDeclaratorSyntax' or 'PropertyDeclarationSyntax', got {syntax.GetType().Name}"); | ||
} | ||
|
||
// new[] { 'a', 'b', 'c' } => "abc" | ||
// new[] { (byte)'a', (byte)'b', (byte)'c' } => "abc"u8 | ||
// "abc".ToCharArray() => "abc" | ||
protected override SyntaxNode? TryReplaceArrayCreationWithInlineLiteralExpression(IOperation operation) | ||
{ | ||
if (operation is IConversionOperation conversion) | ||
{ | ||
operation = conversion.Operand; | ||
} | ||
|
||
if (operation is IArrayCreationOperation arrayCreation && | ||
arrayCreation.GetElementType() is { } elementType) | ||
{ | ||
bool isByte = elementType.SpecialType == SpecialType.System_Byte; | ||
|
||
if (isByte && | ||
(operation.SemanticModel?.Compilation is not CSharpCompilation compilation || | ||
compilation.LanguageVersion < (LanguageVersion)1100)) // LanguageVersion.CSharp11 | ||
{ | ||
// Can't use Utf8StringLiterals | ||
return null; | ||
} | ||
|
||
List<char> values = new(); | ||
|
||
if (arrayCreation.Syntax is ExpressionSyntax creationSyntax && | ||
CSharpUseSearchValuesAnalyzer.IsConstantByteOrCharArrayCreationExpression(operation.SemanticModel!, creationSyntax, values, out _) && | ||
values.Count <= 128 && // Arbitrary limit to avoid emitting huge literals | ||
!ContainsAnyComments(creationSyntax)) // Avoid removing potentially valuable comments | ||
{ | ||
string valuesString = string.Concat(values); | ||
string stringLiteral = SymbolDisplay.FormatLiteral(valuesString, quote: true); | ||
|
||
const SyntaxKind Utf8StringLiteralExpression = (SyntaxKind)8756; | ||
const SyntaxKind Utf8StringLiteralToken = (SyntaxKind)8520; | ||
buyaa-n marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return SyntaxFactory.LiteralExpression( | ||
isByte ? Utf8StringLiteralExpression : SyntaxKind.StringLiteralExpression, | ||
SyntaxFactory.Token( | ||
leading: default, | ||
kind: isByte ? Utf8StringLiteralToken : SyntaxKind.StringLiteralToken, | ||
text: isByte ? $"{stringLiteral}u8" : stringLiteral, | ||
valueText: valuesString, | ||
trailing: default)); | ||
} | ||
} | ||
else if (operation is IInvocationOperation invocation) | ||
{ | ||
if (UseSearchValuesAnalyzer.IsConstantStringToCharArrayInvocation(invocation, out _)) | ||
{ | ||
Debug.Assert(invocation.Instance is not null); | ||
return invocation.Instance!.Syntax; | ||
} | ||
} | ||
|
||
return null; | ||
} | ||
|
||
private static bool ContainsAnyComments(SyntaxNode node) | ||
{ | ||
foreach (SyntaxTrivia trivia in node.DescendantTrivia(node.Span)) | ||
{ | ||
if (trivia.Kind() is SyntaxKind.SingleLineCommentTrivia or SyntaxKind.MultiLineCommentTrivia) | ||
{ | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
} | ||
} |
221 changes: 221 additions & 0 deletions
221
src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Performance/CSharpUseSearchValues.cs
This file contains hidden or 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,221 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System.Collections.Generic; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CSharp; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
using Microsoft.CodeAnalysis.Operations; | ||
using Microsoft.NetCore.Analyzers.Performance; | ||
|
||
namespace Microsoft.NetCore.CSharp.Analyzers.Performance | ||
{ | ||
/// <inheritdoc/> | ||
[DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
public sealed class CSharpUseSearchValuesAnalyzer : UseSearchValuesAnalyzer | ||
{ | ||
// char[] myField = new char[] { 'a', 'b', 'c' }; | ||
// char[] myField = new[] { 'a', 'b', 'c' }; | ||
// char[] myField = "abc".ToCharArray(); | ||
// char[] myField = ConstString.ToCharArray(); | ||
// byte[] myField = new[] { (byte)'a', (byte)'b', (byte)'c' }; | ||
protected override bool IsConstantByteOrCharArrayVariableDeclaratorSyntax(SemanticModel semanticModel, SyntaxNode syntax, out int length) | ||
{ | ||
length = 0; | ||
|
||
return | ||
syntax is VariableDeclaratorSyntax variableDeclarator && | ||
variableDeclarator.Initializer?.Value is { } initializer && | ||
IsConstantByteOrCharArrayCreationExpression(semanticModel, initializer, values: null, out length); | ||
} | ||
|
||
// ReadOnlySpan<char> myProperty => new char[] { 'a', 'b', 'c' }; | ||
// ReadOnlySpan<char> myProperty => new[] { 'a', 'b', 'c' }; | ||
// ReadOnlySpan<char> myProperty => "abc".ToCharArray(); | ||
// ReadOnlySpan<char> myProperty => ConstString.ToCharArray(); | ||
// ReadOnlySpan<byte> myProperty => new[] { (byte)'a', (byte)'b', (byte)'c' }; | ||
// ReadOnlySpan<byte> myProperty => "abc"u8; | ||
// ReadOnlySpan<byte> myProperty { get => "abc"u8; } | ||
// ReadOnlySpan<byte> myProperty { get { return "abc"u8; } } | ||
protected override bool IsConstantByteOrCharReadOnlySpanPropertyDeclarationSyntax(SemanticModel semanticModel, SyntaxNode syntax, out int length) | ||
{ | ||
length = 0; | ||
|
||
return | ||
syntax is PropertyDeclarationSyntax propertyDeclaration && | ||
TryGetPropertyGetterExpression(propertyDeclaration) is { } expression && | ||
(IsConstantByteOrCharArrayCreationExpression(semanticModel, expression, values: null, out length) || IsUtf8StringLiteralExpression(expression, out length)); | ||
} | ||
|
||
protected override bool IsConstantByteOrCharArrayCreationSyntax(SemanticModel semanticModel, SyntaxNode syntax, out int length) | ||
{ | ||
length = 0; | ||
|
||
return | ||
syntax is ExpressionSyntax expression && | ||
IsConstantByteOrCharArrayCreationExpression(semanticModel, expression, values: null, out length); | ||
} | ||
|
||
internal static ExpressionSyntax? TryGetPropertyGetterExpression(PropertyDeclarationSyntax propertyDeclaration) | ||
{ | ||
var expression = propertyDeclaration.ExpressionBody?.Expression; | ||
|
||
if (expression is null && | ||
propertyDeclaration.AccessorList?.Accessors is [var accessor] && | ||
accessor.IsKind(SyntaxKind.GetAccessorDeclaration)) | ||
{ | ||
expression = accessor.ExpressionBody?.Expression; | ||
|
||
if (expression is null && | ||
accessor.Body?.Statements is [var statement] && | ||
statement is ReturnStatementSyntax returnStatement) | ||
{ | ||
expression = returnStatement.Expression; | ||
} | ||
} | ||
|
||
return expression; | ||
} | ||
|
||
// new char[] { 'a', 'b', 'c' }; | ||
// new[] { 'a', 'b', 'c' }; | ||
// new[] { (byte)'a', (byte)'b', (byte)'c' }; | ||
// "abc".ToCharArray() | ||
// ConstString.ToCharArray() | ||
internal static bool IsConstantByteOrCharArrayCreationExpression(SemanticModel semanticModel, ExpressionSyntax expression, List<char>? values, out int length) | ||
{ | ||
length = 0; | ||
|
||
InitializerExpressionSyntax? arrayInitializer = null; | ||
|
||
if (expression is ArrayCreationExpressionSyntax arrayCreation) | ||
{ | ||
arrayInitializer = arrayCreation.Initializer; | ||
} | ||
else if (expression is ImplicitArrayCreationExpressionSyntax implicitArrayCreation) | ||
{ | ||
arrayInitializer = implicitArrayCreation.Initializer; | ||
} | ||
else if (expression is InvocationExpressionSyntax invocation) | ||
{ | ||
if (semanticModel.GetOperation(invocation) is IInvocationOperation invocationOperation && | ||
IsConstantStringToCharArrayInvocation(invocationOperation, out string? value)) | ||
{ | ||
values?.AddRange(value); | ||
length = value.Length; | ||
return true; | ||
} | ||
} | ||
|
||
if (arrayInitializer?.Expressions is { } valueExpressions) | ||
{ | ||
foreach (var valueExpression in valueExpressions) | ||
{ | ||
if (!TryGetByteOrCharLiteral(valueExpression, out char value)) | ||
{ | ||
return false; | ||
} | ||
|
||
values?.Add(value); | ||
} | ||
|
||
length = valueExpressions.Count; | ||
return true; | ||
} | ||
|
||
return false; | ||
|
||
// 'a' or (byte)'a' | ||
static bool TryGetByteOrCharLiteral(ExpressionSyntax? expression, out char value) | ||
{ | ||
if (expression is not null) | ||
{ | ||
if (expression is CastExpressionSyntax cast && | ||
cast.Type is PredefinedTypeSyntax predefinedType && | ||
predefinedType.Keyword.IsKind(SyntaxKind.ByteKeyword)) | ||
{ | ||
expression = cast.Expression; | ||
} | ||
|
||
if (expression.IsKind(SyntaxKind.CharacterLiteralExpression) && | ||
expression is LiteralExpressionSyntax characterLiteral && | ||
characterLiteral.Token.Value is char charValue) | ||
{ | ||
value = charValue; | ||
return true; | ||
} | ||
} | ||
|
||
value = default; | ||
return false; | ||
} | ||
} | ||
|
||
private static bool IsUtf8StringLiteralExpression(ExpressionSyntax expression, out int length) | ||
{ | ||
const SyntaxKind Utf8StringLiteralExpression = (SyntaxKind)8756; | ||
const SyntaxKind Utf8StringLiteralToken = (SyntaxKind)8520; | ||
|
||
if (expression.IsKind(Utf8StringLiteralExpression) && | ||
expression is LiteralExpressionSyntax literal && | ||
literal.Token.IsKind(Utf8StringLiteralToken) && | ||
literal.Token.Value is string value) | ||
{ | ||
length = value.Length; | ||
return true; | ||
} | ||
|
||
length = 0; | ||
return false; | ||
} | ||
|
||
protected override bool ArrayFieldUsesAreLikelyReadOnly(SyntaxNode syntax) | ||
{ | ||
if (syntax is not VariableDeclaratorSyntax variableDeclarator || | ||
variableDeclarator.Identifier.Value is not string fieldName || | ||
syntax.FirstAncestorOrSelf<TypeDeclarationSyntax>() is not { } typeDeclaration) | ||
{ | ||
return false; | ||
} | ||
|
||
// An optimistic implementation that only looks for simple assignments to the field or its array elements. | ||
foreach (var member in typeDeclaration.Members) | ||
{ | ||
bool isCtor = member.IsKind(SyntaxKind.ConstructorDeclaration); | ||
|
||
foreach (var node in member.DescendantNodes()) | ||
{ | ||
if (node.IsKind(SyntaxKind.SimpleAssignmentExpression) && | ||
node is AssignmentExpressionSyntax assignment) | ||
{ | ||
if (assignment.Left.IsKind(SyntaxKind.ElementAccessExpression)) | ||
{ | ||
if (assignment.Left is ElementAccessExpressionSyntax elementAccess && | ||
IsFieldReference(elementAccess.Expression, fieldName)) | ||
{ | ||
// s_array[42] = foo; | ||
return false; | ||
} | ||
} | ||
else if (isCtor) | ||
{ | ||
if (IsFieldReference(assignment.Left, fieldName)) | ||
{ | ||
// s_array = foo; | ||
return false; | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
return true; | ||
|
||
static bool IsFieldReference(ExpressionSyntax expression, string fieldName) => | ||
expression.IsKind(SyntaxKind.IdentifierName) && | ||
expression is IdentifierNameSyntax identifierName && | ||
identifierName.Identifier.Value is string value && | ||
value == fieldName; | ||
} | ||
} | ||
} |
This file contains hidden or 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
This file contains hidden or 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
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.
Uh oh!
There was an error while loading. Please reload this page.