ISerializeSurrogate's implemented in fsharp for immutable types do not deserialize correctly

[greyepoxy]

Summary

ISerializeSurrogate's implemented in fsharp for immutable types do not deserialize correctly because csharp code assumes it has captured the reference to the real object which fsharp cannot edit directly

So for some context, I have been working on a game in fsharp and in the process of trying to figure out why native fsharp types do not always seem to work correctly to hold component data I attempted to write an ISerializeSurrogate for some of the fsharp collection types. Pretty much the only one that consistently works is the Array type which makes since because it is actually a System.Array while the rest are all part of FSharp.Core. As I investigated this I realized that serialization actually seems to be working fine but the issue was actually the cloning behavior and I was able to successfully fix my problems by adding a [<CloneBehavior(CloneBehavior.Reference)>] attribute to the field containing the fsharp collection (a Map in my case). Yay!

Because of all of this investigation I did notice that implementing a ISerializeSurrogate (or the helper class SerializeSurrogate<T>) for an immutable type never works. Looks like the core of the problem is that the serialization code assigns to the surrogate's RealObject parameter and assumes that the Surrogate will just edit the given object. For the fsharp core types though there are no mutable methods to use and I was unfortunately unable to figure out a way to modify the object using its reference.

How to reproduce

• Create a new duality project
• In the created solution add a fsharp duality plugin project
  1. In visual studio File -> New -> Project -> Visual f# -> Portable library
  2. Add references to Duality.dll and DualityPrimitives.dll
  3. Setup post build events (here are mine)

mkdir "$(SolutionDir)../../Plugins"
copy "$(TargetPath)" "$(SolutionDir)../../Plugins"
copy "$(TargetDir)$(TargetName).pdb" "$(SolutionDir)../../Plugins"
copy "$(TargetDir)$(TargetName).xml" "$(SolutionDir)../../Plugins"
xcopy /D "$(TargetDir)FSharp.Core.dll" "$(SolutionDir)../.."
xcopy /D "$(TargetDir)FSharp.Core.xml" "$(SolutionDir)../.."

4. Add plugin source file

namespace BugTest

open Duality

type EconomyOwnershipGameCorePlugin() = 
  inherit CorePlugin()

5. Build and compile should now work and Duality should load the plugin
   • In fsharp implement an ISerializeSurrogate. Here is the one I wrote for map, apologies for all the code was just less work to show what I have...

namespace BugTest

open Duality.Serialization
open System
open System.Reflection
open Duality

type private MapKeyOrValue =
  | MapKey
  | MapValue

type private mapConverter<'k,'v when 'k : comparison>() =
  static let invertedZip values keys =
    Seq.zip keys values

  static let CheckIfIsOfType (typeInfo: TypeInfo, thingToCheck: obj, isMapKeyOrValue: MapKeyOrValue) =
    let isOfType = typeInfo.IsInstanceOfType(thingToCheck)
    if not isOfType
    then
      let keyOrValueString = match isMapKeyOrValue with | MapKey -> "key" | MapValue -> "value"
      Log.Core.WriteWarning(
        "Actual Type '{0}' of Map {3} '{1}' does not match reflected Map {3} type '{2}'. Skipping value.",
        (if thingToCheck <> null then Log.Type(thingToCheck.GetType()) else "unknown"),
        thingToCheck,
        Log.Type(typeInfo),
        keyOrValueString
      )

    isOfType

  static let CheckIfKeyIsOfType (keyTypeInfo: TypeInfo, key: obj) =
    CheckIfIsOfType (keyTypeInfo, key, MapKey)

  static let CheckIfValueIsOfType (valueTypeInfo: TypeInfo, value: obj) =
    CheckIfIsOfType (valueTypeInfo, value, MapValue)

  static let areKeyAndValueOfCorrectTypeAndLogWarningIfNot keyTypeInfo valueTypeInfo =
    (fun (key, value) ->
      CheckIfKeyIsOfType (keyTypeInfo, key)
        && CheckIfValueIsOfType (valueTypeInfo, value)
    )

  static member FromArrays (keys: 'k array) (values: 'v array) =
    let keyType = keys.GetType().GetElementType().GetTypeInfo()
    let valueType = values.GetType().GetElementType().GetTypeInfo()
    keys
    |> Seq.ofArray
    |> Seq.filter (fun key -> key |> box |> isNull |> not)
    |> invertedZip (Seq.ofArray values)
    |> Seq.filter (areKeyAndValueOfCorrectTypeAndLogWarningIfNot keyType valueType)
    |> Map.ofSeq

  static member ToArrays (map: Map<'k, 'v>) =
    map
    |> Map.toArray
    |> Array.unzip

