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

[release/8.0.1xx] Address CA1862 cases to diagnose and to fix and improve messages #6948

Merged
merged 17 commits into from
Sep 16, 2023

Conversation

github-actions[bot]
Copy link
Contributor

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

Backport of #6928 to release/8.0.1xx

/cc @carlossanlop

Fixes dotnet/runtime#89740

Customer Impact

The purpose of analyzer CA1862 is to determine if string comparisons (StartsWith, Contains, IndexOf, CompareTo, abd binary comparisons of string equality) are changing case and culture and suggest using other more performant methods that allow choosing a culture and casing handling properly. In some cases, the analyzer offers a fix, while in other cases it only offer a diagnosis and tells the user to manually determine what to do.

Unfortunately, the analyzer was changing semantics for some of the offered fixes, specifically those where the user was incorrectly comparing different cultures. This PR addresses that problem by avoiding offering fixes for those cases, and additionally improves the messages so that the user gets a more detailed explanation of the problem and what they need to do to address the problem manually.

Testing

  • Modified the existing tests to avoid fixing the code where we shouldn't.
  • Added many more tests to ensure the fixes cover a larger variety of invocations and argument combinations.
  • Added more edge cases, particularly where parenthesis need to get removed.

Risk

Low. The analyzer is new and we're fixing specific cases while preserving the majority of correct ones.

@github-actions github-actions bot requested a review from a team as a code owner September 16, 2023 04:32
@carlossanlop
Copy link
Member

@jeffhandley @ericstj @artl93 can you please consider this backported fix for release/8.0.1xx?

@carlossanlop carlossanlop self-assigned this Sep 16, 2023
@carlossanlop carlossanlop added the servicing-consider For backport requests to servicing branches. label Sep 16, 2023
@carlossanlop carlossanlop added this to the 8.0.1xx milestone Sep 16, 2023
@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Merging #6948 (bffdffe) into release/8.0.1xx (7ec4e89) will decrease coverage by 0.01%.
The diff coverage is 94.64%.

@@                 Coverage Diff                 @@
##           release/8.0.1xx    #6948      +/-   ##
===================================================
- Coverage            96.39%   96.39%   -0.01%     
===================================================
  Files                 1402     1402              
  Lines               331865   332092     +227     
  Branches             11028    11057      +29     
===================================================
+ Hits                319916   320130     +214     
- Misses                9188     9194       +6     
- Partials              2761     2768       +7     

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Please ensure we have translation hand back before GA.

Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

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

M2 approved

@carlossanlop carlossanlop removed the servicing-consider For backport requests to servicing branches. label Sep 16, 2023
@carlossanlop carlossanlop merged commit 8ca50b0 into release/8.0.1xx Sep 16, 2023
11 checks passed
@carlossanlop carlossanlop deleted the backport/pr-6928-to-release/8.0.1xx branch September 16, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants