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

(3/4) Use Span-Based String Concat Analyzer and Fixer #4764

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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,44 @@
// 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.Collections.Generic;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.NetCore.Analyzers.Runtime;

namespace Microsoft.NetCore.CSharp.Analyzers.Runtime
{
[ExportCodeFixProvider(LanguageNames.CSharp)]
public sealed class CSharpUseSpanBasedStringConcatFixer : UseSpanBasedStringConcatFixer
{
private protected override SyntaxNode ReplaceInvocationMethodName(SyntaxGenerator generator, SyntaxNode invocationSyntax, string newName)
{
var memberAccessSyntax = (MemberAccessExpressionSyntax)((InvocationExpressionSyntax)invocationSyntax).Expression;
var oldNameSyntax = memberAccessSyntax.Name;
var newNameSyntax = generator.IdentifierName(newName).WithTriviaFrom(oldNameSyntax);
return invocationSyntax.ReplaceNode(oldNameSyntax, newNameSyntax);
}

private protected override bool IsSystemNamespaceImported(Project project, IReadOnlyList<SyntaxNode> namespaceImports)
{
foreach (var import in namespaceImports)
{
if (import is UsingDirectiveSyntax { Name: IdentifierNameSyntax { Identifier: { ValueText: nameof(System) } } })
return true;
}
return false;
}

private protected override IOperation WalkDownBuiltInImplicitConversionOnConcatOperand(IOperation operand)
{
return UseSpanBasedStringConcat.CSharpWalkDownBuiltInImplicitConversionOnConcatOperand(operand);
}

private protected override bool IsNamedArgument(IArgumentOperation argumentOperation)
{
return argumentOperation.Syntax is ArgumentSyntax argumentSyntax && argumentSyntax.NameColon is not null;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// 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.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.NetCore.Analyzers.Runtime;

namespace Microsoft.NetCore.CSharp.Analyzers.Runtime
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class CSharpUseSpanBasedStringConcat : UseSpanBasedStringConcat
{
private protected override bool TryGetTopMostConcatOperation(IBinaryOperation binaryOperation, [NotNullWhen(true)] out IBinaryOperation? rootBinaryOperation)
{
if (!IsConcatOperation(binaryOperation))
{
rootBinaryOperation = default;
return false;
}

var current = binaryOperation;
while (current.Parent is IBinaryOperation parentBinaryOperation && IsConcatOperation(parentBinaryOperation))
current = parentBinaryOperation;

rootBinaryOperation = current;
return true;

static bool IsConcatOperation(IBinaryOperation operation)
{
return operation.OperatorKind == BinaryOperatorKind.Add &&
operation.Type.SpecialType == SpecialType.System_String;
}
}

private protected override IOperation WalkDownBuiltInImplicitConversionOnConcatOperand(IOperation operand)
{
return CSharpWalkDownBuiltInImplicitConversionOnConcatOperand(operand);
}
}
}
1 change: 1 addition & 0 deletions src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ CA1840 | Performance | Info | UseEnvironmentMembers, [Documentation](https://doc
CA1841 | Performance | Info | PreferDictionaryContainsMethods, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1841)
CA1842 | Performance | Info | DoNotUseWhenAllOrWaitAllWithSingleArgument, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1842)
CA1843 | Performance | Info | DoNotUseWhenAllOrWaitAllWithSingleArgument, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1843)
CA1845 | Performance | Info | UseSpanBasedStringConcat, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1845)

