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

JsonSerializerOptions analyzer #4

Merged
merged 11 commits into from
Sep 27, 2023
1 change: 1 addition & 0 deletions src/FSharp.Analyzers/FSharp.Analyzers.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
<Compile Include="ASTCollecting.fs"/>
<Compile Include="TASTCollecting.fs" />
<Compile Include="StringAnalyzers.fs"/>
<Compile Include="JsonSerializerOptionsAnalyzer.fs" />
</ItemGroup>

<ItemGroup>
Expand Down
78 changes: 78 additions & 0 deletions src/FSharp.Analyzers/JsonSerializerOptionsAnalyzer.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
namespace ``G-Research``.FSharp.Analyzers

open System
open FSharp.Analyzers.SDK
open FSharp.Compiler.Symbols
open FSharp.Compiler.Text
open FSharp.Compiler.Symbols.FSharpExprPatterns
open TASTCollection

module JsonSerializerOptionsAnalyzer =

[<Literal>]
let Code = "GRA-JSONOPTS-001"

[<CliAnalyzer "JsonSerializerOptionsAnalyzer">]
let jsonSerializerOptionsAnalyzer : Analyzer<CliContext> =
fun ctx ->
async {
let state = ResizeArray<range> ()

let namesToWarnAbount =
set
[
"System.Text.Json.JsonSerializer.Deserialize"
"System.Text.Json.JsonSerializer.DeserializeAsync"
"System.Text.Json.JsonSerializer.DeserializeAsyncEnumerable"
"System.Text.Json.JsonSerializer.Serialize"
"System.Text.Json.JsonSerializer.SerializeAsync"
"System.Text.Json.JsonSerializer.SerializeToDocument"
"System.Text.Json.JsonSerializer.SerializeToElement"
"System.Text.Json.JsonSerializer.SerializeToNode"
"System.Text.Json.JsonSerializer.SerializeToUtf8Bytes"
]

let handler : Handler =
let callHandler (range : range) (m : FSharpMemberOrFunctionOrValue) (args : FSharpExpr list) =
let name = String.Join (".", m.DeclaringEntity.Value.FullName, m.DisplayName)
let assemblyName = "System.Text.Json"

let containsSerOptsCtorCall =
args
|> List.exists (
function
| NewObject (objType, _, _) when
objType.FullName = "System.Text.Json.JsonSerializerOptions"
&& objType.Assembly.SimpleName = assemblyName
->
true
| _ -> false
)

if
m.Assembly.SimpleName = assemblyName
&& Set.contains name namesToWarnAbount
&& containsSerOptsCtorCall
then
state.Add range

Handler.CallHandler callHandler

match ctx.TypedTree with
| None -> ()
| Some typedTree -> typedTree.Declarations |> List.iter (visitDeclaration handler)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we have a different take on how we find the information.
In the String analyzers, I went with checking the untyped tree first in order to find invocations. I wonder what the most beneficial way is of doing things.
I genuinely do not know what is best in this case.
Let's discuss this tomorrow maybe.


return
state
|> Seq.map (fun r ->
{
Type = "JsonSerializerOptions analyzer"
Message = "JsonSerializerOptions instances should be cached."
Code = Code
Severity = Warning
Range = r
Fixes = []
}
)
|> Seq.toList
}
110 changes: 110 additions & 0 deletions src/FSharp.Analyzers/TASTCollection.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
namespace ``G-Research``.FSharp.Analyzers

module TASTCollection =

open FSharp.Compiler.Symbols
open FSharp.Compiler.Text
open FSharp.Compiler.Symbols.FSharpExprPatterns

type Handler = | CallHandler of (range -> FSharpMemberOrFunctionOrValue -> FSharpExpr list -> unit)

