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