type private mapSerializer<'k,'v when 'k : comparison>() =
  static member ReadData (reader: IDataReader) =
    let mutable keys = Array.empty<'k>
    let mutable values = Array.empty<'v>
    reader.ReadValue ("keys", &keys)
    reader.ReadValue ("values", &values)
    let actualKeys =
      if isNull keys
      then Array.empty<'k>
      else keys
    let actualValues =
      if isNull values
      then Array.empty<'v>
      else values

    mapConverter.FromArrays actualKeys actualValues

  static member WriteData(writer: IDataWriter, mapToWrite: Map<'k, 'v>) =
    let (keys, values) = mapConverter.ToArrays(mapToWrite)

    writer.WriteValue("keys", keys);
    writer.WriteValue("values", values);

type public MapSurrogate() =
  let mutable realObject: obj = null

  member this.RealObject
    with get () = (this :> ISerializeSurrogate).RealObject
    and set (value: obj) = (this :> ISerializeSurrogate).RealObject <- value

  interface ISerializeSurrogate with
    member _this.ConstructObject(_reader: IDataReader, objType: TypeInfo): obj =
      objType.CreateInstanceOf();

    member _this.MatchesType(t: TypeInfo): bool =
      t.IsGenericType && t.GetGenericTypeDefinition() = typedefof<Map<_,_>>

    member _this.Priority: int = 0

    member _this.RealObject
      with get (): obj = realObject
      and set (v: obj): unit = realObject <- v

    member this.SurrogateObject: ISerializeExplicit = (this :> ISerializeExplicit)

    member _this.WriteConstructorData(writer: IDataWriter): unit = ()

  interface ISerializeExplicit with
    member this.ReadData(reader: IDataReader): unit =
      let t = this.RealObject.GetType()
      let genArgs = t.GenericTypeArguments

      let serializer = typedefof<mapSerializer<_,_>>.MakeGenericType genArgs

      let readData =
        serializer.GetRuntimeMethods()
        |> Seq.find (fun methodInfo -> methodInfo.Name.Contains("ReadData"))

      let results = readData.Invoke(null, [| reader |])

      this.RealObject <- results

    member this.WriteData(writer: IDataWriter): unit =
      let t = this.RealObject.GetType()
      let genArgs = t.GenericTypeArguments

      let serializer = typedefof<mapSerializer<_,_>>.MakeGenericType genArgs

      let writeData =
        serializer.GetRuntimeMethods()
        |> Seq.find (fun methodInfo -> methodInfo.Name.Contains("WriteData"))

      writeData.Invoke(null, [| writer; this.RealObject |]) |> ignore