let rec visitExpr (handler : Handler) (e : FSharpExpr) =
match e with
| AddressOf lvalueExpr -> visitExpr handler lvalueExpr
| AddressSet (lvalueExpr, rvalueExpr) ->
visitExpr handler lvalueExpr
visitExpr handler rvalueExpr
| Application (funcExpr, _typeArgs, argExprs) ->
visitExpr handler funcExpr
visitExprs handler argExprs
| Call (objExprOpt, memberOrFunc, _typeArgs1, _typeArgs2, argExprs) ->
match handler with
| CallHandler f -> f e.Range memberOrFunc argExprs

visitObjArg handler objExprOpt
visitExprs handler argExprs
| Coerce (_targetType, inpExpr) -> visitExpr handler inpExpr
| FastIntegerForLoop (startExpr, limitExpr, consumeExpr, _isUp, _debugPointAtFor, _debugPointAtInOrTo) ->
visitExpr handler startExpr
visitExpr handler limitExpr
visitExpr handler consumeExpr
| ILAsm (_asmCode, _typeArgs, argExprs) -> visitExprs handler argExprs
| ILFieldGet (objExprOpt, _fieldType, _fieldName) -> visitObjArg handler objExprOpt
| ILFieldSet (objExprOpt, _fieldType, _fieldName, _valueExpr) -> visitObjArg handler objExprOpt
| IfThenElse (guardExpr, thenExpr, elseExpr) ->
visitExpr handler guardExpr
visitExpr handler thenExpr
visitExpr handler elseExpr
| Lambda (_lambdaVar, bodyExpr) -> visitExpr handler bodyExpr
| Let ((_bindingVar, bindingExpr, _debugPointAtBinding), bodyExpr) ->
visitExpr handler bindingExpr
visitExpr handler bodyExpr
| LetRec (recursiveBindings, bodyExpr) ->
let recursiveBindings' =
recursiveBindings |> List.map (fun (mfv, expr, _dp) -> (mfv, expr))

List.iter (snd >> visitExpr handler) recursiveBindings'
visitExpr handler bodyExpr
| NewArray (_arrayType, argExprs) -> visitExprs handler argExprs
| NewDelegate (_delegateType, delegateBodyExpr) -> visitExpr handler delegateBodyExpr
| NewObject (_objType, _typeArgs, argExprs) -> visitExprs handler argExprs
| NewRecord (_recordType, argExprs) -> visitExprs handler argExprs
| NewTuple (_tupleType, argExprs) -> visitExprs handler argExprs
| NewUnionCase (_unionType, _unionCase, argExprs) -> visitExprs handler argExprs
| Quote quotedExpr -> visitExpr handler quotedExpr
| FSharpFieldGet (objExprOpt, _recordOrClassType, _fieldInfo) -> visitObjArg handler objExprOpt
| FSharpFieldSet (objExprOpt, _recordOrClassType, _fieldInfo, argExpr) ->
visitObjArg handler objExprOpt
visitExpr handler argExpr
| Sequential (firstExpr, secondExpr) ->
visitExpr handler firstExpr
visitExpr handler secondExpr
| TryFinally (bodyExpr, finalizeExpr, _debugPointAtTry, _debugPointAtFinally) ->
visitExpr handler bodyExpr
visitExpr handler finalizeExpr
| TryWith (bodyExpr, _, _, _catchVar, catchExpr, _debugPointAtTry, _debugPointAtWith) ->
visitExpr handler bodyExpr
visitExpr handler catchExpr
| TupleGet (_tupleType, _tupleElemIndex, tupleExpr) -> visitExpr handler tupleExpr
| DecisionTree (decisionExpr, decisionTargets) ->
visitExpr handler decisionExpr
List.iter (snd >> visitExpr handler) decisionTargets
| DecisionTreeSuccess (_decisionTargetIdx, decisionTargetExprs) -> visitExprs handler decisionTargetExprs
| TypeLambda (_genericParam, bodyExpr) -> visitExpr handler bodyExpr
| TypeTest (_ty, inpExpr) -> visitExpr handler inpExpr
| UnionCaseSet (unionExpr, _unionType, _unionCase, _unionCaseField, valueExpr) ->
visitExpr handler unionExpr
visitExpr handler valueExpr
| UnionCaseGet (unionExpr, _unionType, _unionCase, _unionCaseField) -> visitExpr handler unionExpr
| UnionCaseTest (unionExpr, _unionType, _unionCase) -> visitExpr handler unionExpr
| UnionCaseTag (unionExpr, _unionType) -> visitExpr handler unionExpr
| ObjectExpr (_objType, baseCallExpr, overrides, interfaceImplementations) ->
visitExpr handler baseCallExpr
List.iter (visitObjMember handler) overrides
List.iter (snd >> List.iter (visitObjMember handler)) interfaceImplementations
| TraitCall (_sourceTypes, _traitName, _typeArgs, _typeInstantiation, _argTypes, argExprs) ->
visitExprs handler argExprs
| ValueSet (_valToSet, valueExpr) -> visitExpr handler valueExpr
| WhileLoop (guardExpr, bodyExpr, _debugPointAtWhile) ->
visitExpr handler guardExpr
visitExpr handler bodyExpr
| BaseValue _baseType -> ()
| DefaultValue _defaultType -> ()
| ThisValue _thisType -> ()
| Const (_constValueObj, _constType) -> ()
| Value _valueToGet -> ()
| _ -> ()

