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

Add serde implementation for error-stack #792

Closed
wants to merge 9 commits into from
Closed

Add serde implementation for error-stack #792

wants to merge 9 commits into from

Conversation

indietyp
Copy link
Member

@indietyp indietyp commented Jul 12, 2022

This PR was superseded by #1290


🌟 What is the purpose of this PR?

Create a serde implementation for Report and Frame

🔗 Related links

🚫 Blocked by

🔍 What does this change?

  • serde implementation for Report

📜 Does this require a change to the docs?

Yes. The docs need to be updated to make users know that a serde serialization is provided.

⚠️ Known issues

🐾 Next steps

🛡 What tests cover this?

  • New unit tests and integration tests are required

❓ How to test this?

  1. Unit Tests
  2. Integration/Snapshot Tests

📹 Demo

@github-actions github-actions bot added the area/libs > error-stack Affects the `error-stack` crate (library) label Jul 12, 2022
@indietyp
Copy link
Member Author

indietyp commented Aug 4, 2022

This PR implements a hook-based serialization (like #794) and a new attachment kind for serde (like attach_printable). I am unsure if we should keep both or decide on one of them.

While serde is more intuitive, the hook-based serialization allows for greater control but is much more explicit. (hook based would enable us to serialize backtraces easily, which isn't possible otherwise)

Libraries could also enable the serde (if wanted) and add their data. At the same time, with the hook-based approach, we would need to explicitly state every serde type that we want to serialize. hook based gives more control to the end user.

@TimDiekmann, what do you think? Keep both? Only keep attach_serializable() or only keep hooks?

@TimDiekmann
Copy link
Member

The plan is to remove attach_printable when specialization (min_specialization does not allow specialization on traits) has been stabilized. I don't think it's possible to specialize on two different types, if the second type is not a strict subset of the first type, i.e. (abstracting away the actual specialization-structs)

fn attach<T>();
fn attach<T: Display>();
fn attach<T: Serialize>();

It's not possible, because T could be Display + Serialize and adding a T: Display + Serialize does not help here (yet?). Maybe this is possible with negative trait bounds, but as far as I can tell there is no implementation yet.
In these terms, I'd prefer not to add another attach function.

@indietyp
Copy link
Member Author

indietyp commented Aug 5, 2022

I know that attach_printable is planned for removal once specialization hits. I haven't thought about the ramifications that every type needs to be a strict subset. This, of course, makes this more challenging.

Therefore I will remove all code relating to attach_serializable.

I am pretty new to the concept of specialization in Rust, so here's my question:

Would something like this be possible (with what you linked):

trait SerializeImpl {
   fn serialize(&self) -> Option<&dyn erased_serde::Serialize>;
}

unsafe trait FrameImpl: SerializeImpl {
    ...
}

impl<C> SerializeImpl for ContextFrame<C> where C: Context {
     fn serialize(&self) -> Option<&dyn erased_serde::Serialize> {
       None
     }
}

impl<C> SerializeImpl for ContextFrame<C> where C: Context + serde::Serialize {
    fn serialize(&self) -> Option<&dyn erased_serde::Serialize> {
      Some(<dyn erased_serde::Serialize>::erase(self.context))
    }
}

unsafe impl<C> FrameImpl for ContextFrame<C> where C: Context {
   ...
}

@vilkinsons vilkinsons changed the title serde implementation for error-stack Add serde implementation for error-stack Aug 9, 2022
@indietyp indietyp closed this Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/libs > error-stack Affects the `error-stack` crate (library)
Development

Successfully merging this pull request may close these issues.

2 participants