Avoid implicitly allocating params arrays in loops

[terrajobst]

Find calls to System.* methods inside loops, where those methods take params arrays and those params arrays are being allocated, and hoist the allocation to before the loop if possible.

Category: Performance

[jeffhandley]

Estimates:

• Analyzer: Medium
• Fixer: Large

[carlossanlop]

@terrajobst can you please provide a code example?

[carlossanlop]

• Suggested severity: Info
• Suggested category: Performance
• Suggested milestone: Future

Examples

Let's use the Console.WriteLine overload that takes a params array:

public static class Console
{
  public static void WriteLine(string format, params object?[]? arg);
}

Case 1

Flag

Before:

int i = 0;
while (i++ < 10)
{
    Console.WriteLine("{0}, {1}, {2}, {3}", 1, 2, 3, 4);
}

After:

int i = 0;
object[] arg = new object[] { 1, 2, 3, 4 };
while (i++ < 10)
{
    Console.WriteLine("{0}, {1}, {2}, {3}", arg);
}

Case 2

Do not flag: An element in the passed params array has variable data

int i = 0;
while (i++ < 10)
{
    Console.WriteLine("{0}, {1}, {2}, {3}", i, 2, 3, 4);
}

Case 3

Flag

Before: Variables are used inside the loop, but they weren't modified between their creation and their use:

short a = 10, b = 20, c = 30, d = 40;
int i = 0;
while (i++ < 10)
{
    Console.WriteLine("{0}, {1}, {2}, {3}", a, b, c, d);
}

After:

short a = 10, b = 20, c = 30, d = 40;
int i = 0;
object[] arg = new object[] { a, b, c, d };
while (i++ < 10)
{
    Console.WriteLine("{0}, {1}, {2}, {3}", arg);
}

Case 4

Having this class:

class C
{
    private string _name;
    public C(string name) => _name = name;
    public override string ToString() => _name;
}

Flag

Before:

int i = 0;
while (i++ < 10)
{
    Console.WriteLine("Format {0}, {1}", new C("a"), new C("b"), new C("c"), new C("d"));
}

After: Notice the data type of the array can be preserved as object[]

int i = 0;
object[] arg = new object[] { new C("a"), new C("b"), new C("c"), new C("d") };
while (i++ < 10)
{
    Console.WriteLine("{0}, {1}, {2}, {3}", arg);
}

Case 5

Do not flag: An element in the passed params array has variable data

int i = 0;
while (i++ < 10)
{
    Console.WriteLine("{0}, {1}, {2}, {3}", new C($"{i}"), new C("b"), new C("c"), new C("d"));
}

Case 6

Do not flag: Method does not belong to System.*

class Program
{
    static void Main()
    {
        int i = 0;
        while (i++ < 10)
        {
            MyOwnMethod(1, 2, 3, 4);
        }
    }

    static void MyOwnMethod(params object[] myargs){ }
}

---

Things to consider:

• We need to check that the array values do not get modified between iterations.
• If the analyzer flags a case, the fixer should create the array immediately before the beginning of the loop, using the data type of the method's params array parameter.
• The new array should be named after the method parameter's name, unless a variable with that name exist, then append a number.
• Restrict the analyzer to only check invocations of methods that live inside System.*.
• CS0231: params is always the last parameter in a method signature, so the analyzer should only check if the last parameter is a params array.

[bartonjs]

Video

The proposed fixes, and situations where they won't fix things, seem a bit strange. There's value in highlighting that the array allocation is happening in a loop, but the manner in which it gets fixed (or ignored) is too variable. So we shouldn't have a fixer at this time. (Ideally we'll get params-span and that'll make this go away)

• Category: Performance
• Severity: Hidden

[NewellClark]

I would like to be assigned to this, please.

[Youssef1313]

I think this is ALREADY implemented (except the "in loop" part and the attribute requirement).

https://github.com/dotnet/roslyn-analyzers/blob/a46689a39bd6bbea9761961b4683bb85c84e3be8/src/PerformanceSensitiveAnalyzers/Microsoft.CodeAnalysis.PerformanceSensitiveAnalyzers.sarif#L12-L24

An implementation of this as a "CAxxxx" can benefit from the existing implementation.

[huoyaoyuan]

With params ReadOnlySpan<T> coming, is this still valuable?