Add Analyzer & Codefix - CA1847 - Use String.Contains(Char) instead of String.Contains(String) with single characters

[MeikTranel]

Resolves dotnet/runtime#47180

Outstanding Work:

• roslyn-analyzer violations: Addressed here Add Analyzer & Codefix - CA1847 - Use String.Contains(Char) instead of String.Contains(String) with single characters #4908 (comment)

  • roslyn-analyzers/src/Utilities/Compiler/Options/SymbolNamesWithValueOption.cs

    Line 112 in 802b2a7

    !parts.SymbolName.Contains(".", StringComparison.Ordinal) && !parts.SymbolName.Contains(":", StringComparison.Ordinal))

  • roslyn-analyzers/src/Utilities/Compiler/RulesetToEditorconfigConverter.cs

    Line 163 in 802b2a7

    if (xtext.Value.Contains("\r", StringComparison.Ordinal) || xtext.Value.Contains("\n", StringComparison.Ordinal))

• runtime PR (two small violations of the rule): Use string.Contains(char) wherever possible - fallback to IndexOf if possible runtime#49094

• docs PR: Added documentation for new rule CA1847 - Use string.Contains(char) w… docs#23377

The roslyn-analyzer violations are tricky - they're part of shared code that is used by both frameworks that have String.Contains(char) and those that don't - is it really worth the #if extravaganza here or should i just ignore them?

[codecov]

Codecov Report

Merging #4908 (cb17df3) into release/6.0.1xx (5078028) will decrease coverage by 0.00%.
The diff coverage is 92.74%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #4908      +/-   ##
===================================================
- Coverage            95.59%   95.59%   -0.01%     
===================================================
  Files                 1226     1231       +5     
  Lines               280904   281221     +317     
  Branches             16872    16905      +33     
===================================================
+ Hits                268532   268830     +298     
- Misses               10096    10108      +12     
- Partials              2276     2283       +7     

[MeikTranel]

@sharwell / @Youssef1313
two questions while digging into fixing the outstanding issues:

• Should i create an issue before opening up a PR on the runtime or can i just throw them the PR if the amount of violations are small?
• Same question basically for the docs PR

[MeikTranel]

Two outstanding in this repo violations remain - in the issue we decided that the information severity is probably better suited.

The specific violations in question are part of the shproj compile items, which have lots of different frameworks they're compiled against (See comment for reference #4908 (comment)) - much like the runtime repo i dont think the change would have been worth it, but i'm not firm on that stance - if y'all prefer the ol' #if NETCOREAPP3_1 shuffle i'd be happy to do it 😄.

As for the other repos:

• runtime - PR (Use string.Contains(char) wherever possible - fallback to IndexOf if possible runtime#49094)
• roslyn - no violations found by the test tool (performance impact recorded was also neglible IIRC - i can generate another report if y'all are interested)
• roslyn-analyzers - as said above
• aspnetcore - wasn't able to finish the compile yet

Docs PR: dotnet/docs#23377

[AraHaan]

Personally I think this can be done, on TFM's that does not have String.Contains(char) overload of contains on string could in fact do String.IndexOf(char) > -1 to check if it contains that character on those TFM's only.

In fact I would like it if that was added to the resource description on this rule that is being added.

First of all we need to calculate what TFMs do not contain the String.Contains(char) or for security have to codefix basically do:

#if /*Frameworks that supply the String.Contains(char) overload. */
// the actual fix to change that here.
#else
// the rest of the code but instead replace it with "String.IndexOf(char) > -1".
#endif

I would actually prefer this as it then would guard people who multitarget from scratching their heads and wondering what to do and then decide to revert the code fix and then suppress it in their .editorconfig file.

[MeikTranel]

@AraHaan Like in this PR? dotnet/runtime#49094

In fact I would like it if that was added to the resource description on this rule that is being added.

I am unsure - are you proposing we add this as an addendum to the How to fix section of the documentation or you proposing adding this to the codefix automation itself?

I can see value in the documentation part, but i'm unsure if this is worth setting the precedent for nor am sure if its even possible to detect the context beyond the single target framework compilation.

[AraHaan]

Yes but does that also apply to netcoreapp3.1+ as well too since .NET Core 3.1 implements stardard2.1?

[MeikTranel]

@sharwell can you take a look at this again - i think a resolved all issues raised so far and it feels like the re-request for review went under your radar 😄

[Youssef1313]

Can you target the PR to release/6.0.1xx-preview3 branch?

[mavasani]

Has anyone ever had this luck of being pushed back 5 numbers in a single PR

Yes, that is very common. We have large number of community contributors, and bunch of WIP new analyzer PRs. If you prefer, you can hold off bumping up the rule ID until you have received a complete review/approval and then do a final merge resolution, which should require bumping the rule ID

[mavasani]

@MeikTranel Just FYI: There are 3 more new analyzer PRs opened prior to yours that need a review/approval. I am slowly getting through them, but please expect a bit of delay/merge conflicts as we wade through them.

[MeikTranel]

Haha no issue. My ID update chops have been honed by now 🤣

Stale

[mavasani]

@MeikTranel I think your PR is closest to completion amongst the open PRs. If you can quickly address the last set of review comments, I should be able to merge your PR :). Thanks against for your patience.

[sharwell]

@mavasani We resolved the numbering problem in StyleCop Analyzers by assigning IDs at the time analyzer proposals were approved. We sometimes end up with a gap if a later proposal gets implemented first, but it avoids the conflicts.

[MeikTranel]

@mavasani i was able to move this out successfully like we talked about - theres still some outstanding conflicts but those are ID conflicts again - i can fix those once the regular review stuff has been resolved. Just make sure to tag me so my phone gives me a notification 🙃

[mavasani]

@MeikTranel this is ready to merge once you resolve the merge conflicts.

[MeikTranel]

should be good to go now ;)

[Youssef1313]

Probably not very important, but consider adding a test for constant interpolated strings, i.e return "testString".Contains($"a"); or "testString.Contains($"{CONST_CHAR_DEFINED_ABOVE}");

[MeikTranel]

That use case was specifically excluded in the source issue.

[MeikTranel]

Unless roslyn somehow optimizes constant values in a way that the interpolation with a constant value would end up classifying as a LiteralSyntax?

[Youssef1313]

@MeikTranel I couldn't see anything about it in the comments. Was that said in the API review video?