Skip to content
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

Add analyzer/fixer to suggest using cached SearchValues instances #6898

Merged
merged 27 commits into from
Sep 8, 2023

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
Member

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

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
Contributor

mavasani commented Sep 7, 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

Sure @buyaa-n. Reviewing now

// text.IndexOfAny(new[] { 'a', 'b', 'c' })
return IsConstantByteOrCharSZArrayCreation(arrayCreation, out _);
}
else if (argument is IFieldReferenceOperation fieldReference)
Copy link
Contributor

@mavasani mavasani Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only handle field references in this method, while we handle both field and property references in AreConstantValuesWorthReplacing. Any reason we don't support both of them here? I presume this is because we require references to readonly fields, hence properties do not make sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Span-based overloads below, the properties we're looking for might look like

ReadOnlySpan<byte> MyProperty => new byte[] { ... };

text.IndexOfAny(MyProperty);

which is a reasonable thing someone might write if they know that allocation will be eliminated by the compiler.

For string, the property would instead have to return a char[], which felt like a really odd pattern to use. We could support it though.

@mavasani
Copy link
Contributor

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
Contributor

github-actions bot commented Sep 8, 2023

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

carlossanlop pushed a commit to carlossanlop/roslyn-analyzers that referenced this pull request Sep 13, 2023
…tnet#6898)

* Initial Analyzer implementation

* Code fixer

* Add support for string.IndexOfAny(char[])

* Catch simple cases of array element modification

* Use built-in helper instead of Linq

* Also detect field assignments in ctor

* Move system namespace import to helper method

* Replace array creations wtih string literals

* Add support for more property getter patterns

* Simplify test helper

* Revert Utf8String support :(

* Update tests

* msbuild /t:pack /v:m

* Fix editor renaming

* Exclude string uses on a conditional access

* Add test for array field with const char reference

* Add back Utf8String support

* Update messages/descriptions

* Add support for field initialized from literal.ToCharArray

* More tests for ToCharArray

* Better handle member names that start with _

* Avoid some duplication between Syntax and Operation analysis

* Fix top-level statements and add logic to remove unused members

* ImmutableHashSet, no OfType

* Remove some duplication

* Turn one analyzer test into code fixer tests

* Shorten analyzer title
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