From 7ba6d7d580308d3f81bec5a78b26b7fa53f0140c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Thu, 12 Oct 2023 13:42:29 -0500 Subject: [PATCH 1/4] Don't warn if object creation in local assignment is used as argument --- ...idSingleUseOfLocalJsonSerializerOptions.cs | 22 ++- ...gleUseOfLocalJsonSerializerOptionsTests.cs | 141 +++++++++++++++--- 2 files changed, 137 insertions(+), 26 deletions(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs index 94f84bfc06..48ec4ca459 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs @@ -60,7 +60,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 +83,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; } @@ -254,13 +253,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 +278,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 +306,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..7ff2f86852 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs @@ -275,6 +275,88 @@ 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 +386,7 @@ public Task CS_UseNewLocalOptionsAsArgument_FieldAssignment_NoWarn(string snippe test.TestCode = $$""" using System; using System.Text.Json; + using System.Threading; class Program { @@ -358,27 +441,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] From 7967da881d7ecdd0d69226d716ea422040ce72f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Thu, 12 Oct 2023 21:53:11 -0500 Subject: [PATCH 2/4] Do not warn if options are used in a loop --- ...idSingleUseOfLocalJsonSerializerOptions.cs | 27 +++ ...gleUseOfLocalJsonSerializerOptionsTests.cs | 185 ++++++++++++++++++ 2 files changed, 212 insertions(+) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs index 48ec4ca459..eaf18edb46 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 { @@ -102,6 +103,12 @@ private static bool IsLocalUsedAsArgumentForJsonSerializerOnly(IObjectCreationOp continue; } + // Symbol is declared in a parent scope, so this implies that options are re-used. + 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)) { @@ -155,6 +162,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) diff --git a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs index 7ff2f86852..6675fa75ca 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs @@ -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 @@ -636,6 +731,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 } } From a756ad1e2e5a44cfc13d53adb5087301c1405e26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Fri, 13 Oct 2023 10:22:21 -0500 Subject: [PATCH 3/4] Remove whitespace --- ...AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs index 6675fa75ca..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); @@ -370,7 +370,6 @@ static string MyCustomSerializeMethod(T value, JsonSerializerOptions options) } """); - [Fact] public Task CS_UseNewOptionsAsArgument_InterlockedCompareExchange_NoWarn() => VerifyCS.VerifyAnalyzerAsync(""" @@ -633,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) { @@ -642,7 +641,7 @@ static string Serialize(T value) s_options = {{expression}}; - return JsonSerializer.Serialize(value, opt1); + return JsonSerializer.Serialize(value, opt1); } } """); @@ -657,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); } } """); From dd6ead16b2b056c42a7eecb65367a619dcaacb83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Fri, 13 Oct 2023 10:24:45 -0500 Subject: [PATCH 4/4] Improve comment in IsLocalReferenceInsideChildLoop --- .../Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs index eaf18edb46..1744f92266 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs @@ -103,7 +103,8 @@ private static bool IsLocalUsedAsArgumentForJsonSerializerOnly(IObjectCreationOp continue; } - // Symbol is declared in a parent scope, so this implies that options are re-used. + // 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;