KEMBAR78
Add analyzer/fixer to suggest using cached SearchValues instances by MihaZupan · Pull Request #6898 · dotnet/roslyn-analyzers · GitHub
Skip to content

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Aug 29, 2023

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 (SearchValues is a .NET 8+ API).

The analyzer flags cases like

const string MyValues = "abcdef";
readonly char[] MyValues = new[] { 'a', 'b', 'c', 'd', 'e', 'f' };

span.IndexOfAny("abcdef");
span.IndexOfAny("abcdef"u8);
span.IndexOfAny(MyValues);
span.IndexOfAny(new[] { 'a', 'b', 'c', 'd', 'e', 'f' });
string.IndexOfAny("abc".ToCharArray());

for all {Last}IndexOfAny{Except} and ContainsAny{Except} overloads for byte/char and suggests that they be rewritten as

private static readonly SearchValues<char> s_myChars = SearchValues.Create("abcdef");

span.IndexOfAny(s_myChars);

We'll also rewrite the string.IndexOfAny(char[]) overload to use string.AsSpan().IndexOfAny(SearchValues) instead.
We won't do the same for IndexOfAny(char[], int) or IndexOfAny(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:

  • IDE0300 will suggest replacing new[] { 'a', 'b', 'c' } with ['a', 'b', 'c'], whereas this analyzer does not recognize collection literals.

@MihaZupan MihaZupan requested a review from a team as a code owner August 29, 2023 23:16
@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #6898 (1fe8dd8) into main (2d42aa2) will decrease coverage by 0.01%.
Report is 5 commits behind head on main.
The diff coverage is 95.53%.

@@            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     

Copy link

@buyaa-n buyaa-n left a 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.

@buyaa-n
Copy link

buyaa-n commented Sep 6, 2023

FYI @mavasani we want to add this new analyzer into .NET 8 as This is about a new .NET 8 feature, and it'd be very nice to have a better end-to-end story for the release by having the analyzer/fixer shipping together with the library support it targets. It would be great if you could take a look

@mavasani
Copy link

mavasani commented Sep 7, 2023

Overall, this looks good to me. I primarily looked at the analyzer and test code, just skimmed over the code fixer code.

@MihaZupan
Copy link
Member Author

/backport to release/8.0.1xx

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

Started backporting to release/8.0.1xx: https://github.com/dotnet/roslyn-analyzers/actions/runs/6124792900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Analyzer] Use IndexOfAnyValues instead of inlined or cached array

5 participants