CA1861 Avoid constant arrays as arguments

[steveberdy]

CA1861: Avoid constant arrays as arguments

This analyzer is for performance improvements requested in dotnet/runtime#33794. The purpose of the analyzer and code fix is to extract constant arrays, that are passed on method invocation, as static readonly fields.

Documentation: dotnet/docs#25659

Fixes dotnet/runtime#33794

[steveberdy]

@Youssef1313 I finished work on all feedback, and even included more tests to cover various scenarios. Do you have any more feedback for me? Is this mergeable?

[Evangelink]

@steveberdy You will need to run msbuild /t:pack as stated in the build error (and PR comments):

One or more auto-generated documentation files were either edited manually, or not updated. Please revert changes made to the following files (if manually edited) and run msbuild /t:pack for each solution at the root of the repo to automatically update them:

We will then need to wait for a review from someone at MS. Thanks for the hardwork in the meantime :)

[jeffhandley]

I went ahead and ran dotnet msbuild /t:Pack and pushed to the branch.

[codecov]

Codecov Report

Merging #5383 (fc8992d) into main (89c1a9d) will decrease coverage by 0.02%.
The diff coverage is 97.96%.

❗ Current head fc8992d differs from pull request most recent head bbf1ebe. Consider uploading reports for the commit bbf1ebe to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5383      +/-   ##
==========================================
- Coverage   96.42%   96.40%   -0.02%     
==========================================
  Files        1372     1377       +5     
  Lines      319881   321935    +2054     
  Branches    10271    10434     +163     
==========================================
+ Hits       308438   310364    +1926     
- Misses       8985     9082      +97     
- Partials     2458     2489      +31     

[steveberdy]

@stephentoub Is this ready for merging?

[stephentoub]

@stephentoub Is this ready for merging?

I haven't reviewed it (other than glancing at it earlier to check the span thing I commented on). I expect one or more folks from @jeffhandley's team need to.

[buyaa-n]

@steveberdy please retarget to release/7.0.1xx branch, release release/6.0.1xx now closed for new analyzers.

More info

[steveberdy]

@buyaa-n thank you so much for your patience regarding this PR. Could I get a review for this?

[buyaa-n]

@buyaa-n thank you so much for your patience regarding this PR. Could I get a review for this?

Sorry for the delay, I thought you fixed the fixer issue and tested it with runtime repo again, the fixer still throws when try to apply the codefix in VS:

System.InvalidOperationException : GetCurrentNode returned null with the following node: new string[] { "close" } - line 288
   at Roslyn.Utilities.Contract.Fail(String message,Int32 lineNumber)
   at Microsoft.CodeAnalysis.Editing.SyntaxEditor.GetChangedRoot()
   at Microsoft.NetCore.Analyzers.Runtime.AvoidConstArraysFixer.ExtractConstArrayAsync(SyntaxNode root,SyntaxNode node,SemanticModel model,DocumentEditor editor,SyntaxGenerator generator,ImmutableDictionary`2 properties,CancellationToken cancellationToken)
   at async Microsoft.NetCore.Analyzers.Runtime.AvoidConstArraysFixer.<>c__DisplayClass5_0.<RegisterCodeFixesAsync>b__0(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.CodeActions.CodeAction.GetChangedSolutionAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.CodeActions.CodeAction.ComputeOperationsAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.CodeActions.CodeAction.GetPreviewOperationsAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedAction.GetPreviewResultAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedActionWithNestedFlavors.<>c__DisplayClass11_0.<GetPreviewAsync>b__0(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.Extensions.IExtensionManagerExtensions.PerformFunctionAsync[T](<Unknown Parameters>)

I will try to look later this week

[steveberdy]

I have reproduced that error. Looking into it now.

[steveberdy]

The fixer works for bulk code fixes, but not for individual ones. I'll debug this, but if you have any ideas please let me know.

[steveberdy]

I've identified the error and I've spent many hours trying to remedy it, but to no avail.
The analyzer works well in both testing and release environments, but the fixer only applies well in tests.

The problem is that the GetCurrentNode returns null for new nodes that are inserted, because they don't already exist on the syntax tree. The error is triggered by editor.GetChangedDocument once the DocumentEditor applies the replacement from the editor.ReplaceNode call in the snippet below.

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/AvoidConstArrays.Fixer.cs

Lines 94 to 102 in fec9a2f

	if (isInvoked)
	{
	editor.ReplaceNode(node, generator.WithExpression(identifier, node));
	}
	else
	{
	// add any extra trivia that was after the original argument
	editor.ReplaceNode(node, generator.Argument(identifier).WithTriviaFrom(arrayArgument.Syntax));
	}

As I've said, I've spent hours debugging and trying other potential ways to insert the node without getting this error, but I can't get much success. Can any of you provide better insight? Is there something I'm overlooking?

Thanks in advance.

[buyaa-n]

The analyzer works well in both testing and release environments, but the fixer only applies well in tests.

Thank you for your effort and sorry for delayed response @steveberdy, it sounded like some kind of race condition happening with editor while inserting/replacing the nodes. So, I moved the editor creation:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/AvoidConstArrays.Fixer.cs

Line 40 in fec9a2f

DocumentEditor editor = await DocumentEditor.CreateAsync(document, context.CancellationToken).ConfigureAwait(false);

into the ExtractConstArrayAsync and the issue is fixed.

After the fixer issue is solved, further I evaluated the findings and found following issues:

1. False positives in static read only field/property initializations, also I think we should not exclude static constructor's initializations, repro:

public class A
{
    public static readonly A Connection = new A(new string[] { "close" }); // Exclude array initializations in readonly fields/properties
    private static readonly HashSet<string> s_acceptedDays = new HashSet<string>(
          new string[] { "monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday" },
          StringComparer.OrdinalIgnoreCase
      );

    public static List<string> Property { get; }
  
    public A(string[] arr) { }

    static A() // Exclude array initializations in static constructors
    {
        Property = GetValues(new string[] { "close" });
    }

    private static List<string> GetValues(string[] arr) => null;
}

2. Another issue in fixer, the fixer throws when try to apply suggestion for conditional expression:

System.ArgumentOutOfRangeException : Specified argument was out of the range of valid values.
Parameter name: span
 at Microsoft.CodeAnalysis.SyntaxNode.FindNode(TextSpan span,Boolean findInsideTrivia,Boolean getInnermostNodeForTie)
 at async Microsoft.NetCore.Analyzers.Runtime.AvoidConstArraysFixer.ExtractConstArrayAsync(<Unknown Parameters>)
 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
 at async Microsoft.NetCore.Analyzers.Runtime.AvoidConstArraysFixer.<>c__DisplayClass5_0.<RegisterCodeFixesAsync>b__0(<Unknown Parameters>)
 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
 at async Microsoft.CodeAnalysis.CodeActions.CodeAction.GetChangedSolutionAsync(<Unknown Parameters>)
 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
 at async Microsoft.CodeAnalysis.CodeActions.CodeAction.ComputeOperationsAsync(<Unknown Parameters>)
 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
 at async Microsoft.CodeAnalysis.CodeActions.CodeAction.GetPreviewOperationsAsync(<Unknown Parameters>)
 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
 at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedAction.GetPreviewResultAsync(<Unknown Parameters>)
 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
 at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedActionWithNestedFlavors.PreviewChangesSuggestedAction.CreateAsync(<Unknown Parameters>)
 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
 at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedActionWithNestedFlavors.GetPreviewChangesFlavorAsync(<Unknown Parameters>)
 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
 at async Microsoft.CodeAnalysis.Editor.Implementation.Suggestions.SuggestedActionWithNestedFlavors.CreateAllFlavorsAsync(<Unknown Parameters>)
 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
 at async Microsoft.CodeAnalysis.Extensions.IExtensionManagerExtensions.PerformFunctionAsync[T](<Unknown Parameters>)

The code snippet looks like this:

    if (!de.Properties.Contains("nTSecurityDescriptor"))
        de.RefreshCache(new string[] { "nTSecurityDescriptor" });

3. NIT: The fixed value is misaligned in this case:
   
   It would be great if it can be fixed. Thank you!

[buyaa-n]

The analyzer works well in both testing and release environments, but the fixer only applies well in tests.

Thanks @steveberdy I pushed a fix for this fixer issue that happening runtime and the issue that causing System.ArgumentOutOfRangeException : Specified argument was out of the range of valid values.. Also excluded analysis for static constructor and read-only properties.

Now the analyzer is ready for the final review. @mavasani @Youssef1313 @Evangelink @stephentoub PTAL!

[buyaa-n]

LGTM