Skip to content

Conversation

github-actions[bot]
Copy link

@github-actions github-actions bot commented Sep 8, 2023

Backport of #6898 to release/8.0.1xx

/cc @MihaZupan

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[] { ... }) or readonly 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 the SearchValues API is not available.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #6924 (4aa64c3) into release/8.0.1xx (e2e6382) will decrease coverage by 0.01%.
The diff coverage is 95.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     

@MihaZupan
Copy link
Member

MihaZupan commented Sep 8, 2023

(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[] { ... }) or readonly 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 the SearchValues API is not available.

@buyaa-n
Copy link

buyaa-n commented Sep 8, 2023

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

Thanks, updated the description

@jeffhandley
Copy link
Member

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?

@MihaZupan
Copy link
Member

Oops, yes, should be false positives.

Copy link
Member

@jeffhandley 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 net8 SearchValues 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 in main for aspnetcore, efcore, and perhaps other dotnet org repos where it's likely the older APIs are being used.

Tagging @artl93 for RC2 consideration.

@artl93
Copy link
Member

artl93 commented Sep 8, 2023

M2 approved. Thanks!

@buyaa-n buyaa-n merged commit 249601b into release/8.0.1xx Sep 8, 2023
@buyaa-n buyaa-n deleted the backport/pr-6898-to-release/8.0.1xx branch September 8, 2023 22:32
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.

4 participants