Repository navigation
[Analyzer proposal]: Simplify calls to string.Substring and Memory/Span.Slice #68946
Description
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
Activity
carlossanlop commentedon May 12, 2022
@GrabYourPitchforks your proposal looks ready for review, especially since you already found a bunch of cases in #68937 .
terrajobst commentedon Jun 23, 2022
Video
Category: Maintainability
Severity: Info
buyaa-n commentedon Nov 15, 2022
Estimates:
Analyzer: Small
Fixer: Small
TonyValenti commentedon Dec 4, 2022
Instead of using Substring, would it be better to use the slice syntax?
Ie. String[x..] ?
dstarosta commentedon Jul 21, 2023
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 commentedon Jul 22, 2023
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 commentedon Jul 23, 2023
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 commentedon Jul 25, 2023
I would like to work on this analyzer if that is ok.
carlossanlop commentedon Jul 25, 2023
@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.