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

[Performance] Simplify UseSpanBasedStringConcat for better performance #6870

Merged
merged 2 commits into from
Sep 12, 2023
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
@@ -1,6 +1,5 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. 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;
Expand All @@ -11,20 +10,10 @@ 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)
private protected override bool IsTopMostConcatOperation(IBinaryOperation binaryOperation)
{
if (!IsConcatOperation(binaryOperation))
{
rootBinaryOperation = default;
return false;
}

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

rootBinaryOperation = current;
return true;
return IsConcatOperation(binaryOperation) &&
(binaryOperation.Parent is not IBinaryOperation parentBinary || !IsConcatOperation(parentBinary));

static bool IsConcatOperation(IBinaryOperation operation)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.Linq;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Analyzer.Utilities.PooledObjects;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
Expand All @@ -32,11 +31,9 @@ public abstract class UseSpanBasedStringConcat : DiagnosticAnalyzer
isDataflowRule: false);

/// <summary>
/// If the specified binary operation is a string concatenation operation, we try to walk up to the top-most
/// string-concatenation operation that it is part of. If it is not a string-concatenation operation, we simply
/// return false.
/// Returns true if the specified binary operation is a top-most string concatenation operation
/// </summary>
private protected abstract bool TryGetTopMostConcatOperation(IBinaryOperation binaryOperation, [NotNullWhen(true)] out IBinaryOperation? rootBinaryOperation);
private protected abstract bool IsTopMostConcatOperation(IBinaryOperation binaryOperation);

/// <summary>
/// Remove the built in implicit conversion on operands to concat.
Expand All @@ -59,49 +56,19 @@ private void OnCompilationStart(CompilationStartAnalysisContext context)
if (!RequiredSymbols.TryGetSymbols(context.Compilation, out RequiredSymbols symbols))
return;

context.RegisterOperationBlockStartAction(OnOperationBlockStart);
return;

// Local functions
void OnOperationBlockStart(OperationBlockStartAnalysisContext context)
context.RegisterOperationAction(context =>
{
// Maintain set of all top-most concat operations so we don't report sub-expressions of an
// already-reported violation.
// Report diagnostic only if the operation is the top-most concat operation so we don't report sub-expressions of an
// already-reported violation.
// We also don't report any diagnostic if the concat operation has too many operands for the span-based
// Concat overloads to handle.
var topMostConcatOperations = TemporarySet<IBinaryOperation>.Empty;

context.RegisterOperationAction(PopulateTopMostConcatOperations, OperationKind.Binary);
context.RegisterOperationBlockEndAction(ReportDiagnosticsOnRootConcatOperationsWithSubstringCalls);

void PopulateTopMostConcatOperations(OperationAnalysisContext context)
{
// If the current operation is a string-concatenation operation, walk up to the top-most concat
// operation and add it to the set.
var binary = (IBinaryOperation)context.Operation;
if (!TryGetTopMostConcatOperation(binary, out var topMostConcatOperation))
return;

topMostConcatOperations.Add(topMostConcatOperation, context.CancellationToken);
}

void ReportDiagnosticsOnRootConcatOperationsWithSubstringCalls(OperationBlockAnalysisContext context)
{
// We report diagnostics for all top-most concat operations that contain
// direct or conditional substring invocations when there is an applicable span-based overload of
// the string.Concat method.
// We don't report when the concatenation contains anything other than strings or character literals.
foreach (var operation in topMostConcatOperations.NonConcurrentEnumerable)
{
if (ShouldBeReported(operation))
{
context.ReportDiagnostic(operation.CreateDiagnostic(Rule));
}
}
var binary = (IBinaryOperation)context.Operation;
if (IsTopMostConcatOperation(binary) && ShouldBeReported(binary))
context.ReportDiagnostic(binary.CreateDiagnostic(Rule));
}, OperationKind.Binary);
return;

topMostConcatOperations.Free(context.CancellationToken);
}
}
// Local functions

bool ShouldBeReported(IBinaryOperation topMostConcatOperation)
{
Expand Down Expand Up @@ -133,8 +100,8 @@ bool ShouldBeReported(IBinaryOperation topMostConcatOperation)

bool IsAnyDirectOrConditionalSubstringInvocation(IOperation operation)
{
if (operation is IConditionalAccessOperation conditionallAccessOperation)
operation = conditionallAccessOperation.WhenNotNull;
if (operation is IConditionalAccessOperation conditionalAccessOperation)
operation = conditionalAccessOperation.WhenNotNull;

return operation is IInvocationOperation invocation && symbols.IsAnySubstringMethod(invocation.TargetMethod);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,13 @@ Namespace Microsoft.NetCore.VisualBasic.Analyzers.Runtime
<DiagnosticAnalyzer(LanguageNames.VisualBasic)>
Public NotInheritable Class BasicUseSpanBasedStringConcat : Inherits UseSpanBasedStringConcat

Private Protected Overrides Function TryGetTopMostConcatOperation(binaryOperation As IBinaryOperation, ByRef rootBinaryOperation As IBinaryOperation) As Boolean

Private Protected Overrides Function IsTopMostConcatOperation(binaryOperation As IBinaryOperation) As Boolean
If Not IsStringConcatOperation(binaryOperation) Then
rootBinaryOperation = Nothing
Return False
End If

Dim parentBinaryOperation = binaryOperation
Dim current As IBinaryOperation
Do
current = parentBinaryOperation
parentBinaryOperation = TryCast(WalkUpImplicitConversionToObject(current.Parent), IBinaryOperation)
Loop While parentBinaryOperation IsNot Nothing AndAlso IsStringConcatOperation(parentBinaryOperation)

rootBinaryOperation = current
Return True
Dim parentBinaryOperation = TryCast(WalkUpImplicitConversionToObject(binaryOperation.Parent), IBinaryOperation)
Return parentBinaryOperation Is Nothing OrElse Not IsStringConcatOperation(parentBinaryOperation)
End Function

Private Protected Overrides Function WalkDownBuiltInImplicitConversionOnConcatOperand(operand As IOperation) As IOperation
Expand Down