-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Rust): Support serialization and deserialization of trait objects #1797
base: main
Are you sure you want to change the base?
Conversation
} | ||
|
||
#[derive(Fury, Debug)] | ||
#[polymorphic_traits("Animal")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid annotate polymorphic_traits
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try something else, see if I can get rid of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think of one solution to this issue.
Before this PR, dyn Any
is already supported, so solution below should work? Let me know if there is anything I'm missing.
Suppose the trait already contains a
fn as_any(&self) -> &dyn Any
method, then we can usetrait_object.as_any().type_id()
to get underlying struct's type id, then we don't need thispolymorphic_traits
.
as_any
solution is a frequently used tricks in rust, so I think adding this requirement is an acceptable choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think of one solution to this issue.
Before this PR,
dyn Any
is already supported, so solution below should work? Let me know if there is anything I'm missing.Suppose the trait already contains a
fn as_any(&self) -> &dyn Any
method, then we can usetrait_object.as_any().type_id()
to get underlying struct's type id, then we don't need thispolymorphic_traits
.
as_any
solution is a frequently used tricks in rust, so I think adding this requirement is an acceptable choice.
We already supported dyn Any
, but even if we have the type id
of the underlying struct, I don't quite understand how to cast to the dyn object
through type_id.
My understanding of the solution is as follows
impl Serializer for Box<dyn Foo> {
fn deserialize() {
let type_id_in_binary = fury.reader.i16(); // We read the type_id from binary, which was received from another app.
let concrete_type_deserializer = fury.get_deserializer(type_id_in_binary); // We obtain the deserializer that was registered by the user.
let concrete_instance: Box<dyn Any> = concrete_type_deserializer(); // Call the function pointer, and we get the instance of the underlying struct. The type of the result should be Any, as the function pointer should be compatible with all the deserializers.
concrete_instance as Box<dyn Foo> // We can't do this, as Rust can only cast from primitive types or concrete types. The compiler can't deduce the type of `concrete_instance` at compile time, so it can't create the v-table.
}
}
Let me know if there are some misunderstandings
MaybeTraitObject { ptr, type_id } | ||
} | ||
|
||
pub fn to_trait_object<T: 'static>(self) -> Result<T, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user don't call this function, then T
is leaked?
If it's, we can impl Drop for MaybeTraitObject to release memory..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, I will fixed it.
pub fn get_harness_by_type(&self, type_id: TypeId) -> Option<&Harness> { | ||
self.get_harness(*self.type_id_map.get(&type_id).unwrap()) | ||
pub fn get_class_info_by_fury_type(&self, type_id: u32) -> &ClassInfo { | ||
let type_id = self.fury_type_id_map.get(&type_id).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many unwrap here, how about return a Result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The codebase contain too many unwrap, I will fix it one by one.
What does this PR do?
Support serialization and deserialization of trait objects
The relationship between a trait object and a concrete type is generated by a macro and stored in a Fury instance. We can access the serializer and deserializer using the
Fury type ID
andRust type ID
.Use as follows
We should add
impl_polymorph
for trait which is use for serializingBox<dyn xxxx>
andpolymorphic_traits
for struct which is use for generate code to cast upcast concete type to trait object. Note that rust not support upcast trait yet(rust-lang/rust#65991).