message is not copied in result of hydrateRepeated

[tathanhdinh]

I have a problem in using the function hydrateRepeated to deserialize repeated fields where fields are message themselves. Concretely, I have a Chunk message with a repeated field of Instruction message (the protobuf file can be referenced here, to deserialize the messages, I have used this code. I observe that each Instruction is correctly deserialized, but the output list of instructions contains only null objects.
(that means each message is correctly deserialized, but the deserialized result is not copied into the output list!?).

Looking into the source code of hydrateRepeated, I observe that element is an immutable value (though the argument passed to hydrater is mutable)

let hydrateRepeated<'a> (hydrater:'a ref -> RawField -> unit) propRef rawField =
    let element = Unchecked.defaultof<'a>
    hydrater (ref element) rawField
    propRef := element :: !propRef

so that might be the reason why the deserialized value is not copied into the list. A trivial example is the following code:

let increase (i:int ref) : int =
  i := !i + 1
  !i

let i = 3
increase (ref i)
i

The value of i is always 3. For the function hydrateRepeated, IMHO, the following fix might work

let hydrateRepeated<'a> (hydrater:'a ref -> RawField -> unit) propRef rawField =
    let element = ref Unchecked.defaultof<'a>
    hydrater element rawField
    propRef := !element :: !propRef

[jhugard]

Yup, looks like a bug & looks like your proposed fix should work. Sloppy of me to not have unit tested this. Feel free to add a test, update the code & send a pull request. Will otherwise try to get to this in a couple of days...

On a note, I'm not entirely happy with this particular routine. I had tried to eliminate construction of the throw-away ref object here by refactoring the entire library to use pass-by-reference (byref) - see this branch: https://github.com/ctaggart/froto/tree/pass-by-reference-and-record-serializer. Would be great to get that working, but I don't think it is possible to eliminate the runtime-error because .NET simply does not support byref and generic parameters

[jhugard]

I've fixed the above & added a unit test, but there is now an O(n^2) performance issue.

While writing the unit tests, I noticed that repeated fields were not being kept in order (packed-repeated works, though). As a quick fix, hydrateRepeated does an append to the end of the list for every new element added.

Not sure how to fix this long term without adding additional abstract methods to MessageBase, but that's probably going to be the solution.

[jhugard]

@tathanhdinh - please update your client code to rename hydrateRepeated to hydrateOneRepeatedInstance. Sorry for the inconvenience, but even I was confused at first, thinking that this function would deserialize the entire list of repeated objects (not just a single instance).