-
Notifications
You must be signed in to change notification settings - Fork 466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prefer readonlyspan properties to array fields #5548
Open
NewellClark
wants to merge
25
commits into
dotnet:main
Choose a base branch
from
NewellClark:prefer-readonlyspan-properties-to-array-fields
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
984fc0b
Create files and basic tests
NewellClark 9659c24
Analyzer working
NewellClark b4e54a4
Refactor analyzer visitors
NewellClark 97fd67c
Fix corner cases
NewellClark e5b4c7d
Implement fixer
NewellClark caf4ba2
Refactor and add comments
NewellClark 52ef641
Handle multiple fields in one declaration
NewellClark 3042a69
Merge branch 'release/7.0.1xx' into prefer-readonlyspan-properties-to…
NewellClark 93364df
Fix false positive with jagged arrays
NewellClark 46cca7b
Fix null reference crash
NewellClark d0ec5e1
Merge branch 'main' into prefer-readonlyspan-properties-to-array-fields
NewellClark 60039d1
Use Environment.NewLine in fixer
NewellClark 955a595
Set isReportedAtCompilationEnd to true
NewellClark d8b92bb
Remove unused well known type name SystemSByte
NewellClark 5ad4dd9
Merge branch 'main' into prefer-readonlyspan-properties-to-array-fields
NewellClark e1e27ce
Merge branch 'main' into prefer-readonlyspan-properties-to-array-fields
NewellClark ca627f8
Reimplemented private classes
NewellClark b7db02c
Add unit tests enforcing private-only arrays
NewellClark 118e852
.
NewellClark 29f58b8
Refactored Cache into CandidateCollection
NewellClark 4da31f1
Merge remote-tracking branch 'dotnet/main' into prefer-readonlyspan-p…
NewellClark 924788c
Forgot to run msbuild
NewellClark 2f0dbc3
Apply suggested naming changes
NewellClark 1af039e
Forgot to run msbuild
NewellClark dc430ee
Get required symbols on compilation start
NewellClark File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
172 changes: 172 additions & 0 deletions
172
...Core.Analyzers/Runtime/CSharpPreferReadOnlySpanPropertiesOverReadOnlyArrayFields.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,172 @@ | ||
// 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.Collections.Immutable; | ||
using System.Composition; | ||
using System.Linq; | ||
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.CSharp; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
using Microsoft.CodeAnalysis.Editing; | ||
using Microsoft.CodeAnalysis.Formatting; | ||
using Microsoft.CodeAnalysis.Operations; | ||
using Microsoft.NetCore.Analyzers; | ||
using Microsoft.NetCore.Analyzers.Runtime; | ||
|
||
namespace Microsoft.NetCore.CSharp.Analyzers.Runtime | ||
{ | ||
[ExportCodeFixProvider(LanguageNames.CSharp), Shared] | ||
public sealed class CSharpPreferReadOnlySpanPropertiesOverReadOnlyArrayFieldsFixer : PreferReadOnlySpanPropertiesOverReadOnlyArrayFieldsFixer | ||
{ | ||
public override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
{ | ||
var doc = context.Document; | ||
var root = await doc.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
|
||
if (root.FindNode(context.Diagnostics.First().Location.SourceSpan) is not VariableDeclaratorSyntax variableDeclaratorSyntax || | ||
variableDeclaratorSyntax?.Parent?.Parent is not FieldDeclarationSyntax fieldDeclarationSyntax || | ||
fieldDeclarationSyntax.Declaration.Type is not ArrayTypeSyntax arrayTypeSyntax) | ||
{ | ||
return; | ||
} | ||
|
||
var model = await doc.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); | ||
if (!model.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemReadOnlySpan1, out var readOnlySpanType)) | ||
return; | ||
|
||
var codeAction = CodeAction.Create( | ||
MicrosoftNetCoreAnalyzersResources.PreferReadOnlySpanPropertiesOverReadOnlyArrayFieldsCodeFixTitle, | ||
GetChangedDocumentAsync, | ||
nameof(MicrosoftNetCoreAnalyzersResources.PreferReadOnlySpanPropertiesOverReadOnlyArrayFieldsCodeFixTitle)); | ||
context.RegisterCodeFix(codeAction, context.Diagnostics); | ||
|
||
return; | ||
|
||
// Local functions | ||
|
||
async Task<Document> GetChangedDocumentAsync(CancellationToken token) | ||
{ | ||
// Replace readonly array field declaration with ReadOnlySpan<> property declaration | ||
var editor = await DocumentEditor.CreateAsync(doc, token).ConfigureAwait(false); | ||
FixDeclaration(editor); | ||
await FixAsSpanInvocationsAsync(editor, token).ConfigureAwait(false); | ||
return await Formatter.FormatAsync(editor.GetChangedDocument(), Formatter.Annotation, cancellationToken: token).ConfigureAwait(false); | ||
} | ||
|
||
async Task<ImmutableArray<IOperation>> GetFieldReferenceOperationsRequiringUpdateAsync(CancellationToken token) | ||
{ | ||
var savedSpans = PreferReadOnlySpanPropertiesOverReadOnlyArrayFields.SavedSpanLocation.Deserialize( | ||
context.Diagnostics[0].Properties[PreferReadOnlySpanPropertiesOverReadOnlyArrayFields.FixerDataPropertyName]); | ||
var documentLookup = context.Document.Project.Documents.ToImmutableDictionary(x => x.FilePath); | ||
var builder = ImmutableArray.CreateBuilder<IOperation>(savedSpans.Length); | ||
builder.Count = savedSpans.Length; | ||
|
||
for (int i = 0; i < savedSpans.Length; ++i) | ||
{ | ||
var doc = documentLookup[savedSpans[i].SourceFilePath]; | ||
var root = await doc.GetSyntaxRootAsync(token).ConfigureAwait(false); | ||
var model = await doc.GetSemanticModelAsync(token).ConfigureAwait(false); | ||
var node = root.FindNode(savedSpans[i].Span, getInnermostNodeForTie: true); | ||
builder[i] = model.GetOperation(node, token); | ||
} | ||
|
||
return builder.MoveToImmutable(); | ||
} | ||
|
||
void FixDeclaration(DocumentEditor editor) | ||
{ | ||
var rosNameSyntax = SyntaxFactory.GenericName( | ||
SyntaxFactory.Identifier(readOnlySpanType.Name), | ||
SyntaxFactory.TypeArgumentList(SyntaxFactory.SingletonSeparatedList(arrayTypeSyntax.ElementType))); | ||
var arrowExpressionClauseSyntax = variableDeclaratorSyntax.Initializer is not null ? | ||
SyntaxFactory.ArrowExpressionClause( | ||
SyntaxFactory.Token(SyntaxKind.EqualsGreaterThanToken).WithTriviaFrom(variableDeclaratorSyntax.Initializer.EqualsToken), | ||
variableDeclaratorSyntax.Initializer.Value) : | ||
SyntaxFactory.ArrowExpressionClause( | ||
SyntaxFactory.Token(SyntaxKind.EqualsGreaterThanToken), | ||
SyntaxFactory.MemberAccessExpression( | ||
SyntaxKind.SimpleMemberAccessExpression, | ||
rosNameSyntax, | ||
SyntaxFactory.IdentifierName(nameof(ReadOnlySpan<byte>.Empty)))); | ||
|
||
var declaration = fieldDeclarationSyntax.Declaration; | ||
var modifiersWithoutReadOnlyKeyword = SyntaxFactory.TokenList(fieldDeclarationSyntax.Modifiers.Where(x => !x.IsKind(SyntaxKind.ReadOnlyKeyword))); | ||
|
||
// If multiple fields were declared in a single field declaration, then we need to | ||
// insert the newly-created property declaration after the field declaration, and then remove | ||
// the reported field variable declarator from the field declaration. In other words: | ||
// private static readonly byte[] {|a|} = new[] { ... }, b = new[] { ... }; | ||
// becomes | ||
// private static readonly byte[] b = new[] { ... }; | ||
// private static ReadOnlySpan<byte> a => new[] { ... }; | ||
// where {|...|} indicates the location of the diagnostic. | ||
if (declaration.Variables.Count > 1) | ||
{ | ||
var propertyDeclarationSyntax = SyntaxFactory.PropertyDeclaration(rosNameSyntax, variableDeclaratorSyntax.Identifier) | ||
.WithExpressionBody(arrowExpressionClauseSyntax) | ||
.WithModifiers(modifiersWithoutReadOnlyKeyword) | ||
.WithSemicolonToken(SyntaxFactory.Token(SyntaxKind.SemicolonToken).WithTrailingTrivia(SyntaxFactory.EndOfLine(Environment.NewLine))) | ||
.WithoutLeadingTrivia() | ||
.WithAdditionalAnnotations(Formatter.Annotation); | ||
editor.InsertAfter(fieldDeclarationSyntax, propertyDeclarationSyntax); | ||
|
||
var newDeclaration = declaration.WithVariables(declaration.Variables.Remove(variableDeclaratorSyntax)); | ||
editor.ReplaceNode(declaration, newDeclaration); | ||
} | ||
else | ||
{ | ||
var propertyDeclarationSyntax = SyntaxFactory.PropertyDeclaration(rosNameSyntax, variableDeclaratorSyntax.Identifier) | ||
.WithExpressionBody(arrowExpressionClauseSyntax) | ||
.WithModifiers(modifiersWithoutReadOnlyKeyword) | ||
.WithSemicolonToken(fieldDeclarationSyntax.SemicolonToken) | ||
.WithTriviaFrom(fieldDeclarationSyntax); | ||
editor.ReplaceNode(fieldDeclarationSyntax, propertyDeclarationSyntax); | ||
} | ||
} | ||
|
||
// Update calls to 'AsSpan' | ||
async Task FixAsSpanInvocationsAsync(DocumentEditor editor, CancellationToken token) | ||
{ | ||
|
||
var savedOperations = await GetFieldReferenceOperationsRequiringUpdateAsync(token).ConfigureAwait(false); | ||
foreach (var fieldReference in savedOperations) | ||
{ | ||
// Walk up to the AsSpan invocation operation. | ||
var invocation = (IInvocationOperation)fieldReference.Parent.Parent; | ||
|
||
// If we called 'AsSpan(int)' or 'AsSpan(int, int)', replace with call to appropriate Slice overload. | ||
// Otherwise simply replace the 'AsSpan()' call with the field reference itself. | ||
if (invocation.TargetMethod.Parameters.Length > 1) | ||
{ | ||
var invocationSyntax = (InvocationExpressionSyntax)invocation.Syntax; | ||
var memberAccessSyntax = (MemberAccessExpressionSyntax)invocationSyntax.Expression; | ||
|
||
// If 'AsSpan' was not called via extension method, then memberAccessSyntax.Expression will be the type name | ||
// expression 'MemoryExtensions'. We need to replace it with the first argument in the argument list (which will be the | ||
// reference to the array field) and then remove the first argument from the argument list. | ||
if (invocationSyntax.ArgumentList.Arguments.Count == invocation.Arguments.Length) | ||
{ | ||
var newArgumentList = invocationSyntax.ArgumentList.WithArguments(invocationSyntax.ArgumentList.Arguments.RemoveAt(0)); | ||
editor.ReplaceNode(invocationSyntax.ArgumentList, newArgumentList); | ||
var newExpressionSyntax = fieldReference.Syntax.WithTriviaFrom(memberAccessSyntax.Expression); | ||
editor.ReplaceNode(memberAccessSyntax.Expression, newExpressionSyntax); | ||
} | ||
|
||
var sliceMemberNameSyntax = SyntaxFactory.IdentifierName(nameof(ReadOnlySpan<byte>.Slice)).WithTriviaFrom(memberAccessSyntax.Name); | ||
editor.ReplaceNode(memberAccessSyntax.Name, sliceMemberNameSyntax); | ||
} | ||
else | ||
{ | ||
editor.ReplaceNode(invocation.Syntax, fieldReference.Syntax.WithTriviaFrom(invocation.Syntax)); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
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
14 changes: 14 additions & 0 deletions
14
...ft.NetCore.Analyzers/Runtime/PreferReadOnlySpanPropertiesOverReadOnlyArrayFields.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,14 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System.Collections.Immutable; | ||
using Microsoft.CodeAnalysis.CodeFixes; | ||
|
||
namespace Microsoft.NetCore.Analyzers.Runtime | ||
{ | ||
public abstract class PreferReadOnlySpanPropertiesOverReadOnlyArrayFieldsFixer : CodeFixProvider | ||
{ | ||
public sealed override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(PreferReadOnlySpanPropertiesOverReadOnlyArrayFields.RuleId); | ||
|
||
public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the arguments doesn't match the parameters order?