Recommend using Equals with StringComparison instead of string.ToLower() == otherString.ToLower()

src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Performance/CSharpRecommendCaseInsensitiveStringComparisonFixer.cs

@@ -17,30 +17,27 @@ namespace Microsoft.NetCore.CSharp.Analyzers.Performance
     [ExportCodeFixProvider(LanguageNames.CSharp), Shared]
     public sealed class CSharpRecommendCaseInsensitiveStringComparisonFixer : RecommendCaseInsensitiveStringComparisonFixer
     {
-        protected override List<SyntaxNode> GetNewArguments(SyntaxGenerator generator, IInvocationOperation mainInvocationOperation,
+        protected override IEnumerable<SyntaxNode> GetNewArgumentsForInvocation(SyntaxGenerator generator, string caseChangingApproachValue, 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 ?
+                ExpressionSyntax 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
                 {
@@ -59,10 +56,9 @@ protected override List<SyntaxNode> GetNewArguments(SyntaxGenerator generator, I
                     node.Expression;
 
                 MemberAccessExpressionSyntax? argumentMemberAccessExpression = null;
-                if (argumentExpression is InvocationExpressionSyntax argumentInvocationExpression &&
-                    (argumentMemberAccessExpression = argumentInvocationExpression.Expression as MemberAccessExpressionSyntax) != null)
+                if (argumentExpression is InvocationExpressionSyntax argumentInvocationExpression)
                 {
-                    caseChangingApproachName = GetCaseChangingApproach(argumentMemberAccessExpression.Name.Identifier.ValueText);
+                    argumentMemberAccessExpression = argumentInvocationExpression.Expression as MemberAccessExpressionSyntax;
                 }
 
                 SyntaxNode newArgumentNode;
@@ -84,18 +80,23 @@ protected override List<SyntaxNode> GetNewArguments(SyntaxGenerator generator, I
                     newArgumentNode = node;
                 }
 
-                arguments.Add(newArgumentNode);
+                arguments.Add(newArgumentNode.WithTriviaFrom(node));
             }
 
-            Debug.Assert(caseChangingApproachName != null);
             Debug.Assert(mainInvocationInstance != null);
 
-            SyntaxNode stringComparisonArgument = GetNewStringComparisonArgument(generator,
-                stringComparisonType, caseChangingApproachName!, isAnyArgumentNamed);
+            SyntaxNode stringComparisonArgument = GetNewStringComparisonArgument(generator, stringComparisonType, caseChangingApproachValue, isAnyArgumentNamed);
 
             arguments.Add(stringComparisonArgument);
 
             return arguments;
         }
+
+        protected override IEnumerable<SyntaxNode> GetNewArgumentsForBinary(SyntaxGenerator generator, SyntaxNode rightNode, SyntaxNode typeMemberAccess) =>
+            new List<SyntaxNode>()
+            {
+                generator.Argument(rightNode),
+                generator.Argument(typeMemberAccess)
+            };
     }
 }

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Analyzer.cs

@@ -1,7 +1,10 @@
 // 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.Collections.Immutable;
+using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using Analyzer.Utilities;
 using Analyzer.Utilities.Extensions;
@@ -32,8 +35,13 @@ public sealed class RecommendCaseInsensitiveStringComparisonAnalyzer : Diagnosti
         internal const string StringStartsWithMethodName = "StartsWith";
         internal const string StringCompareToMethodName = "CompareTo";
         internal const string StringComparerCompareMethodName = "Compare";
+        internal const string StringEqualsMethodName = "Equals";
         internal const string StringParameterName = "value";
         internal const string StringComparisonParameterName = "comparisonType";
+        internal const string CaseChangingApproachName = "CaseChangingApproach";
+        internal const string OffendingMethodName = "OffendingMethodName";
+        internal const string LeftOffendingMethodName = "LeftOffendingMethodName";
+        internal const string RightOffendingMethodName = "RightOffendingMethodName";
 
         internal static readonly DiagnosticDescriptor RecommendCaseInsensitiveStringComparisonRule = DiagnosticDescriptorHelper.Create(
             RuleId,
@@ -115,18 +123,12 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
                 ParameterInfo.GetParameterInfo(stringType)
             };
 
-            ParameterInfo[] stringInt32Parameters = new[]
-            {
-                ParameterInfo.GetParameterInfo(stringType),
-                ParameterInfo.GetParameterInfo(int32Type)
-            };
-
-            ParameterInfo[] stringInt32Int32Parameters = new[]
+            // Equals(string)
+            IMethodSymbol? stringEqualsStringMethod = stringType.GetMembers(StringEqualsMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos(stringParameter);
+            if (stringEqualsStringMethod == null)
             {
-                ParameterInfo.GetParameterInfo(stringType),
-                ParameterInfo.GetParameterInfo(int32Type),
-                ParameterInfo.GetParameterInfo(int32Type)
-            };
+                return;
+            }
 
             // Retrieve the diagnosable string overload methods: Contains, IndexOf (3 overloads), StartsWith, CompareTo
 
@@ -153,13 +155,26 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
                 return;
             }
 
