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

StringBuilder usage analyzer idea #1638

Open
Therzok opened this issue Mar 21, 2018 · 3 comments
Open

StringBuilder usage analyzer idea #1638

Therzok opened this issue Mar 21, 2018 · 3 comments
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Feature Request help wanted The issue is up-for-grabs, and can be claimed by commenting
Milestone

Comments

@Therzok
Copy link

Therzok commented Mar 21, 2018

Analyzer package

Could go into Microsoft.CodeQuality.Analyzers

Analyzer

Example: StringBuilder usage can be optimized in some API usage.

public string Append (string text)
{
    var sb = new StringBuilder();
    sb.Append(text.Substring (0, 6));
    sb.Append(text.Substring (2));
    return sb.ToString ();
}

The analyzer would highlight 2 bad usages of substring as parameters to StringBuilder.Append, on the text.Substring calls.

A code fix would be to transform these:

public string Append (string text)
{
    var sb = new StringBuilder();
    sb.Append(text, 0, 6);
    sb.Append(text, 2, text.Length - 2);
    return sb.ToString ();
}

The end result of this analyzer would be removing intermediary string allocations.

@jongleur1983
Copy link

Started to give it a try yesterday, not yet finished.
Current status:
My code finds both patterns from the example. Reporting diagnostics and fixers not done yet (have to figure out how to do that, unit tests only testing these exact examples, yet, so there have to be more.

Hope to get something online this week.

jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 11, 2018
…-path tests

Missing yet:
- resources have no real values
- RuleIds might have to be changed?
- helpLinkUri s missing
- VB variant not working
- FixAll not working properly
@jongleur1983
Copy link

Puhed my current state now - would be happy to get some more input, not expecting the PR to be merged in this state (see PR description)

jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 13, 2018
thanks @mavasani for giving the right hints on what to do.
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 13, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 13, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 13, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 13, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 13, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 13, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 13, 2018
…uage agnostic

a big big thanks to @CyrusNajmabadi for the great help.

meanwhile solving some more review remarks:
- using getInnerMostNodeForTie() on the context
- return tasks directly instead of awaiting async methods
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 13, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 14, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 14, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 14, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 14, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 14, 2018
- rename cancellationToken to ct (was ctx)
- add explaining comments to code generation calls
- improve reliability by save checking that the nodeToFix is an IInvocationExpression. This is checked on the Analyzer and a necessary condition to report the diagnostics, but it's not tightly coupled, so better be safe here.
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 14, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 15, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 16, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 16, 2018
covering named parameters in different order, method chainging of the stringBuilder.Append() and of
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 17, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 17, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 17, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 17, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 17, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 18, 2018
jongleur1983 added a commit to jongleur1983/roslyn-analyzers that referenced this issue Oct 18, 2018
the one-parameter replacement sb.Append(xxxx.Substring(a)) to sb.Append(xxxx, a, xxxx.Length - a)
is 1) dangerous when xxxx has side effects, as it's code is executed twice after the "fix",
and 2) may have performance problems.

Therefore this commit restricts to some Operation types that can be considered to be safe.
provided substrings cover at least one case per operation kind that works.
For the other OperationKinds, where no diagnostic must be reported I did not find ways yet how to create code that works reasonable.
Happy to get hints here or advice what can safely be omitted as test cases. Precondition: The parameter must be of type string to be relevant for the code.
@sharwell sharwell added help wanted The issue is up-for-grabs, and can be claimed by commenting and removed Up for Grabs labels Aug 1, 2019
@Evangelink
Copy link
Member

@mavasani does it also need to get through the runtime team?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Feature Request help wanted The issue is up-for-grabs, and can be claimed by commenting
Projects
None yet
Development

No branches or pull requests

5 participants