### Removed Rules

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1534,6 +1534,18 @@
<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="UseSpanBasedStringConcatDescription" xml:space="preserve">
<value>It is more efficient to use 'AsSpan' and 'string.Concat', instead of 'Substring' and a concatenation operator.</value>
</data>
<data name="UseSpanBasedStringConcatMessage" xml:space="preserve">
<value>Use span-based 'string.Concat' and 'AsSpan' instead of 'Substring'</value>
</data>
<data name="UseSpanBasedStringConcatTitle" xml:space="preserve">
<value>Use span-based 'string.Concat'</value>
</data>
<data name="UseSpanBasedStringConcatCodeFixTitle" xml:space="preserve">
<value>Use 'AsSpan' with 'string.Concat'</value>
</data>
<data name="PreferDictionaryContainsKeyCodeFixTitle" xml:space="preserve">
<value>Use 'ContainsKey'</value>
</data>
Expand Down Expand Up @@ -1589,4 +1601,4 @@
<data name="DoNotUseWhenAllWithSingleTaskFix" xml:space="preserve">
<value>Replace 'WhenAll' call with argument</value>
</data>
</root>
</root>
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
// 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.Generic;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Analyzer.Utilities;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Operations;
using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources;
using RequiredSymbols = Microsoft.NetCore.Analyzers.Runtime.UseSpanBasedStringConcat.RequiredSymbols;

namespace Microsoft.NetCore.Analyzers.Runtime
{
public abstract class UseSpanBasedStringConcatFixer : CodeFixProvider
{
private protected const string AsSpanName = nameof(MemoryExtensions.AsSpan);
private protected const string AsSpanStartParameterName = "start";
private protected const string ToStringName = nameof(ToString);

private protected abstract SyntaxNode ReplaceInvocationMethodName(SyntaxGenerator generator, SyntaxNode invocationSyntax, string newName);

private protected abstract bool IsSystemNamespaceImported(Project project, IReadOnlyList<SyntaxNode> namespaceImports);

private protected abstract IOperation WalkDownBuiltInImplicitConversionOnConcatOperand(IOperation operand);

private protected abstract bool IsNamedArgument(IArgumentOperation argumentOperation);

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

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

if (!RequiredSymbols.TryGetSymbols(compilation, out var symbols))
return;
if (root.FindNode(context.Span, getInnermostNodeForTie: true) is not SyntaxNode concatExpressionSyntax)
return;

// OperatorKind will be BinaryOperatorKind.Concatenate, even when '+' is used instead of '&' in Visual Basic.
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
if (model.GetOperation(concatExpressionSyntax, cancellationToken) is not IBinaryOperation concatOperation
|| concatOperation.OperatorKind is not (BinaryOperatorKind.Add or BinaryOperatorKind.Concatenate))
{
return;
}

var operands = UseSpanBasedStringConcat.FlattenBinaryOperation(concatOperation);

// Bail out if we don't have a long enough span-based string.Concat overload.
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
if (!symbols.TryGetRoscharConcatMethodWithArity(operands.Length, out IMethodSymbol? roscharConcatMethod))
return;

// Bail if none of the operands are a non-conditional substring invocation. This could be the case if the
// only substring invocations in the expression were conditional invocations.
if (!operands.Any(IsAnyNonConditionalSubstringInvocation))
return;

var codeAction = CodeAction.Create(
Resx.UseSpanBasedStringConcatCodeFixTitle,
FixConcatOperationChain,
Resx.UseSpanBasedStringConcatCodeFixTitle);
context.RegisterCodeFix(codeAction, diagnostic);

return;

// Local functions

bool IsAnyNonConditionalSubstringInvocation(IOperation operation)
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
{
var value = WalkDownBuiltInImplicitConversionOnConcatOperand(operation);
return value is IInvocationOperation invocation && symbols.IsAnySubstringMethod(invocation.TargetMethod);
}

async Task<Document> FixConcatOperationChain(CancellationToken cancellationToken)
{
RoslynDebug.Assert(roscharConcatMethod is not null);

var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false);
var generator = editor.Generator;

SyntaxNode stringTypeNameSyntax = generator.TypeExpressionForStaticMemberAccess(symbols.StringType);
SyntaxNode concatMemberAccessSyntax = generator.MemberAccessExpression(stringTypeNameSyntax, roscharConcatMethod.Name);

// Save leading and trailing trivia so it can be attached to the outside of the string.Concat invocation node.
var leadingTrivia = operands.First().Syntax.GetLeadingTrivia();
var trailingTrivia = operands.Last().Syntax.GetTrailingTrivia();

var arguments = ImmutableArray.CreateBuilder<SyntaxNode>(operands.Length);
foreach (var operand in operands)
arguments.Add(ConvertOperandToArgument(symbols, generator, operand));

// Strip off leading and trailing trivia from first and last operand nodes, respectively, and
// reattach it to the outside of the newly-created string.Concat invocation node.
arguments[0] = arguments[0].WithoutLeadingTrivia();
arguments[^1] = arguments[^1].WithoutTrailingTrivia();
SyntaxNode concatMethodInvocationSyntax = generator.InvocationExpression(concatMemberAccessSyntax, arguments.MoveToImmutable())
.WithLeadingTrivia(leadingTrivia)
.WithTrailingTrivia(trailingTrivia);

SyntaxNode newRoot = generator.ReplaceNode(root, concatExpressionSyntax, concatMethodInvocationSyntax);

// Import 'System' namespace if it's absent.
if (!IsSystemNamespaceImported(context.Document.Project, generator.GetNamespaceImports(newRoot)))
{
SyntaxNode systemNamespaceImport = generator.NamespaceImportDeclaration(nameof(System));
newRoot = generator.AddNamespaceImports(newRoot, systemNamespaceImport);
}
Comment on lines +114 to +118
Copy link
Member

@Youssef1313 Youssef1313 Apr 24, 2021

Choose a reason for hiding this comment

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

Would you be able to check if calling WithAddImportsAnnotation on concatMethodInvocationSyntax eliminate the need of IsSystemNamespaceImported?

If WithAddImportsAnnotation didn't work, consider doing a case-insensitive string comparison in the VB override.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WithAddImportsAnnotation only works for syntax nodes that represent types (just tested it).
I did change the VB fixer to use a case-insensitive comparison (good catch).


editor.ReplaceNode(root, newRoot);
return editor.GetChangedDocument();
}
}

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

