-
Notifications
You must be signed in to change notification settings - Fork 463
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
CA1861 Avoid constant arrays as arguments (#5383)
* AvoidConstArrays analyzer drafted * Member name from type reflects original definition * Remove MyCodeAction * Added assembly reference for globalization * Update to ignore params arguments * Added tests for params arguments * perf improvement and added namespaces to all tests * added exclusion for readonly field assignments, more tests * Fix issues in fixer, do not warn for static constructor and readonly properties --------- Co-authored-by: Jeff Handley <jeff.handley@microsoft.com> Co-authored-by: Buyaa Namnan <bunamnan@microsoft.com>
- Loading branch information
3 people
authored
May 8, 2023
1 parent
8f17a8d
commit 3c81890
Showing
23 changed files
with
1,431 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
184 changes: 184 additions & 0 deletions
184
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/AvoidConstArrays.Fixer.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,184 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Linq; | ||
| using System.Globalization; | ||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.Composition; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Analyzer.Utilities; | ||
| using Analyzer.Utilities.Extensions; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.CodeActions; | ||
| using Microsoft.CodeAnalysis.CodeFixes; | ||
| using Microsoft.CodeAnalysis.Editing; | ||
| using Microsoft.CodeAnalysis.Operations; | ||
|
|
||
| namespace Microsoft.NetCore.Analyzers.Runtime | ||
| { | ||
| /// <summary> | ||
| /// CA1861: Avoid constant arrays as arguments. Replace with static readonly arrays. | ||
| /// </summary> | ||
| [ExportCodeFixProvider(LanguageNames.CSharp, LanguageNames.VisualBasic), Shared] | ||
| public sealed class AvoidConstArraysFixer : CodeFixProvider | ||
| { | ||
| public sealed override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(AvoidConstArraysAnalyzer.RuleId); | ||
|
|
||
| private static readonly ImmutableArray<string> s_collectionMemberEndings = ImmutableArray.Create("array", "collection", "enumerable", "list"); | ||
|
|
||
| // See https://github.com/dotnet/roslyn/blob/main/docs/analyzers/FixAllProvider.md for more information on Fix All Providers | ||
| public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; | ||
|
|
||
| public override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
| { | ||
| Document document = context.Document; | ||
| SyntaxNode root = await document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
| SyntaxNode node = root.FindNode(context.Span); | ||
|
|
||
| context.RegisterCodeFix(CodeAction.Create( | ||
| MicrosoftNetCoreAnalyzersResources.AvoidConstArraysCodeFixTitle, | ||
| async ct => await ExtractConstArrayAsync(document, root, node, context.Diagnostics[0].Properties, ct).ConfigureAwait(false), | ||
| equivalenceKey: nameof(MicrosoftNetCoreAnalyzersResources.AvoidConstArraysCodeFixTitle)), | ||
| context.Diagnostics); | ||
| } | ||
|
|
||
| private static async Task<Document> ExtractConstArrayAsync(Document document, SyntaxNode root, SyntaxNode node, | ||
| ImmutableDictionary<string, string?> properties, CancellationToken cancellationToken) | ||
| { | ||
| SemanticModel model = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); | ||
| DocumentEditor editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); | ||
| SyntaxGenerator generator = editor.Generator; | ||
| IArrayCreationOperation arrayArgument = GetArrayCreationOperation(node, model, cancellationToken, out bool isInvoked); | ||
| INamedTypeSymbol containingType = model.GetEnclosingSymbol(node.SpanStart, cancellationToken).ContainingType; | ||
|
|
||
| // Get a valid member name for the extracted constant | ||
| string newMemberName = GetExtractedMemberName(containingType.MemberNames, properties["paramName"] ?? GetMemberNameFromType(arrayArgument)); | ||
|
|
||
| // Get method containing the symbol that is being diagnosed | ||
| IOperation? methodContext = arrayArgument.GetAncestor<IMethodBodyOperation>(OperationKind.MethodBody); | ||
| methodContext ??= arrayArgument.GetAncestor<IBlockOperation>(OperationKind.Block); // VB methods have a different structure than CS methods | ||
|
|
||
| // Create the new member | ||
| SyntaxNode newMember = generator.FieldDeclaration( | ||
| newMemberName, | ||
| generator.TypeExpression(arrayArgument.Type), | ||
| GetAccessibility(methodContext is null ? null : model.GetEnclosingSymbol(methodContext.Syntax.SpanStart, cancellationToken)), | ||
| DeclarationModifiers.Static | DeclarationModifiers.ReadOnly, | ||
| arrayArgument.Syntax.WithoutTrailingTrivia() // don't include extra trivia before the end of the declaration | ||
| ); | ||
|
|
||
| // Add any additional formatting | ||
| if (methodContext is not null) | ||
| { | ||
| newMember = newMember.FormatForExtraction(methodContext.Syntax); | ||
| } | ||
|
|
||
| ISymbol lastFieldOrPropertySymbol = containingType.GetMembers().LastOrDefault(x => x is IFieldSymbol or IPropertySymbol); | ||
| if (lastFieldOrPropertySymbol is not null) | ||
| { | ||
| var span = lastFieldOrPropertySymbol.Locations.First().SourceSpan; | ||
| if (root.FullSpan.Contains(span)) | ||
| { | ||
| // Insert after fields or properties | ||
| SyntaxNode lastFieldOrPropertyNode = root.FindNode(span); | ||
| editor.InsertAfter(generator.GetDeclaration(lastFieldOrPropertyNode), newMember); | ||
| } | ||
| else | ||
| { | ||
| // Span not found | ||
| editor.InsertBefore(methodContext?.Syntax, newMember); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // No fields or properties, insert right before the containing method for simplicity | ||
| editor.InsertBefore(methodContext?.Syntax, newMember); | ||
| } | ||
|
|
||
| // Replace argument with a reference to our new member | ||
| SyntaxNode identifier = generator.IdentifierName(newMemberName); | ||
| 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)); | ||
| } | ||
|
|
||
| // Return changed document | ||
| return editor.GetChangedDocument(); | ||
| } | ||
|
|
||
| private static IArrayCreationOperation GetArrayCreationOperation(SyntaxNode node, SemanticModel model, CancellationToken cancellationToken, out bool isInvoked) | ||
| { | ||
| // The analyzer only passes a diagnostic for two scenarios, each having an IArrayCreationOperation: | ||
| // 1. The node is an IArgumentOperation that is a direct parent of an IArrayCreationOperation | ||
| // 2. The node is an IArrayCreationOperation already, as it was pulled from an | ||
| // invocation, like with LINQ extension methods | ||
|
|
||
| // If this is a LINQ invocation, the node is already an IArrayCreationOperation | ||
| if (model.GetOperation(node, cancellationToken) is IArrayCreationOperation arrayCreation) | ||
| { | ||
| isInvoked = true; | ||
| return arrayCreation; | ||
| } | ||
| // Otherwise, we'll get the IArrayCreationOperation from the argument node's child | ||
| isInvoked = false; | ||
| return (IArrayCreationOperation)model.GetOperation(node.ChildNodes().First(), cancellationToken); | ||
| } | ||
|
|
||
| private static string GetExtractedMemberName(IEnumerable<string> memberNames, string parameterName) | ||
| { | ||
| bool hasCollectionEnding = s_collectionMemberEndings.Any(x => parameterName.EndsWith(x, true, CultureInfo.InvariantCulture)); | ||
|
|
||
| if (parameterName == "source" // for LINQ, "sourceArray" is clearer than "source" | ||
| || (memberNames.Contains(parameterName) && !hasCollectionEnding)) | ||
| { | ||
| parameterName += "Array"; | ||
| } | ||
|
|
||
| if (memberNames.Contains(parameterName)) | ||
| { | ||
| int suffix = 0; | ||
| while (memberNames.Contains(parameterName + suffix)) | ||
| { | ||
| suffix++; | ||
| } | ||
|
|
||
| return parameterName + suffix; | ||
| } | ||
|
|
||
| return parameterName; | ||
| } | ||
|
|
||
| private static string GetMemberNameFromType(IArrayCreationOperation arrayCreationOperation) | ||
| { | ||
| #pragma warning disable CA1308 // Normalize strings to uppercase | ||
| return ((IArrayTypeSymbol)arrayCreationOperation.Type).ElementType.OriginalDefinition.Name.ToLowerInvariant() + "Array"; | ||
| #pragma warning restore CA1308 // Normalize strings to uppercase | ||
| } | ||
|
|
||
| private static Accessibility GetAccessibility(ISymbol? methodSymbol) | ||
| { | ||
| // If private or public, return private since public accessibility not wanted for fields by default | ||
| return methodSymbol?.GetResultantVisibility() switch | ||
| { | ||
| // Return internal if internal | ||
| SymbolVisibility.Internal => Accessibility.Internal, | ||
| _ => Accessibility.Private | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| internal static class SyntaxNodeExtensions | ||
| { | ||
| internal static SyntaxNode FormatForExtraction(this SyntaxNode node, SyntaxNode previouslyContainingNode) | ||
| { | ||
| return node.HasTrailingTrivia ? node : node.WithTrailingTrivia(previouslyContainingNode.GetTrailingTrivia()); | ||
| } | ||
| } | ||
| } |
153 changes: 153 additions & 0 deletions
153
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/AvoidConstArrays.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using System.Linq; | ||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using Analyzer.Utilities; | ||
| using Analyzer.Utilities.Extensions; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.CodeAnalysis.Operations; | ||
|
|
||
| namespace Microsoft.NetCore.Analyzers.Runtime | ||
| { | ||
| using static MicrosoftNetCoreAnalyzersResources; | ||
|
|
||
| /// <summary> | ||
| /// CA1861: Avoid constant arrays as arguments. Replace with static readonly arrays. | ||
| /// </summary> | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] | ||
| public sealed class AvoidConstArraysAnalyzer : DiagnosticAnalyzer | ||
| { | ||
| internal const string RuleId = "CA1861"; | ||
|
|
||
| internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create(RuleId, | ||
| CreateLocalizableResourceString(nameof(AvoidConstArraysTitle)), | ||
| CreateLocalizableResourceString(nameof(AvoidConstArraysMessage)), | ||
| DiagnosticCategory.Performance, | ||
| RuleLevel.IdeSuggestion, | ||
| CreateLocalizableResourceString(nameof(AvoidConstArraysDescription)), | ||
| isPortedFxCopRule: false, | ||
| isDataflowRule: false); | ||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); | ||
|
|
||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.EnableConcurrentExecution(); | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
|
|
||
| context.RegisterCompilationStartAction(context => | ||
| { | ||
| INamedTypeSymbol? readonlySpanType = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemReadOnlySpan1); | ||
| INamedTypeSymbol? functionType = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemFunc2); | ||
| // Analyzes an argument operation | ||
| context.RegisterOperationAction(context => | ||
| { | ||
| IArgumentOperation? argumentOperation; | ||
| if (context.ContainingSymbol is IMethodSymbol method && method.MethodKind == MethodKind.StaticConstructor) | ||
| { | ||
| return; | ||
| } | ||
| if (context.Operation is IArrayCreationOperation arrayCreationOperation) // For arrays passed as arguments | ||
| { | ||
| argumentOperation = arrayCreationOperation.GetAncestor<IArgumentOperation>(OperationKind.Argument); | ||
| // If no argument, return | ||
| // If argument is passed as a params array but isn't itself an array, return | ||
| if (argumentOperation is null || (argumentOperation.Parameter.IsParams && arrayCreationOperation.IsImplicit)) | ||
| { | ||
| return; | ||
| } | ||
| } | ||
| else if (context.Operation is IInvocationOperation invocationOperation) // For arrays passed in extension methods, like in LINQ | ||
| { | ||
| IEnumerable<IOperation> invocationDescendants = invocationOperation.Descendants(); | ||
| if (invocationDescendants.Any(x => x is IArrayCreationOperation) | ||
| && invocationDescendants.Any(x => x is IArgumentOperation)) | ||
| { | ||
| // This is an invocation that contains an array as an argument | ||
| // This will get caught by the first case in another cycle | ||
| return; | ||
| } | ||
| argumentOperation = invocationOperation.Arguments.FirstOrDefault(); | ||
| if (argumentOperation is not null) | ||
| { | ||
| if (argumentOperation.Value is not IConversionOperation conversionOperation | ||
| || conversionOperation.Operand is not IArrayCreationOperation arrayCreation) | ||
| { | ||
| return; | ||
| } | ||
| arrayCreationOperation = arrayCreation; | ||
| } | ||
| else // An invocation, extension or regular, has an argument, unless it's a VB extension method call | ||
| { | ||
| // For VB extension method invocations, find a matching child | ||
| arrayCreationOperation = (IArrayCreationOperation)invocationDescendants | ||
| .FirstOrDefault(x => x is IArrayCreationOperation); | ||
| if (arrayCreationOperation is null) | ||
| { | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| return; | ||
| } | ||
| // Must be literal array | ||
| if (arrayCreationOperation.Initializer.ElementValues.Any(x => x is not ILiteralOperation)) | ||
| { | ||
| return; | ||
| } | ||
| string? paramName = null; | ||
| if (argumentOperation is not null) | ||
| { | ||
| IFieldInitializerOperation? fieldInitializer = argumentOperation.GetAncestor<IFieldInitializerOperation>( | ||
| OperationKind.FieldInitializer, f => f.InitializedFields.Any(x => x.IsReadOnly)); | ||
| IPropertyInitializerOperation? propertyInitializer = argumentOperation.GetAncestor<IPropertyInitializerOperation>( | ||
| OperationKind.PropertyInitializer, p => p.InitializedProperties.Any(x => x.IsReadOnly)); | ||
| if (fieldInitializer is not null || propertyInitializer is not null) | ||
| { | ||
| return; | ||
| } | ||
| ITypeSymbol originalDefinition = argumentOperation.Parameter.Type.OriginalDefinition; | ||
| // Can't be a ReadOnlySpan, as those are already optimized | ||
| if (SymbolEqualityComparer.Default.Equals(readonlySpanType, originalDefinition)) | ||
| { | ||
| return; | ||
| } | ||
| // Check if the parameter is a function so the name can be set to null | ||
| // Otherwise, the parameter name doesn't reflect the array creation as well | ||
| bool isDirectlyInsideLambda = originalDefinition.Equals(functionType); | ||
| // Parameter shouldn't have same containing type as the context, to prevent naming ambiguity | ||
| // Ignore parameter name if we're inside a lambda function | ||
| if (!isDirectlyInsideLambda && !argumentOperation.Parameter.ContainingType.Equals(context.ContainingSymbol.ContainingType)) | ||
| { | ||
| paramName = argumentOperation.Parameter.Name; | ||
| } | ||
| } | ||
| ImmutableDictionary<string, string?>.Builder properties = ImmutableDictionary.CreateBuilder<string, string?>(); | ||
| properties.Add("paramName", paramName); | ||
| context.ReportDiagnostic(arrayCreationOperation.CreateDiagnostic(Rule, properties.ToImmutable())); | ||
| }, | ||
| OperationKind.ArrayCreation, | ||
| OperationKind.Invocation); | ||
| }); | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.