Skip to content
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

(4/4) Prefer 'AsSpan' over 'Substring' analyzer #4806

Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
29037e8
Add files and resources
NewellClark Feb 6, 2021
8a8e73f
Add basic tests
NewellClark Feb 7, 2021
b02bfbf
Add more tests
NewellClark Feb 9, 2021
a82eb49
Merge remote-tracking branch 'dotnet/release/6.0.1xx-preview2' into p…
NewellClark Feb 9, 2021
2a16ad0
Fixer fixes all unambiguous cases
NewellClark Feb 11, 2021
0bd3228
Handle incomplete cases
NewellClark Feb 13, 2021
94889ec
Fix superfluous System imports
NewellClark Feb 14, 2021
8fcaaa4
Add tests
NewellClark Feb 19, 2021
ba5c872
Apply suggested changes
NewellClark Feb 19, 2021
d0eeafd
Apply suggestions from code review
NewellClark Feb 19, 2021
cd202c6
Merge remote-tracking branch 'dotnet/release/6.0.1xx-preview2' into p…
NewellClark Feb 22, 2021
bf3e977
Change ID to CA1846
NewellClark May 8, 2021
b79616d
Merge branch 'release/6.0.1xx-preview4' into prefer-AsSpan-over-Subst…
NewellClark May 8, 2021
4cc4704
Rebuild resource files
NewellClark May 8, 2021
380d1c4
Fix CI
NewellClark May 8, 2021
1c1f94c
.
NewellClark May 8, 2021
36acacd
Force xlf update
NewellClark May 9, 2021
a8e5a51
Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNet…
NewellClark May 18, 2021
74bfc25
Apply suggested changes
NewellClark May 18, 2021
053b2c4
Merge branch 'release/6.0.1xx' into prefer-AsSpan-over-Substring-anal…
NewellClark May 18, 2021
9574d85
Run msbuild to fix CI
NewellClark May 18, 2021
3f76529
Apply suggested changes
NewellClark May 19, 2021
05f8773
Apply suggestions from code review
NewellClark May 20, 2021
34274f9
Apply suggested changes
NewellClark May 20, 2021
34dc2d6
Merge branch 'release/6.0.1xx' into prefer-AsSpan-over-Substring-anal…
NewellClark May 20, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.NetCore.Analyzers.Runtime;

namespace Microsoft.NetCore.CSharp.Analyzers.Runtime
{
[ExportCodeFixProvider(LanguageNames.CSharp)]
public sealed class CSharpPreferAsSpanOverSubstringFixer : PreferAsSpanOverSubstringFixer
{
private protected override void ReplaceInvocationMethodName(SyntaxEditor editor, SyntaxNode memberInvocation, string newName)
{
var cast = (InvocationExpressionSyntax)memberInvocation;
var memberAccessSyntax = (MemberAccessExpressionSyntax)cast.Expression;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we can't be on something else than MemberAccessExpresionSyntax here? Maybe a member binding or something like this.

Copy link
Contributor Author

@NewellClark NewellClark Feb 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way this could be a MemberBindingExpressionSyntax is if the substring invocation is conditional, foo?.Substring(1).
This can't possibly be true because the analyzer detects IArgumentOperations that are IInvocationOperations (after stripping implicit conversions), and the syntax of foo?.Substring(1) is a ConditionalAccessExpressionSyntax, and not an InvocationExpressionSyntax.
I'll add some tests ensuring the analyzer does not report conditional substring invocations, and I'll add some assertions and comments to document this invariant.
I'll also rename the method to ReplaceNonConditionalInvocationMethodName

var newNameSyntax = SyntaxFactory.IdentifierName(newName);
editor.ReplaceNode(memberAccessSyntax.Name, newNameSyntax);
}

private protected override void ReplaceNamedArgumentName(SyntaxEditor editor, SyntaxNode invocation, string oldArgumentName, string newArgumentName)
{
var cast = (InvocationExpressionSyntax)invocation;
var oldNameSyntax = cast.ArgumentList.Arguments
.FirstOrDefault(x => x.NameColon is not null && x.NameColon.Name.Identifier.ValueText == oldArgumentName)?.NameColon.Name;
if (oldNameSyntax is null)
return;
var newNameSyntax = SyntaxFactory.IdentifierName(newArgumentName);
editor.ReplaceNode(oldNameSyntax, newNameSyntax);
}
}
}
4 changes: 4 additions & 0 deletions src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
; Please do not edit this file manually, it should only be updated through code fix application.
### New Rules
Rule ID | Category | Severity | Notes
--------|----------|----------|-------
CA1842 | Performance | Info | PreferAsSpanOverSubstring, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1842)

