-
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
(4/4) Prefer 'AsSpan' over 'Substring' analyzer #4806
Merged
mavasani
merged 25 commits into
dotnet:release/6.0.1xx
from
NewellClark:prefer-AsSpan-over-Substring-analyzer
May 21, 2021
Merged
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
29037e8
Add files and resources
NewellClark 8a8e73f
Add basic tests
NewellClark b02bfbf
Add more tests
NewellClark a82eb49
Merge remote-tracking branch 'dotnet/release/6.0.1xx-preview2' into p…
NewellClark 2a16ad0
Fixer fixes all unambiguous cases
NewellClark 0bd3228
Handle incomplete cases
NewellClark 94889ec
Fix superfluous System imports
NewellClark 8fcaaa4
Add tests
NewellClark ba5c872
Apply suggested changes
NewellClark d0eeafd
Apply suggestions from code review
NewellClark cd202c6
Merge remote-tracking branch 'dotnet/release/6.0.1xx-preview2' into p…
NewellClark bf3e977
Change ID to CA1846
NewellClark b79616d
Merge branch 'release/6.0.1xx-preview4' into prefer-AsSpan-over-Subst…
NewellClark 4cc4704
Rebuild resource files
NewellClark 380d1c4
Fix CI
NewellClark 1c1f94c
.
NewellClark 36acacd
Force xlf update
NewellClark a8e5a51
Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNet…
NewellClark 74bfc25
Apply suggested changes
NewellClark 053b2c4
Merge branch 'release/6.0.1xx' into prefer-AsSpan-over-Substring-anal…
NewellClark 9574d85
Run msbuild to fix CI
NewellClark 3f76529
Apply suggested changes
NewellClark 05f8773
Apply suggestions from code review
NewellClark 34274f9
Apply suggested changes
NewellClark 34dc2d6
Merge branch 'release/6.0.1xx' into prefer-AsSpan-over-Substring-anal…
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
35 changes: 35 additions & 0 deletions
35
...yzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpPreferAsSpanOverSubstring.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,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 ReplaceNonConditionalInvocationMethodName(SyntaxEditor editor, SyntaxNode memberInvocation, string newName) | ||
{ | ||
var cast = (InvocationExpressionSyntax)memberInvocation; | ||
var memberAccessSyntax = (MemberAccessExpressionSyntax)cast.Expression; | ||
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); | ||
} | ||
} | ||
} |
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
105 changes: 105 additions & 0 deletions
105
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/PreferAsSpanOverSubstring.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,105 @@ | ||
// 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 Analyzer.Utilities.Extensions; | ||
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 ReplaceNonConditionalInvocationMethodName(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 (!RequiredSymbols.TryGetSymbols(compilation, out RequiredSymbols symbols) || | ||
root.FindNode(context.Span, getInnermostNodeForTie: true) is not SyntaxNode reportedNode || | ||
model.GetOperation(reportedNode, token) is not IInvocationOperation reportedInvocation) | ||
{ | ||
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.PreferAsSpanOverSubstringCodefixTitle; | ||
var codeAction = CodeAction.Create(title, CreateChangedDocument, title); | ||
context.RegisterCodeFix(codeAction, context.Diagnostics); | ||
NewellClark marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return; | ||
|
||
async Task<Document> CreateChangedDocument(CancellationToken token) | ||
{ | ||
var editor = await DocumentEditor.CreateAsync(document, token).ConfigureAwait(false); | ||
|
||
foreach (var argument in reportedInvocation.Arguments) | ||
{ | ||
IOperation value = argument.Value.WalkDownConversion(c => c.IsImplicit); | ||
IParameterSymbol newParameter = spanBasedOverload.Parameters[argument.Parameter.Ordinal]; | ||
|
||
// Convert Substring invocations to equivalent AsSpan invocations. | ||
if (symbols.IsAnySubstringInvocation(value) && SymbolEqualityComparer.Default.Equals(newParameter.Type, symbols.ReadOnlySpanOfCharType)) | ||
{ | ||
ReplaceNonConditionalInvocationMethodName(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); | ||
} | ||
} | ||
} |
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.
Are we sure we can't be on something else than
MemberAccessExpresionSyntax
here? Maybe a member binding or something like this.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.
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
IArgumentOperation
s that areIInvocationOperation
s (after stripping implicit conversions), and the syntax offoo?.Substring(1)
is aConditionalAccessExpressionSyntax
, and not anInvocationExpressionSyntax
.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