+            ParameterInfo[] stringInt32Parameters = new[]
+            {
+                ParameterInfo.GetParameterInfo(stringType),
+                ParameterInfo.GetParameterInfo(int32Type)
+            };
+
             // IndexOf(string, int startIndex)
             IMethodSymbol? indexOfStringInt32Method = indexOfMethods.GetFirstOrDefaultMemberWithParameterInfos(stringInt32Parameters);
             if (indexOfStringInt32Method == null)
             {
                 return;
             }
 
+            ParameterInfo[] stringInt32Int32Parameters = new[]
+            {
+                ParameterInfo.GetParameterInfo(stringType),
+                ParameterInfo.GetParameterInfo(int32Type),
+                ParameterInfo.GetParameterInfo(int32Type)
+            };
+
             // IndexOf(string, int startIndex, int count)
             IMethodSymbol? indexOfStringInt32Int32Method = indexOfMethods.GetFirstOrDefaultMemberWithParameterInfos(stringInt32Int32Parameters);
             if (indexOfStringInt32Int32Method == null)
@@ -188,51 +203,172 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
                 return;
             }
 
+            // a.ToLower().Method()
             context.RegisterOperationAction(context =>
             {
                 IInvocationOperation caseChangingInvocation = (IInvocationOperation)context.Operation;
-                IMethodSymbol caseChangingMethod = caseChangingInvocation.TargetMethod;
+                AnalyzeInvocation(context, caseChangingInvocation, stringType,
+                    containsStringMethod, startsWithStringMethod, compareToStringMethod,
+                    indexOfStringMethod, indexOfStringInt32Method, indexOfStringInt32Int32Method);
+            }, OperationKind.Invocation);
 
-                if (!caseChangingMethod.Equals(toLowerParameterlessMethod) &&
-                    !caseChangingMethod.Equals(toLowerInvariantParameterlessMethod) &&
-                    !caseChangingMethod.Equals(toUpperParameterlessMethod) &&
-                    !caseChangingMethod.Equals(toUpperInvariantParameterlessMethod))
-                {
-                    return;
-                }
+            // a.ToLower() == b.ToLower()
+            context.RegisterOperationAction(context =>
+            {
+                IBinaryOperation binaryOperation = (IBinaryOperation)context.Operation;
+                AnalyzeBinaryOperation(context, binaryOperation, stringType);
 
-                // Ignore parenthesized operations
-                IOperation? ancestor = caseChangingInvocation.WalkUpParentheses().WalkUpConversion().Parent;
+            }, OperationKind.Binary);
+        }
 
