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

WIP: replace capnpronto with serde + bincode #2800

Closed
wants to merge 1 commit into from

Conversation

sobolevn
Copy link
Contributor

@sobolevn sobolevn commented Mar 21, 2024

Why is this PR marked as WIP?
Well, there's a problem I want to ask about.

Right now we can correctly serialize and deserialize all internal objects, except the fact that TypeVar's ids are not regenerated. This is needed to uniquely identify type vars and to make sure that there are no conflicts.

Old module had this code:

fn type_var(&mut self, reader: &schema::type_::var::Reader<'_>) -> Result<Arc<Type>> {
let serialized_id = reader.get_id();
let id = self.get_or_insert_type_var_id(serialized_id);
Ok(type_::generic_var(id))
}
fn get_or_insert_type_var_id(&mut self, id: u64) -> u64 {
match self.type_var_id_map.get(&id) {
Some(&id) => id,
None => {
let new_id = self.ids.next();
let _ = self.type_var_id_map.insert(id, new_id);
new_id
}
}
}

The first proposed solution by @HarryET #1912 had this logic: https://github.com/HarryET/gleam/blob/b46f227ac18f9bbe2e36de2a7c9b73505ba74282/compiler-core/src/metadata.rs#L22-L42

But, the code above is incomplete (it does not have types_value_constructors, values and accessors processing). It is also not very clean and will require constant modification.

The problem is that serde does not allow simple stateful deserialization. It has DeserializeSeed API which is not an easy API to use and to connect with bincode.

The second problem is that bincode@1.0 will soon be replaced with bincode@2.0 which will not have deserialize_from_seed API. So, I don't think that it is worth doing that as well.

I've also tried https://github.com/Marwes/serde_state to work with the global state, but it is unmaintained.

So, what should we do? :)
Tests are failing because of that.

Most of the credit for this change goes to @HarryET
Thank you!

Closes #1599

@sobolevn sobolevn marked this pull request as draft March 21, 2024 20:07
@HarryET
Copy link
Contributor

HarryET commented Mar 23, 2024

Awesome to see this change back! Happy to have a look if you want but feel bad you've had to sift through that PR 🤣

@sobolevn
Copy link
Contributor Author

I totally need an advice :)

@lpil
Copy link
Member

lpil commented May 20, 2024

Closing due to inactivity. Please feel free to reopen! Thank you

@lpil lpil closed this May 20, 2024
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.

Switch from capnproto to serde
3 participants