Make generated code reusable for other connection implementations

[psychon]

This idea is in the context of #318. There, one needs a way to get the "on the wire bytes" for a request. I do not yet have a good idea on how to do this, but here is a bad one:

Introduce some traits so that the API user can construct e.g. a GetInputFocusRequest struct and the generics figure out the rest.

A quick sketch is as follows, but this introduces seven new traits, which I do not really like. Three new traits are the bare minimum, because one needs to tell apart "has no reply", "has a reply", "has a reply with FDs". Core requests do not have a "special" major opcode while extensions do. That way I ended up with the below.

diff --git a/src/connection.rs b/src/connection.rs
index d71beaf..86dce0f 100644
--- a/src/connection.rs
+++ b/src/connection.rs
@@ -12,7 +12,7 @@ use crate::errors::{ConnectionError, ParseError, ReplyError, ReplyOrIdError};
 use crate::protocol::xproto::Setup;
 use crate::protocol::Event;
 use crate::utils::RawFdContainer;
-use crate::x11_utils::{ExtensionInformation, TryParse, TryParseFd, X11Error};
+use crate::x11_utils::{ExtensionInformation, TryParse, TryParseFd, X11Error, CoreVoidRequest, CoreReplyRequest, ExtensionVoidRequest, ExtensionReplyRequest, ExtensionReplyFDsRequest};
 
 /// Number type used for referring to things that were sent to the server in responses from the
 /// server.
@@ -57,6 +57,54 @@ pub trait RequestConnection {
     /// they are parsed.
     type Buf: AsRef<[u8]> + std::fmt::Debug + Send + Sync + 'static;
 
+    /// Foo
+    fn send_core_void_request<R>(&self, request: &R) -> Result<VoidCookie<'_, Self>, ConnectionError>
+        where R: CoreVoidRequest, R::Reply: TryParse,
+    {
+        let bytes = request.serialize();
+        let slices = bytes.iter().map(|b| IoSlice::new(&*b)).collect::<Vec<_>>();
+        self.send_request_without_reply(&slices, vec![])
+    }
+
+    /// Foo
+    fn send_core_reply_request<R>(&self, request: &R) -> Result<Cookie<'_, Self, R::Reply>, ConnectionError>
+        where R: CoreReplyRequest, R::Reply: TryParse,
+    {
+        let bytes = request.serialize();
+        let slices = bytes.iter().map(|b| IoSlice::new(&*b)).collect::<Vec<_>>();
+        self.send_request_with_reply(&slices, vec![])
+    }
+
+    /// Foo
+    fn send_ext_void_request<R>(&self, request: R) -> Result<VoidCookie<'_, Self>, ConnectionError>
+        where R: ExtensionVoidRequest, R::Reply: TryParse,
+    {
+        let info = self.extension_information(R::EXTENSION_NAME)?.ok_or(ConnectionError::UnsupportedExtension)?;
+        let (bytes, fds) = request.serialize(info.major_opcode);
+        let slices = bytes.iter().map(|b| IoSlice::new(&*b)).collect::<Vec<_>>();
+        self.send_request_without_reply(&slices, fds)
+    }
+
+    /// Foo
+    fn send_ext_reply_request<R>(&self, request: R) -> Result<Cookie<'_, Self, R::Reply>, ConnectionError>
+        where R: ExtensionReplyRequest, R::Reply: TryParse,
+    {
+        let info = self.extension_information(R::EXTENSION_NAME)?.ok_or(ConnectionError::UnsupportedExtension)?;
+        let (bytes, fds) = request.serialize(info.major_opcode);
+        let slices = bytes.iter().map(|b| IoSlice::new(&*b)).collect::<Vec<_>>();
+        self.send_request_with_reply(&slices, fds)
+    }
+
+    /// Foo
+    fn send_ext_reply_with_fds_request<R>(&self, request: R) -> Result<CookieWithFds<'_, Self, R::Reply>, ConnectionError>
+        where R: ExtensionReplyFDsRequest, R::Reply: TryParse,
+    {
+        let info = self.extension_information(R::EXTENSION_NAME)?.ok_or(ConnectionError::UnsupportedExtension)?;
+        let (bytes, fds) = request.serialize(info.major_opcode);
+        let slices = bytes.iter().map(|b| IoSlice::new(&*b)).collect::<Vec<_>>();
+        self.send_request_with_reply_with_fds(&slices, fds)
+    }
+
     /// Send a request with a reply to the server.
     ///
     /// The `bufs` parameter describes the raw bytes that should be sent. The returned cookie
diff --git a/src/lib.rs b/src/lib.rs
index 86fe675..57c5316 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -197,3 +197,56 @@ pub const CURRENT_TIME: Timestamp = 0;
 
 /// This constant can be used to fill unused entries in `Keysym` tables
 pub const NO_SYMBOL: Keysym = 0;
+
+// Some sample impls to show that it is implementable; of course the code generator could do better
+// than this
+
+impl x11_utils::CoreRequest for protocol::xproto::NoOperationRequest {
+    fn serialize(&self) -> connection::PiecewiseBuf<'_> {
+        let mut request0 = vec![
+            127,
+            0,
+            0,
+            0,
+        ];
+        let length = 1u16;
+        request0[2..4].copy_from_slice(&length.to_ne_bytes());
+        vec![request0.into()]
+    }
+}
+impl x11_utils::CoreVoidRequest for protocol::xproto::NoOperationRequest {
+}
+
+impl x11_utils::CoreRequest for protocol::xproto::GetInputFocusRequest {
+    fn serialize(&self) -> connection::PiecewiseBuf<'_> {
+        let mut request0 = vec![
+            43,
+            0,
+            0,
+            0,
+        ];
+        let length = 1u16;
+        request0[2..4].copy_from_slice(&length.to_ne_bytes());
+        vec![request0.into()]
+    }
+}
+impl x11_utils::CoreReplyRequest for protocol::xproto::GetInputFocusRequest {
+}
+
+impl x11_utils::ExtensionRequest for protocol::bigreq::EnableRequest {
+    const EXTENSION_NAME: &'static str = protocol::bigreq::X11_EXTENSION_NAME;
+
+    fn serialize(self, extension_opcode: u8) -> connection::BufWithFds<connection::PiecewiseBuf<'static>> {
+        let mut request0 = vec![
+            extension_opcode,
+            0,
+            0,
+            0,
+        ];
+        let length = 1u16;
+        request0[2..4].copy_from_slice(&length.to_ne_bytes());
+        (vec![request0.into()], vec![])
+    }
+}
+impl x11_utils::ExtensionReplyRequest for protocol::bigreq::EnableRequest {
+}
diff --git a/src/x11_utils.rs b/src/x11_utils.rs
index 1771fdc..cb96fea 100644
--- a/src/x11_utils.rs
+++ b/src/x11_utils.rs
@@ -7,6 +7,7 @@
 use std::convert::TryInto;
 
 use crate::errors::ParseError;
