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

carlossanlop · 2023-06-29T21:10:01Z

Made sure it handles both ==/= and !=/<> (C#/VB)
Moved some code from the fixer to the analyzer and passed the result in the immutable dictionary (follow up of a suggestion by @Youssef1313)
Added trivia support for both this new case and the original

mavasani · 2023-06-30T09:04:39Z

...Microsoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Analyzer.cs

-                    !caseChangingMethod.Equals(toLowerInvariantParameterlessMethod) &&
-                    !caseChangingMethod.Equals(toUpperParameterlessMethod) &&
-                    !caseChangingMethod.Equals(toUpperInvariantParameterlessMethod))
+                if (context.Operation is IInvocationOperation caseChangingInvocation)


Nit: Alternatively, you can just register separate operation callbacks for each operation kind - up to you.

Can you show me an example? I was initially assuming this was possible but I couldn't get it to work.

@carlossanlop

context.RegisterOpationAction(context => { var caseChangingInvocation = (IInvocationOperation)context.Operation; AnalyzeInvocation(context, caseChangingInvocation, stringType, containsStringMethod, startsWithStringMethod, compareToStringMethod, indexOfStringMethod, indexOfStringInt32Method, indexOfStringInt32Int32Method); }, OperationKind.Invocation); context.RegisterOpationAction(context => { var binaryOperation = (IBinaryOperation)context.Operation; AnalyzeBinaryOperation(context, binaryOperation, stringType); }, OperationKind.Binary);

Thanks! I was doing something like that, but don't remember exactly what I was doing differently, it just wouldn't work. Let me test it again.

buyaa-n · 2023-06-30T22:34:34Z

...crosoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Base.Tests.cs

@@ -16,6 +16,16 @@ public abstract class RecommendCaseInsensitiveStringComparison_Base_Tests

        private static readonly string[] ContainsStartsWith = new[] { "Contains", "StartsWith" };
        private static readonly string[] UnnamedArgs = new[] { "", ", 1", ", 1, 1" };
+
+        private static readonly Tuple<string, string>[] CSharpComparisonOperators = new[] {


Should we use ValueTuple instead?

buyaa-n · 2023-06-30T22:37:04Z

...crosoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Base.Tests.cs

+
+        private static readonly Tuple<string, string>[] CSharpComparisonOperators = new[] {
+            Tuple.Create("==", ""),
+            Tuple.Create("!=", "!")


Shouldn't it cover a.ToLower().Equals(b.ToLower()), !a.ToLower().Equals(b.ToLower()), ... scenarios too?

Maybe. It was not contemplated in the approved API proposal. I can ask in the main issue to determine if we want that case handled too. If that's the case, I would fix it in a separate PR.

Maybe. It was not contemplated in the approved API proposal. I can ask in the main issue to determine if we want that case handled too

string == operator just same as string.Equals and != same as !string.Equals, in fact they actually calls string.Equals inside. https://github.com/dotnet/runtime/blob/0fce03e6bb0251f6e5d8abacd97d90f0a1a200f9/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs#L744-L746
I don't think its have to specifically mentioned in the API proposal

buyaa-n · 2023-06-30T22:59:21Z

...Microsoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Analyzer.cs

+                diagnosableMethod.Equals(startsWithStringMethod) ||
+                diagnosableMethod.Equals(indexOfStringMethod) ||
+                diagnosableMethod.Equals(indexOfStringInt32Method) ||
+                diagnosableMethod.Equals(indexOfStringInt32Int32Method))


It seems IMethodSymbol containsStringMethod, IMethodSymbol startsWithStringMethod, IMethodSymbol indexOfStringMethod, IMethodSymbol indexOfStringInt32Method, IMethodSymbol indexOfStringInt32Int32Method all these IMethodSymbols created and used similarly, could we add them into a ImmutableDictionary<IMethodSymbol> and check if it contains the diagnosableMethod?

Can you show me how to pass an ImmutableDictionary<IMethodSymbol>? I am not sure if any of the available CreateDiagnostic method overloads would let me do such thing.

No, sorry I meant ImmutableArray not ImmutableDictionary, I meant populate them once and add them into a ImmutableArray<IMethodSymbol> list and instead of these multiple ORs check if (list.Contains(diagnosableMethod))

I think we can save some space by not allocating a new ImmutableList.

Even though it would be faster for lookup, and it will be cleaner to use without passing so many parameters. Though my concern was more about that all symbols are loaded one by one using Linq, they could have populated with one or 2 loops, even without adding them into a ImmutableArray.

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

Lines 83 to 204 in 4e17f73

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)

};

