cetra3 / tmq

Rust ZeroMQ bindings for Tokio
150 stars 28 forks source link

Make Multipart.iter(&self) return concrete std type #32

Closed liufuyang closed 2 years ago

liufuyang commented 2 years ago

When returning with a concrete collections::vec_deque::Iter, it should then be easier for the end-user to use this package.

cetra3 commented 2 years ago

I'd be interested in what challenges you're running into without a concrete implementation returned.

I think exposing the VecDeque may make it harder for us to refactor later on without a breaking change & I'm not sure that this doesn't constitute a breaking change as well.

I'd be happier we had our own Iter IterMut struct which is returned with this method

liufuyang commented 2 years ago

Thanks for the reply. Perhaps there is a solution as I do not have that much experience on Rust trait usage. But here is the issue. My colleague has some code that looks like this:

/// A 0MQ frame iterator for the [`Multipart`] type.
#[derive(Debug)]
pub struct MultipartFramesIter<'a>(collections::vec_deque::Iter<'a, zmq::Message>);

impl<'a> Frames<'a> for tmq::Multipart {
    type FramesIter = MultipartFramesIter<'a>;

    fn iter_frames(&'a self) -> Self::FramesIter {
        MultipartFramesIter(self.iter())
    }
}

So basically there is a customized type MultipartFramesIter that internally holds some Iter type.

And with current tmq version it gives error like this:

error[E0308]: mismatched types
  --> src/protocol/frames.rs:32:29
   |
32 |         MultipartFramesIter(self.iter())
   |                             ^^^^^^^^^^^ expected struct `std::collections::vec_deque::Iter`, found opaque type
   |
  ::: /Users/fuyangl/.cargo/registry/src/github.com-1ecc6299db9ec823/tmq-0.3.0/src/message.rs:54:27
   |
54 |     pub fn iter(&self) -> impl Iterator<Item = &Message> {
   |                           ------------------------------ the found opaque type
   |
   = note:   expected struct `std::collections::vec_deque::Iter<'_, tmq::Message>`
           found opaque type `impl Iterator<Item = &tmq::Message>`

Then I am not sure how to change our code to make it compile, I have tried to do things like

#[derive(Debug)]
pub struct MultipartFramesIter<'a>(dyn Iterator<Item = &'a zmq::Message>);

#[derive(Debug)]
pub struct MultipartFramesIter<'a>(impl Iterator<Item = &'a zmq::Message>);

But they are not working.

liufuyang commented 2 years ago

Just some extra background:

We are porting code that used to depend on https://github.com/asonix/tokio-zmq see source code here to your crate. And asonix/tokio-zmq was returning concert Iter/IterMut types like this:

    pub fn iter(&self) -> Iter<zmq::Message> {
        self.inner.iter()
    }

    pub fn iter_mut(&mut self) -> IterMut<zmq::Message> {
        self.inner.iter_mut()
    }
liufuyang commented 2 years ago

After some search around it looks like what we could do is to write it as something like this:

/// A 0MQ frame iterator for the [`Multipart`] type.
#[derive(Debug)]
pub struct MultipartFramesIter<'a, T: Iterator<Item = &'a zmq::Message>>(T);

impl<'a> Frames<'a> for tmq::Multipart{

    type FramesIter = MultipartFramesIter<'a, impl Iterator<Item = &'a zmq::Message>>;

    fn iter_frames(&'a self) -> Self::FramesIter {
        MultipartFramesIter(self.iter())
    }
}

Plus using a #![feature(type_alias_impl_trait)] will make it work.

So I created https://github.com/cetra3/tmq/pull/33 to only add the iter_mut() for it.

cetra3 commented 2 years ago

No worries, I will close off this PR & let's focus on the other one.