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

Analyzer and fixer that recommend case insensitive string comparison #6662

Merged
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
c9b4479
Resources and documentation
carlossanlop May 31, 2023
d85c6a8
Analyzer
carlossanlop May 31, 2023
ab3965f
Fixer
carlossanlop May 31, 2023
9516781
Base test class
carlossanlop May 31, 2023
1a407d4
C# tests
carlossanlop May 31, 2023
5d202a2
VisualBasic tests
carlossanlop May 31, 2023
27acb1c
Address suggestion: Inline localizable strings that are used only once.
carlossanlop May 31, 2023
4298b5c
Address suggestion: Prefer GetSpecialType for string.
carlossanlop May 31, 2023
d6a193c
Use explicit.
carlossanlop May 31, 2023
2ca2441
Do not pass argument to diagnostic that does not need it.
carlossanlop May 31, 2023
c441153
Address suggestion: Use Length instead of Count()
carlossanlop May 31, 2023
8de0cd9
Address suggestion: Improve name of caseChangingMethodName
carlossanlop May 31, 2023
4da0528
Address suggestion: Remove token cancellation handling code
carlossanlop May 31, 2023
0011a6a
Address suggestion: Remove unused cancellation token argument.
carlossanlop May 31, 2023
0bfbc0b
Address suggestion: Add missing fixer attribute, seal class
carlossanlop May 31, 2023
5c16a10
Analyzer: Remove unused using.
carlossanlop Jun 1, 2023
492275d
Address suggestion: Use better test pattern. Simplify tests.
carlossanlop Jun 1, 2023
1098548
More tests
carlossanlop Jun 1, 2023
1a2f5ec
Address suggestion: Await tests.
carlossanlop Jun 1, 2023
1617027
Fix spacing warning
carlossanlop Jun 1, 2023
75109c0
Handle all 3 IndexOf overloads that can be converted to the StringCom…
carlossanlop Jun 1, 2023
0de635c
Address suggestion: Walk up parenthesized operations
carlossanlop Jun 1, 2023
a5e4f97
Add "No diagnostic" tests
carlossanlop Jun 2, 2023
f8d5688
Do not fix CompareTo, only suggest.
carlossanlop Jun 2, 2023
5961fab
Remove unused CompareTo fixer code and fix some warnings.
carlossanlop Jun 2, 2023
fee0f45
Add missing entry for second variant of diagnostic rule message in "R…
carlossanlop Jun 2, 2023
fb4f324
Revert md and sarif changes to fix CI
carlossanlop Jun 2, 2023
673d04c
Address suggestion: Use WalkUpParentheses
carlossanlop Jun 2, 2023
349a06c
Add short string for the codefix CodeAction title
carlossanlop Jun 2, 2023
dd47ee2
Consume codefix CodeAction short string in title
carlossanlop Jun 2, 2023
59c0115
Fix CI
carlossanlop Jun 4, 2023
21cfaaf
Remove some unnecessary fixer code
carlossanlop Jun 4, 2023
cb83e9e
Use better equivalenceKey
carlossanlop Jun 4, 2023
d73eb5c
Diagnose inverted pattern
carlossanlop Jun 4, 2023
7ede45e
Tests for named arguments
carlossanlop Jun 4, 2023
ea7d45d
Change resources to be able to address named parameters
carlossanlop Jun 6, 2023
f37c39e
src: Address named parameters
carlossanlop Jun 6, 2023
3f7a020
tests: Adjust tests to verify named parameters
carlossanlop Jun 6, 2023
801fb99
Move ExportCodeFixProvider and Shared attributes to derived types.
carlossanlop Jun 15, 2023
84745f0
public sealed fixers
carlossanlop Jun 15, 2023
2d3cf5b
Improved resource description mentioning allocation. Use single quote…
carlossanlop Jun 15, 2023
986973c
Remove commented code
carlossanlop Jun 15, 2023
0a2182e
Update xlfs
carlossanlop Jun 15, 2023
4778992
Remove commented code
carlossanlop Jun 15, 2023
e2b4b95
Rename to GetNewArguments
carlossanlop Jun 16, 2023
e570710
Merge remote-tracking branch 'dotnet/main' into RecommendCaseInsensit…
carlossanlop Jun 20, 2023
efabebc
Fix build
carlossanlop Jun 20, 2023
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,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.Generic;
using System.Diagnostics;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.NetCore.Analyzers.Performance;

