Skip to content

Fix false-flag of CompareExchange and re-usage with loops #6979

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 13, 2023
Merged

Fix false-flag of CompareExchange and re-usage with loops #6979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7ba6d7d
Don't warn if object creation in local assignment is used as argument
jozkee Oct 12, 2023
7967da8
Do not warn if options are used in a loop
jozkee Oct 13, 2023
a756ad1
Remove whitespace
jozkee Oct 13, 2023
dd6ead1
Improve comment in IsLocalReferenceInsideChildLoop
jozkee Oct 13, 2023
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
50 changes: 42 additions & 8 deletions ...ore/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Diagnostics.CodeAnalysis;
using System;
using System.Collections.Generic;
using System.Diagnostics;

namespace Microsoft.NetCore.Analyzers.Performance
{
Expand Down Expand Up @@ -60,7 +61,7 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context)
if (SymbolEqualityComparer.Default.Equals(typeSymbol, jsonSerializerOptionsSymbol))
{
if (IsCtorUsedAsArgumentForJsonSerializer(operation, jsonSerializerSymbol) ||
IsLocalUsedAsArgumentForJsonSerializerOnly(operation, jsonSerializerSymbol))
IsLocalUsedAsArgumentForJsonSerializerOnly(operation, jsonSerializerSymbol, jsonSerializerOptionsSymbol))
{
context.ReportDiagnostic(operation.CreateDiagnostic(s_Rule));
}
Expand All @@ -83,11 +84,10 @@ private static bool IsArgumentForJsonSerializer(IArgumentOperation argumentOpera
SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingType, jsonSerializerSymbol);
}

private static bool IsLocalUsedAsArgumentForJsonSerializerOnly(IObjectCreationOperation objCreation, INamedTypeSymbol jsonSerializerSymbol)
private static bool IsLocalUsedAsArgumentForJsonSerializerOnly(IObjectCreationOperation objCreation, INamedTypeSymbol jsonSerializerSymbol, INamedTypeSymbol jsonSerializerOptionsSymbol)
{
IOperation operation = WalkUpConditional(objCreation);

if (!IsLocalAssignment(operation, out List<ILocalSymbol>? localSymbols))
if (!IsLocalAssignment(operation, jsonSerializerOptionsSymbol, out List<ILocalSymbol>? localSymbols))
{
return false;
}
Expand All @@ -103,6 +103,13 @@ private static bool IsLocalUsedAsArgumentForJsonSerializerOnly(IObjectCreationOp
continue;
}

// Symbol is declared in a parent scope and referenced inside a loop,
// this implies that options are used more than once.
if (IsLocalReferenceInsideChildLoop(localRefOperation, localBlock!))
{
return false;
}

// Avoid cases that would potentially make the local escape current block scope.
if (IsArgumentOfJsonSerializer(descendant, jsonSerializerSymbol, out bool isArgumentOfInvocation))
{
Expand Down Expand Up @@ -156,6 +163,26 @@ private static bool IsLocalUsedAsArgumentForJsonSerializerOnly(IObjectCreationOp
return operation;
}

private static bool IsLocalReferenceInsideChildLoop(ILocalReferenceOperation localRef, IBlockOperation symbolBlock)
{
IOperation? current = localRef;
while ((current = current?.Parent) is not null)
{
if (current is ILoopOperation loop)
{
Debug.Assert(loop.Body is IBlockOperation);
return loop.Body != symbolBlock;
}

if (current == symbolBlock)
{
return false;
}
}

return false;
}

private static bool IsArgumentOfJsonSerializer(IOperation operation, INamedTypeSymbol jsonSerializerSymbol, out bool isArgumentOfInvocation)
{
if (operation.Parent is IArgumentOperation arg && arg.Parent is IInvocationOperation inv)
Expand Down Expand Up @@ -254,13 +281,19 @@ private static bool IsClosureOnLambdaOrLocalFunction(IOperation operation, IBloc
return block != localBlock;
}

private static bool IsLocalAssignment(IOperation operation, [NotNullWhen(true)] out List<ILocalSymbol>? localSymbols)
private static bool IsLocalAssignment(IOperation operation, INamedTypeSymbol jsonSerializerOptionsSymbol, [NotNullWhen(true)] out List<ILocalSymbol>? localSymbols)
{
localSymbols = null;
IOperation? currentOperation = operation.Parent;

while (currentOperation is not null)
{
// ignore cases where the object creation or one of its parents is used as argument.
if (currentOperation is IArgumentOperation)
{
return false;
}

// for cases like:
// var options;
// options = new JsonSerializerOptions();
Expand All @@ -273,7 +306,8 @@ private static bool IsLocalAssignment(IOperation operation, [NotNullWhen(true)]
{
return false;
}
else if (assignment.Target is ILocalReferenceOperation localRef)
else if (assignment.Target is ILocalReferenceOperation localRef &&
SymbolEqualityComparer.Default.Equals(localRef.Local.Type, jsonSerializerOptionsSymbol))
{
localSymbols ??= new List<ILocalSymbol>();
localSymbols.Add(localRef.Local);
Expand All @@ -300,12 +334,12 @@ private static bool IsLocalAssignment(IOperation operation, [NotNullWhen(true)]
}

var local = GetLocalSymbolFromDeclaration(declaration);
if (local != null)
if (local != null && SymbolEqualityComparer.Default.Equals(local.Type, jsonSerializerOptionsSymbol))
{
localSymbols = new List<ILocalSymbol> { local };
}

return local != null;
return localSymbols != null;
}

currentOperation = currentOperation.Parent;
Expand Down
Loading