SearchValues is 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.

The main benefit is giving users an easy way to boost searching performance by doing the code rewriting for them, as well as raising their awareness of the new feature's existence.

Through telemetry, we know that many of the patterns flagged by this analyzer (e.g.IndexOfAny(new[] { ... }) orreadonly char[] _myField = ...;) are quite often used by users of these APIs.

Testing

Decent code coverage was added as part of this PR.
Running the analyzer against dotnet/runtime flagged only 3 places that we haven't updated manually yet in projects that multi-target older TFMs. It hit no false positives.

Risk

It's another analyzer users may have to disable/adjust code fixes for in projects that also target older TFMs where theSearchValues API is not available.

MihaZupan added27 commits

September 8, 2023 17:41

Initial Analyzer implementation

a1d0a3d

Code fixer

6a6b4f3

Add support for string.IndexOfAny(char[])

6ff5476

Catch simple cases of array element modification

eb3ae18

Use built-in helper instead of Linq

2fe6815

Also detect field assignments in ctor

f7c7805

Move system namespace import to helper method

fa1e65b

Replace array creations wtih string literals

28b4009

Add support for more property getter patterns

6dcd803

Simplify test helper

de38f13

Revert Utf8String support :(

94aa9f1

Update tests

ca0ea6d

msbuild /t:pack /v:m

9a29c03

Fix editor renaming

a4f86a2

Exclude string uses on a conditional access

6bba6e4

Add test for array field with const char reference

645da9b

Add back Utf8String support

5d02e7d

Update messages/descriptions

b84dbfa

Add support for field initialized from literal.ToCharArray

50fe46c

More tests for ToCharArray

c872418

Better handle member names that start with _

b188932

Avoid some duplication between Syntax and Operation analysis

dc827c6

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

d46ff89

ImmutableHashSet, no OfType

fbc54be

Remove some duplication

b4a6d1b

Turn one analyzer test into code fixer tests

6b5fb65

Shorten analyzer title

4aa64c3

github-actionsbot requested a review froma team as acode owner

September 8, 2023 17:41

MihaZupan mentioned this pull request

Sep 8, 2023

[Analyzer] Use IndexOfAnyValues instead of inlined or cached arraydotnet/runtime#78587

Closed

buyaa-n added this to the8.0.1xx milestone

Sep 8, 2023

buyaa-n requested review fromjeffhandley,mavasani andstephentoub

September 8, 2023 17:50

buyaa-n approved these changes

Sep 8, 2023

View reviewed changes

Copy link

codecovbot commentedSep 8, 2023

Codecov Report

Merging#6924 (4aa64c3) intorelease/8.0.1xx (e2e6382) willdecrease coverage by0.01%.
The diff coverage is95.53%.

@@                 Coverage Diff                 @@##           release/8.0.1xx    #6924      +/-   ##===================================================- Coverage            96.40%   96.39%   -0.01%===================================================  Files                 1396     1402       +6       Lines               330339   331860    +1521       Branches             10882    11027     +145     ===================================================+ Hits                318453   319912    +1459- Misses                9159     9186      +27- Partials              2727     2762      +35

Copy link

Member

MihaZupan commentedSep 8, 2023•
edited
Loading

(I'm lacking perms to edit the above comment)

Customer Impact

SearchValues is 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.

The main benefit is giving users an easy way to boost searching performance by doing the code rewriting for them, as well as raising their awareness of the new feature's existence.

Through telemetry, we know that many of the patterns flagged by this analyzer (e.g.IndexOfAny(new[] { ... }) orreadonly char[] _myField = ...;) are quite often used by users of these APIs.

Testing

Risk

It's another analyzer users may have to disable/adjust code fixes for in projects that also target older TFMs where theSearchValues API is not available.

Copy link

buyaa-n commentedSep 8, 2023

(I'm lacking perms to edit the above comment)

Thanks, updated the description

Copy link

Member

jeffhandley commentedSep 8, 2023

It hit no false negatives

Did you mean "false positives" here by chance? As in, a positive match for where the analyzer would raise a diagnostic, but it was an erroneous diagnostic?

Copy link

Member

MihaZupan commentedSep 8, 2023

Oops, yes, should be false positives.

jeffhandley approved these changes

Sep 8, 2023

View reviewed changes

Copy link

Member

jeffhandley left a comment

Choose a reason for hiding this comment

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

This has my support for .NET 8 RC2. The analyzer guides users to adopt the new net8SearchValues API to improve performance of their existing code, completing the end-to-end experience for this new API.

The analyzer is Info-level by default, which means it can be introduced in a non-breaking way.

We should enable the analyzer inmain for aspnetcore, efcore, and perhaps other dotnet org repos where it's likely the older APIs are being used.

Tagging@artl93 for RC2 consideration.