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

Implement Use 'StartsWith' instead of 'IndexOf' analyzer #6295

Merged
merged 28 commits into from Dec 19, 2022
Merged
Show file tree
Hide file tree
Changes from 27 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 Dec 1, 2022
3f41d01
Support fix all
Youssef1313 Dec 1, 2022
cd66a08
Write a test
Youssef1313 Dec 1, 2022
64c5793
address feedback, more tests
Youssef1313 Dec 2, 2022
1af0b0e
Remove unnecessary using
Youssef1313 Dec 2, 2022
4aad448
Merge branch 'main' into starts-with
Youssef1313 Dec 6, 2022
7df3273
Simplify
Youssef1313 Dec 7, 2022
01eaaf4
Refactor
Youssef1313 Dec 7, 2022
2ce9999
wip
Youssef1313 Dec 7, 2022
c81a3e3
wip
Youssef1313 Dec 7, 2022
451ba14
Redundant comment
Youssef1313 Dec 7, 2022
4c43d0f
Remove unused using directive
Youssef1313 Dec 8, 2022
400cde7
Handle some scenarios, fix tests, and add more tests
Youssef1313 Dec 9, 2022
c9817ff
Remove TODO
Youssef1313 Dec 9, 2022
5b5026e
Fix formatting
Youssef1313 Dec 9, 2022
f23bb18
Merge branch 'main' into starts-with
Youssef1313 Dec 16, 2022
e7b76a0
Address review comments
Youssef1313 Dec 17, 2022
6605ad0
Fix build
Youssef1313 Dec 17, 2022
ebb49f1
Fix formatting
Youssef1313 Dec 17, 2022
e4dc78d
Fix build
Youssef1313 Dec 17, 2022
e42cc08
Try with elastic marker
Youssef1313 Dec 17, 2022
58ce1d6
Update generated files
Youssef1313 Dec 17, 2022
167b826
Revert "Try with elastic marker"
Youssef1313 Dec 17, 2022
437db82
Update test
Youssef1313 Dec 17, 2022
d0a198d
Run pack
Youssef1313 Dec 17, 2022
b1ad36e
Revert "Revert "Try with elastic marker""
Youssef1313 Dec 17, 2022
c358bfb
revert
Youssef1313 Dec 17, 2022
d22ec17
Apply suggestions from code review
buyaa-n Dec 19, 2022
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
@@ -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);
}
}
}
1 change: 1 addition & 0 deletions src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
Expand Up @@ -10,6 +10,7 @@ CA1512 | Maintainability | Info | UseExceptionThrowHelpers, [Documentation](http
CA1513 | Maintainability | Info | UseExceptionThrowHelpers, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1513)
CA1856 | Performance | Error | ConstantExpectedAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1856)
CA1857 | Performance | Warning | ConstantExpectedAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1857)
CA1858 | Performance | Info | UseStartsWithInsteadOfIndexOfComparisonWithZero, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1858)

### Removed Rules

Expand Down
Expand Up @@ -2031,6 +2031,18 @@
<data name="PreventNumericIntPtrUIntPtrBehavioralChangesConversionThrowsMessage" xml:space="preserve">
<value>Starting with .NET 7 the explicit conversion '{0}' will throw when overflowing in a checked context. Wrap the expression with an 'unchecked' statement to restore the .NET 6 behavior.</value>
</data>
<data name="UseStartsWithInsteadOfIndexOfComparisonWithZeroCodeFixTitle" xml:space="preserve">
<value>Use 'StartsWith'</value>
</data>
<data name="UseStartsWithInsteadOfIndexOfComparisonWithZeroDescription" xml:space="preserve">
<value>It is both clearer and faster to use 'StartsWith' instead of comparing the result of 'IndexOf' to zero.</value>
</data>
<data name="UseStartsWithInsteadOfIndexOfComparisonWithZeroMessage" xml:space="preserve">
<value>Use 'StartsWith' instead of comparing the result of 'IndexOf' to 0</value>
</data>
<data name="UseStartsWithInsteadOfIndexOfComparisonWithZeroTitle" xml:space="preserve">
<value>Use 'StartsWith' instead of 'IndexOf'</value>
</data>
<data name="UseArgumentNullExceptionThrowHelperTitle" xml:space="preserve">
<value>Use ArgumentNullException throw helper</value>
</data>
Expand All @@ -2052,5 +2064,4 @@
<data name="UseThrowHelperFix" xml:space="preserve">
<value>Use '{0}.{1}'</value>
</data>

</root>
</root>
@@ -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);
}
},
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
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;
}
}
}
@@ -0,0 +1,158 @@
// 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 startsWith in stringType.GetMembers("StartsWith"))
{
if (startsWith is IMethodSymbol startsWithMethod)
{
hasAnyStartsWith = true;
if (startsWithMethod.Parameters is [{ Type.SpecialType: SpecialType.System_Char }])
{
hasStartsWithCharOverload = true;
break;
}
}
}
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved

if (!hasAnyStartsWith)
{
return;
}

var indexOfMethodsBuilder = ImmutableArray.CreateBuilder<(IMethodSymbol IndexOfSymbol, string OverloadPropertyValue)>();
foreach (var indexOf in stringType.GetMembers("IndexOf"))
{
if (indexOf is IMethodSymbol indexOfMethod)
{
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));
}
}
}
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved

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;
}
}
}