• Define a component that uses this data type (here is an example

namespace BugTest

open Duality
open Duality.Cloning

type public test() =
  inherit Component()

  [<CloneBehavior(CloneBehavior.Reference)>]
  let mutable someData = Map.empty<int, float32>

  // Don't expose the map directly as you cannot edit it in the editor
  member _this.ValueOfOne
    with get () = Option.defaultValue 0.0f (someData.TryFind 1)
    and set (value) = someData <- someData.Add (1, value)

• Add the component to a game object in the scene and save the scene
• Check out the resulting xml (here is what was produced for me for the given test component)

<item dataType="Struct" type="BugTest.test" id="3147944685">
  <active dataType="Bool">true</active>
  <gameobj dataType="ObjectRef">3532719645</gameobj>
  <someData dataType="Struct" type="Microsoft.FSharp.Collections.FSharpMap`2[[System.Int32],[System.Single]]" id="4039567885" surrogate="true">
    <header />
    <body>
      <keys dataType="Array" type="System.Int32[]" id="3034686246">1</keys>
      <values dataType="Array" type="System.Single[]" id="245129914">15</values>
    </body>
  </someData>
</item>

• Force a reload of the scene file (close/reopen the editor)
• Check out test component, ValueOfOne should be now zero and if you turn on private variable viewing you can see the map is empty
• Also can add a break point to the ReadData function this line this.RealObject <- results and will see that results does contain the correct map

Some fix ideas

• Re-read from the RealObject prop after the ReadData call. Not sure if this will actually work or not...
• I thought that it might be possible to use ref returns but looks like while f# can read them from c#, the reverse is not possible (bottom of that section). If everything was boxable as an object might be able to provide a 'getObjectRef' function to the plugin with a ref return.

Not sure if you think its super important to mess with this right now. In the short term I probably will be the only one who benefits... Would be happy to investigate a fix if we come up with an idea we want to pursue

Thanks!

[ilexp]

Hey there,

thank you for your in-depth issue description, really appreciate this! 👍

Duality serialization and cloning do in fact treat immutable data types a bit like an occasional special case, and it shows in some of their designs. It should, however, be possible to deal with immutable data types properly:

• For a serializer surrogate, use the ConstructObject and WriteConstructorData methods, but be aware that no child object will be able to back-reference to the surrogate object that way, since its instance is not registered until after the construct call is done.
• For a cloning surrogate, you could use the CreateTargetObjectLate method, but be aware of similar referencing limitations. However, I think your by-reference CloneBehavior solution is a really good solution, as it's exactly how you would handle with immutable collections yourself.

That said, default serialization should be able to deal with most immutable types by simply ignoring their immutability and handling their private data fields directly - if that's not the case, we should take a look why and how they're failing in order to see if there's something that can be done on the system level too. Can you provide some information on how / why exactly this failed?

[greyepoxy]

Yes, you are totally right the default serialization works fine. Sorry tried to mention this in the original context but suspect I wrote to many words (kind of a bad habit... 😢). Serialized content does look a little crazy though since FSharp's Map type is a tree internally.

So safe to say what I am doing is "writing a custom serializer in FSharp for an immutable data type (just Map at the moment)." As you suggested I switched to using the ConstructObject and WriteConstructorData methods and it totally works. 🎉

I don't believe I am going to have any circular child <-> parent references in these types so am not super worried about the back referencing.

If you think it is worth it (and a good idea?) can look into if there is some way to make immutable types less of a special case in the serialization code. My naive guess is that the re-assigning the RealObject prop to obj after the ReadData call should work (probably have to re-register them?, can registration move farther down the function?). If not that is fine happy to have a workaround, thanks for your time!

[greyepoxy]

Ahh it just occurred to me that probably cannot move the registration call because this is how you get circular references to work... hmm well I think I answered my own question then, doesn't look like there is an straight forward fix to this (using return ref's or parameter ref's might still work but not really sure).

At least would it be helpful if I updated the ISerializeSurrogate/ISerializeExplicit summary comments to document this (that immutable objects need to be read/written as part of the ConstructObject and WriteConstructorData methods)?

[ilexp]

Yes, you are totally right the default serialization works fine. Sorry tried to mention this in the original context but suspect I wrote to many words (kind of a bad habit... 😢). Serialized content does look a little crazy though since FSharp's Map type is a tree internally.

Ah, got it. I can imagine, default behavior tends to not hit the most optimal way to represent data for collection types. We've had something similar until recently with HashSet<T>, until Barsonax contributed a surrogate for that.

Ahh it just occurred to me that probably cannot move the registration call because this is how you get circular references to work... hmm well I think I answered my own question then, doesn't look like there is an straight forward fix to this (using return ref's or parameter ref's might still work but not really sure).

Yep, that's pretty much it.

If not that is fine happy to have a workaround, thanks for your time!

Great, glad to hear. Will close this issue then, let us know if something else comes up. And thanks for the thorough description!

At least would it be helpful if I updated the ISerializeSurrogate/ISerializeExplicit summary comments to document this (that immutable objects need to be read/written as part of the ConstructObject and WriteConstructorData methods)?

Sure, always glad to see a new PR, go for it 👍