OpenNefia icon indicating copy to clipboard operation
OpenNefia copied to clipboard

Serializing class instances can mutate their state in damaging ways

Open Ruin0x11 opened this issue 3 years ago • 0 comments

To avoid allocation, the :serialize() function on classes will return self as the table to be serialized by binser. However, there are some classes that have properties that should not be serialized, like pointers to data prototypes. When :serialize() is called on those objects, the current design is such that those properties should be set to nil by that method. :deserialize() is supposed to restore those properties to their intended values when a save is loaded. However, currently :deserialize() is not called on the objects after serializing to restore the removed properties if the game is not loaded after saving. This is very bad.

If the problem this was trying to solve was avoiding unnecessary allocations, then I have to wonder if the performance benefit is actually worth it. However, :deserialize() will act as if the table is the deserialized class instance, as in the table is not in a special serialized format that gets copied to a fresh instance of that class afterwards - the data deserialized by binser is the class instance itself.

Because of that, maybe it would be best to just call :deserialize() directly after calling :serialize(), and declare that the programmer contract for ISerializable is such that calling :serialize() and :deserialize() in order must be an idempotent operation.

Ruin0x11 avatar Apr 29 '21 06:04 Ruin0x11