Skip to content

Commit

Permalink
Address CA1862 cases to diagnose and to fix and improve messages (#6928)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@carlossanlop
carlossanlop authored Sep 16, 2023
1 parent 6bf9333 commit 8611d76
Show file tree
Hide file tree
Showing 24 changed files with 903 additions and 527 deletions.
81 changes: 51 additions & 30 deletions ...soft.NetCore.Analyzers/Performance/CSharpRecommendCaseInsensitiveStringComparisonFixer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
20 changes: 10 additions & 10 deletions src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Loading

0 comments on commit 8611d76

Please sign in to comment.