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

Analyzer proposal: Use String.Contains(char) instead of String.Contains(String) #47180

Closed
xtqqczze opened this issue Dec 20, 2020 · 25 comments · Fixed by dotnet/roslyn-analyzers#4908
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@xtqqczze
Copy link
Contributor

xtqqczze commented Dec 20, 2020

Suggested category: Performance
Suggested default severity: Suggestion
Fix is breaking or non-breaking: Non-breaking

Cause

This rule locates calls to Contains(String) when the string consists of a single char, and suggests using Contains(char) instead

Rule description

When Contains(String) is used with a single char, the call can be safely substituted with Contains(char).

Additional context

#24068
dotnet/coreclr#15740
#36310

@mavasani mavasani transferred this issue from dotnet/roslyn-analyzers Jan 19, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 19, 2021
@mavasani
Copy link
Member

Tagging @carlossanlop @buyaa-n - .NET API analyzer suggestion.

@buyaa-n buyaa-n added api-suggestion Early API idea and discussion, it is NOT ready for implementation code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer labels Jan 19, 2021
@carlossanlop carlossanlop added this to To do in Roslyn Analyzers Jan 20, 2021
@carlossanlop
Copy link
Member

Seems pretty straightforward. There was a previous effort to manually replace instances of String.Contains(string) with String.Contains(char), so an analyzer should automatically find any new instances for us from now on.

Suggested severity: Warning
Suggested category: Performance
Suggested milestone: Future (but we welcome contributions for 6.0)
Analyzer size: Small
Fixer size: Small

Should cover all overloads, and only where a literal is passed as argument:

string str = "whatever";
string find = "a";

// Yes
str.Contains("a");
str.Contains("a", StringComparison.InvariantCultureIgnoreCase);

// No
str.Contains(find);
str.Contains(find, StringComparison.InvariantCultureIgnoreCase);

@carlossanlop carlossanlop moved this from To do to Ready for review in Roslyn Analyzers Jan 22, 2021
@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jan 22, 2021
@carlossanlop carlossanlop added this to the Future milestone Jan 22, 2021
@GrabYourPitchforks
Copy link
Member

This as proposed is not a breaking change, but we need to take care if we extend this analyzer to other methods (IndexOf, etc.). Switching from string to char in those methods is an observable behavioral change.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 4, 2021
@terrajobst
Copy link
Member

terrajobst commented Feb 4, 2021

Video

  • Makes sense
  • Is this the only overload that we should do this for?

@buyaa-n buyaa-n moved this from Ready for review to In progress in Roslyn Analyzers Feb 4, 2021
@buyaa-n buyaa-n moved this from In progress to Approved-Future in Roslyn Analyzers Feb 4, 2021
@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Feb 4, 2021
@MeikTranel
Copy link

@carlossanlop i'd like to help with this one 🙋‍♂️

@buyaa-n buyaa-n assigned buyaa-n and MeikTranel and unassigned buyaa-n Feb 6, 2021
@buyaa-n
Copy link
Member

buyaa-n commented Feb 6, 2021

i'd like to help with this one 🙋‍♂️

@MeikTranel thanks, assigned to you

@carlossanlop carlossanlop removed the help wanted [up-for-grabs] Good issue for external contributors label Feb 6, 2021
@carlossanlop carlossanlop moved this from Approved-Future to In progress in Roslyn Analyzers Feb 10, 2021
@MeikTranel
Copy link

I'm pretty much done except two remaining tasks:

  • handling the short circuit when the String.Contains(char[, StringComparison]) isnt available
  • cleanup and finalizing resource strings etc.

hoping to finish this by this weekend... just one quick question:

How do we handle analyzer tests that are only reasonable on certain frameworks?

image

The obvious solution would be to just remove them from the compiled test assembly by using #if NETCOREAPP but i want to take the temperature if thats cool with the team first 🤣

@buyaa-n
Copy link
Member

buyaa-n commented Feb 22, 2021

How do we handle analyzer tests that are only reasonable on certain frameworks?

You can specify ReferenceAssemblies.NetCore.NetCoreApp30 in the test, not sure which NetCoreApp version you should use. Example: https://github.com/dotnet/roslyn-analyzers/blob/master/src/NetAnalyzers/UnitTests/Microsoft.NetFramework.Analyzers/DoNotUseInsecureDtdProcessingDoNotUseSetInnerXmlTests.cs#L21

Anyway, after you add the "handling the short circuit when the String.Contains(char[, StringComparison]) isn't available" part you don't need to worry about that right? Actually might better to keep the test as is to show that it is bailing out when the char overload is not available

@MeikTranel
Copy link

MeikTranel commented Mar 2, 2021

The draft PR is now up and mostly ready to pass things back to the team - one last requirements question remains tho...
@carlossanlop suggested that the default severity should be Warning - testing against the suggested repositories it occured to me that maybe this should be an Info level diagnostic.
This is probably not the first time y'all encountered this... basically what happens alot is that there would be a multitargeted project across the latest dotnet release like net5.0 as well as some form of netstandard2.0 or even net47 for those still supporting full framework usage.
I'm not sure about the market coverage today, but if roslyn, roslyn-analyzers and runtime are anything to go by then consumers will probably constantly be faced with the issue that only dotnet beyond netcoreapp3.1 and netstandard2.1 has the overloads and the more popular netstandard2.0 doesnt. And now we either have to use compiler defines, severity configs or SuppressWarning statements to work around the fact that only some of the TFMs support it.

