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

Implementing prost::Message externally is annoying, we need to improve it #936

Open
poliorcetics opened this issue Oct 19, 2023 · 1 comment

Comments

@poliorcetics
Copy link

I did #878 and we discussed it on Discord, where @LucioFranco said they don't have the time to maintain and review bigger features PRs like this one.

In the spirit of removing maintenance burden from prost maintainers, I want to improve the experience of implementing prost::Message for one's own type, in downstream crates. This issue is listing the problems and blockers I find, and, if any, the solutions I'm proposing

Required doc(hidden) methods in the trait

Two required methods are doc(hidden): look at the compiled docs and the source for the trait: encode_raw and merge_field are hidden.

This causes a few issues: Rust Analyzer "Implement Missing Members" assist doesn't pick them up, which causes surprising behaviour when just using todo!() to implement missing methods to start trying things out. (This is a bug in rust-analyzer, but still, it means the experience is not as nice)

It also mean the docs don't help at all when looking at the docs for the error source.

Possible solutions

  1. Don't hide the methods: it was done for a reason, so probably a no ? It would at least not be a breaking change. We could document the methods as "not for regular use"
  2. Use a RawMessage trait and make the definition of Message depend on it: Message: RawMessage + Debug + .... breaking change

Implementing prost::Message for new types can be impossible to do efficiently

Using this:

use prost::bytes::{Buf, BufMut};
use prost::encoding::{DecodeContext, WireType};
use prost::DecodeError;

#[derive(Debug)]
pub struct BoxStr(Box<str>);

impl prost::Message for BoxStr {
    fn encoded_len(&self) -> usize {
        todo!()
    }

    fn clear(&mut self) {
        todo!()
    }

    fn encode_raw<B>(&self, buf: &mut B)
    where
        B: BufMut,
        Self: Sized,
    {
        todo!()
    }

    fn merge_field<B>(
        &mut self,
        tag: u32,
        wire_type: WireType,
        buf: &mut B,
        ctx: DecodeContext,
    ) -> Result<(), DecodeError>
    where
        B: Buf,
        Self: Sized,
    {
        todo!()
    }
}

Implementing Message here is impossible to do efficiently:

  1. All methods except clear need a conversion to String to use its Message impl
  2. prost doesn't expose any utilities to work on &str for non-mutating methods
  3. WireType and DecodeContext are doc(hidden) so finding documentation on how to use them is harder than necessary. WireType is fully undocumented too, adding to the task.
@tarcieri
Copy link
Contributor

Note that there's been some discussion of merging the Name trait into Message, which would further complicate external impls of Message.

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

No branches or pull requests

2 participants