namespace Microsoft.NetCore.CSharp.Analyzers.Performance
{
using RCISCAnalyzer = RecommendCaseInsensitiveStringComparisonAnalyzer;

internal class CSharpRecommendCaseInsensitiveStringComparisonFixer : RecommendCaseInsensitiveStringComparisonFixer
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
{
protected override List<SyntaxNode> GetExistingArguments(SyntaxGenerator generator, IInvocationOperation mainInvocationOperation,
INamedTypeSymbol stringComparisonType, out SyntaxNode? mainInvocationInstance)
{
List<SyntaxNode> arguments = new();
bool isAnyArgumentNamed = false;

InvocationExpressionSyntax invocationExpression = (InvocationExpressionSyntax)mainInvocationOperation.Syntax;

string? caseChangingApproachName = null;
bool isChangingCaseInArgument = false;

mainInvocationInstance = null;

if (invocationExpression.Expression is MemberAccessExpressionSyntax memberAccessExpression)
{
var internalExpression = memberAccessExpression.Expression is ParenthesizedExpressionSyntax parenthesizedExpression ?
parenthesizedExpression.Expression :
memberAccessExpression.Expression;

if (internalExpression is InvocationExpressionSyntax internalInvocationExpression &&
internalInvocationExpression.Expression is MemberAccessExpressionSyntax internalMemberAccessExpression)
{
mainInvocationInstance = internalMemberAccessExpression.Expression;
caseChangingApproachName = GetCaseChangingApproach(internalMemberAccessExpression.Name.Identifier.ValueText);
}
else
{
mainInvocationInstance = memberAccessExpression.Expression;
isChangingCaseInArgument = true;
}
}

foreach (ArgumentSyntax node in invocationExpression.ArgumentList.Arguments)
{
string? argumentName = node.NameColon?.Name.Identifier.ValueText;
isAnyArgumentNamed |= argumentName != null;

ExpressionSyntax argumentExpression = node.Expression is ParenthesizedExpressionSyntax argumentParenthesizedExpression ?
argumentParenthesizedExpression.Expression :
node.Expression;

MemberAccessExpressionSyntax? argumentMemberAccessExpression = null;
if (argumentExpression is InvocationExpressionSyntax argumentInvocationExpression &&
(argumentMemberAccessExpression = argumentInvocationExpression.Expression as MemberAccessExpressionSyntax) != null)
{
caseChangingApproachName = GetCaseChangingApproach(argumentMemberAccessExpression.Name.Identifier.ValueText);
}

SyntaxNode newArgumentNode;
if (isChangingCaseInArgument)
{
if (argumentMemberAccessExpression != null)
{
newArgumentNode = argumentName == RCISCAnalyzer.StringParameterName ?
generator.Argument(RCISCAnalyzer.StringParameterName, RefKind.None, argumentMemberAccessExpression.Expression) :
generator.Argument(argumentMemberAccessExpression.Expression);
}
else
{
newArgumentNode = node;
}
}
else
{
newArgumentNode = node;
}

arguments.Add(newArgumentNode);
}

Debug.Assert(caseChangingApproachName != null);
Debug.Assert(mainInvocationInstance != null);

SyntaxNode stringComparisonArgument = GetNewStringComparisonArgument(generator,
stringComparisonType, caseChangingApproachName!, isAnyArgumentNamed);

arguments.Add(stringComparisonArgument);

return arguments;
}
}
}
1 change: 1 addition & 0 deletions src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ CA1858 | Performance | Info | UseStartsWithInsteadOfIndexOfComparisonWithZero, [
CA1859 | Performance | Info | UseConcreteTypeAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1859)
CA1860 | Performance | Info | PreferLengthCountIsEmptyOverAnyAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1860)
CA1861 | Performance | Info | AvoidConstArrays, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1861)
CA1862 | Performance | Info | RecommendCaseInsensitiveStringComparison, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1862)
CA2021 | Reliability | Warning | DoNotCallEnumerableCastOrOfTypeWithIncompatibleTypesAnalyzer, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2021)

