Extra encoder doesn't get called for type with generic params

[Arrow7000]

I have the following type that I want to encode:

type CstNode<'a> =
    { node : 'a
      source : TokenWithSource list }

but I only want to encode the node field's value directly, because the source is not necessary for serialisation, and it would just add clutter to the generated JSON.

I then try to encode a sample record using the following code

let cstNodeEncoder : Encoder<CstNode<'a>> =
    fun (cstNode : CstNode<'a>) ->
        printfn "Calling CstNode encoder" // Log this encoder
        // Encode only the .node field using an auto encoder
        Encode.Auto.generateEncoder () cstNode.node

// We don't care about the decoder because we are only ever going to encode the type
let cstNodeDecoder : Decoder<CstNode<'a>> = fun _ -> failwithf "Not implemented"

let cstCoder =
    Extra.empty
    |> Extra.withCustom cstNodeEncoder cstNodeDecoder

let toJson result =
    Encode.Auto.toString (4, value = result, extra = cstCoder)

[<EntryPoint>]
let main argv =
    { node = 1234; source = List.empty }
    |> List.singleton
    |> toJson
    |> printfn "%A"

    0

Yet I still consistently get this in my console

"[
    {
        "node": 1234,
        "source": []
    }
]"

So it looks like either Encode.Auto.generateEncoder () cstNode.node isn't actually doing anything, or that my extra encoder is never even being called. I'm leaning towards the latter because the printfn statement in the body of the encoder is never appearing in my console, so it looks like it's never being called.

Is it possible that this is because my type includes a generic type parameter which doesn't play nicely with the logic for checking whether a custom encoder needs to be invoked?

UPDATE: it looks like my suspicion was correct, because as soon as I make my encoder accept only a concrete type the encoder does get called

let cstNodeEncoder : Encoder<CstNode<int>> = // note the 'a has been replaced with an int

The console now says

Calling CstNode encoder
"[
    1234
]"

However my CstNode contains a wide variety of types across my codebase, so having to specify explicit encoders for each one would defeat the whole point of using an auto encoder; plus there'd be no way to verify that I've accounted for all the types of generic that CstNode contains across my codebase.

[MangelMaxime]

I think the problem is coming from how we identify custom coders:

Thoth.Json/src/Extra.fs

Lines 11 to 16 in ca5930a

	let inline internal withCustomAndKey (encoder: Encoder<'Value>) (decoder: Decoder<'Value>)
	(extra: ExtraCoders): ExtraCoders =
	{ extra with
	Hash = System.Guid.NewGuid().ToString()
	Coders =
	extra.Coders |> Map.add typeof<'Value>.FullName (Encode.boxEncoder encoder, Decode.boxDecoder decoder) }

We are using typeof<'Value>.FullName which contains the generic information Microsoft.FSharp.Collections.FSharpList1[System.Int32]`.

Supporting your case will require to find a way to identify types with generic not filled and also change some mechanism in how we resolve the auto encoder and decoder. I don't know if detecting types with generic not provided is doable or not

[Arrow7000]

What about instead of always getting the full name of the provided type exactly as is, we get the generic version of the type, both when inserting into the map and when retrieving it? Something like this:

let inline internal withCustomAndKey (encoder: Encoder<'Value>) (decoder: Decoder<'Value>)
           (extra: ExtraCoders): ExtraCoders =
    let theType = typeof<'Value>

    // Get the type with any concrete generic parameters reset so that no matter what the specific type parameters are set, this returns the generic type
    let genericVersionOfType = theType.GetGenericTypeDefinition ()

    { extra with
          Hash = System.Guid.NewGuid().ToString()
          Coders =
              extra.Coders |> Map.add genericVersionOfType.FullName (Encode.boxEncoder encoder, Decode.boxDecoder decoder) }

and similarly at the point where we look for extra coders, instead of

Thoth.Json/src/Encode.fs

Lines 441 to 443 in ca5930a

	and private autoEncoder (extra: Map<string, ref<BoxedEncoder>>) caseStrategy (skipNullField : bool) (t: System.Type) : BoxedEncoder =
	let fullname = t.FullName
	match Map.tryFind fullname extra with

we could do something like:

and private autoEncoder (extra: Map<string, ref<BoxedEncoder>>) caseStrategy (skipNullField : bool) (t: System.Type) : BoxedEncoder =
      let genericVersionOfType = t.GetGenericTypeDefinition ()
      let fullname = genericVersionOfType.FullName
      match Map.tryFind fullname extra with
      | Some encoderRef -> fun v -> encoderRef.contents v

That way we can make a type name for the map key that doesn't include the type set in a generic parameter, both at the point of creating the coder and at the point of determining which coder should be used.

[Arrow7000]

One question that arises with the above is whether we want to include logic for supporting different coders for the same type if it has different type parameters set.

I.e. do we want to let the user define different encoders for CstNode<'a>, CstNode<int>, and CstNode<string>, with the latter two being chosen for ints and strings respectively, and the former being chosen if it's any other type.

Personally I don't think this is necessary because if the user really wants to encode/decode different things differently they should just create a discriminated union containing different types in each variant and add different coders for each of the different types in the variants.

So I think the initial idea of using the type-parameter-less name of the type is all that's needed to make this work.

[MangelMaxime]

GetGenericTypeDefinition seems to be support by Fable so I should be able to make the changes in Thoth.Json.

I will see if this breaks any tests or not.

One question that arises with the above is whether we want to include logic for supporting different coders for the same type if it has different type parameters set.

I think that if the user want different representation depending on the "context" then he should use DUs or manual coder to have full control over what he does.

In general, I think that a type as a single way of being represented. If this is not the case, user should use manual coders.

[Arrow7000]

Yes, agreed. Thank you. I can have a crack at it myself also if that would be helpful?

[Arrow7000]

@MangelMaxime I've had a go at making the changes myself. I haven't been able to run the tests because I can't get things to build on VS, but I've pushed up my work to this branch thoth-org/Thoth.Json.Net@main...Arrow7000:Thoth.Json.Net:use-generic-names-in-extras-map

Lmk if you want me to make more changes and/or submit a PR 🙂

[MangelMaxime]

Hello @Arrow7000 thank you for your work.

I didn't forget about your issue. I am trying to wrapping up another work that I already have started and will have a look at your issue and code after.

I see that you made the changes for Thoth.Json.Net, could you open a PR on that repo and also make the same changes for Thoth.Json? Both projects being still splited we need to do the job twice... 😅

[Arrow7000]

@MangelMaxime apologies for the delay. I've now submitted PRs for both repos. Let me know if you have any questions! Thanks again for looking at this