[Analyzer proposal]: Simplify calls to string.Substring and Memory/Span.Slice

[GrabYourPitchforks]

Inspired by Steve's comment at #68937 (comment).

We should consider promoting usage of Substring/Slice(offset) over Substring/Slice(offset, length) for "slice to the end of the buffer" scenarios. In both cases it makes the code a little easier to read and is less error-prone since it doesn't require the caller to get the length check correct. And for Slice specifically, it will result in slightly better codegen.

// Detect these patterns
someString.Substring(expression, someString.Length - expression);
someMemory.Slice(expression, someMemory.Length - expression);
someSpan.Slice(expression, someSpan.Length - expression);

// And replace them with these:
someString.Substring(expression);
someMemory.Slice(expression);
someSpan.Slice(expression);

Any analyzer / fixer would need to be a little smart and ensure that evaluating neither this nor expression has side effects, otherwise changing the number of occurrences from 2 to 1 would result in an observable behavioral change. This means that we might not be able to trigger the analyzer / fixer if this or expression contains a property accessor, unless we can reason that the property accessor is a dumb field wrapper.

The prevalent patterns that I saw in #68937 were:

value.Substring(local, value.Length - local);
value.Substring(local1 + local2, value.Length - local1 - local2);
value.Substring(local1 + local2, value.Length - (local1 + local2));
value.Substring(field, value.Length - field);
value.Substring(field + const, value.Length - field - const);
value.Substring(field + const, value.Length - (field + const));

Suggested category: Maintainability
Suggested severity: Info

[ghost]

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

Issue Details

---

Inspired by Steve's comment at #68937 (comment).

We should consider promoting usage of Substring/Slice(offset) over Substring/Slice(offset, length) for "slice to the end of the buffer" scenarios. In both cases it makes the code a little easier to read and is less error-prone since it doesn't require the caller to get the length check correct. And for Slice specifically, it will result in slightly better codegen.

// Detect these patterns
someString.Substring(expression, someString.Length - expression);
someMemory.Slice(expression, someMemory.Length - expression);
someSpan.Slice(expression, someSpan.Length - expression);

// And replace them with these:
someString.Substring(expression);
someMemory.Slice(expression);
someSpan.Slice(expression);

Any analyzer / fixer would need to be a little smart and ensure that evaluating neither this nor expression has side effects, otherwise changing the number of occurrences from 2 to 1 would result in an observable behavioral change. This means that we might not be able to trigger the analyzer / fixer if this or expression contains a property accessor, unless we can reason that the property accessor is a dumb field wrapper.

The prevalent patterns that I saw in #68937 were:

value.Substring(local, value.Length - local);
value.Substring(local1 + local2, value.Length - local1 - local2);
value.Substring(local1 + local2, value.Length - (local1 + local2));
value.Substring(field, value.Length - field);
value.Substring(field + const, value.Length - field - const);
value.Substring(field + const, value.Length - (field + const));

Suggested category: Maintainability
Suggested severity: Info

Author:	GrabYourPitchforks
Assignees:	-
Labels:	area-System.Runtime, code-analyzer, code-fixer
Milestone:	-

[carlossanlop]

@GrabYourPitchforks your proposal looks ready for review, especially since you already found a bunch of cases in #68937 .

[terrajobst]

Video

• Looks good as proposed

Category: Maintainability
Severity: Info

[buyaa-n]

Estimates:

Analyzer: Small
Fixer: Small

[TonyValenti]

Instead of using Substring, would it be better to use the slice syntax?

Ie. String[x..] ?

[dstarosta]

I wish it would also report a missing check for length when the optimization is not applicable. That is one of the biggest sources of bugs I have seen with .NET in my experience.

[cybertyche]

Instead of using Substring, would it be better to use the slice syntax?

Ie. String[x..] ?

Given that there is already another analyzer that does that particular recommendation (move from string.Substring(foo) to string[foo..]) it does seem prudent to go directly to the slice syntax rather than create a syntax that another analyzer is just going to flag.

[cybertyche]

On the other hand, the rule that suggests the slice syntax as a simplification of substring is an IDE not a CA warning. It might therefore be good to do it in two parts, one that removes the second parameter (CA) and one that then suggests moving to the slice syntax (IDE).

[mpidash]

I would like to work on this analyzer if that is ok.

[carlossanlop]

@mpidash thank you for offering your help! I'm assigning the issue to you.

If you have any questions, feel free to ask @mavasani, @Youssef1313, @buyaa-n or I for help.