// Equals(string)

IMethodSymbol? stringEqualsStringMethod = stringType.GetMembers(StringEqualsMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos(stringParameter);

if (stringEqualsStringMethod == null)

{

return;

}

// 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;

}

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)

{

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;

}

But as that logic was not added with this PR so its not have to be handled in this PR

buyaa-n · 2023-06-30T23:04:14Z

...Microsoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Analyzer.cs

-                if (!caseChangingMethod.Equals(toLowerParameterlessMethod) &&
-                    !caseChangingMethod.Equals(toLowerInvariantParameterlessMethod) &&
-                    !caseChangingMethod.Equals(toUpperParameterlessMethod) &&
-                    !caseChangingMethod.Equals(toUpperInvariantParameterlessMethod))


It seems these toLowerParameterlessMethod, toLowerInvariantParameterlessMethod, toUpperParameterlessMethod, toUpperInvariantParameterlessMethod symbols not used anymore, could we remove the rows populating them?

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

Lines 95 to 117 in 79db421

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;

}

Don't we need to exit early if we don't find the symbol?

That was actually part of the question, these symbols are not used anywhere, should we keep loading them and exit early? Or they required to be existed to proceed further analysis?

They are used here to exit early if the symbols aren't found.

buyaa-n · 2023-06-30T23:17:51Z

...Microsoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Analyzer.cs

+            ImmutableDictionary<string, string?> dict = new Dictionary<string, string?>()
+            {
+                { CaseChangingApproachName, caseChangingApproach }
+            }.ToImmutableDictionary();


NIT:

Suggested change

ImmutableDictionary<string, string?> dict = new Dictionary<string, string?>()

{

{ CaseChangingApproachName, caseChangingApproach }

}.ToImmutableDictionary();

ImmutableDictionary<string, string> dict = ImmutableDictionary.CreateRange(

new Dictionary<string, string> { { CaseChangingApproachName, caseChangingApproach } });

Is that actually better? I am still allocating a dictionary with the suggestion, so I'm not sure if there's an improvement.
BTW the second typeparam should be a string?, otherwise it complains.

I'm not sure if there's an improvement.

I would expect it to be better, though do not know for sure, that is why I put NIT

carlossanlop

Thanks for your reviews, @buyaa-n, @Youssef1313 and @mavasani, and I appreciate the patience, I had to address some higher priority tasks before I could get to check your feedback.

I addressed most of the feedback and left questions for some of the rest. Can you please take another look?

Edit: CI was failing due to some recent nullability changes in the main branch. I rebased and fixed the issues, then force pushed.

…riant/invariant.

buyaa-n · 2023-07-18T16:22:42Z

...crosoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Base.Tests.cs

