diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs index 94f84bfc06..1744f92266 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs @@ -9,6 +9,7 @@ using System.Diagnostics.CodeAnalysis; using System; using System.Collections.Generic; +using System.Diagnostics; namespace Microsoft.NetCore.Analyzers.Performance { @@ -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)); } @@ -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? localSymbols)) + if (!IsLocalAssignment(operation, jsonSerializerOptionsSymbol, out List? localSymbols)) { return false; } @@ -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)) { @@ -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) @@ -254,13 +281,19 @@ private static bool IsClosureOnLambdaOrLocalFunction(IOperation operation, IBloc return block != localBlock; } - private static bool IsLocalAssignment(IOperation operation, [NotNullWhen(true)] out List? localSymbols) + private static bool IsLocalAssignment(IOperation operation, INamedTypeSymbol jsonSerializerOptionsSymbol, [NotNullWhen(true)] out List? 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(); @@ -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(); localSymbols.Add(localRef.Local); @@ -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 { local }; } - return local != null; + return localSymbols != null; } currentOperation = currentOperation.Parent; diff --git a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs index 63ec104493..fad3c65d7c 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs @@ -45,7 +45,7 @@ internal class Program static void Main(string[] args) { JsonSerializerOptions options = {|CA1869:new JsonSerializerOptions()|}; - options.AllowTrailingCommas = true; + options.AllowTrailingCommas = true; string json = JsonSerializer.Serialize(args, options); Console.WriteLine(json); @@ -237,6 +237,101 @@ string LocalFunc() } """); + [Theory] + [InlineData(""" + for (int i = 0; i < values.Length; i++) + { + JsonSerializerOptions opt = {|CA1869:new JsonSerializerOptions()|}; + concatJson += JsonSerializer.Serialize(values[i], opt); + } + """)] + [InlineData(""" + foreach (T value in values) + { + JsonSerializerOptions opt = {|CA1869:new JsonSerializerOptions()|}; + concatJson += JsonSerializer.Serialize(value, opt); + } + """)] + [InlineData(""" + if (values.Length == 0) + return concatJson; + + int i = 0; + do + { + JsonSerializerOptions opt = {|CA1869:new JsonSerializerOptions()|}; + concatJson += JsonSerializer.Serialize(values[i++], opt); + } + while (i < values.Length); + """)] + [InlineData(""" + int i = 0; + while (i < values.Length) + { + JsonSerializerOptions opt = {|CA1869:new JsonSerializerOptions()|}; + concatJson += JsonSerializer.Serialize(values[i++], opt); + } + """)] + public Task CS_UseNewLocalOptionsAsArgument_JsonSerializerOptions_OnLoop(string loop) + => VerifyCS.VerifyAnalyzerAsync($$""" + using System.Text.Json; + + class Program + { + static string Serialize(T[] values) + { + string concatJson = ""; + + {{loop}} + + return concatJson; + } + } + """); + + [Theory] + [InlineData(""" + For i = 0 To values.Length + Dim opt = {|CA1869:New JsonSerializerOptions()|} + concatJson += JsonSerializer.Serialize(values(i), opt) + Next + """)] + [InlineData(""" + For Each value In values + Dim opt = {|CA1869:New JsonSerializerOptions()|} + concatJson += JsonSerializer.Serialize(value, opt) + Next + """)] + [InlineData(""" + Dim i = 0 + Do While i < values.Length + Dim opt = {|CA1869:New JsonSerializerOptions()|} + concatJson += JsonSerializer.Serialize(values(i), opt) + i = i + 1 + Loop + """)] + [InlineData(""" + Dim i = 0 + Do + Dim opt = {|CA1869:New JsonSerializerOptions()|} + concatJson += JsonSerializer.Serialize(values(i), opt) + i = i + 1 + Loop While i < values.Length + """)] + public Task VB_UseNewLocalOptionsAsArgument_JsonSerializerOptions_OnLoop(string loop) + => VerifyVB.VerifyAnalyzerAsync($$""" + Imports System.Text.Json + + Class Program + Shared Function Serialize(Of T)(values As T()) As String + Dim concatJson = "" + + {{loop}} + + return concatJson + End Function + End Class + """); #endregion #region No Diagnostic Tests @@ -275,6 +370,87 @@ static string MyCustomSerializeMethod(T value, JsonSerializerOptions options) } """); + [Fact] + public Task CS_UseNewOptionsAsArgument_InterlockedCompareExchange_NoWarn() + => VerifyCS.VerifyAnalyzerAsync(""" + using System.Text.Json; + using System.Threading; + + class Program + { + static JsonSerializerOptions s_options; + static string Serialize(T value) + { + return JsonSerializer.Serialize(value, + s_options ?? + Interlocked.CompareExchange(ref s_options, new JsonSerializerOptions() {WriteIndented = true}, null) ?? + s_options); + } + } + """); + + [Fact] + public Task CS_UseNewLocalOptionsAsArgument_MethodWithJsonOptionsArgument_NoWarn() + => VerifyCS.VerifyAnalyzerAsync(""" + using System.Text.Json; + + class Program + { + static string Serialize(T value) + { + JsonSerializerOptions options = TweakOptions(new JsonSerializerOptions()); + return JsonSerializer.Serialize(value, options); + } + + static JsonSerializerOptions TweakOptions(JsonSerializerOptions options) + { + options.WriteIndented = true; + return options; + } + } + """); + + [Fact] + public Task CS_UseNewLocalOptionsAsArgument_MethodWithJsonOptionsArgument_VarDeclaration_ReturnsNonOptions_NoWarn() + => VerifyCS.VerifyAnalyzerAsync(""" + using System.Text.Json; + + class Program + { + static string Serialize(T value) + { + T newValue = ProcessOptions(new JsonSerializerOptions()); + return JsonSerializer.Serialize(newValue); + } + + static T ProcessOptions(JsonSerializerOptions options) + { + return default; + } + } + """); + + [Fact] + public Task CS_UseNewLocalOptionsAsArgument_MethodWithJsonOptionsArgument_ExprStatement_ReturnsNonOptions_NoWarn() + => VerifyCS.VerifyAnalyzerAsync(""" + using System.Text.Json; + + class Program + { + static string Serialize(T value) + { + T newValue; + newValue = ProcessOptions(new JsonSerializerOptions()); + return JsonSerializer.Serialize(newValue); + } + + static T ProcessOptions(JsonSerializerOptions options) + { + return default; + } + } + """); + [Fact] public Task CS_UseNewLocalOptionsAsArgument_EscapeCurrentScope_NonSerializerMethod_NoWarn() => VerifyCS.VerifyAnalyzerAsync(""" @@ -304,6 +480,7 @@ public Task CS_UseNewLocalOptionsAsArgument_FieldAssignment_NoWarn(string snippe test.TestCode = $$""" using System; using System.Text.Json; + using System.Threading; class Program { @@ -358,27 +535,49 @@ private static List CS_UseNewLocalOptionsAsArgument_Assignment_TheoryDat { string target = useField ? "s_options" : "Options"; - return new List() + var l = new List() { - $@"JsonSerializerOptions opt = new JsonSerializerOptions(); - {target} = opt;", - - $@"JsonSerializerOptions opt; - {target} = opt = new JsonSerializerOptions();", - - $@"JsonSerializerOptions opt = {target} = new JsonSerializerOptions();", - - $@"JsonSerializerOptions opt = {target} ??= new JsonSerializerOptions();", - - $@"JsonSerializerOptions opt = new JsonSerializerOptions(); - {target} ??= opt;", + $""" + JsonSerializerOptions opt = new JsonSerializerOptions(); + {target} = opt; + """, + + $""" + JsonSerializerOptions opt; + {target} = opt = new JsonSerializerOptions(); + """, + + $"JsonSerializerOptions opt = {target} = new JsonSerializerOptions();", + + $"JsonSerializerOptions opt = {target} ??= new JsonSerializerOptions();", + + $""" + JsonSerializerOptions opt = new JsonSerializerOptions(); + {target} ??= opt; + """, + + $""" + JsonSerializerOptions opt = new JsonSerializerOptions(); + ({target}, _) = (opt, 42); + """, + + $""" + JsonSerializerOptions opt = new JsonSerializerOptions(); + (({target}, _), _) = ((opt, 42), 42); + """ + }; - $@"JsonSerializerOptions opt = new JsonSerializerOptions(); - ({target}, _) = (opt, 42);", + if (useField) + { + l.Add(""" + JsonSerializerOptions opt = + s_options ?? + Interlocked.CompareExchange(ref s_options, new JsonSerializerOptions() {WriteIndented = true}, null) ?? + s_options; + """); + } - $@"JsonSerializerOptions opt = new JsonSerializerOptions(); - (({target}, _), _) = ((opt, 42), 42);" - }; + return l; } [Fact] @@ -433,7 +632,7 @@ public Task CS_UseNewLocalOptionsAsArgument_MultiAssignment_EscapeCurrentScope_F class Program { - static JsonSerializerOptions s_options; + static JsonSerializerOptions s_options; static string Serialize(T value) { @@ -442,7 +641,7 @@ static string Serialize(T value) s_options = {{expression}}; - return JsonSerializer.Serialize(value, opt1); + return JsonSerializer.Serialize(value, opt1); } } """); @@ -457,14 +656,14 @@ public Task CS_UseNewLocalOptionsAsArgument_MultiAssignment_EscapeCurrentScope_F class Program { - static JsonSerializerOptions s_options; + static JsonSerializerOptions s_options; static string Serialize(T value) { JsonSerializerOptions opt1, opt2; {{expression}} = new JsonSerializerOptions(); - return JsonSerializer.Serialize(value, opt1); + return JsonSerializer.Serialize(value, opt1); } } """); @@ -531,6 +730,96 @@ string LocalFunc() } } """); + + [Theory] + [InlineData(""" + for (int i = 0; i < values.Length; i++) + { + concatJson += JsonSerializer.Serialize(values[i], opt); + } + """)] + [InlineData(""" + foreach (T value in values) + { + concatJson += JsonSerializer.Serialize(value, opt); + } + """)] + [InlineData(""" + if (values.Length == 0) + return concatJson; + + int i = 0; + do + { + concatJson += JsonSerializer.Serialize(values[i++], opt); + } + while (i < values.Length); + """)] + [InlineData(""" + int i = 0; + while (i < values.Length) + { + concatJson += JsonSerializer.Serialize(values[i++], opt); + } + """)] + public Task CS_UseNewLocalOptionsAsArgument_JsonSerializerOptions_BeforeLoop_NoWarn(string loop) + => VerifyCS.VerifyAnalyzerAsync($$""" + using System.Text.Json; + + class Program + { + static string Serialize(T[] values) + { + JsonSerializerOptions opt = new JsonSerializerOptions(); + string concatJson = ""; + + {{loop}} + + return concatJson; + } + } + """); + + [Theory] + [InlineData(""" + For i = 0 To values.Length + concatJson += JsonSerializer.Serialize(values(i), opt) + Next + """)] + [InlineData(""" + For Each value In values + concatJson += JsonSerializer.Serialize(value, opt) + Next + """)] + [InlineData(""" + Dim i = 0 + Do While i < values.Length + concatJson += JsonSerializer.Serialize(values(i), opt) + i = i + 1 + Loop + """)] + [InlineData(""" + Dim i = 0 + Do + concatJson += JsonSerializer.Serialize(values(i), opt) + i = i + 1 + Loop While i < values.Length + """)] + public Task VB_UseNewLocalOptionsAsArgument_JsonSerializerOptions_BeforeLoop_NoWarn(string loop) + => VerifyVB.VerifyAnalyzerAsync($$""" + Imports System.Text.Json + + Class Program + Shared Function Serialize(Of T)(values As T()) As String + Dim opt = New JsonSerializerOptions() + Dim concatJson = "" + + {{loop}} + + return concatJson + End Function + End Class + """); #endregion } }