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

Prefer string.AsSpan() over string.Substring() when parsing #33784

Closed
terrajobst opened this issue Mar 19, 2020 · 9 comments
Closed

Prefer string.AsSpan() over string.Substring() when parsing #33784

terrajobst opened this issue Mar 19, 2020 · 9 comments
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

@terrajobst
Copy link
Member

The rule should flag instances of a pattern like int.Parse(str.Substring(...)) and instead switch to using the span-based int.Parse(str.AsSpan(...)). This would apply to all of the primitive types, and more generally potentially anything that has an overload taking a ReadOnlySpan<char> instead of a string.

Category: Performance

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@jeffhandley jeffhandley added this to the Future milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Small
  • Fixer: Small

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2020
@GrabYourPitchforks
Copy link
Member

Seems reasonable. We should figure out how much we want our analyzers (especially our on-by-default analyzers) to push people into using span. Using ROS<char> increases the concept count for scenarios which are currently very simple.

@bartonjs
Copy link
Member

bartonjs commented Nov 10, 2020

Video

  • Find argument expressions that are str.Substring(...) and there's an overload (or the same overload) that accepts ReadOnlySpan<char> in that position
  • Suggest changing Substring to AsSpan for the call

Category: Performance
Suggested severity: Info

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 10, 2020
@carlossanlop
Copy link
Member

@Mrnikbobjeff also approved in case you already started working on this. Let us know if you'd like to get this issue assigned to you.

@Mrnikbobjeff
Copy link

@Mrnikbobjeff also approved in case you already started working on this. Let us know if you'd like to get this issue assigned to you.

I am currently working on porting my analyzers to be IOperation analyzers to be inline with the pattern already established. While I have a working implementation for a c# analyzer and fixer I still need to port the analyzer at least for this to be viable.

@carlossanlop
Copy link
Member

Would you like me to assign this to you @Mrnikbobjeff or should i mark it up-for-grabs instead?

@carlossanlop
Copy link
Member

I closed #33778 in favor of this one (it was an identical approved proposal).

Note to whoever decides to work on this: Please take a look at this comment in the dup issue, since it contains a list of cases this analyzer should be able to address.

@NewellClark
Copy link
Contributor

I would like to be assigned to this. I'll start work on it immediately.

@AraHaan
Copy link
Member

AraHaan commented Apr 12, 2021

How does one enable this AsSpan diagnostics for netstandard2.0+ for using it over Substring?

Edit: It seems that the rule does not flag all uses of string.Substring, I would like all uses to get a diagnostic for replacing it with AsSpan as it will increase my program's performance indirectly, yes in some cases my code makes substrings and assigns them to variables before passing them to methods and so the AsSpan version could help me greatly in that case.

@buyaa-n buyaa-n closed this as completed Aug 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 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
None yet
Development

No branches or pull requests

10 participants