+use crate::connection::{BufWithFds, PiecewiseBuf};
 use crate::protocol::{request_name, ErrorKind};
 use crate::utils::RawFdContainer;
 
@@ -255,6 +256,43 @@ pub type ReplyParsingFunction =
         &mut Vec<RawFdContainer>,
     ) -> Result<(crate::protocol::Reply, &'a [u8]), ParseError>;
 
+/// A core protocol request
+pub trait CoreRequest: Request {
+    /// Serialize this request into its X11 protocol wire representation.
+    fn serialize(&self) -> PiecewiseBuf<'_>;
+}
+
+/// A core protocol request that does not have a reply
+pub trait CoreVoidRequest: CoreRequest {
+}
+
+/// A core protocol request that has a reply
+pub trait CoreReplyRequest: CoreRequest {
+}
+
+/// A request from an X11 extension
+pub trait ExtensionRequest: Request {
+    /// The protocol name of the extension that this request belongs to.
+    const EXTENSION_NAME: &'static str;
+
+    /// Serialize this request into its X11 protocol wire representation.
+    ///
+    /// The argument is the major opcode of the extension that this request belongs to.
+    fn serialize(self, extension_opcode: u8) -> BufWithFds<PiecewiseBuf<'static>>;
+}
+
+/// An extension request that does not have a reply
+pub trait ExtensionVoidRequest: ExtensionRequest {
+}
+
+/// An extension request that has a reply
+pub trait ExtensionReplyRequest: ExtensionRequest {
+}
+
+/// An extension request that has a reply containing file descriptors
+pub trait ExtensionReplyFDsRequest: ExtensionRequest {
+}
+
 /// A type implementing this trait can be serialized into X11 raw bytes.
 pub trait Serialize {
     /// The value returned by `serialize`.

I am opening this issue to track this idea. But mainly, I hope that someone has a great idea on how to implement this in a simpler way.

(Follow-up work would then be to split up the x11rb crate into some encode/decode layer and the x11rb-sync thingie that provides the blocking behaviour that we already have.)

[yshui]

Since the cookies each have a separate type in these case, I think we does need to have a least 3 traits. If we use the serde machinery and put the extension opcode on the Serializer side, 3 traits would probably be enough.

[psychon]

True... okay, then, what should those three traits look like?
Here is another attempt that goes rid of the not-all-that-necessary traits (and adding the Option necessary to still be able to express core requests):

use crate::connection::{BufWithFds, PiecewiseBuf};

/// An X11 request that does not have a reply
pub trait VoidRequest {
    /// The protocol name of the extension that this request belongs to, or None for core requests
    const EXTENSION_NAME: Option<&'static str>;

    /// Serialize this request into its X11 protocol wire representation.
    ///
    /// The argument is the major opcode of the extension that this request belongs to. For core
    /// requests, the argument may not have any influence
    fn serialize(self, extension_opcode: u8) -> BufWithFds<PiecewiseBuf<'static>>;
}

/// An X11 request that has a reply
pub trait ReplyRequest {
    /// The protocol name of the extension that this request belongs to, or None for core requests
    const EXTENSION_NAME: Option<&'static str>;

    /// Serialize this request into its X11 protocol wire representation.
    ///
    /// The argument is the major opcode of the extension that this request belongs to. For core
    /// requests, the argument may not have any influence
    fn serialize(self, extension_opcode: u8) -> BufWithFds<PiecewiseBuf<'static>>;
}

/// An X11 request that has a reply containing file descriptors
pub trait ReplyFDsRequest {
    /// The protocol name of the extension that this request belongs to, or None for core requests
    const EXTENSION_NAME: Option<&'static str>;

    /// Serialize this request into its X11 protocol wire representation.
    ///
    /// The argument is the major opcode of the extension that this request belongs to. For core
    /// requests, the argument may not have any influence
    fn serialize(self, extension_opcode: u8) -> BufWithFds<PiecewiseBuf<'static>>;
}

The content of all three traits is again identical, so I am still tempted to introduce a fourth trait for this (or... just use the already existing Request trait).

[yshui]

I guess we could use marker traits, i.e. HasReply, HasFd. And then, we implement a serde::Serializer, something that does this:

fn serialize(r: &Request, ext_opcodes: HashMap<Extension, u8>) -> ... {
	...
	if is_serializing_op_code_now {
		opcode += ext_opcodes[current_extension];
	}
}

The Serialize implemented for the requests need to do some special handshakes to give the Serializer information about current extension, etc.

[notgull]

This is actually the direction I'm taking with breadx, see the branch I'm working on here.

For the purposes of ecosystem standardization, I wonder if using the same backing protocol crate would be beneficial. @psychon What are your thoughts on this?

[psychon]

Sure, somewhere somewhen I might already have written that splitting out the protocol stuff might be a good idea. That still seems like something that could be split out. And that's even before you'd want to re-use this code.

I cannot really do much with your link. But do you think the x11rb code would require many changes before it would be usable for you? Or what's your plan?

Currently, I still think that one needs generated code even outside the traits that #657 adds. Otherwise the API is just quite unergonomic for crate users.

Also, I am already working (as time permits) on a branch that implements an async connection based on these new traits. Last time I worked on this, I managed to get a GetInputFocus request & its reply across. Current WIP state is (now) here: https://github.com/psychon/x11rb/tree/async-experiments/x11rb-async

[notgull]

I cannot really do much with your link

Whoops, realized I forgot to commit the protocol files I'd generated there. They should be there now. Definitely not in a finished state, but they should give a good idea of what I'd like to do.

But do you think the x11rb code would require many changes before it would be usable for you? Or what's your plan?

I was trying to design the x11_protocol crate with both breadx and x11rb in mind. I imagine the X11Serialize::to_io_slices function being analogous to the private serialize function found on requests in x11rb. I'm also designing it with other potential use cases in mind; for instance, a redesign of the xtrace utility, or if some madman wants to try reimplementing X.org in Rust.

To answer your question, though: after #657 lands, I could definitely work the current form of the proto module into breadx. You could have some kind of RequestTarget trait that looks something like:

trait RequestTarget<T: Request> {
    type Output;
    fn send_request(&mut self, req: T) -> Self::Output;
}

and implement that on all Connection.