-
Notifications
You must be signed in to change notification settings - Fork 480
Implement Use 'StartsWith' instead of 'IndexOf' analyzer #6295
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 all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
4d68b55
Implement Use 'StartsWith' instead of 'IndexOf' analyzer
Youssef1313 3f41d01
Support fix all
Youssef1313 cd66a08
Write a test
Youssef1313 64c5793
address feedback, more tests
Youssef1313 1af0b0e
Remove unnecessary using
Youssef1313 4aad448
Merge branch 'main' into starts-with
Youssef1313 7df3273
Simplify
Youssef1313 01eaaf4
Refactor
Youssef1313 2ce9999
wip
Youssef1313 c81a3e3
wip
Youssef1313 451ba14
Redundant comment
Youssef1313 4c43d0f
Remove unused using directive
Youssef1313 400cde7
Handle some scenarios, fix tests, and add more tests
Youssef1313 c9817ff
Remove TODO
Youssef1313 5b5026e
Fix formatting
Youssef1313 f23bb18
Merge branch 'main' into starts-with
Youssef1313 e7b76a0
Address review comments
Youssef1313 6605ad0
Fix build
Youssef1313 ebb49f1
Fix formatting
Youssef1313 e4dc78d
Fix build
Youssef1313 e42cc08
Try with elastic marker
Youssef1313 58ce1d6
Update generated files
Youssef1313 167b826
Revert "Try with elastic marker"
Youssef1313 437db82
Update test
Youssef1313 d0a198d
Run pack
Youssef1313 b1ad36e
Revert "Revert "Try with elastic marker""
Youssef1313 c358bfb
revert
Youssef1313 d22ec17
Apply suggestions from code review
buyaa-n 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
45 changes: 45 additions & 0 deletions
45
...Core.Analyzers/Performance/CSharpUseStartsWithInsteadOfIndexOfComparisonWithZero.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,45 @@ | ||
| // 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 Analyzer.Utilities; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.CodeFixes; | ||
| using Microsoft.CodeAnalysis.CSharp; | ||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
| using Microsoft.CodeAnalysis.Editing; | ||
| using Microsoft.NetCore.Analyzers.Performance; | ||
|
|
||
| namespace Microsoft.NetCore.CSharp.Analyzers.Performance | ||
| { | ||
| [ExportCodeFixProvider(LanguageNames.CSharp), Shared] | ||
| public sealed class CSharpUseStartsWithInsteadOfIndexOfComparisonWithZeroCodeFix : UseStartsWithInsteadOfIndexOfComparisonWithZeroCodeFix | ||
| { | ||
| protected override SyntaxNode AppendElasticMarker(SyntaxNode replacement) | ||
| => replacement.WithTrailingTrivia(SyntaxFactory.ElasticMarker); | ||
|
|
||
| protected override SyntaxNode HandleCharStringComparisonOverload(SyntaxGenerator generator, SyntaxNode instance, SyntaxNode[] arguments, bool shouldNegate) | ||
| { | ||
| // For 'x.IndexOf(ch, stringComparison)', we switch to 'x.AsSpan().StartsWith(stackalloc char[1] { ch }, stringComparison)' | ||
| var (argumentSyntax, index) = GetCharacterArgumentAndIndex(arguments); | ||
| arguments[index] = argumentSyntax.WithExpression(SyntaxFactory.StackAllocArrayCreationExpression( | ||
| SyntaxFactory.ArrayType( | ||
| (TypeSyntax)generator.TypeExpression(SpecialType.System_Char), | ||
| SyntaxFactory.SingletonList(SyntaxFactory.ArrayRankSpecifier(SyntaxFactory.SingletonSeparatedList((ExpressionSyntax)generator.LiteralExpression(1))))), | ||
| SyntaxFactory.InitializerExpression(SyntaxKind.ArrayInitializerExpression, SyntaxFactory.SingletonSeparatedList(argumentSyntax.Expression)) | ||
| )); | ||
| instance = generator.InvocationExpression(generator.MemberAccessExpression(instance, "AsSpan")).WithAdditionalAnnotations(new SyntaxAnnotation("SymbolId", "System.MemoryExtensions")).WithAddImportsAnnotation(); | ||
| return CreateStartsWithInvocationFromArguments(generator, instance, arguments, shouldNegate); | ||
| } | ||
|
|
||
| private static (ArgumentSyntax Argument, int Index) GetCharacterArgumentAndIndex(SyntaxNode[] arguments) | ||
| { | ||
| var firstArgument = (ArgumentSyntax)arguments[0]; | ||
| if (firstArgument.NameColon is null or { Name.Identifier.Value: "value" }) | ||
| { | ||
| return (firstArgument, 0); | ||
| } | ||
|
|
||
| return ((ArgumentSyntax)arguments[1], 1); | ||
| } | ||
| } | ||
| } |
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
98 changes: 98 additions & 0 deletions
98
...ft.NetCore.Analyzers/Performance/UseStartsWithInsteadOfIndexOfComparisonWithZero.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,98 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Immutable; | ||
| using System.Diagnostics; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.CodeActions; | ||
| using Microsoft.CodeAnalysis.CodeFixes; | ||
| using Microsoft.CodeAnalysis.Editing; | ||
|
|
||
| namespace Microsoft.NetCore.Analyzers.Performance | ||
| { | ||
| public abstract class UseStartsWithInsteadOfIndexOfComparisonWithZeroCodeFix : CodeFixProvider | ||
| { | ||
| public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(UseStartsWithInsteadOfIndexOfComparisonWithZero.RuleId); | ||
|
|
||
| public override FixAllProvider GetFixAllProvider() | ||
| => WellKnownFixAllProviders.BatchFixer; | ||
|
|
||
| public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
| { | ||
| var document = context.Document; | ||
| var diagnostic = context.Diagnostics[0]; | ||
| var root = await document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
| var node = root.FindNode(context.Span, getInnermostNodeForTie: true); | ||
|
|
||
| context.RegisterCodeFix( | ||
| CodeAction.Create(MicrosoftNetCoreAnalyzersResources.UseStartsWithInsteadOfIndexOfComparisonWithZeroCodeFixTitle, | ||
| createChangedDocument: cancellationToken => | ||
| { | ||
| var instance = root.FindNode(diagnostic.AdditionalLocations[0].SourceSpan); | ||
| var arguments = new SyntaxNode[diagnostic.AdditionalLocations.Count - 1]; | ||
| for (int i = 1; i < diagnostic.AdditionalLocations.Count; i++) | ||
| { | ||
| arguments[i - 1] = root.FindNode(diagnostic.AdditionalLocations[i].SourceSpan); | ||
| } | ||
|
|
||
| var generator = SyntaxGenerator.GetGenerator(document); | ||
| var shouldNegate = diagnostic.Properties.TryGetValue(UseStartsWithInsteadOfIndexOfComparisonWithZero.ShouldNegateKey, out _); | ||
| var compilationHasStartsWithCharOverload = diagnostic.Properties.TryGetKey(UseStartsWithInsteadOfIndexOfComparisonWithZero.CompilationHasStartsWithCharOverloadKey, out _); | ||
| _ = diagnostic.Properties.TryGetValue(UseStartsWithInsteadOfIndexOfComparisonWithZero.ExistingOverloadKey, out var overloadValue); | ||
| switch (overloadValue) | ||
| { | ||
| // For 'IndexOf(string)' and 'IndexOf(string, stringComparison)', we replace with StartsWith(same arguments) | ||
| case UseStartsWithInsteadOfIndexOfComparisonWithZero.OverloadString: | ||
| case UseStartsWithInsteadOfIndexOfComparisonWithZero.OverloadString_StringComparison: | ||
| return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(node, CreateStartsWithInvocationFromArguments(generator, instance, arguments, shouldNegate)))); | ||
|
|
||
| // For 'a.IndexOf(ch, stringComparison)': | ||
| // C#: Use 'a.AsSpan().StartsWith(stackalloc char[1] { ch }, stringComparison)' | ||
| // https://learn.microsoft.com/dotnet/api/system.memoryextensions.startswith?view=net-7.0#system-memoryextensions-startswith(system-readonlyspan((system-char))-system-readonlyspan((system-char))-system-stringcomparison) | ||
| // VB: Use a.StartsWith(c.ToString(), stringComparison) | ||
| case UseStartsWithInsteadOfIndexOfComparisonWithZero.OverloadChar_StringComparison: | ||
| return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(node, HandleCharStringComparisonOverload(generator, instance, arguments, shouldNegate)))); | ||
|
|
||
| // If 'StartsWith(char)' is available, use it. Otherwise check '.Length > 0 && [0] == ch' | ||
| // For negation, we use '.Length == 0 || [0] != ch' | ||
| case UseStartsWithInsteadOfIndexOfComparisonWithZero.OverloadChar: | ||
| if (compilationHasStartsWithCharOverload) | ||
| { | ||
| return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(node, CreateStartsWithInvocationFromArguments(generator, instance, arguments, shouldNegate)))); | ||
| } | ||
|
|
||
| var lengthAccess = generator.MemberAccessExpression(instance, "Length"); | ||
| var zeroLiteral = generator.LiteralExpression(0); | ||
|
|
||
| var indexed = generator.ElementAccessExpression(instance, zeroLiteral); | ||
| var ch = root.FindNode(arguments[0].Span, getInnermostNodeForTie: true); | ||
|
|
||
| var replacement = shouldNegate | ||
| ? generator.LogicalOrExpression( | ||
| generator.ValueEqualsExpression(lengthAccess, zeroLiteral), | ||
| generator.ValueNotEqualsExpression(indexed, ch)) | ||
| : generator.LogicalAndExpression( | ||
| generator.GreaterThanExpression(lengthAccess, zeroLiteral), | ||
| generator.ValueEqualsExpression(indexed, ch)); | ||
|
|
||
| return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(node, AppendElasticMarker(replacement)))); | ||
|
|
||
| default: | ||
| Debug.Fail("This should never happen."); | ||
| return Task.FromResult(document); | ||
| } | ||
| }, | ||
| equivalenceKey: nameof(MicrosoftNetCoreAnalyzersResources.UseStartsWithInsteadOfIndexOfComparisonWithZeroCodeFixTitle)), | ||
| context.Diagnostics); | ||
| } | ||
|
|
||
| protected abstract SyntaxNode HandleCharStringComparisonOverload(SyntaxGenerator generator, SyntaxNode instance, SyntaxNode[] arguments, bool shouldNegate); | ||
| protected abstract SyntaxNode AppendElasticMarker(SyntaxNode replacement); | ||
|
|
||
| protected static SyntaxNode CreateStartsWithInvocationFromArguments(SyntaxGenerator generator, SyntaxNode instance, SyntaxNode[] arguments, bool shouldNegate) | ||
| { | ||
| var expression = generator.InvocationExpression(generator.MemberAccessExpression(instance, "StartsWith"), arguments); | ||
| return shouldNegate ? generator.LogicalNotExpression(expression) : expression; | ||
| } | ||
| } | ||
| } | ||
152 changes: 152 additions & 0 deletions
152
...icrosoft.NetCore.Analyzers/Performance/UseStartsWithInsteadOfIndexOfComparisonWithZero.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,152 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Immutable; | ||
| using System.Linq; | ||
| using Analyzer.Utilities; | ||
| using Analyzer.Utilities.Extensions; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.CodeAnalysis.Operations; | ||
|
|
||
| namespace Microsoft.NetCore.Analyzers.Performance | ||
| { | ||
| using static MicrosoftNetCoreAnalyzersResources; | ||
|
|
||
| [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] | ||
| public sealed class UseStartsWithInsteadOfIndexOfComparisonWithZero : DiagnosticAnalyzer | ||
| { | ||
| internal const string RuleId = "CA1858"; | ||
| internal const string ShouldNegateKey = "ShouldNegate"; | ||
| internal const string CompilationHasStartsWithCharOverloadKey = "CompilationHasStartsWithCharOverload"; | ||
|
|
||
| internal const string ExistingOverloadKey = "ExistingOverload"; | ||
|
|
||
| internal const string OverloadString = "String"; | ||
| internal const string OverloadString_StringComparison = "String,StringComparison"; | ||
| internal const string OverloadChar = "Char"; | ||
| internal const string OverloadChar_StringComparison = "Char,StringComparison"; | ||
|
|
||
| internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create( | ||
| id: RuleId, | ||
| title: CreateLocalizableResourceString(nameof(UseStartsWithInsteadOfIndexOfComparisonWithZeroTitle)), | ||
| messageFormat: CreateLocalizableResourceString(nameof(UseStartsWithInsteadOfIndexOfComparisonWithZeroMessage)), | ||
| category: DiagnosticCategory.Performance, | ||
| ruleLevel: RuleLevel.IdeSuggestion, | ||
| description: CreateLocalizableResourceString(nameof(UseStartsWithInsteadOfIndexOfComparisonWithZeroDescription)), | ||
| isPortedFxCopRule: false, | ||
| isDataflowRule: false | ||
| ); | ||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); | ||
|
|
||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.EnableConcurrentExecution(); | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
|
|
||
| context.RegisterCompilationStartAction(context => | ||
| { | ||
| var stringType = context.Compilation.GetSpecialType(SpecialType.System_String); | ||
| var hasAnyStartsWith = false; | ||
| var hasStartsWithCharOverload = false; | ||
| foreach (var startsWithMethod in stringType.GetMembers("StartsWith").OfType<IMethodSymbol>()) | ||
| { | ||
| hasAnyStartsWith = true; | ||
| if (startsWithMethod.Parameters is [{ Type.SpecialType: SpecialType.System_Char }]) | ||
| { | ||
| hasStartsWithCharOverload = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!hasAnyStartsWith) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var indexOfMethodsBuilder = ImmutableArray.CreateBuilder<(IMethodSymbol IndexOfSymbol, string OverloadPropertyValue)>(); | ||
| foreach (var indexOfMethod in stringType.GetMembers("IndexOf").OfType<IMethodSymbol>()) | ||
| { | ||
| if (indexOfMethod.Parameters is [{ Type.SpecialType: SpecialType.System_String }]) | ||
| { | ||
| indexOfMethodsBuilder.Add((indexOfMethod, OverloadString)); | ||
| } | ||
| else if (indexOfMethod.Parameters is [{ Type.SpecialType: SpecialType.System_Char }]) | ||
| { | ||
| indexOfMethodsBuilder.Add((indexOfMethod, OverloadChar)); | ||
| } | ||
| else if (indexOfMethod.Parameters is [{ Type.SpecialType: SpecialType.System_String }, { Name: "comparisonType" }]) | ||
| { | ||
| indexOfMethodsBuilder.Add((indexOfMethod, OverloadString_StringComparison)); | ||
| } | ||
| else if (indexOfMethod.Parameters is [{ Type.SpecialType: SpecialType.System_Char }, { Name: "comparisonType" }]) | ||
| { | ||
| indexOfMethodsBuilder.Add((indexOfMethod, OverloadChar_StringComparison)); | ||
| } | ||
| } | ||
|
|
||
| if (indexOfMethodsBuilder.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var indexOfMethods = indexOfMethodsBuilder.ToImmutable(); | ||
|
|
||
| context.RegisterOperationAction(context => | ||
| { | ||
| var binaryOperation = (IBinaryOperation)context.Operation; | ||
| if (binaryOperation.OperatorKind is not (BinaryOperatorKind.Equals or BinaryOperatorKind.NotEquals)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (IsIndexOfComparedWithZero(binaryOperation.LeftOperand, binaryOperation.RightOperand, indexOfMethods, out var additionalLocations, out var properties) || | ||
| IsIndexOfComparedWithZero(binaryOperation.RightOperand, binaryOperation.LeftOperand, indexOfMethods, out additionalLocations, out properties)) | ||
| { | ||
| if (binaryOperation.OperatorKind == BinaryOperatorKind.NotEquals) | ||
| { | ||
| properties = properties.Add(ShouldNegateKey, ""); | ||
| } | ||
|
|
||
| if (hasStartsWithCharOverload) | ||
| { | ||
| properties = properties.Add(CompilationHasStartsWithCharOverloadKey, ""); | ||
| } | ||
|
|
||
| context.ReportDiagnostic(binaryOperation.CreateDiagnostic(Rule, additionalLocations, properties)); | ||
| } | ||
| }, OperationKind.Binary); | ||
| }); | ||
| } | ||
|
|
||
| private static bool IsIndexOfComparedWithZero( | ||
| IOperation left, IOperation right, | ||
| ImmutableArray<(IMethodSymbol Symbol, string OverloadPropertyValue)> indexOfMethods, | ||
| out ImmutableArray<Location> additionalLocations, | ||
| out ImmutableDictionary<string, string?> properties) | ||
| { | ||
| properties = ImmutableDictionary<string, string?>.Empty; | ||
|
|
||
| if (right.ConstantValue is { HasValue: true, Value: 0 } && | ||
| left is IInvocationOperation invocation) | ||
| { | ||
| foreach (var (indexOfMethod, overloadPropertyValue) in indexOfMethods) | ||
| { | ||
| if (indexOfMethod.Equals(invocation.TargetMethod, SymbolEqualityComparer.Default)) | ||
| { | ||
| var locationsBuilder = ImmutableArray.CreateBuilder<Location>(); | ||
| locationsBuilder.Add(invocation.Instance.Syntax.GetLocation()); | ||
| locationsBuilder.AddRange(invocation.Arguments.Select(arg => arg.Syntax.GetLocation())); | ||
| additionalLocations = locationsBuilder.ToImmutable(); | ||
|
|
||
| properties = properties.Add(ExistingOverloadKey, overloadPropertyValue); | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| additionalLocations = ImmutableArray<Location>.Empty; | ||
| return false; | ||
| } | ||
| } | ||
| } |
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.