Feedback updates/integration

Author: bartelink

src/FsCodec.SystemTextJson/RejectNullConverter.fs

@@ -1,20 +1,23 @@
 namespace FsCodec.SystemTextJson
 
 open System
-open System.Linq.Expressions
 open System.Text.Json
 open System.Text.Json.Serialization
 
 type RejectNullConverter<'T>() =
     inherit System.Text.Json.Serialization.JsonConverter<'T>()
 
-    static let defaultConverter = JsonSerializerOptions.Default.GetConverter(typeof<'T>) :?> JsonConverter<'T>
+    let defaultConverter = JsonSerializerOptions.Default.GetConverter(typeof<'T>) :?> JsonConverter<'T>
     let msg () = sprintf "Expected value, got null. When rejectNull is true you must explicitly wrap optional %s values in an 'option'" typeof<'T>.Name
 
     override _.HandleNull = true
 
     override _.Read(reader, typeToConvert, options) =
         if reader.TokenType = JsonTokenType.Null then msg () |> nullArg else
+        // PROBLEM: Fails with NRE when RejectNullConverter delegates to Default list<int> Converter
+        // System.NullReferenceException
+        //  at System.Text.Json.Serialization.JsonCollectionConverter`2.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, TCollection& value)
+        // https://github.com/dotnet/runtime/issues/50205 via https://github.com/jet/FsCodec/pull/112#issuecomment-1907633798
         defaultConverter.Read(&reader, typeToConvert, options)
         // Pretty sure the above is the correct approach (and this unsurprisingly loops, blowing the stack)
         // JsonSerializer.Deserialize(&reader, typeToConvert, options) :?> 'T
@@ -26,17 +29,8 @@ type RejectNullConverter<'T>() =
 
 type RejectNullConverterFactory(predicate) =
     inherit JsonConverterFactory()
-    new() =
-        RejectNullConverterFactory(fun (t: Type) ->
-            t.IsGenericType
-            && let gtd = t.GetGenericTypeDefinition() in gtd = typedefof<Set<_>> || gtd = typedefof<list<_>>)
-    override _.CanConvert(t: Type) = predicate t
-
-    override _.CreateConverter(t, _options) =
-        let openConverterType = typedefof<RejectNullConverter<_>>
-        let constructor = openConverterType.MakeGenericType(t).GetConstructors() |> Array.head
-        let newExpression = Expression.New(constructor)
-        let lambda = Expression.Lambda(typeof<ConverterActivator>, newExpression)
+    static let isListOrSet (t: Type) = t.IsGenericType && let g = t.GetGenericTypeDefinition() in g = typedefof<Set<_>> || g = typedefof<list<_>>
+    new() = RejectNullConverterFactory(isListOrSet)
 
-        let activator = lambda.Compile() :?> ConverterActivator
-        activator.Invoke()
+    override _.CanConvert(t: Type) = predicate t
+    override _.CreateConverter(t, _options) = typedefof<RejectNullConverter<_>>.MakeGenericType(t).GetConstructors().[0].Invoke[||] :?> _

tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs

@@ -90,25 +90,25 @@ let [<Fact>] ``RejectNullConverter rejects null lists and Sets`` () =
                 // if x.Kind <> JsonTypeInfoKind.Object then
                 for p in x.Properties do
                     let pt = p.PropertyType
-                    if pt.IsGenericType && (let gtd = pt.GetGenericTypeDefinition() in gtd = typedefof<list<_>> || gtd = typedefof<Set<_>>) then
+                    if pt.IsGenericType && (let d = pt.GetGenericTypeDefinition() in d = typedefof<list<_>> || d = typedefof<Set<_>>) then
                         p.IsRequired <- true)
     let serdes = Options.Create(TypeInfoResolver = tir) |> Serdes
     raises<exn> <@ serdes.Deserialize<WithList> """{"x":0}""" @>
 #else
     let serdes = Options.Create(rejectNull = true) |> Serdes
 
-    // Fails with NRE when RejectNullConverter delegates to Default list<int> Converter
-    // seems akin to https://github.com/dotnet/runtime/issues/86483
+    // PROBLEM: Fails with NRE when RejectNullConverter delegates to Default list<int> Converter
     // let res = serdes.Deserialize<WithList> """{"x":0,"y":[1]}"""
     // test <@ [1] = res.y @>
 
-    // PROBLEM: Doesn't raise
+    // PROBLEM: Doesn't raise as Converter not called
     raises<exn> <@ serdes.Deserialize<WithList> """{"x":0}""" @>
+
     // PROBLEM: doesnt raise - there doesn't seem to be a way to intercept explicitly passed nulls
     // raises<JsonException> <@ serdes.Deserialize<WithList> """{"x":0,"y":null}""" @>
 #endif
 
-#if false // I guess TypeShape is doing a reasaonable thing not propagating
+#if false // I guess TypeShape is doing a reasonable thing not propagating
     // PROBLEM: TypeShape.Generic.exists does not call the predicate if the list or set is `null`
     let res = serdes.Deserialize<WithList> """{"x":0}"""
     let hasNullList = TypeShape.Generic.exists (fun (x: int list) -> obj.ReferenceEquals(x, null))