### Removed Rules
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2054,4 +2054,25 @@ Widening and user defined conversions are not supported with generic types.</val
<data name="PreferIsEmptyOverAnyMessage" xml:space="preserve">
<value>Prefer an 'IsEmpty' check rather than using 'Any()', both for clarity and for performance</value>
</data>
<data name="RecommendCaseInsensitiveStringComparerStringComparisonCodeFixTitle" xml:space="preserve">
<value>Use the string.{0}(string, StringComparison) overload</value>
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="RecommendCaseInsensitiveStringComparerTitle" xml:space="preserve">
<value>Prefer using 'StringComparer' to perform case-insensitive string comparisons</value>
</data>
<data name="RecommendCaseInsensitiveStringComparerDescription" xml:space="preserve">
<value>Avoid calling 'ToLower', 'ToUpper', 'ToLowerInvariant' and 'ToUpperInvariant' to perform case-insensitive string comparisons when using 'CompareTo'. Instead, use 'StringComparer' to perform case-insensitive comparisons.</value>
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="RecommendCaseInsensitiveStringComparerMessage" xml:space="preserve">
<value>Prefer using 'StringComparer' to perform a case-insensitive comparison</value>
</data>
<data name="RecommendCaseInsensitiveStringComparisonTitle" xml:space="preserve">
<value>Prefer the 'StringComparison' method overloads to perform case-insensitive string comparisons</value>
</data>
<data name="RecommendCaseInsensitiveStringComparisonDescription" xml:space="preserve">
<value>Avoid calling 'ToLower', 'ToUpper', 'ToLowerInvariant' and 'ToUpperInvariant' to perform case-insensitive string comparisons. Instead, prefer calling the method overloads of 'Contains', 'IndexOf' and 'StartsWith' that take a 'StringComparison' enum value to perform case-insensitive comparisons.</value>
</data>
<data name="RecommendCaseInsensitiveStringComparisonMessage" xml:space="preserve">
<value>Prefer the string comparison method overload of '{0}' that takes a 'StringComparison' enum value to perform a case-insensitive comparison</value>
</data>
</root>
Original file line number Diff line number Diff line change
@@ -0,0 +1,238 @@
// 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 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;

/// <summary>
/// CA1862: Prefer the StringComparison method overloads to perform case-insensitive string comparisons.
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
public sealed class RecommendCaseInsensitiveStringComparisonAnalyzer : DiagnosticAnalyzer
{
internal const string RuleId = "CA1862";

internal const string StringComparisonInvariantCultureIgnoreCaseName = "InvariantCultureIgnoreCase";
internal const string StringComparisonCurrentCultureIgnoreCaseName = "CurrentCultureIgnoreCase";
internal const string StringToLowerMethodName = "ToLower";
internal const string StringToUpperMethodName = "ToUpper";
internal const string StringToLowerInvariantMethodName = "ToLowerInvariant";
internal const string StringToUpperInvariantMethodName = "ToUpperInvariant";
internal const string StringContainsMethodName = "Contains";
internal const string StringIndexOfMethodName = "IndexOf";
internal const string StringStartsWithMethodName = "StartsWith";
internal const string StringCompareToMethodName = "CompareTo";
internal const string StringComparerCompareMethodName = "Compare";
internal const string StringParameterName = "value";
internal const string StringComparisonParameterName = "comparisonType";

internal static readonly DiagnosticDescriptor RecommendCaseInsensitiveStringComparisonRule = DiagnosticDescriptorHelper.Create(
RuleId,
CreateLocalizableResourceString(nameof(RecommendCaseInsensitiveStringComparisonTitle)),
CreateLocalizableResourceString(nameof(RecommendCaseInsensitiveStringComparisonMessage)),
DiagnosticCategory.Performance,
RuleLevel.IdeSuggestion,
CreateLocalizableResourceString(nameof(RecommendCaseInsensitiveStringComparisonDescription)),
isPortedFxCopRule: false,
isDataflowRule: false);

internal static readonly DiagnosticDescriptor RecommendCaseInsensitiveStringComparerRule = DiagnosticDescriptorHelper.Create(
RuleId,
CreateLocalizableResourceString(nameof(RecommendCaseInsensitiveStringComparerTitle)),
CreateLocalizableResourceString(nameof(RecommendCaseInsensitiveStringComparerMessage)),
DiagnosticCategory.Performance,
RuleLevel.IdeSuggestion,
CreateLocalizableResourceString(nameof(RecommendCaseInsensitiveStringComparerDescription)),
isPortedFxCopRule: false,
isDataflowRule: false);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(
RecommendCaseInsensitiveStringComparisonRule, RecommendCaseInsensitiveStringComparerRule);

public override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
context.RegisterCompilationStartAction(AnalyzeCompilationStart);
}