and visitExprs f exprs = List.iter (visitExpr f) exprs

and visitObjArg f objOpt = Option.iter (visitExpr f) objOpt

and visitObjMember f memb = visitExpr f memb.Body

let rec visitDeclaration f d =
match d with
| FSharpImplementationFileDeclaration.Entity (_e, subDecls) ->
for subDecl in subDecls do
visitDeclaration f subDecl
| FSharpImplementationFileDeclaration.MemberOrFunctionOrValue (_v, _vs, e) -> visitExpr f e
| FSharpImplementationFileDeclaration.InitAction e -> visitExpr f e
1 change: 1 addition & 0 deletions tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
<ItemGroup>
<Compile Include="Testing.fs" />
<Compile Include="StringAnalyzerTests.fs" />
<Compile Include="JsonSerializerOptionsAnalyzerTests.fs" />
</ItemGroup>

<ItemGroup>
Expand Down
58 changes: 58 additions & 0 deletions tests/FSharp.Analyzers.Tests/JsonSerializerOptionsAnalyzerTests.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
namespace ``G-Research``.FSharp.Analyzers.Tests

module JsonSerializerOptionsAnalyzerTests =

open System.Collections
open System.IO
open NUnit.Framework
open FSharp.Compiler.CodeAnalysis
open FSharp.Analyzers.SDK.Testing
open ``G-Research``.FSharp.Analyzers
open Testing

let mutable projectOptions : FSharpProjectOptions = FSharpProjectOptions.zero

[<SetUp>]
let Setup () =
task {
let! options =
mkOptionsFromProject
"net7.0"
nojaf marked this conversation as resolved.
Show resolved Hide resolved
[
{
Name = "System.Text.Json"
Version = "7.0.3"
}
]

projectOptions <- options
}

type TestCases() =
static member DataFolder = Path.Combine (__SOURCE_DIRECTORY__, "..", "data")

interface IEnumerable with
member _.GetEnumerator () : IEnumerator =
let jsonSerializerOptionsTests =
Path.Combine (TestCases.DataFolder, "jsonserializeroptions")

Directory.EnumerateFiles (jsonSerializerOptionsTests, "*.fs")
|> Seq.map (fun f ->
let fileName = Path.GetRelativePath (TestCases.DataFolder, f)
[| fileName :> obj |]
)
|> fun s -> s.GetEnumerator ()