+        private static readonly ValueTuple<string, string>[] Cultures = new[] {
+            ValueTuple.Create("ToLower", "CurrentCultureIgnoreCase"),
+            ValueTuple.Create("ToUpper", "CurrentCultureIgnoreCase"),
+            ValueTuple.Create("ToLowerInvariant", "InvariantCultureIgnoreCase"),
+            ValueTuple.Create("ToUpperInvariant", "InvariantCultureIgnoreCase")


Suggested change

private static readonly ValueTuple<string, string>[] Cultures = new[] {

ValueTuple.Create("ToLower", "CurrentCultureIgnoreCase"),

ValueTuple.Create("ToUpper", "CurrentCultureIgnoreCase"),

ValueTuple.Create("ToLowerInvariant", "InvariantCultureIgnoreCase"),

ValueTuple.Create("ToUpperInvariant", "InvariantCultureIgnoreCase")

private static readonly (string, string)[] Cultures = new[] {

("ToLower", "CurrentCultureIgnoreCase"),

("ToUpper", "CurrentCultureIgnoreCase"),

("ToLowerInvariant", "InvariantCultureIgnoreCase"),

("ToUpperInvariant", "InvariantCultureIgnoreCase")

I like it. But it makes me sad that we don't have that analyzer enabled in this repo, I didn't get the automatic suggestion.

buyaa-n · 2023-07-18T16:23:52Z

...crosoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Base.Tests.cs

+        private static readonly ValueTuple<string, string>[] CSharpComparisonOperators = new[] {
+            ValueTuple.Create("==", ""),
+            ValueTuple.Create("!=", "!")
+        };
+        private static readonly ValueTuple<string, string>[] VisualBasicComparisonOperators = new[] {
+            ValueTuple.Create("=", ""),
+            ValueTuple.Create("<>", "Not ")
+        };


Suggested change

private static readonly ValueTuple<string, string>[] CSharpComparisonOperators = new[] {

ValueTuple.Create("==", ""),

ValueTuple.Create("!=", "!")

};

private static readonly ValueTuple<string, string>[] VisualBasicComparisonOperators = new[] {

ValueTuple.Create("=", ""),

ValueTuple.Create("<>", "Not ")

};

private static readonly (string, string)[] CSharpComparisonOperators = new[] {

("==", ""),

("!=", "!")

};

private static readonly (string, string)[] VisualBasicComparisonOperators = new[] {

("=", ""),

("<>", "Not ")

};

buyaa-n

Looks good with some comments:

Fixes [Analyzer] string.ToLower() == otherString.ToLower() runtime#78607

The PR doesn't add a new diagnostic nor new messaging, just want to make sure that the
existing messaging and ID could apply as is for [Analyzer] string.ToLower() == otherString.ToLower() runtime#78607
Please file an issue if you plan to cover a.ToLower().Equals(b.ToLower()), !a.ToLower().Equals(b.ToLower()) in tests later

Bellarmine-Head · 2023-07-22T08:38:32Z

I think defaulting to one of the culture-ignore-case options is unsafe, and would suggest you at least consider OrdinalIgnoreCase before going ahead further with this, and related analyzers.

I refer you to:- dotnet/runtime#78442 (comment)

Also: https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#choosing-a-stringcomparison-member-for-your-method-call

carlossanlop · 2023-07-26T18:39:58Z

The PR doesn't add a new diagnostic nor new messaging, just want to make sure that the
existing messaging and ID could apply as is for [Analyzer] string.ToLower() == otherString.ToLower() runtime#78607

You're right, @buyaa-n. Maybe I should add a new, more appropriate message specifically for the cases handled in this PR. Let me send a new commit.

…ng.Method()' comparison case.

buyaa-n · 2023-07-26T21:09:57Z

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

+  </data>
+    <data name="RecommendCaseInsensitiveStringEqualsTitle" xml:space="preserve">
+    <value>Prefer using 'string.Equals(string, StringComparison)' to perform case-insensitive string comparisons</value>
+  </data>


Seems this is using the same ID as the existing string comparison analyzers, this could cause an issue or even if it is not cause an issue I believe only one of RecommendCaseInsensitiveStringEqualsTitle and RecommendCaseInsensitiveStringComparisonTitle will be used. It looks you can just use the same title RecommendCaseInsensitiveStringComparisonTitle for all.

buyaa-n · 2023-07-26T21:11:25Z

src/NetAnalyzers/RulesMissingDocumentation.md

@@ -8,7 +8,7 @@ CA1512 | <https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-
 CA1513 | <https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1513> | Use ObjectDisposedException throw helper |
 CA1856 | <https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1856> | Incorrect usage of ConstantExpected attribute |
 CA1857 | <https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1857> | A constant is expected for the parameter |
-CA1862 | <https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1862> | Prefer using 'StringComparer' to perform case-insensitive string comparisons |
+CA1862 | <https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1862> | Prefer using 'string.Equals(string, StringComparison)' to perform case-insensitive string comparisons |


I think the old title suit for all diagnostics

Actually, neither this change nor the previous text were appropriate enough for all 3 different cases. The correct message that applies to all 3 is:
Prefer using 'StringComparison' method overloads to perform case-insensitive string comparisons.

…r() == otherString.ToLower() (dotnet#6720) * Analyzer: Use string.Format with CultureInfo for title. * Almost, only needs to handle inequality * Fix inequality case, fix trivia, still need to add C# test * Add C# tests for trivia and string method * Add tests for diagnostic-only, no fix, for Equals that don't match variant/invariant. * Use ValueTuple in tests * Register operation actions separately. * Fix recent nullability changes * Use simplified valuetuple creation in tests * Add extra rule and resource messages for the 'string.Method() == string.Method()' comparison case. * msbuild /t:pack * Address suggestion

Bellarmine-Head · 2023-07-27T07:26:34Z

@carlossanlop - did you take my comments

Recommend using Equals with StringComparison instead of string.ToLower() == otherString.ToLower() #6720 (comment)
.NET 8 developers can verify more APIs for correct usage to speed up their development runtime#78442 (comment)

into consideration?

It's all very well to say "Recommend using Equals with StringComparison instead of string.ToLower() == otherString.ToLower()", but which StringComparison enum value or values?

Unless this issue and also dotnet/runtime#78606 consider and prominently expose the OrdinalIgnoreCase option, then I would say that both issues have been implemented incorrectly.

The trouble with this table is that the right-hand column is a literal, or rote, translation of the left-hand column. This translation, however, ignores a very important option: StringComparison.OrdinalIgnoreCase.

In over twenty years of C# and .NET I have never had to compare "linguistic" strings wrt case-insensitivity. And I haven't code-reviewed any code that does so. On the other hand, comparing the following strings wrt case-insensitivity:

Case-insensitive internal identifiers.
Case-insensitive identifiers in standards such as XML and HTTP.
File paths.
Registry keys and values.
Environment variables.
Resource identifiers (for example, handle names).
Case-insensitive security-related settings.

is something I (and others in the code I've reviewed) do 20 times a day, every day.

And as the Microsoft documentation makes clear that for these cases, we should be using OrdinalIgnoreCase (as I have done since it was first introduced in .NET 2, I believe). For one thing: it's important for security.

I would ask you to comment on the above before closing this issue. Also: I would like you to re-consider the implementation of dotnet/runtime#78606 in light of the above.

As it stands, I will be avoiding these two analyzers and will be warning my team to do the same. I believe that the analyzers currently stand at odds with the established and correct Microsoft documentation here. This is predicated on my belief that the vast majority of case-insensitive string comparisons are carried out on non-linguistic strings (e.g. identifiers).

Bellarmine-Head · 2023-07-31T08:05:28Z

@carlossanlop @MichalStrehovsky

I'm not saying (above) that people would never want to compare "linguistic" / "cultural" strings in a case-insensitive manner. It might happen, and it's good that those options exist in StringComparison and StringComparer. But what I'm suggesting is that most of the time people will be comparing non-linguistic identifiers in a case-insensitive manner, and this analyzer and also dotnet/runtime#78606 should be weighted in favour of that.

And what I'm saying most emphatically is the fact that neither analyzer seems to consider the OrdinalIgnoreCase compare option - the Microsoft recommended option for non-linguistic strings - must be regarded as a serious shortcoming... one that mandates the implementation of these two analyzers to be re-evaluated.

As I said over here, the fact that the GH issue dotnet/runtime#78606 doesn't mention the word "ordinal" at all is a serious omission.

carlossanlop requested review from mavasani, buyaa-n and Youssef1313 June 29, 2023 21:10

carlossanlop changed the title ~~Recommend v2~~ Recommend using Equals with StringComparison for string.ToLower() == otherString.ToLower() Jun 29, 2023

carlossanlop marked this pull request as ready for review June 29, 2023 21:11

carlossanlop requested a review from a team as a code owner June 29, 2023 21:11

carlossanlop changed the title ~~Recommend using Equals with StringComparison for string.ToLower() == otherString.ToLower()~~ Recommend using Equals with StringComparison instead of string.ToLower() == otherString.ToLower() Jun 29, 2023

carlossanlop mentioned this pull request Jun 29, 2023

[Analyzer] string.ToLower() == otherString.ToLower() dotnet/runtime#78607

Closed

This comment was marked as outdated.

Sign in to view

mavasani reviewed Jun 30, 2023

View reviewed changes

buyaa-n reviewed Jun 30, 2023

View reviewed changes

carlossanlop commented Jul 17, 2023

View reviewed changes

carlossanlop added 8 commits July 17, 2023 15:11

Analyzer: Use string.Format with CultureInfo for title.

7cc2e29

Almost, only needs to handle inequality

842a82f

Fix inequality case, fix trivia, still need to add C# test

a344cc6

Add C# tests for trivia and string method

8d5e8ec

Add tests for diagnostic-only, no fix, for Equals that don't match va…

fa70b2e

…riant/invariant.

Use ValueTuple in tests

044aedc

Register operation actions separately.

adfcc80

Fix recent nullability changes

4e17f73

carlossanlop force-pushed the RecommendV2 branch from bd55933 to 4e17f73 Compare July 17, 2023 22:16

buyaa-n reviewed Jul 18, 2023

View reviewed changes

buyaa-n approved these changes Jul 18, 2023

View reviewed changes

carlossanlop added 3 commits July 26, 2023 11:40

Use simplified valuetuple creation in tests

13b8d47

Add extra rule and resource messages for the 'string.Method() == stri…

e04c8f9

…ng.Method()' comparison case.

msbuild /t:pack

c373caf

buyaa-n reviewed Jul 26, 2023

View reviewed changes

Address suggestion

adfdb0d

carlossanlop enabled auto-merge (squash) July 26, 2023 21:42

carlossanlop merged commit f23133f into dotnet:main Jul 26, 2023

carlossanlop deleted the RecommendV2 branch July 26, 2023 21:55

carlossanlop mentioned this pull request Jul 26, 2023

Fix hits for new analyzer CA1862 dotnet/runtime#89539

Merged

MichalStrehovsky mentioned this pull request Jul 29, 2023

[main] Update dependencies from dotnet/roslyn-analyzers dotnet/runtime#89630

Merged

Bellarmine-Head mentioned this pull request Aug 1, 2023

CA1862 suggestion changing semantics of the comparison. dotnet/runtime#89740

Closed

Bellarmine-Head mentioned this pull request Aug 15, 2023

.NET 8 developers can verify more APIs for correct usage to speed up their development dotnet/runtime#78442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

carlossanlop commented Jun 29, 2023 •

edited

Loading

This comment was marked as outdated.

mavasani Jun 30, 2023

carlossanlop Jul 6, 2023

Youssef1313 Jul 6, 2023

carlossanlop Jul 6, 2023

buyaa-n Jun 30, 2023

buyaa-n Jun 30, 2023

carlossanlop Jul 17, 2023

buyaa-n Jul 18, 2023

buyaa-n Jun 30, 2023

carlossanlop Jul 17, 2023

buyaa-n Jul 17, 2023 •

edited

Loading

carlossanlop Jul 17, 2023

buyaa-n Jul 18, 2023

buyaa-n Jun 30, 2023

carlossanlop Jul 17, 2023

buyaa-n Jul 17, 2023 •

edited

Loading

carlossanlop Jul 26, 2023

buyaa-n Jun 30, 2023

carlossanlop Jul 17, 2023

buyaa-n Jul 17, 2023

carlossanlop left a comment •

edited

Loading

buyaa-n Jul 18, 2023

carlossanlop Jul 26, 2023

buyaa-n Jul 18, 2023

buyaa-n left a comment

Bellarmine-Head commented Jul 22, 2023

carlossanlop commented Jul 26, 2023

buyaa-n Jul 26, 2023

carlossanlop Jul 26, 2023

buyaa-n Jul 26, 2023 •

edited

Loading

carlossanlop Jul 26, 2023

Bellarmine-Head commented Jul 27, 2023 •

edited

Loading

Bellarmine-Head commented Jul 31, 2023 •

edited

Loading

	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)
	};

	// Equals(string)
	IMethodSymbol? stringEqualsStringMethod = stringType.GetMembers(StringEqualsMethodName).OfType<IMethodSymbol>().GetFirstOrDefaultMemberWithParameterInfos(stringParameter);
	if (stringEqualsStringMethod == null)
	{
	return;
	}

	// 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;
	}

	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)
	{
	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;
	}

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

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

Conversation

carlossanlop commented Jun 29, 2023 • edited Loading

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n left a comment

Choose a reason for hiding this comment

Bellarmine-Head commented Jul 22, 2023

carlossanlop commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bellarmine-Head commented Jul 27, 2023 • edited Loading

Bellarmine-Head commented Jul 31, 2023 • edited Loading

carlossanlop commented Jun 29, 2023 •

edited

Loading

buyaa-n Jul 17, 2023 •

edited

Loading

buyaa-n Jul 17, 2023 •

edited

Loading

carlossanlop left a comment •

edited

Loading

buyaa-n Jul 26, 2023 •

edited

Loading

Bellarmine-Head commented Jul 27, 2023 •

edited

Loading

Bellarmine-Head commented Jul 31, 2023 •

edited

Loading