search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.25k stars 4.73k forks source link

Add analyzer to flag string comparisons without an explicit comparer/comparison #86989

Open drewnoakes opened 1 year ago

drewnoakes commented 1 year ago

Describe the problem you are trying to solve

In the .NET Project System (and in CPS) we want to always be explicit about the comparison kind when comparing strings.

path1 == path2 // bad
path1.Equals(path2) // bad
StringComparer.OrdinalIgnoreCase.Equals(path1, path2) // good
path1.Equals(path2, StringComparison.OrdinalIgnoreCase) // good

This is something that's called out a lot during code review. It'd be more convenient and reliable if an analyzer did this for us.

Describe suggestions on how to achieve the rule

Flagging any use of the == operator on strings or the string.Equals(string) method would be a start. If that were all, we might be able to use the banned API analyzer (not sure it supports operators though.)

However there are other cases too, such as:

new HashSet<string>() // should specify comparer
new Dictionary<string, T>() // should specify key comparer
ImmutableDictionary.Create<string, T>() // should specify key comparer

The banned API analyzer cannot be used for these as there's no way to specify specific instantiations (including partial ones) of generic types with that analyzer.

Furthermore, in the case of string comparisons, it would be possible to provide a codefix in a way that's not possible with the banned API analyzer.

Additional context

There are potentially other APIs this could apply to, such as string.IndexOf(string) and friends, but those seem to come up less often (and the banned API analyzer could handle those). The main one is the == operator for strings I think.

mavasani commented 1 year ago

We do have CA1307: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1307. Not sure if it flags use of equality operators.

mavasani commented 1 year ago

Tests: https://github.com/dotnet/roslyn-analyzers/blob/main/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/SpecifyStringComparisonTests.cs

mavasani commented 1 year ago

Related: https://github.com/dotnet/roslyn-analyzers/blob/main/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseOrdinalStringComparisonTests.Fixer.cs

Not sure if we also have a rule that flags the use of equality operator

EgorBo commented 1 year ago

Filed https://github.com/dotnet/runtime/issues/86999 since there is a codegen difference between some of them.

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.

Issue Details
### Describe the problem you are trying to solve In the .NET Project System (and in CPS) we want to always be explicit about the comparison kind when comparing strings. ```c# path1 == path2 // bad path1.Equals(path2) // bad StringComparer.OrdinalIgnoreCase.Equals(path1, path2) // good path1.Equals(path2, StringComparison.OrdinalIgnoreCase) // good ``` This is something that's called out a lot during code review. It'd be more convenient and reliable if an analyzer did this for us. ### Describe suggestions on how to achieve the rule Flagging any use of the `==` operator on strings or the `string.Equals(string)` method would be a start. If that were all, we might be able to use the banned API analyzer (not sure it supports operators though.) However there are other cases too, such as: ```c# new HashSet() // should specify comparer new Dictionary() // should specify key comparer ImmutableDictionary.Create() // should specify key comparer ``` The banned API analyzer cannot be used for these as there's no way to specify specific instantiations (including partial ones) of generic types with that analyzer. Furthermore, in the case of string comparisons, it would be possible to provide a codefix in a way that's not possible with the banned API analyzer. ### Additional context There are potentially other APIs this could apply to, such as `string.IndexOf(string)` and friends, but those seem to come up less often (and the banned API analyzer could handle those). The main one is the `==` operator for strings I think.
Author: drewnoakes
Assignees: -
Labels: `area-System.Runtime`, `untriaged`, `code-analyzer`
Milestone: -
drewnoakes commented 1 year ago

Thanks @mavasani, CA1307 does help with this a bit. I enabled it in https://github.com/dotnet/project-system/pull/9064.

Would be great if this could also flag ==, ToDictionary<string, T>, ToHashSet<T> and friends.

rampaa commented 6 months ago

Related: https://github.com/dotnet/runtime/issues/52399

rampaa commented 6 months ago

Currently always being explicit about the string comparer might result in performance regression. Consider the following:

myList.Contains("Test"); will use the List.Contains method, but myList.Contains("Test", StringComparer.Ordinal); will use the Enumerable.Contains-0-system-collections-generic-iequalitycomparer((-0)))) method.