Skip to content
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

Serialize for robj #305

Merged
merged 2 commits into from
Oct 20, 2021
Merged

Serialize for robj #305

merged 2 commits into from
Oct 20, 2021

Conversation

andy-thomason
Copy link
Contributor

Implement Serialize for Robj allowing all Robj types to be serialized.

For example, a Robj could be converted to JSON and the Serializer trait can include Robj types.

This PR also introduces the Rany type which makes generation of polymorphic code easier.

@@ -23,7 +23,7 @@ paste = "1.0.5"
serde = { version = "1.0", features = ["derive"], optional = true }

[features]
default = []
default = ["serde"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it enabling serde by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should't. I'll remove this before we merge.

Comment on lines +468 to +470
pub fn deparse(&self) -> Result<Robj> {
use crate as extendr_api;
call!("deparse", self)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little bit concerned about deparse, which is later used for serialization.
Can we encounter deparsing problems on R side?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deparse should work error-free. Once we have the Strings wrapper, we should return that.

I do trap an error just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should maybe concatenate the strings and return a String.

Comment on lines +551 to +576
impl ser::Serialize for Environment {
fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
where
S: ser::Serializer,
{
serializer.serialize_unit()
}
}

impl ser::Serialize for Promise {
fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
where
S: ser::Serializer,
{
serializer.serialize_unit()
}
}

impl ser::Serialize for Language {
fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
where
S: ser::Serializer,
{
serializer.serialize_unit()
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that serialize_unit() serializes nothing. Is it a good idea to allow serialization of these types?
For instance, Environment can be serialized and deserizliaed as some key-value collection, if it does not contain non-serializable objects (like some connection or file handle).
Not sure about promises, but language can perhaps be serialized similar to how functions are handled, using deparse.
Language AFAIK contains no closures, so it is just an AST, which can be serialized and deserialized back.

I point this out because I am concerned about the roundtripability of serialization.
For instance, we can serialize x ~ y and then deserialize it and evaluate it in some context, but this won't work with promise. Perhaps we should not support serialization for such types at all and just return an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is still a work in progress.

Comment on lines +679 to +688
Rany::Dot(_dot) => serializer.serialize_unit(),
Rany::Any(_any) => serializer.serialize_unit(),
Rany::List(value) => value.serialize(serializer),
Rany::Expression(value) => value.serialize(serializer),
Rany::Bytecode(_bytecode) => serializer.serialize_unit(),
Rany::ExternalPtr(_externalptr) => serializer.serialize_unit(),
Rany::WeakRef(_weakref) => serializer.serialize_unit(),
Rany::Raw(value) => value.serialize(serializer),
Rany::S4(value) => value.serialize(serializer),
Rany::Unknown(_unknown) => serializer.serialize_unit(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Do we really need to serialize non-serializable as a unit (Rust's ())?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise. A work in progress.

At present we don't have any way of representing these types and ExternalPtr is generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I would very much like to cover every R type.

@andy-thomason andy-thomason marked this pull request as draft October 15, 2021 09:59
Copy link
Member

@Ilia-Kosenkov Ilia-Kosenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, no more questions.

@andy-thomason andy-thomason marked this pull request as ready for review October 20, 2021 10:31
@andy-thomason andy-thomason merged commit 67060ca into master Oct 20, 2021
@andy-thomason andy-thomason deleted the serialize-for-robj branch October 20, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants