CA1861: Avoid constant arrays improvements

[Youssef1313]

• https://github.com/dotnet/roslyn-analyzers/blob/80ac9265df5c8d70cbb25b57057b0f2772ed7d2a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/AvoidConstArrays.cs#L61 I don't see why the analyzer shouldn't report this case. It's no different than being explicit.

• https://github.com/dotnet/roslyn-analyzers/blob/80ac9265df5c8d70cbb25b57057b0f2772ed7d2a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/AvoidConstArrays.cs#L105-L110 I don't see why it must be a literal array. If it's referencing constants or static readonly fields, it should still be reported and benefit from the performance optimization.

[mavasani]

@buyaa-n @carlossanlop for further triage.

[manfred-brands]

I agree with the first point, in IL the code generated for the following three statements is the same, but only the explicit one gets an CA1861 warning. I know the last one is reported in #7090

string trimName1 = name.Trim('.', ',', ';', ' ');
string trimName2 = name.Trim(new[] { '.', ',', ';', ' ' });
string trimName3 = name.Trim(['.', ',', ';', ' ']);

An example for the 2nd point where the array creation does not trigger CA1861 because the members are not literals.

const char dot = '.';
const char comma = ',';
const char semicolon = ';';
const char space = ' ';

string trimName4 = name.Trim(new[] { dot, comma, semicolon, space });

[manfred-brands]

@mavasani If you agree, I'm willing to create a PR for the above.

[manfred-brands]

I'm not sure if you want to put it in this improvement ticket, but I just did a review where someone changed the in-line array creation to creating a local and referencing that one:

string[] parts = line.Split(new[] { ',', ';' }, StringSplitOptions.RemoveEmptyEntries);

into

char[] separators = new[] { ',', ';' };
string[] parts = line.Split(separators, StringSplitOptions.RemoveEmptyEntries);

This will generate the same IL code. Should CA1861 also trigger when the parameter is a local created from constants?

[steveberdy]

@manfred-brands I see slightly different IL code for both of those code snippets

[manfred-brands]

@steveberdy The differences I see are

• An additional stloc.0 followed by a ldloc.0 to store and then load the variable.
• And the ldarg.1 for the line parameter (this argument) is done either before or after the array creation

Both however create a new array purely for the method which is the point CA1861 tries to address.

[steveberdy]

Should CA1861 also trigger when the parameter is a local created from constants?

When I was creating the analyzer, we discussed that there could be potential cases where the array was mutated after being passed as an argument. In the case of the array being a local, the odds are higher for it being mutated.

I think this is out of the scope of the analyzer. In general, constant, un-mutated locals would be better off as static readonly (generally speaking, but not always), so perhaps that would warrant a separate analyzer. With CA1861, it's really a different analysis since we're dealing with a direct argument that we know is an array of constants.