Prefer string.AsSpan over string.Substring

[terrajobst]

Somewhat more generally, any time string.Substring is used as an argument to something where there's an equivalent overload that takes a ReadOnlySpan<char> (e.g. StringBuilder.Append(string) vs StringBuilder.Append(ReadOnlySpan<char>)), the case can be flagged to be changed to use AsSpan instead.

Category: Performance

[jeffhandley]

Estimates:

• Analyzer: Small
• Fixer: Small

[carlossanlop]

Suggested severity: Info

We implemented a very similar analyzer for Stream.ReadAsync and Stream.WriteAsync that suggested preferring the memory-based overloads. The logic can be very similar here so whoever works on this can use that analyzer for reference: dotnet/roslyn-analyzers#3592

static void Main()
{
    int n = 0;
    string s = "My string";

    // Substring overload with 1 argument
    // Before
    MyMethod(s.Substring(1));
    // After
    MyMethod(s.AsSpan(1));


   // Substring overload with 2 arguments
    // Before
    MyMethod(s.Substring(1, 3));
    // After
    MyMethod(s.AsSpan(1, 3));


    // Extra arguments, find overload with same types except string->ROS
    // Before
    MyMethod(n, s.Substring(1));
    // After
    MyMethod(n, s.AsSpan(1));


    // Named arguments, match overload with string->ROS in same position
    // Before
    MyMethod(s: s.Substring(1), n: n);
    // After
    MyMethod(ros: s.AsSpan(1), n: n);


    // Named arguments, multiple strings, match overload with strings->ROS in same positions
    // Before
    MyMethod(s2: s.Substring(1), n: n, s1: s.Substring(2));
    // After
    MyMethod(ros2: s.AsSpan(1), n: n, ros1: s.Substring(2));
}

static void MyMethod(string s);
static void MyMethod(ReadOnlySpan<char> ros);
static void MyMethod(int n, string s);
static void MyMethod(int n, ReadOnlySpan<char> ros);
static void MyMethod(int n, string s1, string s2);
static void MyMethod(int n, ReadOnlySpan<char> ros1, ReadOnlySpan<char> ros2);

[stephentoub]

Suggested severity: Warning

I don't think any purely perf-focused rules like this should break a build (which warning can do if warning-as-error is enabled). It should likely be Info.

[terrajobst]

Video

• On by default as info seems fine

[carlossanlop]

@terrajobst I just noticed this issue is almost identical to #33784.

The only difference is that the other issue is specifically targeted to substrings passed as arguments to Parse methods of primitive types, while this issue is specifically targeted to substrings passed as arguments to any methods that take a string.

Is there a reason I'm missing for which there are two separate issues? Couldn't we have one analyzer target all methods?

Note: The other issue was approved back in November, and is already assigned to someone working on it. So if we have to close one, it should be this one.

[carlossanlop]

Talked to @terrajobst about this. This is a duplicate approval of #33784. Since the other one was approved first, I'll keep that one open.