### Removed Rules

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1510,4 +1510,13 @@
<value>and all other platforms</value>
<comment>This call site is reachable on: 'windows' 10.0.2000 and later, and all other platforms</comment>
</data>
<data name="PreferAsSpanOverSubstringDescription" xml:space="preserve">
<value>'AsSpan' accomplishes the same thing as 'Substring' without incurring the allocation and O(n) string copy.</value>
</data>
<data name="PreferAsSpanOverSubstringMessage" xml:space="preserve">
<value>Prefer 'AsSpan' over 'Substring' when span-based overloads are available.</value>
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="PreferAsSpanOverSubstringTitle" xml:space="preserve">
<value>Prefer 'AsSpan' over 'Substring'</value>
</data>
</root>
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Immutable;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Operations;
using RequiredSymbols = Microsoft.NetCore.Analyzers.Runtime.PreferAsSpanOverSubstring.RequiredSymbols;

namespace Microsoft.NetCore.Analyzers.Runtime
{
public abstract class PreferAsSpanOverSubstringFixer : CodeFixProvider
{
private const string SubstringStartIndexArgumentName = "startIndex";
private const string AsSpanStartArgumentName = "start";

private protected abstract void ReplaceInvocationMethodName(SyntaxEditor editor, SyntaxNode memberInvocation, string newName);

private protected abstract void ReplaceNamedArgumentName(SyntaxEditor editor, SyntaxNode invocation, string oldArgumentName, string newArgumentName);

public sealed override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(PreferAsSpanOverSubstring.RuleId);

public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var document = context.Document;
var token = context.CancellationToken;
SyntaxNode root = await document.GetSyntaxRootAsync(token).ConfigureAwait(false);
SemanticModel model = await document.GetSemanticModelAsync(token).ConfigureAwait(false);
var compilation = model.Compilation;

if (root.FindNode(context.Span, getInnermostNodeForTie: true) is not SyntaxNode reportedNode || model.GetOperation(reportedNode, token) is not IInvocationOperation reportedInvocation)
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
return;
if (!RequiredSymbols.TryGetSymbols(compilation, out RequiredSymbols symbols))
return;

var bestCandidates = PreferAsSpanOverSubstring.GetBestSpanBasedOverloads(symbols, reportedInvocation, context.CancellationToken);
// We only apply the fix if there is an unambiguous best overload.
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
if (bestCandidates.Length != 1)
return;
IMethodSymbol spanBasedOverload = bestCandidates[0];

string title = MicrosoftNetCoreAnalyzersResources.PreferAsSpanOverSubstringTitle;
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
var codeAction = CodeAction.Create(title, CreateChangedDocument, title);
context.RegisterCodeFix(codeAction, context.Diagnostics);
NewellClark marked this conversation as resolved.
Show resolved Hide resolved

async Task<Document> CreateChangedDocument(CancellationToken token)
{
var editor = await DocumentEditor.CreateAsync(document, token).ConfigureAwait(false);

foreach (var argument in reportedInvocation.Arguments)
{
IOperation value = PreferAsSpanOverSubstring.WalkDownImplicitConversions(argument.Value);
IParameterSymbol newParameter = spanBasedOverload.Parameters[argument.Parameter.Ordinal];

// Convert Substring invocations to equivalent AsSpan invocations.
if (symbols.IsAnySubstringInvocation(value) && SymbolEqualityComparer.Default.Equals(newParameter.Type, symbols.RoscharType))
{
ReplaceInvocationMethodName(editor, value.Syntax, nameof(MemoryExtensions.AsSpan));
// Ensure named Substring arguments get renamed to their equivalent AsSpan counterparts.
ReplaceNamedArgumentName(editor, value.Syntax, SubstringStartIndexArgumentName, AsSpanStartArgumentName);
}

// Ensure named arguments on the original overload are renamed to their
// ordinal counterparts on the new overload.
string oldArgumentName = argument.Parameter.Name;
string newArgumentName = newParameter.Name;
ReplaceNamedArgumentName(editor, reportedInvocation.Syntax, oldArgumentName, newArgumentName);
}

// Import System namespace if necessary.
if (!IsMemoryExtensionsInScope(symbols, reportedInvocation))
{
SyntaxNode withoutSystemImport = editor.GetChangedRoot();
SyntaxNode systemNamespaceImportStatement = editor.Generator.NamespaceImportDeclaration(nameof(System));
SyntaxNode withSystemImport = editor.Generator.AddNamespaceImports(withoutSystemImport, systemNamespaceImportStatement);
editor.ReplaceNode(editor.OriginalRoot, withSystemImport);
}

return editor.GetChangedDocument();
}
}

public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

private static bool IsMemoryExtensionsInScope(in RequiredSymbols symbols, IInvocationOperation invocation)
{
var model = invocation.SemanticModel;
int position = invocation.Syntax.SpanStart;
const string name = nameof(MemoryExtensions);

return model.LookupNamespacesAndTypes(position, name: name)
.Contains(symbols.MemoryExtensionsType, SymbolEqualityComparer.Default);
}
}
}
Loading