@carlossanlop
Copy link
Member

@MeikTranel can you share the error output you get if you turn on this analyzer in the runtime repo and then build it?

To test your analyzer in runtime:

  • Clone dotnet/runtime
  • Edit this file:
    <Rules AnalyzerId="Microsoft.CodeAnalysis.NetAnalyzers" RuleNamespace="Microsoft.CodeAnalysis.NetAnalyzers">
  • Add a row with your RuleId
  • Go to C:\Users\<your_usename>\.nuget\packages\microsoft.codeanalysis.netanalyzers\<latest_version>\analyzers\dotnet\cs backup the original DLLs you find there, then copy and paste the ones you generated when building your roslyn-analyzers branch with your new analyzer.
  • Build runtime with .\build.cmd clr+libs
  • Share the list of errors you get so we get an idea of how noisy it is.

We should keep in mind this analyzer may improve the runtime performance if we fix all the suggested instances, so a warning severity makes sense.

@mavasani
Copy link
Member

mavasani commented Mar 2, 2021

We should keep in mind this analyzer may improve the runtime performance if we fix all the suggested instances, so a warning severity makes sense.

@carlossanlop I thought the initial bar for analyzers to be warnings by default was that they need to flag critical functional issues. Performance improvement analyzers should not be warnings by default. Has this stance changed recently? Can you confirm in the dotnet/runtime triage meeting?

@buyaa-n
Copy link
Member

buyaa-n commented Mar 2, 2021

basically what happens alot is that there would be a multitargeted project across the latest dotnet release like net5.0 as well as some form of netstandard2.0 or even net47 for those still supporting full framework usage.
I'm not sure about the market coverage today, but if roslyn, roslyn-analyzers and runtime are anything to go by then consumers will probably constantly be faced with the issue that only dotnet beyond netcoreapp3.1 and netstandard2.1 has the overloads and the more popular netstandard2.0 doesnt. And now we either have to use compiler defines, severity configs or SuppressWarning statements to work around the fact that only some of the TFMs support it.

@MeikTranel isn't the analyzer bail out when the overload not exist? If it does then i wouldn't expect it to warn or make noise for lower versions

@MeikTranel
Copy link

isn't the analyzer bail out when the overload not exist?

Yes and that works as you expected but when a single project builds against two TFMs where one offers the overload and the other doesn't, you're left with a fixer that'll immediately generate a build error for the older target framework.

This is a general issue with analyzers that remind of better alternatives that were only introduced in later successions of the frameworks.

@buyaa-n
Copy link
Member

buyaa-n commented Mar 3, 2021

Oh sorry, I misunderstood, that makes sense, now i see the original description had Suggested default severity: Suggestion but in our triage, we made it Suggested severity: Warning i guess we did that because was not expecting false positives. And looks like we didn't talk about the severity in the API review meeting. By accounting for the false positives in the multitargeted environment i agree with changing the severity into Info level.

For the failures in the runtime repo, i am not sure if we want to ifdef those multitargeted builds, but it would be helpful if you could attach the list or warnings found, if you run the build with /p:TreatWarningsAsErrors=false option you can get all (or most) warnings happening in the build without failure
cc @terrajobst @bartonjs @stephentoub

@mavasani
Copy link
Member

mavasani commented Mar 3, 2021

in our triage, we made it Suggested severity: Warning i guess we did that because was not expecting a false positives.

IMO, analyzers reporting performance issues should never be a warning by default in the .NET SDK, even if we expect no false positives. Only critical functional or security issues should be candidate for being warnings by default.

@stephentoub
Copy link
Member

This should not be a warning.

@buyaa-n
Copy link
Member

buyaa-n commented Mar 3, 2021

IMO, analyzers reporting performance issues should never be a warning by default in the .NET SDK, even if we expect no false positives. Only critical functional or security issues should be candidate for being warnings by default.

@mavasani that makes sense to me, i will keep that in mind while triaging

@buyaa-n
Copy link
Member

buyaa-n commented Mar 3, 2021

@stephentoub do we want to fix the warnings found in runtime? Sounds like we need to do ifdef for multitargeted builds

@MeikTranel
Copy link

MeikTranel commented Mar 3, 2021

I have the fixes ready for runtime locally - it's just one that would actually surface for consumers and another one in testing/infrastructure code. It's pretty late in Germany so I hope y'all can live with me opening up the PR tomorrow 😊

Should I open a separate issue for that in runtime or is it okay to just throw them the PR?

@buyaa-n
Copy link
Member

buyaa-n commented Mar 3, 2021

Thanks, @MeikTranel, sounds good, i don't think we need an issue with that, you can reference this issue in your PR

MeikTranel added a commit to MeikTranel/runtime that referenced this issue Mar 3, 2021
…the overload isnt available

string.Contains(char) offers better performance since by design it only needs a single iteration loop.
In preparation for consuming a new analyzer we want this to be fixed in the runtime beforehand.

Discussion: dotnet#47180
@stephentoub
Copy link
Member

This was implemented as CA1847. Thanks!

@xtqqczze
Copy link
Contributor Author

Fixed by dotnet/roslyn-analyzers#4908.

@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 12, 2021
@buyaa-n buyaa-n moved this from In progress to Done in Roslyn Analyzers Aug 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
No open projects
9 participants