Address CA1862 cases to diagnose and to fix and improve messages (#6928)

* Fix incorrect test cases, add missing tests, add comments.

* Ensure the tests for "diagnostic but no fix" are actually not fixing it.

* Improve messages

* More test fixes

* Modify the analyzer and fixer logic, still need to figure out parenthesis walk up

* Apply suggestions

* Fix formatting

* msbuild -t:pack and xlfs

* Additional test adjustments.

* Fix parenthesis handling.

* Address roslyn suggestions

* Fix roslyn suggestions

* Fix roslyn suggestions

* Fix test cases for IndexOf with named unordered arguments.

* Use string constant instead of loading it.

* Revert the code fix title prefixes. Move the Equals one next to the other two.

* Update messages with suggestions

Author: carlossanlop

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

@@ -17,70 +17,91 @@ namespace Microsoft.NetCore.CSharp.Analyzers.Performance
     [ExportCodeFixProvider(LanguageNames.CSharp), Shared]
     public sealed class CSharpRecommendCaseInsensitiveStringComparisonFixer : RecommendCaseInsensitiveStringComparisonFixer
     {
-        protected override IEnumerable<SyntaxNode> GetNewArgumentsForInvocation(SyntaxGenerator generator, string caseChangingApproachValue, IInvocationOperation mainInvocationOperation,
-            INamedTypeSymbol stringComparisonType, out SyntaxNode? mainInvocationInstance)
+        protected override IEnumerable<SyntaxNode> GetNewArgumentsForInvocation(SyntaxGenerator generator,
+            string caseChangingApproachValue, IInvocationOperation mainInvocationOperation, INamedTypeSymbol stringComparisonType,
+            string? leftOffendingMethod, string? rightOffendingMethod, out SyntaxNode? mainInvocationInstance)
         {
-            List<SyntaxNode> arguments = new();
-            bool isAnyArgumentNamed = false;
-
             InvocationExpressionSyntax invocationExpression = (InvocationExpressionSyntax)mainInvocationOperation.Syntax;
 
-            bool isChangingCaseInArgument = false;
             mainInvocationInstance = null;
 
             if (invocationExpression.Expression is MemberAccessExpressionSyntax memberAccessExpression)
             {
-                ExpressionSyntax internalExpression = memberAccessExpression.Expression is ParenthesizedExpressionSyntax parenthesizedExpression ?
-                    parenthesizedExpression.Expression :
-                    memberAccessExpression.Expression;
+                ExpressionSyntax internalExpression = memberAccessExpression.Expression;
+                while (internalExpression is ParenthesizedExpressionSyntax parenthesizedExpression)
+                {
+                    internalExpression = parenthesizedExpression.Expression;
+                }
 
-                if (internalExpression is InvocationExpressionSyntax internalInvocationExpression &&
-                    internalInvocationExpression.Expression is MemberAccessExpressionSyntax internalMemberAccessExpression)
+                if (leftOffendingMethod != null &&
+                    internalExpression is InvocationExpressionSyntax internalInvocationExpression &&
+                    internalInvocationExpression.Expression is MemberAccessExpressionSyntax internalMemberAccessExpression &&
+                    internalMemberAccessExpression.Name.Identifier.Text == leftOffendingMethod)
                 {
+                    // We know we have an instance invocation that is an offending method, retrieve just the instance
                     mainInvocationInstance = internalMemberAccessExpression.Expression;
                 }
                 else
                 {
                     mainInvocationInstance = memberAccessExpression.Expression;
-                    isChangingCaseInArgument = true;
                 }
             }
 
-            foreach (ArgumentSyntax node in invocationExpression.ArgumentList.Arguments)
-            {
-                string? argumentName = node.NameColon?.Name.Identifier.ValueText;
-                isAnyArgumentNamed |= argumentName != null;
+            List<SyntaxNode> arguments = new();
+            bool isAnyArgumentNamed = false;
 
-                ExpressionSyntax argumentExpression = node.Expression is ParenthesizedExpressionSyntax argumentParenthesizedExpression ?
-                    argumentParenthesizedExpression.Expression :
-                    node.Expression;
+            foreach (IArgumentOperation arg in mainInvocationOperation.Arguments)
+            {
+                SyntaxNode newArgumentNode;
 
-                MemberAccessExpressionSyntax? argumentMemberAccessExpression = null;
-                if (argumentExpression is InvocationExpressionSyntax argumentInvocationExpression)
+                // When accessing the main invocation operation arguments, the bottom operation is retrieved
+                // so we need to go up until we find the actual argument syntax ancestor, and skip through any
+                // parenthesized syntax nodes
+                SyntaxNode actualArgumentNode = arg.Syntax;
+                while (actualArgumentNode is not ArgumentSyntax)
                 {
-                    argumentMemberAccessExpression = argumentInvocationExpression.Expression as MemberAccessExpressionSyntax;
+                    actualArgumentNode = actualArgumentNode.Parent!;
                 }
 
-                SyntaxNode newArgumentNode;
-                if (isChangingCaseInArgument)
+                string? argumentName = ((ArgumentSyntax)actualArgumentNode).NameColon?.Name.Identifier.ValueText;
+                isAnyArgumentNamed |= argumentName != null;
+
+                // The arguments could be named and out of order, so we need to detect the string parameter
+                // and remove the offending invocation if there is one
+                if (rightOffendingMethod != null && arg.Parameter?.Type?.Name == StringTypeName)
                 {
-                    if (argumentMemberAccessExpression != null)
+                    ExpressionSyntax? desiredExpression = null;
+                    if (arg.Syntax is ArgumentSyntax argumentExpression)
+                    {
+                        desiredExpression = argumentExpression.Expression;
+                        while (desiredExpression is ParenthesizedExpressionSyntax parenthesizedExpression)
+                        {
+                            desiredExpression = parenthesizedExpression.Expression;
+                        }
+                    }
+                    else if (arg.Syntax is InvocationExpressionSyntax argumentInvocationExpression)
+                    {
+                        desiredExpression = argumentInvocationExpression;
+                    }
+
+                    if (desiredExpression is InvocationExpressionSyntax invocation &&
+                        invocation.Expression is MemberAccessExpressionSyntax argumentMemberAccessExpression)
                     {
                         newArgumentNode = argumentName == RCISCAnalyzer.StringParameterName ?
-                            generator.Argument(RCISCAnalyzer.StringParameterName, RefKind.None, argumentMemberAccessExpression.Expression) :
-                            generator.Argument(argumentMemberAccessExpression.Expression);
+                                generator.Argument(RCISCAnalyzer.StringParameterName, RefKind.None, argumentMemberAccessExpression.Expression) :
+                                generator.Argument(argumentMemberAccessExpression.Expression);
                     }
                     else
                     {
-                        newArgumentNode = node;
+                        newArgumentNode = arg.Syntax;
                     }
                 }
                 else
                 {
-                    newArgumentNode = node;
+                    newArgumentNode = arg.Syntax;
                 }
 
-                arguments.Add(newArgumentNode.WithTriviaFrom(node));
+                arguments.Add(newArgumentNode.WithTriviaFrom(arg.Syntax));
             }
 
             Debug.Assert(mainInvocationInstance != null);

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

@@ -2024,29 +2024,29 @@ Widening and user defined conversions are not supported with generic types.</val
   <data name="RecommendCaseInsensitiveStringComparerStringComparisonCodeFixTitle" xml:space="preserve">
     <value>Use the 'string.{0}(string, StringComparison)' overload</value>
   </data>
-  <data name="RecommendCaseInsensitiveStringEqualsCodeFixTitle" xml:space="preserve">
-    <value>Use 'string.Equals(string, StringComparison)'</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', because they lead to an allocation. Instead, use 'StringComparer' to perform case-insensitive comparisons.</value>
+    <value>Avoid calling 'ToLower', 'ToUpper', 'ToLowerInvariant' and 'ToUpperInvariant' to perform case-insensitive string comparisons when using 'CompareTo', because they lead to an allocation. Instead, use 'StringComparer' to perform case-insensitive comparisons. Switching to using 'StringComparer' might cause subtle changes in behavior, so it's important to conduct thorough testing after applying the suggestion. Additionally, if a culturally sensitive comparison is not required, consider using 'StringComparer.OrdinalIgnoreCase'.</value>
   </data>
   <data name="RecommendCaseInsensitiveStringComparerMessage" xml:space="preserve">
-    <value>Prefer using 'StringComparer' to perform a case-insensitive comparison</value>
+    <value>Prefer using 'StringComparer' to perform a case-insensitive comparison, but keep in mind that this might cause subtle changes in behavior, so make sure to conduct thorough testing after applying the suggestion, or if culturally sensitive comparison is not required, consider using 'StringComparer.OrdinalIgnoreCase'</value>
+  </data>
+  <data name="RecommendCaseInsensitiveStringEqualsCodeFixTitle" xml:space="preserve">
+    <value>Use 'string.Equals(string, StringComparison)'</value>
   </data>
     <data name="RecommendCaseInsensitiveStringEqualsDescription" xml:space="preserve">
-    <value>Avoid calling 'ToLower', 'ToUpper', 'ToLowerInvariant' and 'ToUpperInvariant' to perform case-insensitive string comparisons, as in 'string.ToLower() == string.ToLower()', because they lead to an allocation. Instead, use 'string.Equals(string, StringComparison)' to perform case-insensitive comparisons.</value>
+    <value>Avoid calling 'ToLower', 'ToUpper', 'ToLowerInvariant' and 'ToUpperInvariant' to perform case-insensitive string comparisons, as in 'string.ToLower() == string.ToLower()', because they lead to an allocation. Instead, use 'string.Equals(string, StringComparison)' to perform case-insensitive comparisons. Switching to using an overload that takes a 'StringComparison' might cause subtle changes in behavior, so it's important to conduct thorough testing after applying the suggestion. Additionally, if a culturally sensitive comparison is not required, consider using 'StringComparison.OrdinalIgnoreCase'.</value>
   </data>
     <data name="RecommendCaseInsensitiveStringEqualsMessage" xml:space="preserve">
-    <value>Prefer using 'string.Equals(string, StringComparison)' to perform a case-insensitive comparison</value>
+    <value>Prefer using 'string.Equals(string, StringComparison)' to perform a case-insensitive comparison, but keep in mind that this might cause subtle changes in behavior, so make sure to conduct thorough testing after applying the suggestion, or if culturally sensitive comparison is not required, consider using 'StringComparison.OrdinalIgnoreCase'</value>
   </data>
   <data name="RecommendCaseInsensitiveStringComparisonTitle" xml:space="preserve">
-    <value>Prefer the 'StringComparison' method overloads to perform case-insensitive string comparisons</value>
+    <value>Use 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 because they lead to an allocation. Instead, prefer calling the method overloads of 'Contains', 'IndexOf' and 'StartsWith' that take a 'StringComparison' enum value to perform case-insensitive comparisons.</value>
+    <value>Avoid calling 'ToLower', 'ToUpper', 'ToLowerInvariant' and 'ToUpperInvariant' to perform case-insensitive string comparisons because they lead to an allocation. Instead, prefer calling the method overloads of 'Contains', 'IndexOf' and 'StartsWith' that take a 'StringComparison' enum value to perform case-insensitive comparisons. Switching to using an overload that takes a 'StringComparison' might cause subtle changes in behavior, so it's important to conduct thorough testing after applying the suggestion. Additionally, if a culturally sensitive comparison is not required, consider using 'StringComparison.OrdinalIgnoreCase'.</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>
+    <value>Prefer the string comparison method overload of '{0}' that takes a 'StringComparison' enum value to perform a case-insensitive comparison, but keep in mind that this might cause subtle changes in behavior, so make sure to conduct thorough testing after applying the suggestion, or if culturally sensitive comparison is not required, consider using 'StringComparison.OrdinalIgnoreCase'</value>
   </data>
   <data name="PreferDictionaryTryAddValueCodeFixTitle" xml:space="preserve">
     <value>Use 'TryAdd(TKey, TValue)'</value>