Add analyzer for flagging single-use of local JsonSerializerOptions #6850
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6850 +/- ##
========================================
Coverage 96.39% 96.39%
========================================
Files 1401 1403 +2
Lines 330341 330954 +613
Branches 10845 10889 +44
========================================
+ Hits 318437 319029 +592
- Misses 9181 9191 +10
- Partials 2723 2734 +11 |
...s/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs
Outdated
Show resolved
Hide resolved
...s/Core/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptions.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.cs.xlf
Outdated
Show resolved
Hide resolved
...s/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs
Outdated
Show resolved
Hide resolved
...s/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Outdated
Show resolved
Hide resolved
...s/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs
Outdated
Show resolved
Hide resolved
...s/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs
Outdated
Show resolved
Hide resolved
...s/Microsoft.NetCore.Analyzers/Performance/AvoidSingleUseOfLocalJsonSerializerOptionsTests.cs
Outdated
Show resolved
Hide resolved
| JsonSerializerOptions opt = new JsonSerializerOptions(); | ||
| Action lambda = () => | ||
| { | ||
| JsonSerializer.Serialize(value, opt); |
There was a problem hiding this comment.
NIT: Wonder why it should not warn here ...
There was a problem hiding this comment.
Because opt is initialized outside of lambda and once used in lambda it can escape current scope.
This is mentioned in the PR description:
If any of the locals potentially escapes current block scope, we will bail.
There was a problem hiding this comment.
Well, this just looks like false negative to me, I guess I am not understanding the point of this analyzer.
There was a problem hiding this comment.
We don't want the analyzer to catch all possible single uses, just the 80% of cases that are very common.
| private static bool IsTupleForDeconstructionTargetingFieldOrProperty(IOperation operation) | ||
| { | ||
| IOperation? current = operation.Parent; | ||
|
|
||
| if (current is not ITupleOperation tuple) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| Stack<int> depth = new Stack<int>(); | ||
| depth.Push(tuple.Elements.IndexOf(operation)); | ||
|
|
||
| // walk-up right-hand nested tuples. | ||
| while (tuple.Parent is ITupleOperation parent) | ||
| { | ||
| depth.Push(parent.Elements.IndexOf(tuple)); | ||
| tuple = parent; | ||
| } | ||
|
|
||
| current = tuple.WalkUpConversion().Parent; | ||
| if (current is not IDeconstructionAssignmentOperation deconstruction) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // walk-down left-hand nested tuples and see if it targets a field or property. | ||
| if (deconstruction.Target is not ITupleOperation deconstructionTarget) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| tuple = deconstructionTarget; | ||
|
|
||
| IOperation? target = null; | ||
| while (depth.Count > 0) | ||
| { | ||
| int idx = depth.Pop(); | ||
| target = tuple.Elements[idx]; | ||
|
|
||
| if (target is ITupleOperation targetAsTuple) | ||
| { | ||
| tuple = targetAsTuple; | ||
| } | ||
| else if (depth.Count > 0) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return target is IFieldReferenceOperation or IPropertyReferenceOperation; |
There was a problem hiding this comment.
NIT: Lots if effort for checking if the tuple targeting field or property, did not see a test case that flags a usage with tuple, is there any case that should be flagged? isn't it better to just bail out if the option is used within an tuple?
There was a problem hiding this comment.
Yes, it is probably too much effort for a rare case. But it doesn't hurt to have it. I can add the missing test you are referring to.
| { | ||
| static void Main(string[] args) | ||
| { | ||
| string json = JsonSerializer.Serialize(args, {|CA1869:new JsonSerializerOptions { AllowTrailingCommas = true }|}); |
There was a problem hiding this comment.
NIT: there is only one diagnostic exit for this analyzer, so could use only [| |] brackets without the diagnostic ID, here and everywhere else
| string json = JsonSerializer.Serialize(args, {|CA1869:new JsonSerializerOptions { AllowTrailingCommas = true }|}); | |
| string json = JsonSerializer.Serialize(args, [|new JsonSerializerOptions { AllowTrailingCommas = true }|]); |
There was a problem hiding this comment.
Couldn't be mixed-up in the future if another analyzer hits the same operation?
There was a problem hiding this comment.
Nope, separate analyzers do not interfere each other in the tests.
Closes dotnet/runtime#65396
This analyzer doesn't use data flow analysis.
It tracks the local references on the left-hand side of the
new JsonSerializerOptions().If any of the locals potentially escapes current block scope, we will bail.
Escaping current block scope encompasses: