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: Introduce an analyzer to recommend the more efficient string splitting features in .NET 8 #85487

Open
geeknoid opened this issue Apr 27, 2023 · 3 comments
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 help wanted [up-for-grabs] Good issue for external contributors partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@geeknoid
Copy link
Member

geeknoid commented Apr 27, 2023

Introduce a performance-centric analyzer that detects uses of String.Split and recommends the new more efficient string splitting functionality from MemoryExtensions. I'm not sure a fixer can be provided here, it might be too complicated for that. But having an analyzer to point out an obvious place that can get a perf boost is still useful.

Category : Performance
Severity : Info

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 27, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 27, 2023
@danmoseley danmoseley added code-analyzer Marks an issue that suggests a Roslyn analyzer area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 27, 2023
@ghost
Copy link

ghost commented Apr 27, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Introduce a performance-centric analyzer that detects uses of String.Split and recommends the new more efficient string splitting functionality from MemoryExtensions. I'm not sure a fixer can be provided here, it might be too complicated for that. But having an analyzer to point out an obvious place that can get a perf boost is still useful.

Author: geeknoid
Assignees: -
Labels:

area-System.Runtime, untriaged, code-analyzer

Milestone: -

@jeffhandley jeffhandley added api-suggestion Early API idea and discussion, it is NOT ready for implementation partner-impact This issue impacts a partner who needs to be kept updated labels May 17, 2023
@buyaa-n buyaa-n added this to the 8.0.0 milestone May 17, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 17, 2023
@buyaa-n
Copy link
Contributor

buyaa-n commented May 25, 2023

[Triage note]: We shouldn't flag for all string.Split cases, need to look for patterns where the new API could be used. Based on the pattern a fixer will be offered. When #934 implemented we could replace more string.Split cases

Category : Performance
Severity : Info

Question:

  • What pattern the analyzer should look for?

  • We might also want to flag String.Trim, String.TrimEnt, String.TrimStart and suggest new Trim, TrimEnd, TrimStart from MemoryExtensions. Maybe with different ID as these could have code-fixers.

@buyaa-n buyaa-n 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 labels May 30, 2023
@bartonjs
Copy link
Member

bartonjs commented Jun 1, 2023

Video

Out of a concern of false positives, it seems like the initial version of this analyzer should be limited to those callsites where a count was passed into string.Split, as that pattern has a perfect analogue in the span-based split.

Using uncounted string.Split in a foreach would be better handled by an iterator-based split (which we don't have yet). Other uses of uncounted split might be analyzable, but they're harder to describe, so they should probably be a different diagnostic ID (once the pattern can be analyzed) just to keep the docs/explanation sane.

The suggestion regarding Trim/TrimEnd/TrimStart may have merit, but seems like it might need some more thought (as it depends heavily on what happens after the call to Trim)... and should be addressed in a separate issue, with more details/examples.

Category: Performance
Severity: Info

@bartonjs bartonjs 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 Jun 1, 2023
@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Jun 1, 2023
@tannergooding tannergooding modified the milestones: 8.0.0, Future Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 help wanted [up-for-grabs] Good issue for external contributors partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

No branches or pull requests

6 participants