Implement Use 'StartsWith' instead of 'IndexOf' analyzer

[Youssef1313]

Fixes dotnet/runtime#78608

cc @stephentoub @tarekgh @mavasani

[codecov]

Codecov Report

Merging #6295 (c358bfb) into main (7af823a) will increase coverage by 0.00%.
The diff coverage is 98.72%.

❗ Current head c358bfb differs from pull request most recent head d22ec17. Consider uploading reports for the commit d22ec17 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6295    +/-   ##
========================================
  Coverage   96.08%   96.09%            
========================================
  Files        1357     1362     +5     
  Lines      315287   316086   +799     
  Branches    10184    10209    +25     
========================================
+ Hits       302951   303740   +789     
- Misses       9904     9909     +5     
- Partials     2432     2437     +5     

[tarekgh]

@Youssef1313 thanks a lot for submitting the PR.

You may expect a slow response from me as I am currently off. May others get a chance to comment too.

[tarekgh]

UseStartsWithInsteadOfIndexOfComparisonWithZeroTests

We need to add a test cases for IndexOf overloads which takes 2 arguments (with StringComparison) too.

[Youssef1313]

@tarekgh What about IndexOf(char, StringComparison)? There is no char overload in StartsWith that accepts StringComparison. Should we not report? Or use StartsWith(string, StringComparison)? Or something else?

[tarekgh]

That is a good question. If it will not be complicated to implement, we can use StartsWith(ReadOnlySpan, ReadOnlySpan, StringComparison). In general, I see the IndexOf overloads that takes string is higher priority as these used in main scenario. We can support the char overloads later if we need to.

CC @stephentoub if has input here.

[Youssef1313]

@stephentoub What do you think?

[tarekgh]

@Youssef1313 how complex would it be if implement IndexOf(char, StringComparison) as I suggested above?

[Youssef1313]

Found dotnet/roslyn#61432, but it doesn't seem like the comment there applies in the test project in this repo (I don't see we reference Newtonsoft.Json in test project - maybe another dependency does and we get it as a transitive dependency?).

@sharwell Can you help please?

[Youssef1313]

@tarekgh One more question is IndexOf(Char). There is StartsWith(Char), but it is newer and might not be available (e.g, in .NET Standard 2.0 IndexOf(Char) exists but StartsWith(Char) doesn't). In this case (only if StartsWith(Char) doesn't exist) should we replace IndexOf('a') with StartsWith('a'.ToString())/StartsWith("a")?

[tarekgh]

No, the whole point of this analyzer is the performance.

for s.IndexOf(c) == 0 you can convert it to

s.length > 0 && s[0] == c

[Youssef1313]

Makes sense.

I started looking into IndexOf(char, StringComparison) and there is one thing.

we can use StartsWith(ReadOnlySpan, ReadOnlySpan, StringComparison)

Here, we need to change the char to a ReadOnlySpan<char>

The ReadOnlySpan<T>(T) ctor is only available in .NET 7, the other way is to use stackalloc char[1] { ch }. Is that okay? What about VB? I'm also not sure about ReadOnlySpan (ref structs in general) support in VB.

[tarekgh]

to use stackalloc char[1] { ch }. Is that okay?

Yes, this should be Ok. You just convert the IndexOf call too string.AsSpan().StartsWith(stackalloc char[1] { chr }, stringComparison);

What about VB? I'm also not sure about ReadOnlySpan (ref structs in general) support in VB.

Considering it is rare to find the pattern IndexOf(char, StringComparison) == 0 in VB. I think for this case we either exclude this rule from there or we can go with the fixer that allocates the string from the character. I mean use StartsWith(char.ToString(), StringComparison).

[Youssef1313]

@tarekgh I handled IndexOf(char, StringComparison) overload

For VB, if char is hardcoded character literal, I switch it to a hardcoded string literal. Otherwise, I call .ToString()
For C#, I use AsSpan().StartsWith(stackalloc ...), though if language version is < 8, this will produce an error. I think we won't care that much for this niche case where the user can simply upgrade their language version.

[tarekgh]

@Youssef1313 thanks a lot for getting this ready.

@buyaa-n could you please have a look? The changes LGTM. You may merge it after reviewing too. Thanks!

[tarekgh]

@Youssef1313 could you please fix the merge conflicts?

@jeffhandley is there anyone who can look and merge this one? The changes look good to me.

[buyaa-n]

Could you add tests for the cases that should not be flagged?

[buyaa-n]

Left few comments, overall LGTM, thank you!

[Youssef1313]

@buyaa-n Thanks for the review!

I addressed your feedback. Can you take another look? Thanks!

[buyaa-n]

I addressed your feedback. Can you take another look? Thanks!

Looks great, thank you!

[Youssef1313]

Thanks @buyaa-n for the quick review!

I opened the docs PR: dotnet/docs#33114