private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
{
// Retrieve the essential types: string, StringComparison, StringComparer

INamedTypeSymbol stringType = context.Compilation.GetSpecialType(SpecialType.System_String);
INamedTypeSymbol int32Type = context.Compilation.GetSpecialType(SpecialType.System_Int32);

if (!context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemStringComparison, out INamedTypeSymbol? stringComparisonType))
{
return;
}

if (!context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemStringComparer, out INamedTypeSymbol? stringComparerType))
{
return;
}

// Retrieve the offending parameterless methods: ToLower, ToLowerInvariant, ToUpper, ToUpperInvariant

IMethodSymbol? toLowerParameterlessMethod = stringType.GetMembers(StringToLowerMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos();
if (toLowerParameterlessMethod == null)
{
return;
}

IMethodSymbol? toLowerInvariantParameterlessMethod = stringType.GetMembers(StringToLowerInvariantMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos();
if (toLowerInvariantParameterlessMethod == null)
{
return;
}

IMethodSymbol? toUpperParameterlessMethod = stringType.GetMembers(StringToUpperMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos();
if (toUpperParameterlessMethod == null)
{
return;
}

IMethodSymbol? toUpperInvariantParameterlessMethod = stringType.GetMembers(StringToUpperInvariantMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos();
if (toUpperInvariantParameterlessMethod == null)
{
return;
}

// Create the different expected parameter combinations

ParameterInfo[] stringParameter = new[]
{
ParameterInfo.GetParameterInfo(stringType)
};

ParameterInfo[] stringInt32Parameters = new[]
{
ParameterInfo.GetParameterInfo(stringType),
ParameterInfo.GetParameterInfo(int32Type)
};

ParameterInfo[] stringInt32Int32Parameters = new[]
{
ParameterInfo.GetParameterInfo(stringType),
ParameterInfo.GetParameterInfo(int32Type),
ParameterInfo.GetParameterInfo(int32Type)
};

// Retrieve the diagnosable string overload methods: Contains, IndexOf (3 overloads), StartsWith, CompareTo

// Contains(string)
IMethodSymbol? containsStringMethod = stringType.GetMembers(StringContainsMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos(stringParameter);
if (containsStringMethod == null)
{
return;
}

// StartsWith(string)
IMethodSymbol? startsWithStringMethod = stringType.GetMembers(StringStartsWithMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos(stringParameter);
if (startsWithStringMethod == null)
{
return;
}

IEnumerable<IMethodSymbol> indexOfMethods = stringType.GetMembers(StringIndexOfMethodName).OfType<IMethodSymbol>();

// IndexOf(string)
IMethodSymbol? indexOfStringMethod = indexOfMethods.GetFirstOrDefaultMemberWithParameterInfos(stringParameter);
if (indexOfStringMethod == null)
{
return;
}

// IndexOf(string, int startIndex)
IMethodSymbol? indexOfStringInt32Method = indexOfMethods.GetFirstOrDefaultMemberWithParameterInfos(stringInt32Parameters);
if (indexOfStringInt32Method == null)
{
return;
}

// IndexOf(string, int startIndex, int count)
IMethodSymbol? indexOfStringInt32Int32Method = indexOfMethods.GetFirstOrDefaultMemberWithParameterInfos(stringInt32Int32Parameters);
if (indexOfStringInt32Int32Method == null)
{
return;
}

// CompareTo(string)
IMethodSymbol? compareToStringMethod = stringType.GetMembers(StringCompareToMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos(stringParameter);
if (compareToStringMethod == null)
{
return;
}

// Retrieve the StringComparer properties that need to be flagged: CurrentCultureIgnoreCase, InvariantCultureIgnoreCase

IEnumerable<IPropertySymbol> ccicPropertyGroup = stringComparerType.GetMembers(StringComparisonCurrentCultureIgnoreCaseName).OfType<IPropertySymbol>();
if (!ccicPropertyGroup.Any())
{
return;
}

IEnumerable<IPropertySymbol> icicPropertyGroup = stringComparerType.GetMembers(StringComparisonInvariantCultureIgnoreCaseName).OfType<IPropertySymbol>();
if (!icicPropertyGroup.Any())
{
return;
}

context.RegisterOperationAction(context =>
{
IInvocationOperation caseChangingInvocation = (IInvocationOperation)context.Operation;
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
IMethodSymbol caseChangingMethod = caseChangingInvocation.TargetMethod;

if (!caseChangingMethod.Equals(toLowerParameterlessMethod) &&
!caseChangingMethod.Equals(toLowerInvariantParameterlessMethod) &&
!caseChangingMethod.Equals(toUpperParameterlessMethod) &&
!caseChangingMethod.Equals(toUpperInvariantParameterlessMethod))
{
return;
}

// Ignore parenthesized operations
IOperation? ancestor = caseChangingInvocation.WalkUpParentheses().WalkUpConversion().Parent;

IInvocationOperation diagnosableInvocation;
if (ancestor is IInvocationOperation invocationAncestor)
{
diagnosableInvocation = invocationAncestor;
}
else if (ancestor is IArgumentOperation argumentAncestor && argumentAncestor.Parent is IInvocationOperation argumentInvocationAncestor)
{
diagnosableInvocation = argumentInvocationAncestor;
}
else
Copy link
Contributor

@mavasani mavasani Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider also detecting simple patterns across statements, such as below?

var loweredStr = str.ToLower();
if (loweredStr.Contains(anotherStr))

We would need to check that we have exactly one reference to the intermediate temporary variable.

If yes, we already added another analyzer which implements a simple heuristic for detecting above patterns: https://github.com/dotnet/roslyn-analyzers/blob/main/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/PreferStringContainsOverIndexOfAnalyzer.cs. If not for v1, but feel it may be a worthwhile enhancement, you may also consider filing a tracking issue for this enhancement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, V2. Opened: #6689

{
return;
}

IMethodSymbol diagnosableMethod = diagnosableInvocation.TargetMethod;

if (diagnosableMethod.Equals(containsStringMethod) ||
diagnosableMethod.Equals(startsWithStringMethod) ||
diagnosableMethod.Equals(indexOfStringMethod) ||
diagnosableMethod.Equals(indexOfStringInt32Method) ||
diagnosableMethod.Equals(indexOfStringInt32Int32Method))
{
context.ReportDiagnostic(diagnosableInvocation.CreateDiagnostic(RecommendCaseInsensitiveStringComparisonRule, diagnosableMethod.Name));
}
else if (diagnosableMethod.Equals(compareToStringMethod))
{
context.ReportDiagnostic(diagnosableInvocation.CreateDiagnostic(RecommendCaseInsensitiveStringComparerRule));
}
Comment on lines +231 to +234
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this will be a false-negative in some cases. Previously, I mentioned that the codefix produces code that is semantically different. Is it a false negative in the first place?

Or maybe report CompareTo under a separate ID to give user more control to disable it alone?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed your concern about CompareTo changing the semantics. I special-cased CompareTo so that it only triggers the analyzer but not the codefix. Check out the unit tests to see how I separate them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this comment is about whether we should actually warn or not, and if we should warn, whether it should be a separate ID or not.
Per #6662 (comment), I'm personally okay with doing this in a follow-up (unless @mavasani thinks this should be in this PR)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah got it. Yes, let's continue the discussion separately.

}, OperationKind.Invocation);
}
}
}
Loading