Replace local allocations with stack allocated spans

[terrajobst]

Flag places where known small temporary arrays of primitives (e.g. with a small constant length / where the total size of sizeof(T)*length can be determined to be less than some threshold) not inside any loop and not passed around could be replaced by span-based stackalloc.

Category: Performance

[jeffhandley]

Estimates:

• Analyzer: Large
• Fixer: Not Applicable

[carlossanlop]

Suggested severity: info

C# built in types sizes: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/integral-numeric-types#characteristics-of-the-integral-types

Examples:

// Restrictions:
// sizeof(T)*length -> Suggested max total size of 1 KB
// Limited to only primitive types
// Not passed to other methods
// Not used inside loops

// Flagged and suggested
int[] arr = new int[5];
Span<int> arrs = stackalloc int[5];

// Flagged and suggested
byte[] arr2 = new byte[] {​​ 1, 2, 3 }​​;
Span<byte> arr2s = stackalloc byte[] {​​ 1, 2, 3 }​​;

// Not flagged due to larger than suggested limit for its size
//     Size limitation for flagging:
//     sizeof(long) = 64 bit = 8B
//     max length for long = 128
long[] p = new long[130];

while (true)
{
    // Not flagged due to creation in loop
    int[] x = new int[3];
}

// Not flagged due to passed around
byte[] y = new byte[4];
MyMethod(y);

// Not flagged due to not primitive
MyClass[] w = new MyClass[8];

[GrabYourPitchforks]

stackalloc T[] is inherently more dangerous than new T[] for two main reasons: (a) it's not guaranteed to zero-init; and (b) it could cause an uncatchable StackOverflowException at runtime and terminate the process. Both of these are application-visible behavioral changes beyond mere performance considerations.

I'd suggest two modifications in order to address these concerns:

1. Don't suggest replacing new T[] with stackalloc T[] if [SkipLocalsInit] is defined within the method, type, or module. If this check isn't practical, consider using the compiler's /unsafe+ flag as a proxy for this information.

2. Stand up a second analyzer alongside this one that detects whether stackalloc T[] is given a non-const argument. This can help provide some protections for developers who write new T[50], follow the analyzer's suggestion to change it to stackalloc T[50], and at some future point replace the const 50 with a variable without knowing the full impact of doing so.

[stephentoub]

We should probably just close this. There are too many gotchas.