private SyntaxNode ConvertOperandToArgument(in RequiredSymbols symbols, SyntaxGenerator generator, IOperation operand)
{
var value = WalkDownBuiltInImplicitConversionOnConcatOperand(operand);

// Convert substring invocations to equivalent AsSpan invocation.
if (value is IInvocationOperation invocation && symbols.IsAnySubstringMethod(invocation.TargetMethod))
{
SyntaxNode invocationSyntax = invocation.Syntax;

// Swap out parameter names if named-arguments are used.
if (TryGetNamedStartIndexArgument(symbols, invocation, out var namedStartIndexArgument))
{
var renamedArgumentSyntax = generator.Argument(AsSpanStartParameterName, RefKind.None, namedStartIndexArgument.Value.Syntax);
invocationSyntax = generator.ReplaceNode(invocationSyntax, namedStartIndexArgument.Syntax, renamedArgumentSyntax);
}
var asSpanInvocationSyntax = ReplaceInvocationMethodName(generator, invocationSyntax, AsSpanName);
return generator.Argument(asSpanInvocationSyntax);
}
// Character literals become string literals.
else if (value.Type.SpecialType == SpecialType.System_Char && value is ILiteralOperation literalOperation && literalOperation.ConstantValue.HasValue)
{
var stringLiteral = generator.LiteralExpression(literalOperation.ConstantValue.Value.ToString()).WithTriviaFrom(literalOperation.Syntax);
return generator.Argument(stringLiteral);
}
else
{
return generator.Argument(value.Syntax);
}

bool TryGetNamedStartIndexArgument(in RequiredSymbols symbols, IInvocationOperation substringInvocation, [NotNullWhen(true)] out IArgumentOperation? namedStartIndexArgument)
{
RoslynDebug.Assert(symbols.IsAnySubstringMethod(substringInvocation.TargetMethod));

foreach (var argument in substringInvocation.Arguments)
{
if (IsNamedArgument(argument) && symbols.IsAnySubstringStartIndexParameter(argument.Parameter))
{
namedStartIndexArgument = argument;
return true;
}
}

namedStartIndexArgument = default;
return false;
}
}
}
}
Loading