[<TestCaseSource(typeof<TestCases>)>]
let JsonSerializerOptionsTests (fileName : string) =
task {
let dataFolder = TestCases.DataFolder
let fileName = Path.Combine (dataFolder, fileName)

let! messages =
File.ReadAllText fileName
|> getContext projectOptions
|> JsonSerializerOptionsAnalyzer.jsonSerializerOptionsAnalyzer

do! assertExpected fileName messages
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module M

open System.Text.Json

let f (json: string) (jsonStream: System.IO.Stream) =
let _ = JsonSerializer.Deserialize<string>(json, JsonSerializerOptions ())
let _ = JsonSerializer.DeserializeAsync<string>(jsonStream, JsonSerializerOptions ())
let _ = JsonSerializer.DeserializeAsyncEnumerable<string>(jsonStream, JsonSerializerOptions ())
()
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
GRA-JSONOPTS-001 | Warning | (6,12 - 6,78) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (7,12 - 7,89) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (8,12 - 8,99) | JsonSerializerOptions instances should be cached.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module M

open System.Text.Json

let f (json: string) (jsonStream: System.IO.Stream) =
let _ = JsonSerializer.Deserialize<string>(json, new JsonSerializerOptions ())
let _ = JsonSerializer.DeserializeAsync<string>(jsonStream, new JsonSerializerOptions ())
let _ = JsonSerializer.DeserializeAsyncEnumerable<string>(jsonStream, new JsonSerializerOptions ())
()
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
GRA-JSONOPTS-001 | Warning | (6,12 - 6,82) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (7,12 - 7,93) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (8,12 - 8,103) | JsonSerializerOptions instances should be cached.
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
module M

open System.Text.Json

let cachedOptions = new JsonSerializerOptions ()

let f (json: string) (jsonStream: System.IO.Stream) =
let _ = JsonSerializer.Serialize<string>(json, cachedOptions)
let _ = JsonSerializer.SerializeAsync<string>(jsonStream, json, cachedOptions)
let _ = JsonSerializer.SerializeToDocument<string>(json, cachedOptions)
let _ = JsonSerializer.SerializeToElement<string>(json, cachedOptions)
let _ = JsonSerializer.SerializeToNode<string>(json, cachedOptions)
let _ = JsonSerializer.SerializeToUtf8Bytes<string>(json, cachedOptions)
()
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

12 changes: 12 additions & 0 deletions tests/data/jsonserializeroptions/Serialize call with ctor.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
module M

open System.Text.Json

let f (json: string) (jsonStream: System.IO.Stream) =
let _ = JsonSerializer.Serialize<string>(json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeAsync<string>(jsonStream, json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeToDocument<string>(json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeToElement<string>(json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeToNode<string>(json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeToUtf8Bytes<string>(json, new JsonSerializerOptions ())
()
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
GRA-JSONOPTS-001 | Warning | (6,12 - 6,80) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (7,12 - 7,97) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (8,12 - 8,90) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (9,12 - 9,89) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (10,12 - 10,86) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (11,12 - 11,91) | JsonSerializerOptions instances should be cached.
12 changes: 12 additions & 0 deletions tests/data/jsonserializeroptions/Serialize calls with new ctor.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
module M

open System.Text.Json

let f (json: string) (jsonStream: System.IO.Stream) =
let _ = JsonSerializer.Serialize<string>(json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeAsync<string>(jsonStream, json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeToDocument<string>(json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeToElement<string>(json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeToNode<string>(json, new JsonSerializerOptions ())
let _ = JsonSerializer.SerializeToUtf8Bytes<string>(json, new JsonSerializerOptions ())
()
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
GRA-JSONOPTS-001 | Warning | (6,12 - 6,80) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (7,12 - 7,97) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (8,12 - 8,90) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (9,12 - 9,89) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (10,12 - 10,86) | JsonSerializerOptions instances should be cached.
GRA-JSONOPTS-001 | Warning | (11,12 - 11,91) | JsonSerializerOptions instances should be cached.