-                IInvocationOperation diagnosableInvocation;
-                if (ancestor is IInvocationOperation invocationAncestor)
-                {
-                    diagnosableInvocation = invocationAncestor;
-                }
-                else if (ancestor is IArgumentOperation argumentAncestor && argumentAncestor.Parent is IInvocationOperation argumentInvocationAncestor)
-                {
-                    diagnosableInvocation = argumentInvocationAncestor;
-                }
-                else
-                {
-                    return;
-                }
+        private static void AnalyzeInvocation(OperationAnalysisContext context, IInvocationOperation caseChangingInvocation, INamedTypeSymbol stringType,
+            IMethodSymbol containsStringMethod, IMethodSymbol startsWithStringMethod, IMethodSymbol compareToStringMethod,
+            IMethodSymbol indexOfStringMethod, IMethodSymbol indexOfStringInt32Method, IMethodSymbol indexOfStringInt32Int32Method)
+        {
+            if (!IsOffendingMethod(caseChangingInvocation, stringType, out string? offendingMethodName))
+            {
+                return;
+            }
 
-                IMethodSymbol diagnosableMethod = diagnosableInvocation.TargetMethod;
+            if (!TryGetInvocationWithoutParentheses(caseChangingInvocation, out IInvocationOperation? diagnosableInvocation))
+            {
+                return;
+            }
 
-                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))
+            string caseChangingApproach = GetCaseChangingApproach(offendingMethodName);
+
+            ImmutableDictionary<string, string?> dict = new Dictionary<string, string?>()
+            {
+                { CaseChangingApproachName, caseChangingApproach }
+            }.ToImmutableDictionary();
+
+            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, dict, diagnosableMethod.Name));
+            }
+            else if (diagnosableMethod.Equals(compareToStringMethod))
+            {
+                context.ReportDiagnostic(diagnosableInvocation.CreateDiagnostic(RecommendCaseInsensitiveStringComparerRule, dict));
+            }
+        }
+
+        private static void AnalyzeBinaryOperation(OperationAnalysisContext context, IBinaryOperation binaryOperation, INamedTypeSymbol stringType)
+        {
+            if (binaryOperation.OperatorKind is not BinaryOperatorKind.Equals and not BinaryOperatorKind.NotEquals)
+            {
+                return;
+            }
+
+            bool atLeastOneOffendingInvocation = false;
+
+            string? leftOffendingMethodName = null;
+            if (TryGetInvocationWithoutParentheses(binaryOperation.LeftOperand, out IInvocationOperation? leftInvocation))
+            {
+                atLeastOneOffendingInvocation = IsOffendingMethod(leftInvocation, stringType, out leftOffendingMethodName);
+            }
+
+            string? rightOffendingMethodName = null;
+            if (TryGetInvocationWithoutParentheses(binaryOperation.RightOperand, out IInvocationOperation? rightInvocation))
+            {
+                atLeastOneOffendingInvocation = IsOffendingMethod(rightInvocation, stringType, out rightOffendingMethodName);
+            }
+
+            if (atLeastOneOffendingInvocation)
+            {
+                string caseChangingApproachValue = GetPreferredCaseChangingApproach(leftOffendingMethodName, rightOffendingMethodName);
+
+                ImmutableDictionary<string, string?> dict = new Dictionary<string, string?>()
                 {
-                    context.ReportDiagnostic(diagnosableInvocation.CreateDiagnostic(RecommendCaseInsensitiveStringComparerRule));
-                }
-            }, OperationKind.Invocation);
+                    { CaseChangingApproachName, caseChangingApproachValue }
+                }.ToImmutableDictionary();
+
+                context.ReportDiagnostic(
+                    binaryOperation.CreateDiagnostic(RecommendCaseInsensitiveStringComparerRule, dict));
+            }
+        }
+
+        private static bool TryGetInvocationWithoutParentheses(IOperation operation,
+            [NotNullWhen(returnValue: true)] out IInvocationOperation? diagnosableInvocation)
+        {
+            diagnosableInvocation = null;
+            if (operation is not IInvocationOperation invocationOperation)
+            {
+                return false;
+            }
+
+            IOperation? ancestor = invocationOperation.WalkUpParentheses().WalkUpConversion().Parent;
+
+            if (ancestor is IInvocationOperation invocationAncestor)
+            {
+                diagnosableInvocation = invocationAncestor;
+            }
+            else if (ancestor is IArgumentOperation argumentAncestor && argumentAncestor.Parent is IInvocationOperation argumentInvocationAncestor)
+            {
+                diagnosableInvocation = argumentInvocationAncestor;
+            }
+            else
+            {
+                diagnosableInvocation = invocationOperation;
+            }
+
+            return diagnosableInvocation != null;
+        }
+
+        private static bool IsOffendingMethod(IInvocationOperation invocation, ITypeSymbol stringType,
+            [NotNullWhen(returnValue: true)] out string? offendingMethodName)
+        {
+            offendingMethodName = null;
+
+            if (invocation.Instance == null || invocation.Instance.Type == null)
+            {
+                return false;
+            }
+
+            if (!invocation.Instance.Type.Equals(stringType))
+            {
+                return false;
+            }
+
+            if (!invocation.TargetMethod.Name.Equals(StringToLowerMethodName, StringComparison.Ordinal) &&
+                !invocation.TargetMethod.Name.Equals(StringToLowerInvariantMethodName, StringComparison.Ordinal) &&
+                !invocation.TargetMethod.Name.Equals(StringToUpperMethodName, StringComparison.Ordinal) &&
+                !invocation.TargetMethod.Name.Equals(StringToUpperInvariantMethodName, StringComparison.Ordinal))
+            {
+                return false;
+            }
+
+            offendingMethodName = invocation.TargetMethod.Name;
+            return true;
+        }
+
+        private static string GetCaseChangingApproach(string methodName)
+        {
+            if (methodName is StringToLowerMethodName or StringToUpperMethodName)
+            {
+                return StringComparisonCurrentCultureIgnoreCaseName;
+            }
+
+            Debug.Assert(methodName is StringToLowerInvariantMethodName or StringToUpperInvariantMethodName);
+            return StringComparisonInvariantCultureIgnoreCaseName;
+        }
+
+        private static string GetPreferredCaseChangingApproach(string? leftOffendingMethodName, string? rightOffendingMethodName)
+        {
+            if (leftOffendingMethodName is StringToLowerMethodName or StringToUpperMethodName ||
+                rightOffendingMethodName is StringToLowerMethodName or StringToUpperMethodName)
+            {
+                return StringComparisonCurrentCultureIgnoreCaseName;
+            }
+
+            Debug.Assert(leftOffendingMethodName is StringToLowerInvariantMethodName or StringToUpperInvariantMethodName ||
+                         rightOffendingMethodName is StringToLowerInvariantMethodName or StringToUpperInvariantMethodName);
+            return StringComparisonInvariantCultureIgnoreCaseName;
         }
     }
 }