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 16 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
}
}
}
1 change: 1 addition & 0 deletions src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -2031,6 +2031,15 @@
<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="UseStartsWithInsteadOfIndexOfComparisonWithZeroDescription" xml:space="preserve">
<value>It is both clearer and likely faster to use 'StartsWith' instead of comparing the result of 'IndexOf' to zero.</value>
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
</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'</value>
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="UseArgumentNullExceptionThrowHelperTitle" xml:space="preserve">
<value>Use ArgumentNullException throw helper</value>
</data>
Expand All @@ -2052,5 +2061,4 @@
<data name="UseThrowHelperFix" xml:space="preserve">
<value>Use '{0}.{1}'</value>
</data>

</root>
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// 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.Linq;
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.UseStartsWithInsteadOfIndexOfComparisonWithZeroTitle,
jeffhandley marked this conversation as resolved.
Show resolved Hide resolved
createChangedDocument: async 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 _);

_ = 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 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 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:
var semanticModel = await document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false);
var startsWithSymbols = semanticModel.Compilation.GetSpecialType(SpecialType.System_String).GetMembers("StartsWith");
if (startsWithSymbols.Any(s => s is IMethodSymbol { Parameters: [{ Type.SpecialType: SpecialType.System_Char }] }))
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
{
return 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 document.WithSyntaxRoot(root.ReplaceNode(node, AppendElasticMarker(replacement)));

default:
Debug.Fail("This should never happen.");
return document;
}

},
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
equivalenceKey: nameof(MicrosoftNetCoreAnalyzersResources.UseStartsWithInsteadOfIndexOfComparisonWithZeroTitle)),
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;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
// 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 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: "CA1858",
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
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);
if (stringType.GetMembers("StartsWith").FirstOrDefault() is not IMethodSymbol)
{
return;
}

var indexOf = stringType.GetMembers("IndexOf").OfType<IMethodSymbol>();
var indexOfMethodsBuilder = ImmutableArray.CreateBuilder<(IMethodSymbol IndexOfSymbol, string OverloadPropertyValue)>();
AddIfNotNull((indexOf.SingleOrDefault(s => s.Parameters is [{ Type.SpecialType: SpecialType.System_String }]), OverloadString));
AddIfNotNull((indexOf.SingleOrDefault(s => s.Parameters is [{ Type.SpecialType: SpecialType.System_Char }]), OverloadChar));
AddIfNotNull((indexOf.SingleOrDefault(s => s.Parameters is [{ Type.SpecialType: SpecialType.System_String }, { Name: "comparisonType" }]), OverloadString_StringComparison));
AddIfNotNull((indexOf.SingleOrDefault(s => s.Parameters is [{ Type.SpecialType: SpecialType.System_Char }, { Name: "comparisonType" }]), 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, "");
}

context.ReportDiagnostic(binaryOperation.CreateDiagnostic(Rule, additionalLocations, properties));
}

}, OperationKind.Binary);

void AddIfNotNull((IMethodSymbol? Symbol, string OverloadPropertyValue) item)
{
if (item.Symbol is not null)
{
indexOfMethodsBuilder.Add((item.Symbol, item.OverloadPropertyValue));
}
}
});
}

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.Parameters.Length == invocation.Arguments.Length && indexOfMethod.Equals(invocation.TargetMethod, SymbolEqualityComparer.Default))
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
{
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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3007,6 +3007,21 @@
<target state="translated">Preferovat možnost Vymazat před možností Fill</target>
<note />
</trans-unit>
<trans-unit id="UseStartsWithInsteadOfIndexOfComparisonWithZeroDescription">
<source>It is both clearer and likely faster to use 'StartsWith' instead of comparing the result of 'IndexOf' to zero.</source>
<target state="new">It is both clearer and likely faster to use 'StartsWith' instead of comparing the result of 'IndexOf' to zero.</target>
<note />
</trans-unit>
<trans-unit id="UseStartsWithInsteadOfIndexOfComparisonWithZeroMessage">
<source>Use 'StartsWith' instead of comparing the result of 'IndexOf' to 0</source>
<target state="new">Use 'StartsWith' instead of comparing the result of 'IndexOf' to 0</target>
<note />
</trans-unit>
<trans-unit id="UseStartsWithInsteadOfIndexOfComparisonWithZeroTitle">
<source>Use 'StartsWith'</source>
<target state="new">Use 'StartsWith'</target>
<note />
</trans-unit>
<trans-unit id="UseStringEqualsOverStringCompareCodeFixTitle">
<source>Use 'string.Equals'</source>
<target state="translated">Použít string.Equals</target>
Expand Down
Loading