-
Notifications
You must be signed in to change notification settings - Fork 480
Add analyzer/fixer to suggest using cached SearchValues instances #6898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6898 +/- ##
==========================================
- Coverage 96.40% 96.40% -0.01%
==========================================
Files 1403 1402 -1
Lines 331075 331964 +889
Branches 10893 11036 +143
==========================================
+ Hits 319166 320015 +849
- Misses 9178 9186 +8
- Partials 2731 2763 +32 |
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseSearchValues.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the great test coverage, in general LGTM, I would let @mavasani take a look to the new utilities and some of approaches used in the analyzer/fixer.
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/UseSearchValuesTests.cs
Show resolved
Hide resolved
src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/UseSearchValuesTests.cs
Show resolved
Hide resolved
src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Performance/CSharpUseSearchValues.Fixer.cs
Show resolved
Hide resolved
|
FYI @mavasani we want to add this new analyzer into .NET 8 as |
src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Performance/CSharpUseSearchValues.Fixer.cs
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseSearchValues.cs
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseSearchValues.cs
Show resolved
Hide resolved
|
Overall, this looks good to me. I primarily looked at the analyzer and test code, just skimmed over the code fixer code. |
|
/backport to release/8.0.1xx |
|
Started backporting to release/8.0.1xx: https://github.com/dotnet/roslyn-analyzers/actions/runs/6124792900 |
Fixes dotnet/runtime#78587
Flags 3 cases in dotnet/runtime, all of them in projects where we've avoided making the switch because they multi-target to older TFMs (
SearchValuesis a .NET 8+ API).The analyzer flags cases like
for all
{Last}IndexOfAny{Except}andContainsAny{Except}overloads forbyte/charand suggests that they be rewritten asWe'll also rewrite the
string.IndexOfAny(char[])overload to usestring.AsSpan().IndexOfAny(SearchValues)instead.We won't do the same for
IndexOfAny(char[], int)orIndexOfAny(char[], int, int)as they would require us to inject more logic to account for the differences in behavior (returning offset from start vs. from 0)If the values are specified by a constant in an appropriate scope, we'll reuse said constant to feed the
SearchValues.Create.If we're not using the original member and there are now no other uses, we'll remove it.
For a full list of patterns that we do/don't recognize, see this test source.
Potential improvements:
IDE0300will suggest replacingnew[] { 'a', 'b', 'c' }with['a', 'b', 'c'], whereas this analyzer does not recognize collection literals.