rojo-rbx / rbx-dom

Roblox DOM and (de)serialization implementation in Rust
MIT License
113 stars 45 forks source link

Traverse inheritance tree to find defaults in rbx_binary serializer #420

Closed kennethloeffler closed 2 months ago

kennethloeffler commented 3 months ago

To satisfy some requirements for #418, this PR changes rbx_binary's serializer to traverse up the inheritance tree of the reflection database when locating default values to write for instances that have missing properties.

find_default_property could be a method on ReflectionDatabase, but since it is not clear if this operation will have any utility outside of rbx_binary's serializer, it is kept internal.

This adds a small amount of overhead when locating properties that have defaults located further up the inheritance tree, and for instances that have properties unknown to the reflection database. The overhead is likely small, but it is probably worth benchmarking to ensure it is not too great.

Dekkonot commented 3 months ago

I think putting this method on a reflection database is probably worthwhile. A use case I imagine is lazily evaluating the default value of a property for a tool like Lune.

kennethloeffler commented 3 months ago

I'm noticing some inconsistency between the parameters of ReflectionDatabase::superclasses and ReflectionDatabase::find_default_property. find_default_property accepts a class descriptor, but superclasses takes a class name. It might be better if these were the same. I'd rather not have find_default_property accept a string class name because looking up an initial ClassDescriptor is redundant for rbx_binary's usage, so should we change superclasses so it accepts a ClassDescriptor instead of a &str?

Dekkonot commented 2 months ago

Sorry for the delay in getting back to you on this. Yeah, I suppose we should probably adjust superclasses to use a ClassDescriptor. It's one less lookup that it has to do and doesn't really change the use of the method. I'll handle that in a separate PR.